[PATCH] regulator: core: resolve supply voltage deferral silently

[PATCH] regulator: core: resolve supply voltage deferral silently

[Brian Norris]

Voltage-controlled regulators depend on their supply regulator for
retrieving their voltage, and so they might return -EPROBE_DEFER at this
stage. Our caller already attempts to resolve supplies and retry, so we
shouldn't be printing this error to logs.

Quiets log messages like this, on Rockchip RK3399 Gru/Kevin boards:

[    1.033057] ppvar_bigcpu: failed to get the current voltage: -EPROBE_DEFER
...
[    1.036735] ppvar_litcpu: failed to get the current voltage: -EPROBE_DEFER
...
[    1.040366] ppvar_gpu: failed to get the current voltage: -EPROBE_DEFER
...
[    1.044086] ppvar_centerlogic: failed to get the current voltage: -EPROBE_DEFER

Fixes: 21e39809fd7c ("regulator: vctrl: Avoid lockdep warning in enable/disable ops")
Cc: Chen-Yu Tsai <wenst@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/regulator/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ca6caba8a191..85783fb3aadf 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1151,9 +1151,10 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 		}
 
 		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));
 			return current_uV;
 		}
 
-- 
2.33.0.259.gc128427fd7-goog

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Brian Norris]

On Wed, Sep 1, 2021 at 8:09 AM Mark Brown <broonie@kernel.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

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

[Chen-Yu Tsai]

On Thu, Sep 2, 2021 at 4:06 AM Brian Norris <briannorris@chromium.org> wrote:
>
> On Wed, Sep 1, 2021 at 8:09 AM Mark Brown <broonie@kernel.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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

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@kernel.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, this is a prime example of just randomly shoving Fixes
lines onto things :( .  It's perfectly fine to send patches without a
Fixes line, adding them when they're not really related at best creates
noise and at worst causes problems with backporting (especially given
the whole pulling random commits into stable thing we have these days).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Brian Norris]

On Thu, Sep 2, 2021 at 10:06 AM Mark Brown <broonie@kernel.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@kernel.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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]

On Thu, Sep 02, 2021 at 03:41:02PM -0700, Brian Norris wrote:
> On Thu, Sep 2, 2021 at 10:06 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Sep 01, 2021 at 01:06:28PM -0700, Brian Norris wrote:

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

The point is the "another" bit - by just picking a random commit you
will cause people to think that an issue was introduced by that commit
which in turn means that people will for example use the presence of
that commit as a guide to backporting.  They may not backport things far
enough since the random commit isn't there, or backport things too far
if the actual issue was introduced later which can be even worse as it
can introduce breakage where it wasn't before.

In terms of not understanding the issue here is that the patch didn't
pass the smell test, it was your explanation that helped here not the
pointing at a driver change that lacks obvious relevance.  I really
don't know what the reader is supposed to infer about the change from
the commit, 

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

I don't think anyone came up with anything more tasteful to do with that
hardware, like I say the hardware is itself very hacky.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Brian Norris]

On Fri, Sep 3, 2021 at 4:11 AM Mark Brown <broonie@kernel.org> wrote:
> In terms of not understanding the issue here is that the patch didn't
> pass the smell test, it was your explanation that helped here not the
> pointing at a driver change that lacks obvious relevance.  I really
> don't know what the reader is supposed to infer about the change from
> the commit,

OK, I might see where you're coming from. Personally, I'd still like
to reference the commit in some way, because I've never used bypass
mode that I'm aware of, but the mentioned commit added a new case that
I do care about. I like documenting motivation, where reasonable --
but apparently I need to do a better job of that part.

> I don't think anyone came up with anything more tasteful to do with that
> hardware, like I say the hardware is itself very hacky.

OK. I guess I was specifically asking about this patch (and the new
usage of ->supply in commit 21e39809fd7c, to some extent). If the
usage of ->supply is the best we can/should do, then we can leave
21e39809fd7c alone. If you don't care to "fix" this log message, then
I can forget about $subject patch too.

Or, do you only want me to improve the commit message (drop the Fixes,
mention bypass mode, etc.) and resubmit?

Brian

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On Fri, Sep 03, 2021 at 10:09:04AM -0700, Brian Norris wrote:
> On Fri, Sep 3, 2021 at 4:11 AM Mark Brown <broonie@kernel.org> wrote:

> > In terms of not understanding the issue here is that the patch didn't
> > pass the smell test, it was your explanation that helped here not the
> > pointing at a driver change that lacks obvious relevance.  I really
> > don't know what the reader is supposed to infer about the change from
> > the commit,

> OK, I might see where you're coming from. Personally, I'd still like
> to reference the commit in some way, because I've never used bypass
> mode that I'm aware of, but the mentioned commit added a new case that
> I do care about. I like documenting motivation, where reasonable --
> but apparently I need to do a better job of that part.

A reference is fine - it's just that people actively use Fixes to mean
"the specific commit introduced a bug and anything with the referenced
commit might want this fix" which is different to "The change in this
commit caused me to notice this other thing" which is more what you've
got here.  For this case some words in the commit log saying something
like "The foo change in commit X ($subject) exposed $existing_problem"
is probably what you're looking for.

> > I don't think anyone came up with anything more tasteful to do with that
> > hardware, like I say the hardware is itself very hacky.

> OK. I guess I was specifically asking about this patch (and the new
> usage of ->supply in commit 21e39809fd7c, to some extent). If the
> usage of ->supply is the best we can/should do, then we can leave
> 21e39809fd7c alone. If you don't care to "fix" this log message, then
> I can forget about $subject patch too.

> Or, do you only want me to improve the commit message (drop the Fixes,
> mention bypass mode, etc.) and resubmit?

I've queued it for v5.16, I'm likely to move it to v5.15 - it's just
warnings on a very small subset of systems but OTOH it's pretty safe.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Brian Norris]

On Fri, Sep 3, 2021 at 10:54 AM Mark Brown <broonie@kernel.org> wrote:
> I've queued it for v5.16, I'm likely to move it to v5.15 - it's just
> warnings on a very small subset of systems but OTOH it's pretty safe.

Thanks! I'm OK with either. It's not a critical issue (log spam), but
in a way it's a small regression.

Brian

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

[Mark Brown]

On Thu, 26 Aug 2021 12:40:17 -0700, Brian Norris wrote:
> Voltage-controlled regulators depend on their supply regulator for
> retrieving their voltage, and so they might return -EPROBE_DEFER at this
> stage. Our caller already attempts to resolve supplies and retry, so we
> shouldn't be printing this error to logs.
> 
> Quiets log messages like this, on Rockchip RK3399 Gru/Kevin boards:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: core: resolve supply voltage deferral silently
      commit: adea283117225281ecf537171a06dd6e430bd8db

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark