linux-kernel - Re: Multiple regulators for one device [was drm/panfrost: add devfreq regulator support]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Apr 2020 14:42:13 +0100
From:   Steven Price <steven.price@....com>
To:     Clément Péron <peron.clem@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Nishanth Menon <nm@...com>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Viresh Kumar <vireshk@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Alyssa Rosenzweig <alyssa.rosenzweig@...labora.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Multiple regulators for one device [was drm/panfrost: add devfreq
 regulator support]

On 14/04/2020 20:16, Clément Péron wrote:
> Hi Mark,
> 
> On Tue, 14 Apr 2020 at 20:55, Mark Brown <broonie@...nel.org> wrote:
>>
>> On Tue, Apr 14, 2020 at 08:20:23PM +0200, Clément Péron wrote:
>>> Hi Liam and Mark,
>>
>> You might want to flag stuff like this in the subject line, I very
>> nearly deleted this without opening it since most of the email I get
>> about panfrost appears to be coming from me having sent patches rather
>> than being relevant.
> 
> Ok will do next time,
> 
>>
>>> We are having an issue with Panfrost driver registering two times the
>>> same regulator and giving an error when trying to create the debugfs
>>> folder.
>>
>>> Could you clarify if it is allowed for a device to register two times
>>> the same regulator?
>>
>>> I check Documentation/power/regulator/regulator.rst but this point is
>>> not specified.
>>
>> We don't actively prevent it and I can't think what other than debugfs
>> might run into problems (and that's just a warning) but it does seem
>> like a weird thing to want to do and like it's pointing to some
>> confusion in your code with two different parts of the device
>> controlling the same supply independently.  What's the use case here?
> 
> Panfrost first probe clock, reset and regulator in device_init:
> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/gpu/drm/panfrost/panfrost_drv.c#L602
> Then it probe for optional devfreq, devfreq will get opp tables and
> also get regulator again.
> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/gpu/drm/panfrost/panfrost_drv.c#L609
> 
> That's can be reworked and Panfrost can only probe regulator if there
> is no opp-table.

This is what I was thinking about looking at. But it may make sense 
instead to extend the regulator API to allow multiple regualtor_get() 
calls for a single device. I haven't had time to dig into how difficult 
this would be.

> But if multiple regulator is not an issue and as each request is logic.
> The first in device_init assure to enable the regulator and the second
> in OPP assure the voltage level.
> 
> Maybe we can just fix this warning?

 From what I can see in the code, just silencing the warning would lead 
to 'odd' behaviour with debugfs. The first struct regulator Panfrost 
acquires is the one that is used purely for turning the GPU on (no 
voltage scaling). The second struct regulator is the one which is 
obtained by the OPP framework. However the debugfs entries point into 
the actual struct regulator, so it would be far more logical/useful if 
those were pointing into the second struct regulator.

Ideally calling regulator_get a second time for the same device would 
simply return the same struct regulator object (with a reference count 
increment).

Perhaps a better approach would be for Panfrost to hand over the struct 
regulator objects it has already got to the OPP framework. I.e. open 
code dev_pm_opp_set_regulators(), but instead of calling 
regulator_get_optional() simply populate the regulators we already have?

The other benefit of that is it would provide a clear hand-over of 
responsibility between Panfrost handling it's own regulators and the OPP 
framework picking up the work. The disadvantage is that Panfrost would 
have to track whether the regulators have been handed over or not.

Steve