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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]