Re: [PATCH v1] iopoll: Tweak readx_poll_timeout sleep range

[Doug Anderson]

Hi,


On Thu, Jun 13, 2019 at 9:37 AM Marc Gonzalez <marc.w.gonzalez@xxxxxxx> 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@xxxxxxxxxxxx

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