[PATCH 10/15] checkpatch: Remove broken sleep/delay related checks

Anna-Maria Behnsen posted 15 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH 10/15] checkpatch: Remove broken sleep/delay related checks
Posted by Anna-Maria Behnsen 1 year, 3 months ago
checkpatch.pl checks for several things related to sleep and delay
functions. In all warnings the outdated documentation is referenced. All
broken parts are listed one by one in the following with an explanation why
this check is broken. For a basic background of those functions please also
refere to the updated function descriptions of udelay(), nsleep_range() and
msleep().

Be aware: The change is done with a perl knowledge of the level "I'm able
to spell perl".

The following checks are broken:

- Check: (! ($delay < 10) )
  Message: "usleep_range is preferred over udelay;
            see Documentation/timers/timers-howto.rst\n"
  Why is the check broken: When it is an atomic context, udelay() is
                           mandatory.

- Check: ($min eq $max)
  Message:  "usleep_range should not use min == max args;
             see Documentation/timers/timers-howto.rst\n"
  Why is the check broken: When the requested accuracy for the sleep
                           duration requires it, it is also valid to use
                           min == max.

- Check: ($delay > 2000)
  Message: "long udelay - prefer mdelay;
            see arch/arm/include/asm/delay.h\n"
  Why is the check broken: The threshold when to start using mdelay() to
                           prevent an overflow depends on
                           MAX_UDELAY_MS. This value is architecture
                           dependent. The used value for the check and
                           reference is arm specific. Generic would be 5ms,
                           but this would "break" arm, loongarch and mips
                           and also the arm value might "break" mips and
                           loongarch in some configurations.

- Check: ($1 < 20)
  Message: "msleep < 20ms can sleep for up to 20ms;
            see Documentation/timers/timers-howto.rst\n"
  Why is the check broken: The message is simply wrong. msleep() will not
                           sleep (and never did it before) for at least
                           20ms. This threshold cannot be hard coded as it
                           depends on the required accuracy and also on HZ
                           (jiffie granularity and timer wheel bucket/level
                           granularity). See msleep() and also the
                           USLEEP_RANGE_UPPER_BOUND value.

Remove all broken checks. Remove also no longer required checkpatch
documentation section about USLEEP_RANGE.

Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
 Documentation/dev-tools/checkpatch.rst |  6 ------
 scripts/checkpatch.pl                  | 34 ----------------------------------
 2 files changed, 40 deletions(-)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index a9fac978a525..f5c27be9e673 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -466,12 +466,6 @@ API usage
   **UAPI_INCLUDE**
     No #include statements in include/uapi should use a uapi/ path.
 
-  **USLEEP_RANGE**
-    usleep_range() should be preferred over udelay(). The proper way of
-    using usleep_range() is mentioned in the kernel docs.
-
-    See: https://www.kernel.org/doc/html/latest/timers/timers-howto.html#delays-information-on-the-various-kernel-delay-sleep-mechanisms
-
 
 Comments
 --------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ba3359bdd1fa..80497da4aaac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6601,28 +6601,6 @@ sub process {
 			}
 		}
 
-# prefer usleep_range over udelay
-		if ($line =~ /\budelay\s*\(\s*(\d+)\s*\)/) {
-			my $delay = $1;
-			# ignore udelay's < 10, however
-			if (! ($delay < 10) ) {
-				CHK("USLEEP_RANGE",
-				    "usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst\n" . $herecurr);
-			}
-			if ($delay > 2000) {
-				WARN("LONG_UDELAY",
-				     "long udelay - prefer mdelay; see arch/arm/include/asm/delay.h\n" . $herecurr);
-			}
-		}
-
-# warn about unexpectedly long msleep's
-		if ($line =~ /\bmsleep\s*\((\d+)\);/) {
-			if ($1 < 20) {
-				WARN("MSLEEP",
-				     "msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst\n" . $herecurr);
-			}
-		}
-
 # check for comparisons of jiffies
 		if ($line =~ /\bjiffies\s*$Compare|$Compare\s*jiffies\b/) {
 			WARN("JIFFIES_COMPARISON",
@@ -7079,18 +7057,6 @@ sub process {
 			}
 		}
 
-# check usleep_range arguments
-		if ($perl_version_ok &&
-		    defined $stat &&
-		    $stat =~ /^\+(?:.*?)\busleep_range\s*\(\s*($FuncArg)\s*,\s*($FuncArg)\s*\)/) {
-			my $min = $1;
-			my $max = $7;
-			if ($min eq $max) {
-				WARN("USLEEP_RANGE",
-				     "usleep_range should not use min == max args; see Documentation/timers/timers-howto.rst\n" . "$here\n$stat\n");
-			}
-		}
-
 # check for naked sscanf
 		if ($perl_version_ok &&
 		    defined $stat &&

-- 
2.39.2
Re: [PATCH 10/15] checkpatch: Remove broken sleep/delay related checks
Posted by Joe Perches 1 year, 3 months ago
On Wed, 2024-09-04 at 15:05 +0200, Anna-Maria Behnsen wrote:
> checkpatch.pl checks for several things related to sleep and delay
> functions. In all warnings the outdated documentation is referenced. All
> broken parts are listed one by one in the following with an explanation why
> this check is broken. For a basic background of those functions please also
> refere to the updated function descriptions of udelay(), nsleep_range() and
> msleep().
[]
> - Check: ($1 < 20)
>   Message: "msleep < 20ms can sleep for up to 20ms;
>             see Documentation/timers/timers-howto.rst\n"
>   Why is the check broken: The message is simply wrong. msleep() will not
>                            sleep (and never did it before)

While it might have changed, the "never did it before" is
not correct.

https://lore.kernel.org/all/15327.1186166232@lwn.net/

> Remove all broken checks. Remove also no longer required checkpatch
> documentation section about USLEEP_RANGE.

It'd be useful to remove the appropriate now unnecessary
verbose sections from Documentation/dev-tools/checkpatch.rst
Re: [PATCH 10/15] checkpatch: Remove broken sleep/delay related checks
Posted by Anna-Maria Behnsen 1 year, 3 months ago
Joe Perches <joe@perches.com> writes:

> On Wed, 2024-09-04 at 15:05 +0200, Anna-Maria Behnsen wrote:
>> checkpatch.pl checks for several things related to sleep and delay
>> functions. In all warnings the outdated documentation is referenced. All
>> broken parts are listed one by one in the following with an explanation why
>> this check is broken. For a basic background of those functions please also
>> refere to the updated function descriptions of udelay(), nsleep_range() and
>> msleep().
> []
>> - Check: ($1 < 20)
>>   Message: "msleep < 20ms can sleep for up to 20ms;
>>             see Documentation/timers/timers-howto.rst\n"
>>   Why is the check broken: The message is simply wrong. msleep() will not
>>                            sleep (and never did it before)
>
> While it might have changed, the "never did it before" is
> not correct.
>
> https://lore.kernel.org/all/15327.1186166232@lwn.net/

Thanks for the fast reply!

Yes you are right. I wasn't able to read properly and had in my mind
that it says that msleep will sleep at least 20ms when I was writing
this "never did it before" - my fault.

The point I want to make here is that those 20ms cannot be hard coded,
as this is HZ dependent (same thing as it was before). I will rephrase
it to:

      Why is the check broken: msleep(1) might sleep up to 20ms but only
                               on a HZ=100 system. On a HZ=1000 system
                               this will be 2ms. This means, the
                               threshold cannot be hard coded as it
                               depends on HZ (jiffy granularity and
                               timer wheel bucket/level granularity) and
                               also on the required accuracy of the
                               callsite. See msleep() and also the
                               USLEEP_RANGE_UPPER_BOUND value.

>> Remove all broken checks. Remove also no longer required checkpatch
>> documentation section about USLEEP_RANGE.
>
> It'd be useful to remove the appropriate now unnecessary
> verbose sections from Documentation/dev-tools/checkpatch.rst

Maybe my coffee does not do it's job yet - which part should I remove? I
already remove the USLEEP_RANGE part. For MSLEEP and LONG_UDELAY there
wasn't anything documented.

Or should I rephrase the commit message somehow?

Thanks,

	Anna-Maria