Message-ID: <20200721201808.GD4845@sirena.org.uk>
Date:   Tue, 21 Jul 2020 21:18:08 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        John Stultz <john.stultz@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support
 for sync_state() callbacks

On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote:
> On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@...nel.org> wrote:
> > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:

> > > There are Android devices that exhibit the issue in the example where
> > > regulator-X is an LDO, device-A is a camera device and device-B and
> > > device-C are UFS and USB. To avoid this, they have their own downstream
> > > changes to the regulator framework.

> > Can you provide any references to these bodges?

> This is the best I could dig up. The site is slow as molasses. I don't
> want to focus or critique any specific vendor's downstream code
> though. Providing these links just to prove that this is a real issue.

The issue is not that I can't see that there might be a problem.  The
issue I have is that you seem to be starting from the point of a
specific solution that happens to work for your systems and then working
back from that rather than reasoning forwards from the problem to come
to a solution with the result that I can't explain the design you're
proposing.  Even with this version I have difficulty telling from the
changelog that the change doesn't restrict things based on the consumers
of a given regulator but rather based on the PMIC and I don't see why
this is being done.

> Search for "proxy" here. You'll notice how that will also need changes
> to regulator header files, etc. The 4.9 kernel is the latest publicly
> available version AFAIK.
> https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9

Oh, the Qualcomm stuff - which looks a lot like what you've got here.

> > As I indicated on my previous review this doesn't seem OK, given that a
> > huge proportion of the regulators on most systems are part of a single
> > PMIC this means that devices won't be able to fully control regulators
> > for the majority of the boot process, possibly quite a long time after
> > userspace has started in systems where not all devices have drivers.

...

> I think the default behavior should be for functional correctness
> (example in the commit text) and then go for optimization (being able
> to power off regulators before 30s into boot). Even with the timeout
> set, this series makes it much easier for driver developers to ensure
> functional correctness instead of playing initcall chicken between the
> supplier and the N consumers.

I don't see how any of this stuff about initcall chicken or whatever is
relevant here.  You're replying to my concern that your change isn't
just waiting for all the consumers of a given regulator, it's waiting
for every other consumer of any other regulator or random other feature
that happens to be supplied by the same PMIC but only that PMIC.  That
is not init ordering, it just seems like an arbitrary delay even once
all the consumers are ready and I can't see any particular logic behind
it.  It's not just waiting for all the users of the individual resource
to be active but it's also not waiting for the system as a whole,
instead it's waiting for some effectively random grouping of devices
spread over the whole system to appear.  I can't articulate a reason why
we'd do that, it seems to be combining the worst aspects of the two
approaches.

Please engage with this issue, it is crucial but you keep on sending the
same thing without explaining why so I'm left guessing and I really
can't come up with anything.

> Actually on systems without all the drivers, I'd argue the correct
> behavior is this patch series + regulator_cleanup_timeout=-1. This
> patch series will prevent system instability/unusability (Eg: missing
> display backlight driver) at the cost of power optimization. However,
> to allow turning off boot on regulators in systems without all the
> drivers where it happens to not cause functional correctness issues,
> we have the timeout default to 30s.

It's also preventing turning off regulators that don't have any
consumers at all, not even potential ones.  Those are the main target
for the cleanup of idle regulators.

> > I don't understand the motivation for doing things this way.  Like I
> > said last time it really feels like this turns the whole mechanism into
> > a very complicated way of implementing a new initcall.

> Treating this as a "LATER_initcall()" has several issues.
> 1. initcall levels set a max limit on the depth of device dependency.
> Since DT devices are added at arch initcall level, that gives us about
> 5 levels if you ignore the _sync ones. Adding more isn't going to
> scale or solve the problem because in reality, the dependencies are
> much deeper.

I don't follow what you're saying here at all, sorry.  What does the
depth of the dependency graph have to do with how late we action
anything?  A lot of what you're saying here seems to be based on jumping
to abandoning deferred probe which is a bit of a leap here.

> 6. If the answer is, "have userspace tell us when all modules are
> loaded" -- then we are depending on userspace for functional
> correctness AND for turning off regulators. Which IMHO is worse than
> this patch series.

> 7. If we somehow manage to add a "LATER_initcall" that doesn't have
> the issues above, it has to work for all frameworks. So, it has to
> come after ALL the devices in the system have probed. Not just "all
> devices of a supplier". So, again, it's worse than this patch series,
> at least for systems where all the drivers are present.

Run the initcall when all devices in the system have a driver bound
which as far as I can tell seems to be what the end result of this
series is intended to be anyway, just implemented with much more
complexity.  That doesn't need any involvement from userspace, though I
can't see why we'd want to do that rather than just work per regulator.

> > >    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
> > >       for the regulator and an enable vote is made.

> > If something was left partially set up by the bootloader this means that
> > drivers are no longer able to remove power from the device as part of
> > getting it into a known good state even if they are the only consumer.

> If they really NEED to turn power off to get to a known good state,
> then they really need to be exclusive consumers.

There's *need* to and there's can do on systems that can support the
thing, the same driver will often be able to handle both situations.
This also goes to a risk of regressions, it's a fairly major
transformation to be doing.  These issues would be massively mitigated
if this were per regulator as I have repeatedly suggested.

> 1. It will allow adding/enabling new drivers without worrying about
> the system crashing due to the example scenario mentioned in the
> commit text.

Or just don't set constraints which alow things to be changed until
you're ready...

> 2. In my development flow, I had to do some stuff manually and then
> load some modules. Without this series, when some of the regulator
> drivers were built in, the regulators would get turned off after the
> 30s timeout before I could do my manual stuff. That would kill the
> system. Or if I start off the boot and walk away to get coffee, I'd
> come back to a dead device. It was super annoying to deal with this.

That's a new one, the normal thing would've been to let the driver load
and then remove it to load whatever test stuff.  I'm not sure that's
super practical to be honest, taking advantage of it really does seem to
require a vertically integrated system where the DT exactly matches what
you want at runtime.

> 3. When the regulator drivers are loaded as modules (after 30s
> timeout), some of the boot on regulators are never turned off until
> one of their consumers starts making requests. For example, if a
> regulator that supplies some camera component is left on by the
> bootloader, it would never get turned off unless you open the camera
> app. With this series, the regulator would get turned off after the
> camera driver probes.

Right, that's an issue but there seem to be less invasive solutions.

> To be clear, I understand the cases you are mentioning and I'm not
> discounting them. But compared to the 30 seconds of additional "on
> time", the functional correctness issues are more important. I'm not
> saying this series is the perfect solution, but it's certainly better
> than what we have now and we have the default behavior to be as least
> disruptive as possible to systems that work fine without this series.
> And if I find incremental improvements in the future, I'll send
> patches for that. But I'd hate to see perfect be the enemy of the
> good.

Having the requests from drivers be acted on promptly is also a
potential correctness and robustness issue.  You seem very focused on
the disabling of unused regulators for power reasons case but that's
really not the only thing going on here, there's also just the need to
control things as part of what the device is supposed to be doing.
Systems aren't always built to let us do all the things we're really
supposed to do with hardware so it's often advantageous to have drivers
that will do these things but cope gracefully, this means that it
doesn't always make sense to use exclusive gets even where you really
actually want things to happen as requested if they can.  Often the
devices are in practice robust enough most of the time things are fine
if the requests get ignored which is why systems can be built that way
but strictly we're not supposed to do that so where we can we should
avoid it.  Forcing consumers to be exclusive means that we need to add a
call that lets these consumers figure out which kind of get to use and
then have the consumer have multiple code paths for that (or more
realistically factor that out into the core, but then we could even more
easily just not have the system wide restrictions).

There's also issues around being able to reason about what the system is
supposed to do and why, hence my concerns about waiting for all devices
on the PMIC to appear.  Like I said above this is absolutely crucial.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists