udelay vs usleep_range

udelay vs usleep_range

[Gargi Sharma]

Hi all,

I ran the checkpatch script over drivers/staging/nvec.c and got the
following warning

 udelay(33);

CHECK: usleep_range is preferred over udelay; see
Documentation/timers/timers-howto.txt

I checked out the timers-howto.txt and for usleep_range, one has to specify
the lower and upper limit for the timer. How does one decide what the upper
limit will be if one is to change the function from udelay to usleep_range?

Thanks!
Gargi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20160911/14b83409/attachment.html 

udelay vs usleep_range

[Nicholas Mc Guire]

On Sun, Sep 11, 2016 at 09:28:29PM +0530, Gargi Sharma wrote:
> Hi all,
> 
> I ran the checkpatch script over drivers/staging/nvec.c and got the
> following warning
> 
>  udelay(33);
> 
> CHECK: usleep_range is preferred over udelay; see
> Documentation/timers/timers-howto.txt
> 
> I checked out the timers-howto.txt and for usleep_range, one has to specify
> the lower and upper limit for the timer. How does one decide what the upper
> limit will be if one is to change the function from udelay to usleep_range?
>
generally you would try to figure out what the rational for the delay is
and what boundary it might have. Eg. the udelay(100) in 
drivers/staging/nvec/nvec.c carries the comment:
         * We experience less incomplete messages with this delay than without
         * it, but we don't know why. Help is appreciated.
indicating that this time might actually not be that prcise
and even if you can just provide a small range say 100 to 110 microseconds
this would help the timer subsystem a lot. 

If you can not come up with a proposal based on comments (dont worry about
the time being wrong - you will send your patch to the driver maintainer
for review anyway), then you can always simply send mail to the author
and ask. Actually it would be good if the 33 microseconds would be explained
by some comment rather than leaving these "magic" numbers to speculation.

As this is a set of slow devices (kbd, mouse, power) and any current system
must be asumed to exhibit jitter in the range of atleast 10s of microseconds
you most likely can safely change 33 microseconds to a range of (30,40)
which already can allow significant optimization of runtime timer handling.

So the fix might not only contain a API change to usleep_range() but also
change the values to something meaningful in a macro like 
#define I2C_RX_DELAY 	30 /* minimum waiting time for I2C command response */
usleep_range(I2C_RX_DELAY, I2C_RX_DELAY + 10) 

To find out which list to ask if unsure use:
  scripts/get_maintainer.pl drivers/staging/nvec/nvec.c
and check for those marked with "open list" or 
  git blame -L 630,640  drivers/staging/nvec/nvec.c
(in this case the udelay(33) was on line 634) to ask the
author of that line of code where the udelay(33) comes from.

thx!
hofrat

udelay vs usleep_range

[Peter Senna Tschudin]

Hi Gargi,

On Sun, Sep 11, 2016 at 5:58 PM, Gargi Sharma <gs051095@gmail.com> wrote:
> Hi all,
>
> I ran the checkpatch script over drivers/staging/nvec.c and got the
> following warning
>
>  udelay(33);
>
> CHECK: usleep_range is preferred over udelay; see
> Documentation/timers/timers-howto.txt

Which version of checkpatch are you using? Mine didn't catch that.

$ git status
HEAD detached at next-20160909

$ ./scripts/checkpatch.pl -f ./drivers/staging/nvec/nvec.c
total: 0 errors, 0 warnings, 988 lines checked

./drivers/staging/nvec/nvec.c has no obvious style problems and is
ready for submission.

But looking into the code on line 634 where I found the udelay(33), I
have the impression that this is a false positive, something your
checkpatch didn't catch properly. That call is inside a function named
nvec_interrupt(), and the line:

        err = devm_request_irq(&pdev->dev, nvec->irq, nvec_interrupt,
0, "nvec", nvec);

declares it as the interrupt handler. The interrupt handler can't
sleep(interrupt handler runs in the atomic context in terminology of
timers-howto.txt), and using udelay() seems to be the thing to do
here.

>
> I checked out the timers-howto.txt and for usleep_range, one has to specify
> the lower and upper limit for the timer. How does one decide what the upper
> limit will be if one is to change the function from udelay to usleep_range?

That usually depends on hardware specifications, and on the precision
of the method used. Depending on the case the hardware may allow a
range for waiting, and depending on which function you are using for
waiting it is not guaranteed to get the precision you asked for. The
range is a solution to minimize impact on the system while
guaranteeing that the range will be met.

udelay vs usleep_range

[Nicholas Mc Guire]

On Sun, Sep 11, 2016 at 06:37:40PM +0200, Peter Senna Tschudin wrote:
> Hi Gargi,
> 
> On Sun, Sep 11, 2016 at 5:58 PM, Gargi Sharma <gs051095@gmail.com> wrote:
> > Hi all,
> >
> > I ran the checkpatch script over drivers/staging/nvec.c and got the
> > following warning
> >
> >  udelay(33);
> >
> > CHECK: usleep_range is preferred over udelay; see
> > Documentation/timers/timers-howto.txt
> 
> Which version of checkpatch are you using? Mine didn't catch that.
> 
> $ git status
> HEAD detached at next-20160909
> 
> $ ./scripts/checkpatch.pl -f ./drivers/staging/nvec/nvec.c
> total: 0 errors, 0 warnings, 988 lines checked
>

use ./scripts/checkpatch.pl -strict -f ./drivers/staging/nvec/nvec.c

to see these issues.

thx!
hofrat
 

udelay vs usleep_range

[Nicholas Mc Guire]

> 
> But looking into the code on line 634 where I found the udelay(33), I
> have the impression that this is a false positive, something your
> checkpatch didn't catch properly. That call is inside a function named
> nvec_interrupt(), and the line:
> 
>         err = devm_request_irq(&pdev->dev, nvec->irq, nvec_interrupt,
> 0, "nvec", nvec);
> 
> declares it as the interrupt handler. The interrupt handler can't
> sleep(interrupt handler runs in the atomic context in terminology of
> timers-howto.txt), and using udelay() seems to be the thing to do
> here.
>
devm_request_irq
  -> devm_request_threaded_irq 
       -> request_threaded_irq

so this should be ok here.

thx!
hofrat