linux-kernel - Re: [PATCH] iio: sx9310: Prefer async probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VWrXwDwoxBK9y2ZeFEoG-edO4WktdSW0MFZLfbcS2hLQ@mail.gmail.com>
Date:   Mon, 31 Aug 2020 11:59:37 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Daniel Campello <campello@...omium.org>,
        Enrico Granata <egranata@...omium.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Dmitry Torokhov <dtor@...omium.org>
Subject: Re: [PATCH] iio: sx9310: Prefer async probe

Hi,

On Sat, Aug 29, 2020 at 10:18 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sat, 29 Aug 2020 09:56:41 -0700
> Doug Anderson <dianders@...omium.org> wrote:
>
> > Hi,
> >
> > On Sat, Aug 29, 2020 at 8:12 AM Jonathan Cameron <jic23@...nel.org> wrote:
> > >
> > > On Fri, 28 Aug 2020 17:01:18 -0700
> > > Douglas Anderson <dianders@...omium.org> 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@...omium.org>
> > > > ---
> > > > 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.

One other note that I should have mentioned: anything that's buildable
as a module is essentially already testing this path.  Modules are
probed asynchronously and in parallel.  Possibly the solution for my
boot speed woes in this case is to just build this as a module since I
don't see any immediate need why it has to be builtin (so with that
realization I may not actually need this patch).  That being said, it
still seems nice to enable async probe in case someone has a reason to
build this in.


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

OK, I used function graph to get a trace.  Some of the time is simply
i2c transfers, which we do A LOT of during initialization (and i2c
transfers aren't known for being instant).  However, that's not the
big problem.  Check out the regmap_read_poll_timeout() in
sx9310_init_compensation().  20 ms per sleep with a 2 second timeout.
This seems to eat over around 220 ms of the time on my system.

So how do you want to do this?  Do you want me to re-post the patch
and mention the regmap_read_poll_timeout() in the commit message?  Do
you want to just add that to the commit message yourself?

Thanks!

-Doug