Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <bf70f01d-41ac-dcdc-b503-5e0eb1e70329@codeaurora.org>
References: <20180814170617.100087-1-dianders@chromium.org>
 <20180814170617.100087-2-dianders@chromium.org> <bf70f01d-41ac-dcdc-b503-5e0eb1e70329@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 14 Aug 2018 13:03:07 -0700
Message-ID: <CAD=FV=VNCOyyq5+xvSudTmtBgPm4-s7gj7GRCvuL5vSmA6yDVA@mail.gmail.com>
Subject: Re: [PATCH 1/4] regulator: core: If consumers don't call
 regulator_set_load() assume max
To:     David Collins <collinsd@codeaurora.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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Tue, Aug 14, 2018 at 11:30 AM, David Collins <collinsd@codeaurora.org> wrote:
> The behavior introduced by this patch seems like an undesirable hack to
> me.  It goes against the general philosophy within the regulator framework
> of taking no action unless directed to do so by an explicit consumer
> request (or special device tree property).  We should assume that
> consumers make requests to meet their needs instead of assuming that they
> are missing important votes required by their hardware.

I don't agree so I guess it will be up to Mark to decide.

Specifically I will note that there are boatloads of drivers out there
that use the regulator framework but don't have a call to
regulator_set_load() in them.  Are these drivers all broken?  I don't
think so.  IMO the regulator_set_load() API is an optional call for
drivers that they can use to optimize power usage, not a required API.


>> --- 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 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.


-Doug