Re: [PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Yury Norov <yury.norov@gmail.com>]

On Wed, Apr 09, 2025 at 11:43:44PM +0800, Kuan-Wei Chiu wrote:
> Redesign the parity8() helper as parity_odd(), changing its input type
> from u8 to u64 to support broader use cases and its return type from
> int to bool to clearly reflect the function's binary output. The
> function now returns true for odd parity and false for even parity,
> making its behavior more intuitive based on the name.
> 
> Also mark the function with __attribute_const__ to enable better
> compiler optimization, as the result depends solely on its input and
> has no side effects.
> 
> While more efficient implementations may exist, further optimization is
> postponed until a use case in performance-critical paths arises.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  arch/x86/kernel/bootflag.c               |  4 ++--
>  drivers/hwmon/spd5118.c                  |  2 +-
>  drivers/i3c/master/dw-i3c-master.c       |  2 +-
>  drivers/i3c/master/i3c-master-cdns.c     |  2 +-
>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
>  include/linux/bitops.h                   | 19 ++++++++++++-------
>  6 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> index 73274d76ce16..86aae4b2bfd5 100644
> --- a/arch/x86/kernel/bootflag.c
> +++ b/arch/x86/kernel/bootflag.c
> @@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
>  	unsigned long flags;
>  
>  	if (sbf_port != -1) {
> -		if (!parity8(v))
> +		if (!parity_odd(v))
>  			v ^= SBF_PARITY;
>  
>  		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
> @@ -57,7 +57,7 @@ static bool __init sbf_value_valid(u8 v)
>  {
>  	if (v & SBF_RESERVED)		/* Reserved bits */
>  		return false;
> -	if (!parity8(v))
> +	if (!parity_odd(v))
>  		return false;
>  
>  	return true;
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 358152868d96..15761f2ca4e9 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -298,7 +298,7 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
>   */
>  static bool spd5118_vendor_valid(u8 bank, u8 id)
>  {
> -	if (parity8(bank) == 0 || parity8(id) == 0)
> +	if (!parity_odd(bank) || !parity_odd(id))
>  		return false;
>  
>  	id &= 0x7f;
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 611c22b72c15..dc61d87fcd94 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -867,7 +867,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
>  		master->devs[pos].addr = ret;
>  		last_addr = ret;
>  
> -		ret |= parity8(ret) ? 0 : BIT(7);
> +		ret |= parity_odd(ret) ? 0 : BIT(7);
>  
>  		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
>  		       master->regs +
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index fd3752cea654..df14f978a388 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -889,7 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
>  	ret |= (addr & GENMASK(9, 7)) << 6;
>  
>  	/* RR0[0] = ~XOR(addr[6:0]) */
> -	ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
> +	ret |= parity_odd(addr & 0x7f) ? 0 : BIT(0);
>  
>  	return ret;
>  }
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> index 85c4916972e4..d692a299607d 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> @@ -114,7 +114,7 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
>  	dat_w0 = dat_w0_read(dat_idx);
>  	dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY);
>  	dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
> -		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
> +		  (parity_odd(address) ? 0 : DAT_0_DYNADDR_PARITY);
>  	dat_w0_write(dat_idx, dat_w0);
>  }
>  
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f..7c4c8afccef1 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -230,35 +230,40 @@ static inline int get_count_order_long(unsigned long l)
>  }
>  
>  /**
> - * parity8 - get the parity of an u8 value
> - * @value: the value to be examined
> + * parity_odd - get the parity of an u64 value
> + * @val: the value to be examined
>   *
> - * Determine the parity of the u8 argument.
> + * Determine the parity of the u64 argument.
>   *
>   * Returns:
> - * 0 for even parity, 1 for odd parity
> + * false for even parity, true for odd parity
>   *
>   * Note: This function informs you about the current parity. Example to bail
>   * out when parity is odd:
>   *
> - *	if (parity8(val) == 1)
> + *	if (parity_odd(val))
>   *		return -EBADMSG;
>   *
>   * If you need to calculate a parity bit, you need to draw the conclusion from
>   * this result yourself. Example to enforce odd parity, parity bit is bit 7:
>   *
> - *	if (parity8(val) == 0)
> + *	if (!parity_odd(val))
>   *		val ^= BIT(7);
>   */
> -static inline int parity8(u8 val)
> +#ifndef parity_odd

Please don't add this guard. We've got no any arch implementations
so far, and this is a dead code. Those adding arch code will also
add the ifdefery.

> +static inline __attribute_const__ bool parity_odd(u64 val)
>  {
>  	/*
>  	 * One explanation of this algorithm:
>  	 * https://funloop.org/codex/problem/parity/README.html
>  	 */
> +	val ^= val >> 32;
> +	val ^= val >> 16;
> +	val ^= val >> 8;
>  	val ^= val >> 4;
>  	return (0x6996 >> (val & 0xf)) & 1;
>  }
> +#endif
>  
>  /**
>   * __ffs64 - find first set bit in a 64 bit word
> -- 
> 2.34.1