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. >> 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. Cheers Jon [0] http://marc.info/?l=linux-tegra&m=145935416701022&w=2
