checkpatch: Add warnings for use of mdelay()
Subsystems (2)
PATCH 1/1 checkpatch: Add warnings for use of mdelay()
mdelay is a busy-wait loop which is wasteful. If at all possible, callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context, however, delays of 1ms+ in atomic context are rather expensive, so a warning for this case is probably appropriate as well to encourage people to move such expensive delays outside of atomic context
--- scripts/checkpatch.pl | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
scripts/checkpatch.pl +5 -0
@@ -2570,6 +2570,11 @@ sub process { } } +# check the patch for use of mdelay + if ($line =~ /\bmdelay\(/) { + WARN("use of mdelay() found: msleep() is the preferred API.\n" . $line ); + } + # warn about #ifdefs in C files # if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) { # print "#ifdef in C files should be avoided\n";
Signature · 5 lines
1.7.0.2 -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Joe Perches
maintainerscripts/checkpatch.pl sub process { · line 2574
Quoted 1 line · Israel Schlesinger
scripts/checkpatch.pl +1
-0+ if ($line =~ /\bmdelay\(/) {
That should be:
if ($line =~ /\bmdelay\s*\(/) {Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
mdelay is a busy-wait loop which is wasteful. If at all possible, callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context, however, delays of 1ms+ in atomic context are rather expensive, so a warning for this case is probably appropriate as well to encourage people to move such expensive delays outside of atomic context
Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave a 1ms delay. My patch to fix msleep() at that time didn't get in due to concerns about the cost of using hrtimers. Perhaps msleep() has gotten better, but, if not, actually getting a 1ms delay remains a valid reason for using mdelay() instead IMO. It made a difference of a few seconds at open time for a driver I was doing at the time.
jon
Quoted 18 lines · Jonathan Corbet
Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
mdelay is a busy-wait loop which is wasteful. If at all possible, callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context, however, delays of 1ms+ in atomic context are rather expensive, so a warning for this case is probably appropriate as well to encourage people to move such expensive delays outside of atomic context
Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave a 1ms delay. My patch to fix
msleep()at that time didn't get in due to concerns about the cost of using hrtimers. Perhapsmsleep()has gotten better, but, if not, actually getting a 1ms delay remains a valid reason for usingmdelay()instead IMO. It made a difference of a few seconds at open time for a driver I was doing at the time.
Quoted 6 lines
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Check out the recently added usleep in -tip, and the checkpatch patch pending in my queue that fixes that case (I'll send in a few hours ;) )
-pat
Signature · 2 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Andrew Morton
Acked-by Arjan van de VenQuoted 26 lines · On Tue, 27 Jul 2010 10:32:54 -0700 Patrick Pannuto <[email protected]>…
Quoted 18 lines · Jonathan Corbet
Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
mdelay is a busy-wait loop which is wasteful. If at all possible, callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context, however, delays of 1ms+ in atomic context are rather expensive, so a warning for this case is probably appropriate as well to encourage people to move such expensive delays outside of atomic context
Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave a 1ms delay. My patch to fix
msleep()at that time didn't get in due to concerns about the cost of using hrtimers. Perhapsmsleep()has gotten better, but, if not, actually getting a 1ms delay remains a valid reason for usingmdelay()instead IMO. It made a difference of a few seconds at open time for a driver I was doing at the time.Quoted 6 lines
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 3 lines
Check out the recently added usleep in -tip, and the checkpatch patch pending in my queue that fixes that case (I'll send in a few hours ;) )
The message should point people at usleep_range(), I'd suggest. It's a more power-friendly way of sleeping.
That assumes that the below patch gets merged - the people who handle timer-related things are presently, err, asleep.
From: Patrick Pannuto <[email protected]>
usleep[_range] are finer precision implementations of msleep and are designed to be drop-in replacements for udelay where a precise sleep / busy-wait is unnecessary. They also allow an easy interface to specify slack when a precise (ish) wakeup is unnecessary to help minimize wakeups
---
include/linux/delay.h | 6 ++++++ kernel/timer.c | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+)
include/linux/delay.h renamed from include/linux/delay.h~timers-add-usleep-timer +6 -0
@@ -45,6 +45,12 @@ extern unsigned long lpj_fine;void calibrate_delay (void );void msleep (unsigned int msecs);unsigned long msleep_interruptible (unsigned int msecs); +void usleep_range (unsigned long min,unsigned long max); + +static inline void usleep (unsigned long usecs) +{ +usleep_range (usecs, usecs); +}static inline void ssleep (unsigned int seconds) {
kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +22 -0
@@ -1763,3 +1763,25 @@ unsigned long msleep_interruptible(unsig }EXPORT_SYMBOL (msleep_interruptible); + +static int __sched do_usleep_range(unsigned long min, unsigned long max) +{ +ktime_t kmin; +unsigned long delta; + + kmin =ktime_set (0 , min *NSEC_PER_USEC ); + delta = max - min; +return schedule_hrtimeout_range (&kmin, delta,HRTIMER_MODE_REL ); +} + +/** +* usleep_range - Drop in replacement for udelay where wakeup is flexible +* @min: Minimum time in usecs to sleep +* @max: Maximum time in usecs to sleep +*/ +void usleep_range (unsigned long min,unsigned long max) +{ +__set_current_state (TASK_UNINTERRUPTIBLE ); +do_usleep_range (min, max); +} +EXPORT_SYMBOL (usleep_range);
_
Andy Whitcroft
maintainerQuoted 79 lines · Andrew Morton
Quoted 26 lines · On Tue, 27 Jul 2010 10:32:54 -0700 Patrick Pannuto <[email protected]>…
Quoted 18 lines · Jonathan Corbet
Quoted 7 lines · On Tue, 27 Jul 2010 10:11:11 -0700 Israel Schlesinger <[email protected]>…
mdelay is a busy-wait loop which is wasteful. If at all possible, callers should use msleep instead of mdelay.
The only time mdelay is really appropriate is in atomic context, however, delays of 1ms+ in atomic context are rather expensive, so a warning for this case is probably appropriate as well to encourage people to move such expensive delays outside of atomic context
Once upon a time, msleep(1) would sleep for 20ms, while mdelay(1) gave a 1ms delay. My patch to fix
msleep()at that time didn't get in due to concerns about the cost of using hrtimers. Perhapsmsleep()has gotten better, but, if not, actually getting a 1ms delay remains a valid reason for usingmdelay()instead IMO. It made a difference of a few seconds at open time for a driver I was doing at the time.Quoted 6 lines
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/Quoted 3 lines
Check out the recently added usleep in -tip, and the checkpatch patch pending in my queue that fixes that case (I'll send in a few hours ;) )
The message should point people at
usleep_range(), I'd suggest. It's a more power-friendly way of sleeping.That assumes that the below patch gets merged - the people who handle timer-related things are presently, err, asleep.
From: Patrick Pannuto <[email protected]>
usleep[_range] are finer precision implementations of msleep and are designed to be drop-in replacements for udelay where a precise sleep / busy-wait is unnecessary. They also allow an easy interface to specify slack when a precise (ish) wakeup is unnecessary to help minimize wakeups
Signed-off-by Patrick Pannuto <[email protected]>Acked-by Arjan van de Ven <[email protected]>Cc Ingo Molnar <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Andrew Morton <[email protected]>---
include/linux/delay.h | 6 ++++++ kernel/timer.c | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+)diff -puN
include/linux/delay.h~timers-add-usleep-timerinclude/linux/delay.hinclude/linux/delay.h renamed from include/linux/delay.h~timers-add-usleep-timer +6
-0@@ -45,6 +45,12 @@ extern unsigned long lpj_fine;void calibrate_delay (void );void msleep (unsigned int msecs);unsigned long msleep_interruptible (unsigned int msecs); +void usleep_range (unsigned long min,unsigned long max); + +static inline void usleep (unsigned long usecs) +{ +usleep_range (usecs, usecs); +}static inline void ssleep (unsigned int seconds) {diff -puN
kernel/timer.c~timers-add-usleep-timerkernel/timer.ckernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +0
-0@@ -1763,3 +1763,25 @@ unsigned long msleep_interruptible(unsig }
kernel/timer.c unsigned long msleep_interruptible(unsig · line 1773
Quoted 10 lines
kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +8
-0EXPORT_SYMBOL (msleep_interruptible); + +static int __sched do_usleep_range(unsigned long min, unsigned long max) +{ +ktime_t kmin; +unsigned long delta; + + kmin =ktime_set (0 , min *NSEC_PER_USEC ); + delta = max - min;
If this interface is taking a min and max in micro-seconds, then does not the delta need also to be converted to nano-seconds?
schedule_hrtimeout_range seems to call hrtimer_set_expires_range_ns which seems to generally be called with 'delta_ns'. Something like:
delta = (max - min) * NSEC_PER_USEC;
Quoted 4 lines
kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +4
-0+return schedule_hrtimeout_range (&kmin, delta,HRTIMER_MODE_REL ); +} + +/**
kernel/timer.c unsigned long msleep_interruptible(unsig · line 1787
Quoted 10 lines
kernel/timer.c renamed from kernel/timer.c~timers-add-usleep-timer +10
-0+* usleep_range - Drop in replacement for udelay where wakeup is flexible +* @min: Minimum time in usecs to sleep +* @max: Maximum time in usecs to sleep +*/ +void usleep_range (unsigned long min,unsigned long max) +{ +__set_current_state (TASK_UNINTERRUPTIBLE ); +do_usleep_range (min, max); +} +EXPORT_SYMBOL (usleep_range);
-apw