Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932917AbcDKODr (ORCPT ); Mon, 11 Apr 2016 10:03:47 -0400 Received: from lists.s-osg.org ([54.187.51.154]:41635 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932318AbcDKODo (ORCPT ); Mon, 11 Apr 2016 10:03:44 -0400 Subject: Re: [PATCH 1/5] regulator: core: Resolve supply earlier To: Jon Hunter , Thierry Reding References: <1460038959-21592-1-git-send-email-thierry.reding@gmail.com> <570B8376.6030505@nvidia.com> <20160411114612.GD17743@ulmo.ba.sec> <570B964E.5010708@nvidia.com> From: Javier Martinez Canillas Cc: Mark Brown , Liam Girdwood , linux-kernel@vger.kernel.org Message-ID: <570BAEB2.6040803@osg.samsung.com> Date: Mon, 11 Apr 2016 10:03:30 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <570B964E.5010708@nvidia.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4381 Lines: 116 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 >>>> >>>> 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 >>>> Signed-off-by: Thierry Reding >>>> --- >>>> 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