Re: [PATCH v4 1/2] bitops: Define generic __bitrev8/16/32 for reuse

[Yury Norov <ynorov@nvidia.com>]

On Mon, Apr 20, 2026 at 09:12:51PM +0800, Jinjie Ruan wrote:
> Define generic __bitrev8/16/32 using the implementation
> in <linux/bitrev.h>, so they can be reused in <asm/bitrev.h>,
> such as RISCV.
> 
> And introduce a NEED_BYTE_REVERSE_TABLE Kconfig option, so byte_rev_table

Please split the move and new config option into different patches,
for bisectability.

> is only compiled when !HAVE_ARCH_BITREVERSE or when an architecture (like
> RISC-V) explicitly selects it as a fallback. This avoids bloating the .data
> section for architectures that have full hardware bit-reverse support and
> don't need the table.
> 
> Reviewed-by: Yury Norov <ynorov@nvidia.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

If I recall correctly, the option was suggested by
David. Maybe credit him with a tag?

> ---
>  include/asm-generic/bitops/__bitrev.h | 25 +++++++++++++++++++++++++
>  include/linux/bitrev.h                | 20 ++++----------------
>  lib/Kconfig                           |  4 ++++
>  lib/bitrev.c                          |  4 ++--
>  4 files changed, 35 insertions(+), 18 deletions(-)
>  create mode 100644 include/asm-generic/bitops/__bitrev.h
> 
> diff --git a/include/asm-generic/bitops/__bitrev.h b/include/asm-generic/bitops/__bitrev.h
> new file mode 100644
> index 000000000000..9d7d3d369364
> --- /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_NEED_BYTE_REVERSE_TABLE

I would rather say CONFIG_GENERIC_BITREVERSE.

> +#include <asm/types.h>
> +
> +extern u8 const byte_rev_table[256];
> +static __always_inline __attribute_const__ u8 generic___bitrev8(u8 byte)
> +{
> +	return byte_rev_table[byte];
> +}
> +
> +static __always_inline __attribute_const__ u16 generic___bitrev16(u16 x)
> +{
> +	return (generic___bitrev8(x & 0xff) << 8) | generic___bitrev8(x >> 8);
> +}
> +
> +static __always_inline __attribute_const__ u32 generic___bitrev32(u32 x)
> +{
> +	return (generic___bitrev16(x & 0xffff) << 16) | generic___bitrev16(x >> 16);
> +}
> +#endif /* CONFIG_NEED_BYTE_REVERSE_TABLE */
> +
> +#endif /* _ASM_GENERIC_BITOPS___BITREV_H_ */
> diff --git a/include/linux/bitrev.h b/include/linux/bitrev.h
> index d35b8ec1c485..11620a70e776 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
>  #endif /* CONFIG_HAVE_ARCH_BITREVERSE */
>  
>  #define __bitrev8x4(x)	(__bitrev32(swab32(x)))
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 0f2fb9610647..75cbb647b1da 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -62,6 +62,10 @@ config HAVE_ARCH_BITREVERSE
>  	  This option enables the use of hardware bit-reversal instructions on
>  	  architectures which support such operations.
>  
> +config NEED_BYTE_REVERSE_TABLE
> +	bool
> +	default y if !HAVE_ARCH_BITREVERSE

bool + default = def_bool

I don't need the table if I don't enable BITREVERSE. The bitrev.c
will not be compiled in that case, but .config will be polluted
with useless and misleading NEED_BYTE_REVERSE_TABLE option. So
altogether this should be:

 config GENERIC_BITREVERSE
 	def_bool !HAVE_ARCH_BITREVERSE
        depends on BITREVERSE

Can you add a help section, so that the others would understand why
arch and generic implementations are not mutually exclusive, and when
they need to select this?

> +
>  config ARCH_HAS_STRNCPY_FROM_USER
>  	bool
>  
> diff --git a/lib/bitrev.c b/lib/bitrev.c
> index 81b56e0a7f32..d9d5ee00229c 100644
> --- a/lib/bitrev.c
> +++ b/lib/bitrev.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -#ifndef CONFIG_HAVE_ARCH_BITREVERSE
> +#ifdef CONFIG_NEED_BYTE_REVERSE_TABLE
>  #include <linux/types.h>
>  #include <linux/module.h>
>  #include <linux/bitrev.h>
> @@ -44,4 +44,4 @@ const u8 byte_rev_table[256] = {
>  };
>  EXPORT_SYMBOL_GPL(byte_rev_table);
>  
> -#endif /* CONFIG_HAVE_ARCH_BITREVERSE */
> +#endif /* CONFIG_NEED_BYTE_REVERSE_TABLE */
> -- 
> 2.34.1