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
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
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.
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
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
participants (3)
-
Gargi Sharma -
Nicholas Mc Guire -
Peter Senna Tschudin