[PATCH 0/4 -tip] delay documentation and checkpatch additions

[PATCH 0/4 -tip] delay documentation and checkpatch additions

[Patrick Pannuto]

This patchset depends on:
commit 22b8f15c2f7130bb0386f548428df2ffd4e81903
Author: Patrick Pannuto <ppannuto@codeaurora.org>
Date:   Mon Jul 19 15:09:26 2010 -0700

    timer: Added usleep[_range] timer

on tip/master


The series first introduces some documentation explaining the various
linux delay mechanisms, and then adds some checkpatch rules to help
pick the most appropriate timer.

[PATCH 1/4] Documentation: Add timers/delays.txt

[Patrick Pannuto]

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, 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,2000)

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 <ppannuto@codeaurora.org>
---
 Documentation/timers/delays.txt |   97 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/timers/delays.txt

diff --git a/Documentation/timers/delays.txt b/Documentation/timers/delays.txt
new file mode 100644
index 0000000..12fcb7e
--- /dev/null
+++ b/Documentation/timers/delays.txt
@@ -0,0 +1,97 @@
+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 initimately
+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.
+
+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(unsigned long usecs)
+		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.
+
+		- usleep vs usleep_range:
+			Since usleep is built on top of high-resolution timers,
+			you will trigger an interrupt almost *exactly* when your
+			sleep expires; normally, sleeps (by their nature) do not
+			need this kind of precision. The *much* friendlier
+			usleep_range allows the kernel to complete your sleep
+			any time in the given range, likely when some other
+			interrupt has already woken up the kernel for some other
+			reason.
+
+	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_INTERRUBTIBLE 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 varient.
-- 
1.7.2

[PATCH 2/4] Checkpatch: prefer usleep_range over udelay

[Patrick Pannuto]

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

Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
---
 scripts/checkpatch.pl |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..e3625ac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2570,6 +2570,14 @@ sub process {
 			}
 		}
 
+# prefer usleep_range over udelay
+		if ($line =~ /\budelay\s*\((.+)\);/) {
+			# ignore udelay's < 10, however
+			if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
+				WARN("usleep_range is preferred over udelay; see Documentation/timers/delays.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

[PATCH 3/4] Checkpatch: prefer usleep_range over usleep

[Patrick Pannuto]

In (almost) every case, usleep_range is better than
usleep, as the precise (ish) wakeup - more accurately
the extra interrupt - from usleep is unnecessary.

usleep_range gives a much better chance of coalescing
processor wakeups.

Signed-off-by: Patrick Pannuto <ppannuto@codeaurora.org>
---
 scripts/checkpatch.pl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3625ac..0650ab9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2578,6 +2578,11 @@ sub process {
 			}
 		}
 
+# prefer usleep_range over usleep
+		if ($line =~ /\busleep\s*\(.+\);/) {
+			WARN("usleep_range is preferred over usleep; see Documentation/timers/delays.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

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

[Patrick Pannuto]

As explained in Documentation/timers/delays.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 <ppannuto@codeaurora.org>
---
 scripts/checkpatch.pl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0650ab9..bbc2e76 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2583,6 +2583,13 @@ sub process {
 			WARN("usleep_range is preferred over usleep; see Documentation/timers/delays.txt\n" . $line);
 		}
 
+# 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/delays.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

Re: [PATCH 2/4] Checkpatch: prefer usleep_range over udelay

[Joe Perches]

On Tue, 2010-07-27 at 15:39 -0700, Patrick Pannuto wrote:
> When possible, sleeping is (usually) better than delaying,
> however, don't bother callers of udelay < 10us, as those
> cases are not with the switch to usleep_range
> +# prefer usleep_range over udelay
> +		if ($line =~ /\budelay\s*\((.+)\);/) {
> +			# ignore udelay's < 10, however

This doesn't handle these cases:

	udelay(MY_DEFINED_DELAY)
	udelay( 100 )

Shouldn't this be:

		if (($line =~ /\budelay\s*\(\s*(\w+)\s*\)/ {
			WARN("usleep_range is preferred over udelay; see Documentation/timers/delays.txt\n" . $line);

Maybe these should be converted from WARN to CHK

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

[Patrick Pannuto]

As explained in Documentation/timers/delays.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 <ppannuto@codeaurora.org>
---
 scripts/checkpatch.pl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 892ae62..0ca2ea5 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/delays.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

Re: [PATCH 4/4] Checkpatch: warn about unexpectedly long msleep's

[Andrew Morton]

On Wed, 28 Jul 2010 12:33:07 -0700
Patrick Pannuto <ppannuto@codeaurora.org> wrote:

> As explained in Documentation/timers/delays.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 <ppannuto@codeaurora.org>
> ---
>  scripts/checkpatch.pl |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 892ae62..0ca2ea5 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/delays.txt\n" . $line);

Should be timers-howto.txt?

Re: [PATCH 4/4] Checkpatch: warn about unexpectedly long msleep's

[Patrick Pannuto]

>> +# 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/delays.txt\n" . $line);
> 
> Should be timers-howto.txt?
> 
> 

Yes, will fix

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

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

[Patrick Pannuto]

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 <ppannuto@codeaurora.org>
---
 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