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

[Joe Perches]

On Wed, 2024-09-11 at 07:13 +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().
> 
> 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.

There is a runtime setup cost to use usleep_range.

I believe udelay should generally be used when there
is a specific microsecond time delay required.

> 
> - 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.

It likely won't "break", just perhaps be inefficient.

> - Check: ($1 < 20)
>   Message: "msleep < 20ms can sleep for up to 20ms;
>             see Documentation/timers/timers-howto.rst\n"
>   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. Update checkpatch documentation accordingly.
> 
> Cc: Andy Whitcroft <apw@xxxxxxxxxxxxx>
> Cc: Joe Perches <joe@xxxxxxxxxxx>
> Cc: Dwaipayan Ray <dwaipayanray1@xxxxxxxxx>
> Signed-off-by: Anna-Maria Behnsen <anna-maria@xxxxxxxxxxxxx>
> ---
> v2: Rephrase commit message
> ---
>  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 &&
>