[2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

  As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
always-on/boot-on regulators"), we now might have a big delay during
probe of fixed regulators. That can have a significant boot speed
impact. Let's mitigate this by preferring async probe for fixed
regulators. The regulator framework itself has no issues with
regulators probing in an asynchronous way. The fixed regulator driver
is fairly straightforward and also has no issues.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/fixed.c | 1 +
 1 file changed, 1 insertion(+)
  

On Mon, Mar 13, 2023 at 11:18:20AM -0700, Douglas Anderson wrote:
> As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
> always-on/boot-on regulators"), we now might have a big delay during
> probe of fixed regulators. That can have a significant boot speed
> impact. Let's mitigate this by preferring async probe for fixed
> regulators. The regulator framework itself has no issues with
> regulators probing in an asynchronous way. The fixed regulator driver
> is fairly straightforward and also has no issues.

This is going to be true for all regulators...
  

Hi,

On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 13, 2023 at 11:18:20AM -0700, Douglas Anderson wrote:
> > As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
> > always-on/boot-on regulators"), we now might have a big delay during
> > probe of fixed regulators. That can have a significant boot speed
> > impact. Let's mitigate this by preferring async probe for fixed
> > regulators. The regulator framework itself has no issues with
> > regulators probing in an asynchronous way. The fixed regulator driver
> > is fairly straightforward and also has no issues.
>
> This is going to be true for all regulators...

Sure, that's true. So what are you suggesting?

There has always been a hope that someday we could just turn on async
probe everywhere. Folks at ChromeOS / Android at Google have looked at
this on and off over the years. Most recently +briannorris looked at
and described it as "no small task". There were a lot of kinks across
the various subsystems. I think +Saravana also has it on his wishlist
to tackle, too.

For the most part, the best we've been able to do is to identify
devices that happened to be involved in long delays at bootup and add
the flag to those devices after confirming that they worked OK. It's
not super elegant but it's pragmatic. That's what I've done here. If
there are other regulator drivers that are known to be involved in big
delays during probe time then we should add the flag there, too. Right
now, "off-on-delay" is only for fixed regulators, though there's no
real reason it shouldn't be applied to all regulators. I suppose some
of the other regulator related delays could affect pretty much any
regulator, though I guess normally those aren't associated with
always-on / boot-on regulators?

If you want, I can submit a pile of patches adding this flag to more
regulators. I did that in the past for mmc:

31ae403513be mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that are
newer than 5.4
d86472ae8b20 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v5.4
a1a489197a07 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that
existed in v4.19
7320915c8861 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that
existed in v4.14
2a99f3fa85ea mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v4.9
21b2cec61c04 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v4.4


-Doug
  

On Mon, Mar 13, 2023 at 12:49:37PM -0700, Doug Anderson wrote:
> On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:

> > This is going to be true for all regulators...

> Sure, that's true. So what are you suggesting?

That we shouldn't be making this change for just one driver, if you can
write an identical commit log for most if not all drivers but are just
making the change for one random driver then that suggests something is
being missed somewhere.  Either there's something special about this
driver or we should do things consistently.
  

Hi,

On Tue, Mar 14, 2023 at 6:29 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 13, 2023 at 12:49:37PM -0700, Doug Anderson wrote:
> > On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > This is going to be true for all regulators...
>
> > Sure, that's true. So what are you suggesting?
>
> That we shouldn't be making this change for just one driver, if you can
> write an identical commit log for most if not all drivers but are just
> making the change for one random driver then that suggests something is
> being missed somewhere.  Either there's something special about this
> driver or we should do things consistently.

The "special" things are:

* It's been confirmed that this one random driver is involved in
slowing down boot a significant amount.

* The fixed regulator driver is among the simplest of the regulators
and doesn't have the complex interactions that are typically
associated with async probe problems.

For reference, some of the problems I'm aware of that have been seen
in the past when trying to enable async probe more broadly:

a) Apparently, on ACPI child devices aren't guaranteed to be probed
before parent devices once you turn on async probe. This is not
typically the case with device-tree probed devices where children show
up due to of_platform_populate() which is called by a parent device.
This can be handled properly but often isn't.

b) Some drivers try to poke directly at other devices and can get
confused if the other device is probing at the same time. One example
was dual-MIPI on Rockchip [1]. Again, this can be handled properly but
often isn't because nobody tested it.

c) Dynamically allocated ID numbers can change unpredictably from boot
to boot with async probe. This showed up on MMC where eMMC and SD
would change each boot between "mmcblk0" and "mmcblk1".

d) Async probe (obviously) changes timing and that can expose latent
bugs. Almost always those bugs should have been fixed anyway.

e) Async probe tends to stress out other driver's (consumers of the
device that's now probing async) error handling (since they are more
likely to see -EPROBE_DEFER) and can expose latent bugs there.


Let's take an example that I have a tiny bit of familiarity with:
max77686. This is not too strange of a device. It's implemented as a
MFD and has sub-drivers for PMIC (regulator), RTC, and clock.

I think we can be pretty confident that a) above isn't a problem. The
MFD driver (the parent) populates the regulator (the child) with
devm_mfd_add_devices() and it won't do that until the (MFD) parent is
ready. So far so good.

I think we can also be fairly certain that c) isn't a problem, and
probably it's not a problem anywhere for regulators. Nobody (that I'm
aware of) relies on the stability of a dynamically allocated number
for a regulator.

Issues d) and e) could be an issue for max77686, but in almost all
cases where they are we'd want to fix those latent bugs anyway since
they could have hit even without async probe.

...but b) _could_ be a problem. Specifically, today I think that
(unless some of its supplies aren't available at probe time) the PMIC
driver will _always_ finish probing before the RTC/clock driver
because of the order specified in the source code:

  { .name = "max77686-pmic", },
  { .name = "max77686-rtc", },
  { .name = "max77686-clk", },

One would need to do deeper code inspection (and, ideally, testing) to
find out if indeed the RTC driver and clock driver will have problems
if the regulator is not finished probing before their probes start.
Hopefully everything here is fine, but it's the kind of place where
maybe somebody was sloppy because, today, the order is guaranteed. In
general drivers for multi-function devices are more likely to interact
directly with sibling drivers outside of the normal driver framework
and (in my experience) are more likely to be relying on probe order.

If someone wanted to carefully inspect the max77686 code (especially
the interactions between all the MFD sub-drivers) and test enabling
async probe there then it would be a nice improvement. ...switching
over and crossing our fingers might work OK but it also might not.


If we look at the fixed regulator driver and compare, we're in a
simpler state. Sure, we could still expose latent timing bugs or
latent wrong error handling in regulator consumers, but those should
be fixed anyway. The fixed regulator driver doesn't reach directly
into other devices through private APIs and pretty much just relies on
the regulator core for most of its work. The regulator core should be
async-robust because of needing to deal with regulators that show up
as modules.


I guess the last thing I'll say is that PROBE_PREFER_ASYNCHRONOUS was
added specifically so that we could enable this on drivers that were
found to be slow to boot and that were tested to work with async
probe. Without being able to add PROBE_PREFER_ASYNCHRONOUS people were
open-coding solutions per driver to speed probe.


[1] https://lore.kernel.org/r/20221019170255.2.I6b985b0ca372b7e35c6d9ea970b24bcb262d4fc1@changeid
  

On Tue, Mar 14, 2023 at 09:35:28AM -0700, Doug Anderson wrote:

> The "special" things are:

> * It's been confirmed that this one random driver is involved in
> slowing down boot a significant amount.

> * The fixed regulator driver is among the simplest of the regulators
> and doesn't have the complex interactions that are typically
> associated with async probe problems.

I would be surprised to see regulators being in that category.

> For reference, some of the problems I'm aware of that have been seen
> in the past when trying to enable async probe more broadly:

> a) Apparently, on ACPI child devices aren't guaranteed to be probed
> before parent devices once you turn on async probe. This is not
> typically the case with device-tree probed devices where children show
> up due to of_platform_populate() which is called by a parent device.
> This can be handled properly but often isn't.

That seems unlikely to be an issue here.

> b) Some drivers try to poke directly at other devices and can get
> confused if the other device is probing at the same time. One example
> was dual-MIPI on Rockchip [1]. Again, this can be handled properly but
> often isn't because nobody tested it.

Again, unlikely to be an issue for regulators - where multiple
regulators interact with each other that's already coordinated through
the core.

> c) Dynamically allocated ID numbers can change unpredictably from boot
> to boot with async probe. This showed up on MMC where eMMC and SD
> would change each boot between "mmcblk0" and "mmcblk1".

No numbers exposed to userspace here so that'll be fine.

> d) Async probe (obviously) changes timing and that can expose latent
> bugs. Almost always those bugs should have been fixed anyway.

> e) Async probe tends to stress out other driver's (consumers of the
> device that's now probing async) error handling (since they are more
> likely to see -EPROBE_DEFER) and can expose latent bugs there.

Right, not sure I'd worry about either of those though and given how
widley used fixed regualtors are I'd expect it to be one of the riskiest
here.

> ...but b) _could_ be a problem. Specifically, today I think that
> (unless some of its supplies aren't available at probe time) the PMIC
> driver will _always_ finish probing before the RTC/clock driver
> because of the order specified in the source code:

>   { .name = "max77686-pmic", },
>   { .name = "max77686-rtc", },
>   { .name = "max77686-clk", },

> One would need to do deeper code inspection (and, ideally, testing) to
> find out if indeed the RTC driver and clock driver will have problems
> if the regulator is not finished probing before their probes start.

Behind the scenes interactions would be quite unusual, the power demands
of the on chip devices are typically minimal so there's not much to do -
it's typically all always on functions.  Could happen of course but it'd
be surprising.

> I guess the last thing I'll say is that PROBE_PREFER_ASYNCHRONOUS was
> added specifically so that we could enable this on drivers that were
> found to be slow to boot and that were tested to work with async
> probe. Without being able to add PROBE_PREFER_ASYNCHRONOUS people were
> open-coding solutions per driver to speed probe.

Sure, but my point is that at least every regulator with a ramp time
(which will be most of them, even those that don't have one they tell
the kernel about one probably should) is going to have a similar issue
so could probably benefit from a similar solution.