Re: [PATCH v4 5/7] linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] log2: 32-bit 1UL shift overflow in roundup/down_pow_of_two64()
--

commit 43925e5ad2f800f4d000698aad5a8426cb02653d
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

linux/log2.h: Add 64-bit roundup/down_pow_of_two64() variants

This commit adds 64-bit roundup_pow_of_two64() and rounddown_pow_of_two64()
helpers in include/linux/log2.h so 64-bit inputs are not truncated to
unsigned long on 32-bit architectures.

> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index e17ceb32e0c97..180e5702f6779 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h

[ ... ]

> @@ -163,6 +183,23 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  	__ilog2_u64(n)			\
>   )
>  
> +/**
> + * roundup_pow_of_two64 - round the given value up to nearest power of two
> + * @n: parameter
> + *
> + * round the given value up to the nearest power of two
> + * - the result is undefined when n == 0
> + * - this can be used to initialise global variables from constant data
> + */
> +#define roundup_pow_of_two64(n)			\
> +(						\
> +	__builtin_constant_p(n) ? (		\
> +		((n) == 1) ? 1 :		\
> +		(1UL << (ilog2((n) - 1) + 1))	\
> +				   ) :		\
> +	__roundup_pow_of_two64(n)		\
> + )
> +
>  /**
>   * roundup_pow_of_two - round the given value up to nearest power of two
>   * @n: parameter
> @@ -180,6 +217,21 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  	__roundup_pow_of_two(n)			\
>   )
>  
> +/**
> + * rounddown_pow_of_two64 - round the given value down to nearest power of two
> + * @n: parameter
> + *
> + * round the given value down to the nearest power of two
> + * - the result is undefined when n == 0
> + * - this can be used to initialise global variables from constant data
> + */
> +#define rounddown_pow_of_two64(n)		\
> +(						\
> +	__builtin_constant_p(n) ? (		\
> +		(1UL << ilog2(n))) :		\
> +	__rounddown_pow_of_two64(n)		\
> + )

[Severity: Medium]
In include/linux/log2.h, __roundup_pow_of_two64() and
__rounddown_pow_of_two64() shift 1ULL, but the __builtin_constant_p(n)
branches in roundup_pow_of_two64() and rounddown_pow_of_two64() still shift
1UL:

	(1UL << (ilog2((n) - 1) + 1))

and:

	(1UL << ilog2(n))

On 32-bit architectures where unsigned long is 32 bits wide, the left shift
is evaluated in the type of its left operand (unsigned long) before the
conditional expression is converted to u64. When n is a compile-time constant
with n > (1ULL << 31) in roundup_pow_of_two64(n) or n >= (1ULL << 32) in
rounddown_pow_of_two64(n), the shift count is at least 32, which overflows
the 32-bit 1UL operand and triggers a shift-count-overflow warning while
truncating the result.

Should both __builtin_constant_p(n) branches use 1ULL instead of 1UL (and
1ULL for the (n) == 1 case in roundup_pow_of_two64())?

> +
>  /**
>   * rounddown_pow_of_two - round the given value down to nearest power of two
>   * @n: parameter

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021124956.4250-1-ilpo.jarvinen@linux.intel.com?part=5