| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ba245a06048af9eee61cea8c8fb79a331900cc73.camel@perches.com>
Date: Tue, 10 Sep 2024 23:44:11 -0700
From: Joe Perches <joe@...ches.com>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker
<frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Jonathan
Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, Len Brown <len.brown@...el.com>, "Rafael
J. Wysocki" <rafael@...nel.org>, Andy Whitcroft <apw@...onical.com>,
Dwaipayan Ray <dwaipayanray1@...il.com>
Subject: Re: [PATCH v2 10/15] checkpatch: Remove broken sleep/delay related
checks
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@...onical.com>
> Cc: Joe Perches <joe@...ches.com>
> Cc: Dwaipayan Ray <dwaipayanray1@...il.com>
> Signed-off-by: Anna-Maria Behnsen <anna-maria@...utronix.de>
> ---
> 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 &&
>
Powered by blists - more mailing lists