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

[Jonathan Cameron]

On Sat, 29 Aug 2020 09:56:41 -0700
Doug Anderson <dianders@xxxxxxxxxxxx> wrote:

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

That fits with my understanding.  Would be great to have it on
by default though I guess it could make for some really hard to debug
race conditions.


> 
> 
> > 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.
Agreed. However, I'd like a comment next to the place we turn it on
saying what delays we are trying to mitigate by enabling it in this
driver.

Jonathan

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