linux-kernel - Re: [PATCH v1] iopoll: Tweak readx_poll

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] [day] [month] [year] [list]

Date:   Thu, 13 Jun 2019 10:10:57 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Marc Gonzalez <marc.w.gonzalez@...e.fr>
Cc:     Arnd Bergmann <arnd@...db.de>, Will Deacon <will.deacon@....com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Bjorn Helgaas <helgaas@...nel.org>
Subject: Re: [PATCH v1] iopoll: Tweak readx_poll_timeout sleep range

Hi,


On Thu, Jun 13, 2019 at 9:37 AM Marc Gonzalez <marc.w.gonzalez@...e.fr> wrote:
>
> On 13/06/2019 18:11, Doug Anderson wrote:
>
> > On Thu, Jun 13, 2019 at 9:04 AM Marc Gonzalez wrote:
> >
> >> Hmmm, I expect the typical use-case to be:
> >> "HW manual states operation X completes in 100 µs.
> >> Let's call usleep_range(100, foo); before hitting the reg."
> >>
> >> And foo needs to be a "reasonable" value: big enough to be able
> >> to merge several requests, low enough not to wait too long after
> >> the HW is ready.
> >>
> >> In this case, I'd say usleep_range(100, 200); makes sense.
> >>
> >> Come to think of it, I'm not sure min=26 (or min=50) makes sense...
> >> Why wait *less* than what the user specified?
> >
> > IIRC usleep_range() nearly always tries to sleep for the max.  My
> > recollection of the design is that you only end up with something less
> > than the max if the system was going to wake up anyway.  In such a
> > case it seems like it wouldn't be insane to go and check if the
> > condition is already true if 25% of the time has passed.  Maybe you'll
> > get lucky and you can return early.
> >
> > Are you actually seeing problems with the / 4, or is this patch just a
> > result of code inspection?
>
> No actual issue. I just ran into a driver calling:
>
>         readl_poll_timeout(status, val, val & mask, 1, 1000);
>
> and it seemed... unwise(?) to call usleep_range(1, 1);
>
> But if, as you say, usleep_range() aims for the max

It was certainly what we found in:

https://lkml.kernel.org/r/1444265321-16768-6-git-send-email-dianders@chromium.org

...in fact, at one point in time I had a patch cooked up that would
change the behavior during boot where (presumably) we'd rather boot
faster.  ...but after fixing dwc2 it didn't actually have much of an
impact elsewhere.


> then I guess it's
> not a big deal to issue an early read or 3... Meh

IMO it seems like the driver should be fixed.  It should either specify:

a) the (well defined) 0 for the delay, which means no delay.

b) a more sane delay


-Doug