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+ASDXMLBpF6bQLCoxkN-+CqjxOX-ujzYBTV1f=zU1J7fFNuDA@mail.gmail.com>
Date:   Wed, 1 Sep 2021 13:06:28 -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 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).

Brian