linux-kernel - Re: [PATCH] regulator: core: resolve supply voltage deferral silently

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]

Message-ID: <CA+ASDXPJO=F+fa2zE4LDdRzMjgiugJfuzZKnZ-pndagVtw++Ug@mail.gmail.com>
Date:   Thu, 2 Sep 2021 15:41:02 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Chen-Yu Tsai <wenst@...omium.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: core: resolve supply voltage deferral silently

On Thu, Sep 2, 2021 at 10:06 AM Mark Brown <broonie@...nel.org> wrote:
> On Wed, Sep 01, 2021 at 01:06:28PM -0700, Brian Norris wrote:
> > On Wed, Sep 1, 2021 at 8:09 AM Mark Brown <broonie@...nel.org> wrote:
>
> > > This doesn't make sense to me.  Why are we getting as far as trying to
> > > read the voltage if we've been told to defer probe?  This suggests that
> > > we ought to be doing this earlier on.  I see that the logic is already
> > > there to handle a deferral being generated here but it looks off.
>
> > Take a look at the commit this "Fixes":
>
> > 21e39809fd7c ("regulator: vctrl: Avoid lockdep warning in enable/disable ops")
>
> That driver change is at most tangentially related to the code that's
> being updated,

It introduced another case where we hit a spurious error log. And
below, you admit that you didn't understand what this is fixing
without that pointer. I guess we disagree.

> > Frankly, I'm not sure if we're abusing regulator framework features
> > (particularly, around use of ->supply) in commit 21e39809fd7c, or if
> > this is just a lacking area in the framework. I'm interested in
> > whether you have thoughts on doing this Better(TM).
>
> That's definitely an abuse of the API, the hardware design is pretty
> much a gross hack anywhere as far as I remember.  As Chen-Yu says I'd
> only expect this to be possible in the case where the supply is in
> bypass mode and hasn't got its own parent.  In any case I can see why
> it's happening now...

Well the hardware exists, the driver exists, and it all worked OK
until somewhat recently (and now it works again, thanks to Chen-Yu).
What should we do here, then? Just leave the "abuse" in place?

We *did* attempt some kind of alternative solution here, but it's
really not that easy. AFAICT, there isn't a good way for one regulator
to lock another, without exposing quite a bit more regulator-core
features to drivers. I think either the driver would need to access to
the |struct ww_acquire_ctx| in some way, or else we'd need to teach
the regulator core about the vctrl dependency, such that
regulator_lock_dependent() can handle the locking properly for us.

Brian