Hello, On 04/11/2016 08:19 AM, Jon Hunter wrote: > > 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 <[email protected]> >>>> >>>> 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 <[email protected]> >>>> Signed-off-by: Thierry Reding <[email protected]> >>>> --- >>>> 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. >
I guess Thierry didn't see that error because it only happens on platforms that do legacy (non-DT) regulators lookup. For OF registered regulators, the lookup logic doesn't grab the regulator list mutex. That was in fact why I didn't notice that issue introduced by my patch that later was fixed by Jon. >>> 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(®ulator_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. > Sure, I'll be able to test the patches on the platform where I had issues that motivated that change. But as mentioned in the other email, I think this patch will cause regressions on other platforms due moving the supply resolution to registration again. > Cheers > Jon > > [0] http://marc.info/?l=linux-tegra&m=145935416701022&w=2 > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
