[PATCH] checkpatch: Add warnings for use of mdelay()

[PATCH] checkpatch: Add warnings for use of mdelay()

[Israel Schlesinger]

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

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd88f11..b3f7f71 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -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";
-- 
1.7.0.2

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

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Joe Perches]

On Tue, 2010-07-27 at 10:11 -0700, Israel Schlesinger wrote:
> +		if ($line =~ /\bmdelay\(/) {
 
That should be:

		if ($line =~ /\bmdelay\s*\(/) {

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Jonathan Corbet]

On Tue, 27 Jul 2010 10:11:11 -0700
Israel Schlesinger <israels@codeaurora.org> wrote:

> 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

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Patrick Pannuto]

On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> On Tue, 27 Jul 2010 10:11:11 -0700
> Israel Schlesinger <israels@codeaurora.org> wrote:
> 
>> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> 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

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

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Andrew Morton]

On Tue, 27 Jul 2010 10:32:54 -0700
Patrick Pannuto <ppannuto@codeaurora.org> wrote:

> On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> > On Tue, 27 Jul 2010 10:11:11 -0700
> > Israel Schlesinger <israels@codeaurora.org> wrote:
> > 
> >> 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
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > 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 ;) )
> 

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

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 <ppannuto@codeaurora.org>
Acked-by: Arjan van de Ven <arjan@linux.intel.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/delay.h |    6 ++++++
 kernel/timer.c        |   22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff -puN include/linux/delay.h~timers-add-usleep-timer include/linux/delay.h
--- a/include/linux/delay.h~timers-add-usleep-timer
+++ a/include/linux/delay.h
@@ -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-timer kernel/timer.c
--- a/kernel/timer.c~timers-add-usleep-timer
+++ a/kernel/timer.c
@@ -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);
_

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Andy Whitcroft]

On Tue, Jul 27, 2010 at 12:16:10PM -0700, Andrew Morton wrote:
> On Tue, 27 Jul 2010 10:32:54 -0700
> Patrick Pannuto <ppannuto@codeaurora.org> wrote:
> 
> > On 07/27/2010 10:31 AM, Jonathan Corbet wrote:
> > > On Tue, 27 Jul 2010 10:11:11 -0700
> > > Israel Schlesinger <israels@codeaurora.org> wrote:
> > > 
> > >> 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
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > 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 ;) )
> > 
> 
> 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 <ppannuto@codeaurora.org>
> 
> 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 <ppannuto@codeaurora.org>
> Acked-by: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  include/linux/delay.h |    6 ++++++
>  kernel/timer.c        |   22 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff -puN include/linux/delay.h~timers-add-usleep-timer include/linux/delay.h
> --- a/include/linux/delay.h~timers-add-usleep-timer
> +++ a/include/linux/delay.h
> @@ -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-timer kernel/timer.c
> --- a/kernel/timer.c~timers-add-usleep-timer
> +++ a/kernel/timer.c
> @@ -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;

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;

> +	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);

-apw

[PATCH] checkpatch: Add warnings for use of mdelay()

[Prakruthi Deepak Heragu]

mdelay() is not a preferred API to be used to insert delay in the kernel
code unless the context is atomic. Instead, msleep() API can be used.
This patch introduces this warning.

Signed-off-by: Israel Schlesinger <israels@codeaurora.org>
Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
Signed-off-by: Prakruthi Deepak Heragu <pheragu@codeaurora.org>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9c0550..14bba3f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5572,6 +5572,12 @@ sub process {
 			     "Comparing get_jiffies_64() is almost always wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
 		}
 
+# check the patch for use of mdelay
+		if ($line =~ /\bmdelay\s*\(/) {
+			WARN("MDELAY",
+			     "use of mdelay() found: msleep() is the preferred API.\n" . $herecurr );
+		}
+
 # warn about #ifdefs in C files
 #		if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
 #			print "#ifdef in C files should be avoided\n";
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Joe Perches]

On Wed, 2018-07-04 at 11:18 -0700, Prakruthi Deepak Heragu wrote:
> mdelay() is not a preferred API to be used to insert delay in the kernel
> code unless the context is atomic. Instead, msleep() API can be used.
> This patch introduces this warning.

[]

> Signed-off-by: Israel Schlesinger <israels@codeaurora.org>
> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
> Signed-off-by: Prakruthi Deepak Heragu <pheragu@codeaurora.org>

Really? 3 sign-offs for one trivial patch while
getting simple whitespace issues wrong?

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5572,6 +5572,12 @@ sub process {
>  			     "Comparing get_jiffies_64() is almost always wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
>  		}
>  
> +# check the patch for use of mdelay
> +		if ($line =~ /\bmdelay\s*\(/) {
> +			WARN("MDELAY",
> +			     "use of mdelay() found: msleep() is the preferred API.\n" . $herecurr );

No space after $herecurr

> +		}
> +

NACK - I think this is unreasonable.

checkpatch is stupid and can only remain that way.

It cannot check for any particular use in atomic
that is appropriate and should not warn with a
false positive when the use is appropriate.

$ git grep -w mdelay | wc -l
2700

How many of those are correct?

If you want a check like this to be useful, write
something for coccinelle or smatch.

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Dan Carpenter]

Neither Smatch nor Coccinelle do a good job tracking when you're in
atomic context.  I've wanted to add this to Smatch but even then it
would be to warn that "We're holding a spinlock so we can't sleep".
It's trickier to say for sure when you're not holding a lock...

regards,
dan carpenter

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Julia Lawall]

On Thu, 5 Jul 2018, Dan Carpenter wrote:

> Neither Smatch nor Coccinelle do a good job tracking when you're in
> atomic context.  I've wanted to add this to Smatch but even then it
> would be to warn that "We're holding a spinlock so we can't sleep".
> It's trickier to say for sure when you're not holding a lock...

Jia-Ju Bai is working on this.  The tool is available on github.  It's
still being improved, though, so perhaps it's not yet ready for eg 0-day
inclusion.  He can give more details.

julia

Re: [PATCH] checkpatch: Add warnings for use of mdelay()

[Jia-Ju Bai]

On 2018/7/6 13:49, Julia Lawall wrote:
>
> On Thu, 5 Jul 2018, Dan Carpenter wrote:
>
>> Neither Smatch nor Coccinelle do a good job tracking when you're in
>> atomic context.  I've wanted to add this to Smatch but even then it
>> would be to warn that "We're holding a spinlock so we can't sleep".
>> It's trickier to say for sure when you're not holding a lock...
> Jia-Ju Bai is working on this.  The tool is available on github.  It's
> still being improved, though, so perhaps it's not yet ready for eg 0-day
> inclusion.  He can give more details.

Thanks for Julia's recommendation :)

I am doing the similar work with Julia, from the beginning of this year.
We develop two new LLVM-based tools to find two problems in the Linux 
kernel:
(1) Sleeping in atomic context. The tool is named DSAC.
(2) Using non-sleep function calls in non-atomic context. The tool is 
named DCNS.

We handle two common examples of atomic context:
(1) Holding a spinlock.
(2) In an interrupt handler.

DSAC and DCNS can basically work now, and some of the defects found by 
them have been confirmed and fixed in the Linux kernel.
But these tools are still being improved.

In fact, I encounter a hard problem when writing the tools, namely how 
to accurately and completely handle function pointer calls.
I have handled the function pointer in form of data structure field, but 
I do not find a good way to handle the function pointer that is used as 
a function argument.
Can someone give me good advice?

We also have made slides introducing DSAC and DCNS tools.
If you are interested in our work, I can send you the slides :)


Best wishes,
Jia-Ju Bai