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: <CAGXv+5EpjHtHPY0jQ5m4iZ8hvEc=7XCnK33_YQUT+0Oi+WUfFg@mail.gmail.com>
Date:   Thu, 2 Sep 2021 12:09:45 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Brian Norris <briannorris@...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 4:06 AM Brian Norris <briannorris@...omium.org> wrote:
>
> On Wed, Sep 1, 2021 at 8:09 AM Mark Brown <broonie@...nel.org> wrote:
> >
> > On Thu, Aug 26, 2021 at 12:40:17PM -0700, Brian Norris wrote:
> >
> > >               if (current_uV < 0) {
> > > -                     rdev_err(rdev,
> > > -                              "failed to get the current voltage: %pe\n",
> > > -                              ERR_PTR(current_uV));
> > > +                     if (current_uV != -EPROBE_DEFER)
> > > +                             rdev_err(rdev,
> > > +                                      "failed to get the current voltage: %pe\n",
> > > +                                      ERR_PTR(current_uV));
> >
> > 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")
>
> The target |rdev| hasn't deferred probe, but it's telling the
> regulator core to DEFER because the supply (which is required for
> |rdev| to "get" its present voltage) isn't yet resolved. So the probe
> deferral isn't really about the device framework, but about the
> regulator framework.
>
> If this were a device framework deferral, then agreed, this would be
> bad -- we can't guarantee, for one, that the second try will not also
> defer. But in this case, vctrl_probe() has already ensured that its
> supply regualator is there (devm_regulator_get(..., "ctrl")) -- it
> just isn't wired up into |rdev->supply| yet.
>
> 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).

I do think we are slightly stretching the use of ->supply, but IIUC
this error message path will also get hit if the regulator in question
is in bypass mode and the core needs to read the voltage of its supply,
which is exactly the case described here:

https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L5475

ChenYu