Re: [PATCH] iio: sx9310: Prefer async probe

[Doug Anderson]

Hi,

On Sat, Aug 29, 2020 at 8:12 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 28 Aug 2020 17:01:18 -0700
> Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> > On one board I found that:
> >   probe of 5-0028 returned 1 after 259547 usecs
> >
> > There's no reason to block probe of all other devices on our probe.
> > Turn on async probe.
> >
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> > NOTE: I haven't done any analysis of the driver to see _why_ it's so
> > slow, only that I have measured it to be slow.  Someone could
> > certainly take the time to profile / optimize it, but in any case it
> > still won't hurt to be async.
>
> Hmm. It is vanishingly rare to use that flag

My guess is that people just haven't been spending as much time
optimizing boot performance recently.  I've been trying to do this and
finding that there are quite a few drivers that could benefit from
this flag.

In theory this flag should probably be on by default and it looks like
that was Dmitry's original intention but the state of the world 5
years ago was that it wasn't quite ready for this.  I think, in
particular, drivers that are more core to the system (IOMMUs, clocks,
regulators, etc) may not have been ready, but misc peripherals should
be no problem.


> so I'm not particularly
> keen on starting to deploy it when we don't know why a particular
> driver is taking so long.  I agree it should be safe but I don't
> like oddities I don't understand!
>
> There are some sleeps in there but they are all of the order of a few
> msecs.
>
> Could it be there is a regulator that is coming up very slowly?
>
> Any other ideas?

I can do a little bit of profiling next week, but even if we get this
down from 250 ms to 10 ms I'd still like to see async probe turned on.
There's no reason for it to be off and every little bit counts.


> Jonathan
>
> >
> > This is a very safe flag to turn on since:
> >
> > 1. It's not like our probe order was defined by anything anyway.  When
> > we probe is at the whim of when our i2c controller probes and that can
> > be any time.
> >
> > 2. If some other driver needs us then they have to handle the fact
> > that we might not have probed yet anyway.
> >
> > 3. There may be other drivers probing at the same time as us anyway
> > because _they_ used async probe.
> >
> > While I won't say that it's impossible to tickle a bug by turning on
> > async probe, I would assert that in almost all cases the bug was
> > already there and needed to be fixed anyway.
> >
> > ALSO NOTE: measurement / testing was done on the downstream Chrome OS
> > 5.4 tree.  I confirmed compiling on mainline.
> >
> >  drivers/iio/proximity/sx9310.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index dc2e11b43431..444cafc53408 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -1054,6 +1054,7 @@ static struct i2c_driver sx9310_driver = {
> >               .acpi_match_table = ACPI_PTR(sx9310_acpi_match),
> >               .of_match_table = of_match_ptr(sx9310_of_match),
> >               .pm = &sx9310_pm_ops,
> > +             .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> >       },
> >       .probe          = sx9310_probe,
> >       .id_table       = sx9310_id,
>