linux-kernel - Re: [PATCH 06/15] timers: Update function descriptions of sleep/delay related functions

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87frqe60xj.ffs@tglx>
Date: Thu, 05 Sep 2024 08:59:20 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker
 <frederic@...nel.org>, Jonathan Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, Len Brown <len.brown@...el.com>, "Rafael
 J. Wysocki" <rafael@...nel.org>, Anna-Maria Behnsen
 <anna-maria@...utronix.de>, Arnd Bergmann <arnd@...db.de>,
 linux-arch@...r.kernel.org, Andrew Morton <akpm@...uxfoundation.org>,
 Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers
 <ndesaulniers@...gle.com>
Subject: Re: [PATCH 06/15] timers: Update function descriptions of
 sleep/delay related functions

On Wed, Sep 04 2024 at 15:04, Anna-Maria Behnsen wrote:
> +/**
> + * udelay - Inserting a delay based on microseconds with busy waiting
> + * @n:	requested delay in microseconds

....

> + * Impelementation details for udelay() only:

Implementation

> + * * The weird n/20000 thing suppresses a "comparison is always false due to
> + *   limited range of data type" warning with non-const 8-bit arguments.
> + * * 0x10c7 is 2**32 / 1000000 (rounded up)
>   */

That spello aside, I don't see how this information is interesting for
the user of udelay(). It's really a implementation detail and the user
does not care about this piece of art at all.

Though that made me look at this voodoo and the magic constants in
detail.  The division was added in a87e553fabe8 ("asm-generic: delay.h
fix udelay and ndelay for 8 bit args") to work around a compiler which
is upset about the comparision even when __builtin_constant_p(arg) is
false:

   warning: comparison is always false due to limited range of data type

The changelog is silent about the compiler version. I assume it's clang
because clang still complains on a plain (n) > 20000 when udelay() is
invoked with a u8 variable as argument:

   warning: result of comparison of constant 20000 with
            expression of type 'unsigned char' is always false
            [-Wtautological-constant-out-of-range-compare]

while gcc does not care.

The change log explains further that type casting 'n' in the comparison
does not cure it. Contemporary clang seems to be less stupid and

	if ((unsigned long)(n) >= 20000)			\

compiles just fine. Though assumed that some older clang version failed
and is still allowed to be used for compiling the kernel we have to work
around it.

However, instead of proliferating this voodoo can we please convert it
into something comprehensible?

/*
 * The microseconds delay multiplicator is used to convert a constant
 * microseconds value to a <INSERT COHERENT EXPLANATION>.
 */
#define UDELAY_CONST_MULT  ((unsigned long)DIV_ROUND_UP(1ULL << 32, USEC_PER_SEC))

/*
 * The maximum constant udelay value picked out of thin air
 * to avoid <INSERT COHERENT EXPLANATION>.
 */
#define UDELAY_CONST_MAX   20000

/**
 * udelay - .....
 */
static __always_inline void udelay(unsigned long usec)
{
        /*
	 * <INSERT COHERENT EXPLANATION> for this construct
         */
	if (__builtin_constant_p(usec)) {
		if (usec >= UDELAY_CONST_MAX)
			__bad_udelay();
		else
			__const_udelay(usec * UDELAY_CONST_MULT);
	} else {
		__udelay(usec);
	}
}

Both gcc and clang optimize this correctly with -O2. If there are
ancient compilers which fail to do so: *shrug*. 

> + * See udelay() for basic information about ndelay() and it's variants.
> + *
> + * Impelmentation details for ndelay():

vs.

> + * Impelementation details for udelay() only:

above. Can you please make your mind up which mis-spelled variant to
pick? :)

>  /**
>   * msleep_interruptible - sleep waiting for signals
> - * @msecs: Time in milliseconds to sleep for
> + * @msecs:	Requested sleep duration in milliseconds
> + *
> + * See msleep() for some basic information.
> + *
> + * The difference between msleep() and msleep_interruptible() is that the sleep
> + * could be interrupted by a signal delivery and then returns early.
> + *
> + * Returns the remaining time of the sleep duration transformed to msecs (see
> + * schedule_timeout() for details).

  Returns: The remaining ...

Thanks,

        tglx