Re: [PATCH 3/6] bitops: Define generic __bitrev8/16/32 for reuse

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The #ifdef CONFIG_GENERIC_BITREVERSE guard in <asm-generic/bitops/__bitrev.h> introduces build regressions (implicit function declarations) for modules and uninstantiated inline functions.
--

commit d29d1db3f8ae8fed09b1d7cc283d610df9bcb8b6
Author: Jinjie Ruan <ruanjinjie@huawei.com>

bitops: Define generic __bitrev8/16/32 for reuse

This commit extracts the generic bit reversal functions into a new
<asm-generic/bitops/__bitrev.h> header, allowing architectures to reuse them.

> diff --git a/include/asm-generic/bitops/__bitrev.h b/include/asm-generic/bitops/__bitrev.h
> new file mode 100644
> index 0000000000000..f06af929678d6
> --- /dev/null
> +++ b/include/asm-generic/bitops/__bitrev.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_GENERIC_BITOPS___BITREV_H_
> +#define _ASM_GENERIC_BITOPS___BITREV_H_
> +
> +#ifdef CONFIG_GENERIC_BITREVERSE

Will this cause build regressions for modules when CONFIG_BITREVERSE=m?

If CONFIG_GENERIC_BITREVERSE is a bool depending on CONFIG_BITREVERSE, when
CONFIG_BITREVERSE=m, the bool evaluates to n. This causes
CONFIG_GENERIC_BITREVERSE to be n, meaning the generic___bitrev* declarations
are removed. Any module calling bitrev8() would then fail to compile due to an
implicit function declaration.

This also means that compiling bitrev.o based on CONFIG_GENERIC_BITREVERSE
prevents it from being built as a module.

> +#include <asm/types.h>
> +
> +extern u8 const byte_rev_table[256];
> +static __always_inline __attribute_const__ u8 generic___bitrev8(u8 byte)

[ ... ]

> diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
> index d35b8ec1c485c..11620a70e7762 100644
> --- a/include/linux/bitrev.h
> +++ b/include/linux/bitrev.h
> @@ -12,22 +12,10 @@
>  #define __bitrev8 __arch_bitrev8
>  
>  #else
> -extern u8 const byte_rev_table[256];
> -static inline u8 __bitrev8(u8 byte)
> -{
> -	return byte_rev_table[byte];
> -}
> -
> -static inline u16 __bitrev16(u16 x)
> -{
> -	return (__bitrev8(x & 0xff) << 8) | __bitrev8(x >> 8);
> -}
> -
> -static inline u32 __bitrev32(u32 x)
> -{
> -	return (__bitrev16(x & 0xffff) << 16) | __bitrev16(x >> 16);
> -}
> -
> +#include <asm-generic/bitops/__bitrev.h>
> +#define __bitrev32 generic___bitrev32
> +#define __bitrev16 generic___bitrev16
> +#define __bitrev8 generic___bitrev8

Does this introduce header fragility if CONFIG_BITREVERSE=n?

If CONFIG_BITREVERSE=n, the guard in <asm-generic/bitops/__bitrev.h> is
skipped, but __bitrev32 is still unconditionally mapped to generic___bitrev32
here.

Any C file including <linux/bitrev.h> that contains an uninstantiated static
inline function using bitrev32() will fail to compile with
-Werror=implicit-function-declaration. Previously, unconditional declarations
in <linux/bitrev.h> safely deferred such errors to link-time.

>  #endif /* CONFIG_HAVE_ARCH_BITREVERSE */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430211351.658193-1-ynorov@nvidia.com?part=3