Re: [PATCH 1/5] regulator: core: Resolve supply earlier

[Jon Hunter]

On 11/04/16 12:46, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote:
>> Hi Thierry,
>>
>> On 07/04/16 15:22, Thierry Reding wrote:
>>> From: Thierry Reding <treding@xxxxxxxxxx>
>>>
>>> Subsequent patches will need access to the parent supply from within the
>>> set_machine_constraints() function to properly implement bypass mode. If
>>> the parent supply hasn't been resolved by that time the voltage can't be
>>> queried.
>>>
>>> Also, by making sure the supply is resolved early most of the changes in
>>> set_machine_constraints() don't have to be undone if resolution fails.
>>>
>>> Suggested-by: Mark Brown <broonie@xxxxxxxxxx>
>>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>> ---
>>>  drivers/regulator/core.c | 27 +++++++++++++++++++++------
>>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index 2786d251b1cc..cc0333a79924 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>>  
>>>  	dev_set_drvdata(&rdev->dev, rdev);
>>>  
>>> +	if (init_data && init_data->supply_regulator)
>>> +		rdev->supply_name = init_data->supply_regulator;
>>> +	else if (regulator_desc->supply_name)
>>> +		rdev->supply_name = regulator_desc->supply_name;
>>> +
>>> +	/*
>>> +	 * set_machine_constraints() needs the supply to be resolved in order
>>> +	 * to support querying the current voltage in bypass mode. Resolve it
>>> +	 * here to more easily handle deferred probing.
>>> +	 */
>>> +	ret = regulator_resolve_supply(rdev);
>>> +	if (ret < 0)
>>> +		goto scrub;
>>> +
>>
>> Thanks for sending this. However, I think that calling
>> regulator_resolve_supply() can cause a deadlock, because the
>> regulator_list_mutex is held at this point and
>> regulator_resolve_supply() calls regulator_dev_lookup() which may try to
>> request the mutex again.
> 
> True... I never encountered that case in my testing. I'm not sure
> exactly why, though.

I believe that you may see it on Tegra114 [0], however, that was the
only tegra board I have seen a deadlock here in the past.

>> So may be we need to move this call after the call to
>> regulator_of_get_init_data() before we acquire the mutex.
> 
> I don't think that'll work. regulator_resolve_supply() depends on some
> operations performed much later (such as rdev->dev.parent being set).

Hmmm ... yes I was not sure if there was something else needed.

> Perhaps moving the locking of the regulator_list_mutex down instead
> could work. It seems to me like the first place where it would need to
> be held is set_machine_constraints().

Yes either that or we add a variable to regulator_resolve_supply() and
regulator_dev_lookup() that indicates if the mutex is already held.
Moving the acquistion of mutex would be best/cleaner if that is ok.

>> Also, if we add this call, then I am wondering if we still need ...
>>
>> 	class_for_each_device(&regulator_class, NULL, NULL,
>> 			      regulator_register_resolve_supply);
> 
> Possibly not. That line was introduced to hook up existing orphan
> regulators with their parents when they were registered, but I guess
> since we now always defer probe if a parent isn't registered yet the
> line would become a no-op.

OK. I added Javier to the thread as he added this so whatever we propose
hopefully he can test as well.

Cheers
Jon

[0] http://marc.info/?l=linux-tegra&m=145935416701022&w=2