diff mbox series

[RFC,v1,4/4] regulator: core: Move regulator supply resolving to the probe function

Message ID 20230218083252.2044423-5-saravanak@google.com
State New
Headers
Series Simplify regulator supply resolution code by offloading to driver core |

Commit Message

Saravana Kannan Feb. 18, 2023, 8:32 a.m. UTC 
  We can simplify the regulator's supply resolving code if we resolve the
supply in the regulator's probe function. This allows us to:

- Consolidate the supply resolution code to one place.
- Avoid the need for recursion by allow driver core to take care of
  handling dependencies.
- Avoid races and simplify locking by reusing the guarantees provided by
  driver core.
- Avoid last minute/lazy resolving during regulator_get().
- Simplify error handling because we can assume the supply has been
  resolved once a regulator is probed.
- Allow driver core to use device links/fw_devlink, where available, to
  resolve the regulator supplies in the optimal order.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

Comments

Mark Brown Feb. 22, 2023, 10:51 p.m. UTC | #1 
On Sat, Feb 18, 2023 at 12:32:51AM -0800, Saravana Kannan wrote:
> We can simplify the regulator's supply resolving code if we resolve the
> supply in the regulator's probe function. This allows us to:
> 
> - Consolidate the supply resolution code to one place.
> - Avoid the need for recursion by allow driver core to take care of
>   handling dependencies.
> - Avoid races and simplify locking by reusing the guarantees provided by
>   driver core.
> - Avoid last minute/lazy resolving during regulator_get().
> - Simplify error handling because we can assume the supply has been
>   resolved once a regulator is probed.
> - Allow driver core to use device links/fw_devlink, where available, to
>   resolve the regulator supplies in the optimal order.

It would be good if you had noted the issues with moving the
constraint initialistion that you mentioned elsewhere in the
thread here - from the review I've done thus far that is the
biggest issue this creates - it means that we will not do any
needed hardware configuration until all the parents have sorted
themselves out (which may never even happen), increasing the
amount of time that the system is running out of spec.

> +	if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
>  		return r;
>  
>  	r = regulator_lookup_by_name(supply);
> -	if (r)
> +	if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
>  		return r;
>  
>  	return ERR_PTR(-ENODEV);

This will return -ENODEV in the case where we have found a device
but it didn't probe yet.  It's probably more appropriate to
return -EPROBE_DEFER like we do when we know about a DT link.
This is also adding a leak of the get_device() from the looks of
it.

Shouldn't this be using device_is_bound() rather than peering at
the links?  Or alternatively if device_is_bound() does something
different to peering at the links isn't that really confusing?

> @@ -2050,13 +2050,6 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  		}
>  	}
>  
> -	/* Recursively resolve the supply of the supply */
> -	ret = regulator_resolve_supply(r);
> -	if (ret < 0) {
> -		put_device(&r->dev);
> -		goto out;
> -	}
> -
>  	/*
>  	 * Recheck rdev->supply with rdev->mutex lock held to avoid a race
>  	 * between rdev->supply null check and setting rdev->supply in

Your commit message says this should avoid the need to worry
about locking so much so do we still need to recheck things?  We
still seem to be doing all the same things we were doing before.
diff mbox series

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d5f9fdd79c14..f3bf74d1a81d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1952,7 +1952,7 @@  static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 		if (node) {
 			r = of_find_regulator_by_node(node);
 			of_node_put(node);
-			if (r)
+			if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
 				return r;
 
 			/*
@@ -1982,11 +1982,11 @@  static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	}
 	mutex_unlock(&regulator_list_mutex);
 
-	if (r)
+	if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
 		return r;
 
 	r = regulator_lookup_by_name(supply);
-	if (r)
+	if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
 		return r;
 
 	return ERR_PTR(-ENODEV);
@@ -2050,13 +2050,6 @@  static int regulator_resolve_supply(struct regulator_dev *rdev)
 		}
 	}
 
-	/* Recursively resolve the supply of the supply */
-	ret = regulator_resolve_supply(r);
-	if (ret < 0) {
-		put_device(&r->dev);
-		goto out;
-	}
-
 	/*
 	 * Recheck rdev->supply with rdev->mutex lock held to avoid a race
 	 * between rdev->supply null check and setting rdev->supply in
@@ -2178,13 +2171,6 @@  struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	ret = regulator_resolve_supply(rdev);
-	if (ret < 0) {
-		regulator = ERR_PTR(ret);
-		put_device(&rdev->dev);
-		return regulator;
-	}
-
 	if (!try_module_get(rdev->owner)) {
 		regulator = ERR_PTR(-EPROBE_DEFER);
 		put_device(&rdev->dev);
@@ -5649,9 +5635,6 @@  regulator_register(struct device *dev,
 	regulator_resolve_coupling(rdev);
 	mutex_unlock(&regulator_list_mutex);
 
-	/* try to resolve regulators supply since a new one was registered */
-	bus_for_each_dev(&regulator_bus, NULL, NULL,
-			 regulator_register_resolve_supply);
 	kfree(config);
 	return rdev;
 
@@ -5782,7 +5765,9 @@  static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
 
 static int regulator_drv_probe(struct device *dev)
 {
-	return 0;
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	return regulator_resolve_supply(rdev);
 }
 
 static struct device_driver regulator_drv = {