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

[Anna-Maria Behnsen <anna-maria@linutronix.de>]

Frederic Weisbecker <frederic@kernel.org> writes:

> 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@canonical.com>
>> Cc: Joe Perches <joe@perches.com>
>> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
>> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
>> ---
>> 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?

I removed it accidentially... It was my plan to keep the min > max
static check.

I'll send a v4 for this patch.

Thanks,

	Anna-Maria