Re: [PATCH v3 16/16] checkpatch: Remove broken sleep/delay related checks

[Frederic Weisbecker]

Le Mon, Oct 14, 2024 at 10:22:33AM +0200, Anna-Maria Behnsen a écrit :
> 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: 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>
> ---
> v3: Move it to the end of the queue and adapt it to the new patch which
>     removes the link to the outdated documentation before.
> v2: Rephrase commit message
> ---
>  Documentation/dev-tools/checkpatch.rst |  4 ----
>  scripts/checkpatch.pl                  | 38 ----------------------------------
>  2 files changed, 42 deletions(-)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index abb3ff682076..f5c27be9e673 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -466,10 +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.
> -
>  
>  Comments
>  --------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 98790fe5115d..34d4b5beda29 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6591,28 +6591,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 function description of usleep_range() and udelay().\n" . $herecurr);
> -			}
> -			if ($delay > 2000) {
> -				WARN("LONG_UDELAY",
> -				     "long udelay - prefer mdelay; see function description of mdelay().\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 function description of msleep().\n" . $herecurr);
> -			}
> -		}
> -
>  # check for comparisons of jiffies
>  		if ($line =~ /\bjiffies\s*$Compare|$Compare\s*jiffies\b/) {
>  			WARN("JIFFIES_COMPARISON",
> @@ -7069,22 +7047,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 function description of usleep_range().\n" . "$here\n$stat\n");
> -			} elsif ($min =~ /^\d+$/ && $max =~ /^\d+$/ &&
> -				 $min > $max) {
> -				WARN("USLEEP_RANGE",
> -				     "usleep_range args reversed, use min then max;  see function description of usleep_range().\n" . "$here\n$stat\n");

Why not keep the min > max static check?

Thanks.

> -			}
> -		}
> -
>  # check for naked sscanf
>  		if ($perl_version_ok &&
>  		    defined $stat &&
> 
> -- 
> 2.39.5
>