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

From: Brian Norris <briannorris@chromium.org>
To: Mark Brown <broonie@kernel.org>
Cc: Chen-Yu Tsai <wenst@chromium.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] regulator: core: resolve supply voltage deferral silently
Date: Thu, 2 Sep 2021 15:41:02 -0700	[thread overview]
Message-ID: <CA+ASDXPJO=F+fa2zE4LDdRzMjgiugJfuzZKnZ-pndagVtw++Ug@mail.gmail.com> (raw)
In-Reply-To: <20210902170613.GG11164@sirena.org.uk>

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