2010-08-02 22:01:21

by Patrick Pannuto

[permalink] [raw]
Subject: [PATCH v3 0/4] Add usleep_range

Changes from v2:

* Remove usleep API
It seemed the weak consensus that the best way to
force callers to consider the cost / implications
of the underlying hrtimers was to leave only the
range API as an option. Given the dangers of an
implicit slack coupled with the potential
proliferation of interrupts with no slack, forcing
callers to supply a range seems the best choice.

* Documentation updated to reflect changes

* Fix checkpatch warnings to point to the correct file

[PATCH v3 1/4] timer: Added usleep_range timer
[PATCH v3 2/4] Documentation: Add timers/timers-howto.txt
[PATCH v3 3/4] Checkpatch: prefer usleep_range over udelay
[PATCH v3 4/4] Checkpatch: warn about unexpectedly long msleep's


2010-08-02 22:01:26

by Patrick Pannuto

[permalink] [raw]
Subject: [PATCH 2/4] Documentation: Add timers/timers-howto.txt

This file seeks to explain the nuances in various delays;
many driver writers are not necessarily familiar with the
various kernel timers, their shortfalls, and quirks. When
faced with

ndelay, udelay, mdelay, usleep_range, msleep, and msleep_interrubtible

the question "How do I just wait 1 ms for my hardware to
latch?" has the non-intuitive "best" answer:
usleep_range(1000,1500)

This patch is followed by a series of checkpatch additions
that seek to help kernel hackers pick the best delay.

Signed-off-by: Patrick Pannuto <[email protected]>
---
Documentation/timers/timers-howto.txt | 105 +++++++++++++++++++++++++++++++++
1 files changed, 105 insertions(+), 0 deletions(-)
create mode 100644 Documentation/timers/timers-howto.txt

diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
new file mode 100644
index 0000000..c9ef29d
--- /dev/null
+++ b/Documentation/timers/timers-howto.txt
@@ -0,0 +1,105 @@
+delays - Information on the various kernel delay / sleep mechanisms
+-------------------------------------------------------------------
+
+This document seeks to answer the common question: "What is the
+RightWay (TM) to insert a delay?"
+
+This question is most often faced by driver writers who have to
+deal with hardware delays and who may not be the most intimately
+familiar with the inner workings of the Linux Kernel.
+
+
+Inserting Delays
+----------------
+
+The first, and most important, question you need to ask is "Is my
+code in an atomic context?" This should be followed closely by "Does
+it really need to delay in atomic context?" If so...
+
+ATOMIC CONTEXT:
+ You must use the *delay family of functions. These
+ functions use the jiffie estimation of clock speed
+ and will busy wait for enough loop cycles to achieve
+ the desired delay:
+
+ ndelay(unsigned long nsecs)
+ udelay(unsigned long usecs)
+ mdelay(unsgined long msecs)
+
+ udelay is the generally preferred API; ndelay-level
+ precision may not actually exist on many non-PC devices.
+
+ mdelay is macro wrapper around udelay, to account for
+ possible overflow when passing large arguments to udelay.
+ In general, use of mdelay is discouraged and code should
+ be refactored to allow for the use of msleep.
+
+NON-ATOMIC CONTEXT:
+ You should use the *sleep[_range] family of functions.
+ There are a few more options here, while any of them may
+ work correctly, using the "right" sleep function will
+ help the scheduler, power management, and just make your
+ driver better :)
+
+ -- Backed by busy-wait loop:
+ udelay(unsigned long usecs)
+ -- Backed by hrtimers:
+ usleep_range(unsigned long min, unsigned long max)
+ -- Backed by jiffies / legacy_timers
+ msleep(unsigned long msecs)
+ msleep_interruptible(unsigned long msecs)
+
+ Unlike the *delay family, the underlying mechanism
+ driving each of these calls varies, thus there are
+ quirks you should be aware of.
+
+
+ SLEEPING FOR "A FEW" USECS ( < ~10us? ):
+ * Use udelay
+
+ - Why not usleep?
+ On slower systems, (embedded, OR perhaps a speed-
+ stepped PC!) the overhead of setting up the hrtimers
+ for usleep *may* not be worth it. Such an evaluation
+ will obviously depend on your specific situation, but
+ it is something to be aware of.
+
+ SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
+ * Use usleep_range
+
+ - Why not msleep for (1ms - 20ms)?
+ Explained originally here:
+ http://lkml.org/lkml/2007/8/3/250
+ msleep(1~20) may not do what the caller intends, and
+ will often sleep longer (~20 ms actual sleep for any
+ value given in the 1~20ms range). In many cases this
+ is not the desired behavior.
+
+ - Why is there no "usleep" / What is a good range?
+ Since usleep_range is built on top of hrtimers, the
+ wakeup will be very precise (ish), thus a simple
+ usleep function would likely introduce a large number
+ of undesired interrupts.
+
+ With the introduction of a range, the scheduler is
+ free to coalesce your wakeup with any other wakeup
+ that may have happened for other reasons, or at the
+ worst case, fire an interrupt for your upper bound.
+
+ The larger a range you supply, the greater a chance
+ that you will not trigger an interrupt; this should
+ be balanced with what is an acceptable upper bound on
+ delay / performance for your specific code path. Exact
+ tolerances here are very situation specific, thus it
+ is left to the caller to determine a reasonable range.
+
+ SLEEPING FOR LARGER MSECS ( 10ms+ )
+ * Use msleep or possibly msleep_interruptible
+
+ - What's the difference?
+ msleep sets the current task to TASK_UNINTERRUPTIBLE
+ whereas msleep_interruptible sets the current task to
+ TASK_INTERRUPTIBLE before scheduling the sleep. In
+ short, the difference is whether the sleep can be ended
+ early by a signal. In general, just use msleep unless
+ you know you have a need for the interruptible variant.
--
1.7.2

2010-08-02 22:01:25

by Patrick Pannuto

[permalink] [raw]
Subject: [PATCH 3/4] Checkpatch: prefer usleep_range over udelay

When possible, sleeping is (usually) better than delaying;
however, don't bother callers of udelay < 10us, as those
cases are generally not worth the switch to usleep

Signed-off-by: Patrick Pannuto <[email protected]>
---
scripts/checkpatch.pl | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..1698c63 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2570,6 +2570,14 @@ sub process {
}
}

+# prefer usleep_range over udelay
+ if (($line =~ /\budelay\s*\(\s*(\w+)\s*\)/ {
+ # ignore udelay's < 10, however
+ if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
+ CHK("usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\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";
--
1.7.2

2010-08-02 22:01:23

by Patrick Pannuto

[permalink] [raw]
Subject: [PATCH 1/4] timer: Added usleep_range timer

usleep_range is a finer precision implementations of msleep
and is designed to be a drop-in replacement for udelay where
a precise sleep / busy-wait is unnecessary.

Since an easy interface to hrtimers could lead to an undesired
proliferation of interrupts, we provide only a "range" API,
forcing the caller to think about an acceptable tolerance on
both ends and hopefully avoiding introducing another interrupt.

*** INTRO ***

As discussed here ( http://lkml.org/lkml/2007/8/3/250 ), msleep(1) is not
precise enough for many drivers (yes, sleep precision is an unfair notion,
but consistently sleeping for ~an order of magnitude greater than requested
is worth fixing). This patch adds a usleep API so that udelay does not have
to be used. Obviously not every udelay can be replaced (those in atomic
contexts or being used for simple bitbanging come to mind), but there are
many, many examples of

mydriver_write(...)
/* Wait for hardware to latch */
udelay(100)

in various drivers where a busy-wait loop is neither beneficial nor
necessary, but msleep simply does not provide enough precision and people
are using a busy-wait loop instead.

*** CONCERNS FROM THE RFC ***

Why is udelay a problem / necessary? Most callers of udelay are in device/
driver initialization code, which is serial...

As I see it, there is only benefit to sleeping over a delay; the
notion of "refactoring" areas that use udelay was presented, but
I see usleep as the refactoring. Consider i2c, if the bus is busy,
you need to wait a bit (say 100us) before trying again, your
current options are:

* udelay(100)
* msleep(1) <-- As noted above, actually as high as ~20ms
on some platforms, so not really an option
* Manually set up an hrtimer to try again in 100us (which
is what usleep does anyway...)

People choose the udelay route because it is EASY; we need to
provide a better easy route.

Device / driver / boot code is *currently* serial, but every few
months someone makes noise about parallelizing boot, and IMHO, a
little forward-thinking now is one less thing to worry about
if/when that ever happens

udelay's could be preempted

Sure, but if udelay plans on looping 1000 times, and it gets
preempted on loop 200, whenever it's scheduled again, it is
going to do the next 800 loops.

Is the interruptible case needed?

Probably not, but I see usleep as a very logical parallel to msleep,
so it made sense to include the "full" API. Processors are getting
faster (albeit not as quickly as they are becoming more parallel),
so if someone wanted to be interruptible for a few usecs, why not
let them? If this is a contentious point, I'm happy to remove it.

*** OTHER THOUGHTS ***

I believe there is also value in exposing the usleep_range option; it gives
the scheduler a lot more flexibility and allows the programmer to express
his intent much more clearly; it's something I would hope future driver
writers will take advantage of.

To get the results in the NUMBERS section below, I literally s/udelay/usleep
the kernel tree; I had to go in and undo the changes to the USB drivers, but
everything else booted successfully; I find that extremely telling in and
of itself -- many people are using a delay API where a sleep will suit them
just fine.

*** SOME ATTEMPTS AT NUMBERS ***

It turns out that calculating quantifiable benefit on this is challenging,
so instead I will simply present the current state of things, and I hope
this to be sufficient:

How many udelay calls are there in 2.6.35-rc5?

udealy(ARG) >= | COUNT
1000 | 319
500 | 414
100 | 1146
20 | 1832

I am working on Android, so that is my focus for this. The following table
is a modified usleep that simply printk's the amount of time requested to
sleep; these tests were run on a kernel with udelay >= 20 --> usleep

"boot" is power-on to lock screen
"power collapse" is when the power button is pushed and the device suspends
"resume" is when the power button is pushed and the lock screen is displayed
(no touchscreen events or anything, just turning on the display)
"use device" is from the unlock swipe to clicking around a bit; there is no
sd card in this phone, so fail loading music, video, camera

ACTION | TOTAL NUMBER OF USLEEP CALLS | NET TIME (us)
boot | 22 | 1250
power-collapse | 9 | 1200
resume | 5 | 500
use device | 59 | 7700

The most interesting category to me is the "use device" field; 7700us of
busy-wait time that could be put towards better responsiveness, or at the
least less power usage.

Signed-off-by: Patrick Pannuto <[email protected]>
---
include/linux/delay.h | 1 +
kernel/timer.c | 22 ++++++++++++++++++++++
2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/include/linux/delay.h b/include/linux/delay.h
index fd832c6..a6ecb34 100644
--- a/include/linux/delay.h
+++ b/include/linux/delay.h
@@ -45,6 +45,7 @@ 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 ssleep(unsigned int seconds)
{
diff --git a/kernel/timer.c b/kernel/timer.c
index ee305c8..c2253dd 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1750,3 +1750,25 @@ unsigned long msleep_interruptible(unsigned int msecs)
}

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) * NSEC_PER_USEC;
+ 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);
--
1.7.2

2010-08-02 22:01:56

by Patrick Pannuto

[permalink] [raw]
Subject: [PATCH 4/4] Checkpatch: warn about unexpectedly long msleep's

As explained in Documentation/timers/timers-howto.txt, msleep's
of < 20ms may sleep for as long as 20ms. Caller's of
msleep(1) or msleep(2), etc are likely not to expect this
quirky behavior - warn them.

Signed-off-by: Patrick Pannuto <[email protected]>
---
scripts/checkpatch.pl | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1698c63..8536c33 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2578,6 +2578,13 @@ sub process {
}
}

+# warn about unexpectedly long msleep's
+ if ($line =~ /\bmsleep\s*\((\d+)\);/) {
+ if ($1 < 20) {
+ WARN("msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\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";
--
1.7.2

2010-08-03 22:52:13

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 1/4] timer: Added usleep_range timer

On Mon, 2 Aug 2010 15:01:04 -0700
Patrick Pannuto <[email protected]> wrote:

> usleep_range is a finer precision implementations of msleep
> and is designed to be a drop-in replacement for udelay where
> a precise sleep / busy-wait is unnecessary.

Thomas has merged an older version of this patch into his tree. He
doesn't appear to have merged the other three patches. I queued a
revert of the old patch to remove it from linux-next and I then queued
the other three.

Now I'll just sit here and see what happens.

2010-08-04 09:26:22

by Patrick Pannuto

[permalink] [raw]
Subject: [tip:timers/core] Documentation: Add timers/timers-howto.txt

Commit-ID: 0fcb80818bc3ade5befd409051089f710adcf7b0
Gitweb: http://git.kernel.org/tip/0fcb80818bc3ade5befd409051089f710adcf7b0
Author: Patrick Pannuto <[email protected]>
AuthorDate: Mon, 2 Aug 2010 15:01:05 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 4 Aug 2010 11:00:45 +0200

Documentation: Add timers/timers-howto.txt

This file seeks to explain the nuances in various delays;
many driver writers are not necessarily familiar with the
various kernel timers, their shortfalls, and quirks. When
faced with

ndelay, udelay, mdelay, usleep_range, msleep, and msleep_interrubtible

the question "How do I just wait 1 ms for my hardware to
latch?" has the non-intuitive "best" answer:
usleep_range(1000,1500)

This patch is followed by a series of checkpatch additions
that seek to help kernel hackers pick the best delay.

Signed-off-by: Patrick Pannuto <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Randy Dunlap <[email protected]>
Cc: Andrew Morton <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
Documentation/timers/timers-howto.txt | 105 +++++++++++++++++++++++++++++++++
1 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/Documentation/timers/timers-howto.txt b/Documentation/timers/timers-howto.txt
new file mode 100644
index 0000000..c9ef29d
--- /dev/null
+++ b/Documentation/timers/timers-howto.txt
@@ -0,0 +1,105 @@
+delays - Information on the various kernel delay / sleep mechanisms
+-------------------------------------------------------------------
+
+This document seeks to answer the common question: "What is the
+RightWay (TM) to insert a delay?"
+
+This question is most often faced by driver writers who have to
+deal with hardware delays and who may not be the most intimately
+familiar with the inner workings of the Linux Kernel.
+
+
+Inserting Delays
+----------------
+
+The first, and most important, question you need to ask is "Is my
+code in an atomic context?" This should be followed closely by "Does
+it really need to delay in atomic context?" If so...
+
+ATOMIC CONTEXT:
+ You must use the *delay family of functions. These
+ functions use the jiffie estimation of clock speed
+ and will busy wait for enough loop cycles to achieve
+ the desired delay:
+
+ ndelay(unsigned long nsecs)
+ udelay(unsigned long usecs)
+ mdelay(unsgined long msecs)
+
+ udelay is the generally preferred API; ndelay-level
+ precision may not actually exist on many non-PC devices.
+
+ mdelay is macro wrapper around udelay, to account for
+ possible overflow when passing large arguments to udelay.
+ In general, use of mdelay is discouraged and code should
+ be refactored to allow for the use of msleep.
+
+NON-ATOMIC CONTEXT:
+ You should use the *sleep[_range] family of functions.
+ There are a few more options here, while any of them may
+ work correctly, using the "right" sleep function will
+ help the scheduler, power management, and just make your
+ driver better :)
+
+ -- Backed by busy-wait loop:
+ udelay(unsigned long usecs)
+ -- Backed by hrtimers:
+ usleep_range(unsigned long min, unsigned long max)
+ -- Backed by jiffies / legacy_timers
+ msleep(unsigned long msecs)
+ msleep_interruptible(unsigned long msecs)
+
+ Unlike the *delay family, the underlying mechanism
+ driving each of these calls varies, thus there are
+ quirks you should be aware of.
+
+
+ SLEEPING FOR "A FEW" USECS ( < ~10us? ):
+ * Use udelay
+
+ - Why not usleep?
+ On slower systems, (embedded, OR perhaps a speed-
+ stepped PC!) the overhead of setting up the hrtimers
+ for usleep *may* not be worth it. Such an evaluation
+ will obviously depend on your specific situation, but
+ it is something to be aware of.
+
+ SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
+ * Use usleep_range
+
+ - Why not msleep for (1ms - 20ms)?
+ Explained originally here:
+ http://lkml.org/lkml/2007/8/3/250
+ msleep(1~20) may not do what the caller intends, and
+ will often sleep longer (~20 ms actual sleep for any
+ value given in the 1~20ms range). In many cases this
+ is not the desired behavior.
+
+ - Why is there no "usleep" / What is a good range?
+ Since usleep_range is built on top of hrtimers, the
+ wakeup will be very precise (ish), thus a simple
+ usleep function would likely introduce a large number
+ of undesired interrupts.
+
+ With the introduction of a range, the scheduler is
+ free to coalesce your wakeup with any other wakeup
+ that may have happened for other reasons, or at the
+ worst case, fire an interrupt for your upper bound.
+
+ The larger a range you supply, the greater a chance
+ that you will not trigger an interrupt; this should
+ be balanced with what is an acceptable upper bound on
+ delay / performance for your specific code path. Exact
+ tolerances here are very situation specific, thus it
+ is left to the caller to determine a reasonable range.
+
+ SLEEPING FOR LARGER MSECS ( 10ms+ )
+ * Use msleep or possibly msleep_interruptible
+
+ - What's the difference?
+ msleep sets the current task to TASK_UNINTERRUPTIBLE
+ whereas msleep_interruptible sets the current task to
+ TASK_INTERRUPTIBLE before scheduling the sleep. In
+ short, the difference is whether the sleep can be ended
+ early by a signal. In general, just use msleep unless
+ you know you have a need for the interruptible variant.

2010-08-04 09:26:43

by Patrick Pannuto

[permalink] [raw]
Subject: [tip:timers/core] Checkpatch: Prefer usleep_range over udelay

Commit-ID: 022d60db2ab6e97f93c98808d08fd883003048c7
Gitweb: http://git.kernel.org/tip/022d60db2ab6e97f93c98808d08fd883003048c7
Author: Patrick Pannuto <[email protected]>
AuthorDate: Mon, 2 Aug 2010 15:01:06 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 4 Aug 2010 11:00:45 +0200

Checkpatch: Prefer usleep_range over udelay

When possible, sleeping is (usually) better than delaying;
however, don't bother callers of udelay < 10us, as those
cases are generally not worth the switch to usleep

Signed-off-by: Patrick Pannuto <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
scripts/checkpatch.pl | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..1698c63 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2570,6 +2570,14 @@ sub process {
}
}

+# prefer usleep_range over udelay
+ if (($line =~ /\budelay\s*\(\s*(\w+)\s*\)/ {
+ # ignore udelay's < 10, however
+ if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
+ CHK("usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\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";

2010-08-04 09:27:01

by Patrick Pannuto

[permalink] [raw]
Subject: [tip:timers/core] Checkpatch: Warn about unexpectedly long msleep's

Commit-ID: d76db7e68cb9a886056a92c3f0fef56af5ca5ee8
Gitweb: http://git.kernel.org/tip/d76db7e68cb9a886056a92c3f0fef56af5ca5ee8
Author: Patrick Pannuto <[email protected]>
AuthorDate: Mon, 2 Aug 2010 15:01:07 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 4 Aug 2010 11:00:45 +0200

Checkpatch: Warn about unexpectedly long msleep's

As explained in Documentation/timers/timers-howto.txt, msleep's
of < 20ms may sleep for as long as 20ms. Caller's of
msleep(1) or msleep(2), etc are likely not to expect this
quirky behavior - warn them.

Signed-off-by: Patrick Pannuto <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Joe Perches <[email protected]>
Cc: Andrew Morton <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
scripts/checkpatch.pl | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1698c63..8536c33 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2578,6 +2578,13 @@ sub process {
}
}

+# warn about unexpectedly long msleep's
+ if ($line =~ /\bmsleep\s*\((\d+)\);/) {
+ if ($1 < 20) {
+ WARN("msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\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";

2010-08-04 23:23:08

by Patrick Pannuto

[permalink] [raw]
Subject: Re: [tip:timers/core] Checkpatch: Prefer usleep_range over udelay

On 08/04/2010 02:25 AM, tip-bot for Patrick Pannuto wrote:
> Commit-ID: 022d60db2ab6e97f93c98808d08fd883003048c7
> Gitweb: http://git.kernel.org/tip/022d60db2ab6e97f93c98808d08fd883003048c7
> Author: Patrick Pannuto <[email protected]>
> AuthorDate: Mon, 2 Aug 2010 15:01:06 -0700
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Wed, 4 Aug 2010 11:00:45 +0200
>
> Checkpatch: Prefer usleep_range over udelay
>
> When possible, sleeping is (usually) better than delaying;
> however, don't bother callers of udelay < 10us, as those
> cases are generally not worth the switch to usleep
>
> Signed-off-by: Patrick Pannuto <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Joe Perches <[email protected]>
> Cc: Andrew Morton <[email protected]>
> LKML-Reference: <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> scripts/checkpatch.pl | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd88f11..1698c63 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2570,6 +2570,14 @@ sub process {
> }
> }
>
> +# prefer usleep_range over udelay
> + if (($line =~ /\budelay\s*\(\s*(\w+)\s*\)/ {
> + # ignore udelay's < 10, however
> + if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
> + CHK("usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\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";


Andrew Morton's patch to fix the parentheses imbalance in the original
patch was missed (sorry for the initial error). This breaks checkpatch
in tip; could you pull in this too from -mm to tip?

Sorry for the extra trouble

-Pat


(FROM ANDREW):

The patch titled
checkpatch-prefer-usleep_range-over-udelay-fix
has been added to the -mm tree. Its filename is
checkpatch-prefer-usleep_range-over-udelay-fix.patch

Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: checkpatch-prefer-usleep_range-over-udelay-fix
From: Andrew Morton <[email protected]>

syntax error at scripts/checkpatch.pl line 2598, near "/\budelay\s*\(\s*(\w+)\s*\)/ {"

mismatched parentheses?

Cc: Ingo Molnar <[email protected]>
Cc: Patrick Pannuto <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---

scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN scripts/checkpatch.pl~checkpatch-prefer-usleep_range-over-udelay-fix scripts/checkpatch.pl
--- a/scripts/checkpatch.pl~checkpatch-prefer-usleep_range-over-udelay-fix
+++ a/scripts/checkpatch.pl
@@ -2595,7 +2595,7 @@ sub process {
}

# prefer usleep_range over udelay
- if (($line =~ /\budelay\s*\(\s*(\w+)\s*\)/ {
+ if ($line =~ /\budelay\s*\(\s*(\w+)\s*\)/) {
# ignore udelay's < 10, however
if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
CHK("usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\n" . $line);



--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum