Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max

From: David Collins <collinsd@codeaurora.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Mark Brown <broonie@kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/4] regulator: core: If consumers don't call regulator_set_load() assume max
Date: Tue, 14 Aug 2018 14:59:01 -0700	[thread overview]
Message-ID: <aa2bd1f6-74b4-a63b-d115-88d6e16ed95d@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=VNCOyyq5+xvSudTmtBgPm4-s7gj7GRCvuL5vSmA6yDVA@mail.gmail.com>

Hi,

On 08/14/2018 01:03 PM, Doug Anderson wrote:
> On Tue, Aug 14, 2018 at 11:30 AM, David Collins <collinsd@codeaurora.org> wrote:>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -732,6 +732,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>>       struct regulator *sibling;
>>>       int current_uA = 0, output_uV, input_uV, err;
>>>       unsigned int mode;
>>> +     bool any_unset = false;
>>>
>>>       lockdep_assert_held_once(&rdev->mutex);
>>>
>>> @@ -751,11 +752,17 @@ static int drms_uA_update(struct regulator_dev *rdev)
>>>               return -EINVAL;
>>>
>>>       /* calc total requested load */
>>> -     list_for_each_entry(sibling, &rdev->consumer_list, list)
>>> +     list_for_each_entry(sibling, &rdev->consumer_list, list) {
>>>               current_uA += sibling->uA_load;
>>> +             if (!sibling->uA_load_set)
>>> +                     any_unset = true;
>>> +     }
>>>
>>>       current_uA += rdev->constraints->system_load;
>>>
>>> +     if (any_unset)
>>> +             current_uA = INT_MAX;
>>> +
>>
>> This check will incorrectly result in a constant load request of INT_MAX
>> for all regulators that have at least one child regulator.  This is the
>> case because such child regulators are present in rdev->consumer_list and
>> because regulator_set_load() requests are not propagated up to parent
>> regulators.  Thus, the regulator structs for child regulators will always
>> have uA_load==0 and uA_load_set==false.
> 
> Ah, interesting.
> 
> ...but doesn't this same bug exist anyway, just in the opposite
> direction?  Without my patch we'll try to request a 0 mA load in this
> case which seems just as wrong (or perhaps worse?).  I guess on RPMh
> regulator you're "saved" because the RPMh hardware itself knows the
> parent/child relationship and knows to ignore this 0 mA load, but it's
> still a bug in the overall Linux framework...

I'm not sure what existing bug you are referring to here.  Where is a 0 mA
load being requested?  Could you list the consumer function calls and the
behavior on the framework side that you're envisioning?

The RPMh hardware never sees requests with units of mA (though its
predecessor RPM SMD did).  Instead, mode requests sent to RPMh are raw
PMIC MODE_CTL register values.  RPMh then applies max aggregation of these
register values across masters (APPS, modem, etc).  Also note that the
RPMh knowledge of parent/child relationships is only utilized in enable
control and voltage headroom management.  It does not apply to mode control.

> I have no idea how we ought to propagate regulator_set_load() to
> parents though.  That seems like a tough thing to try to handle
> automagically.

I attempted it seven years ago with two revisions of a patch series [1]
and [2].  The feature wasn't completed though.  Perhaps something along
the same lines could be reintroduced now that child regulators are handled
the same way as ordinary consumers within the regulator framework.

Thanks,
David

[1]: https://lkml.org/lkml/2011/3/28/246
[2]: https://lkml.org/lkml/2011/3/28/530

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project