[RFC PATCH 0/2] add generic parity calculation for u8

[RFC PATCH 0/2] add generic parity calculation for u8

[Wolfram Sang]

Here is a small series to provide generic parity calculation in the
kernel. There are already some open coded versions. I especially want to
avoid more of that in the I3C world, where odd parity is frequently
used. This series will allow for cleanups there. More detailed
motivation can be found in patch 1.

Based on 6.13-rc2. Build tested only, as I don't have SPD5118 HW.
Still RFC to discuss the placement in bitops.h and maybe Guenter can
test this series?

Looking forward to comments.


Wolfram Sang (2):
  bitops: add generic parity calculation for u8
  hwmon: (spd5118) Use generic parity calculation

 drivers/hwmon/spd5118.c |  8 +-------
 include/linux/bitops.h  | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.45.2

[RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Wolfram Sang]

There are multiple open coded implementations for getting the parity of
a byte in the kernel, even using different approaches. Take the pretty
efficient version from SPD5118 driver and make it generally available by
putting it into the bitops header. As long as there is just one parity
calculation helper, the creation of a distinct 'parity.h' header was
discarded. Also, the usage of hweight8() for architectures having a
popcnt instruction is postponed until a use case within hot paths is
desired. The motivation for this patch is the frequent use of odd parity
in the I3C specification and to simplify drivers there.

Changes compared to the original SPD5118 version are the addition of
kernel documentation, switching the return type from bool to int, and
renaming the argument of the function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index ba35bbf07798..4ed430934ffc 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -229,6 +229,37 @@ static inline int get_count_order_long(unsigned long l)
 	return (int)fls_long(--l);
 }
 
+/**
+ * get_parity8 - get the parity of an u8 value
+ * @value: the value to be examined
+ *
+ * Determine the parity of the u8 argument.
+ *
+ * Returns:
+ * 0 for even parity, 1 for odd parity
+ *
+ * Note: This function informs you about the current parity. Example to bail
+ * out when parity is odd:
+ *
+ *	if (get_parity8(val) == 1)
+ *		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 (get_parity8(val) == 0)
+ *		val |= BIT(7);
+ */
+static inline int get_parity8(u8 val)
+{
+	/*
+	 * One explanation of this algorithm:
+	 * https://funloop.org/codex/problem/parity/README.html
+	 */
+	val ^= val >> 4;
+	return (0x6996 >> (val & 0xf)) & 1;
+}
+
 /**
  * __ffs64 - find first set bit in a 64 bit word
  * @word: The 64 bit word
-- 
2.45.2

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Guenter Roeck]

On 12/14/24 00:58, Wolfram Sang wrote:
> There are multiple open coded implementations for getting the parity of
> a byte in the kernel, even using different approaches. Take the pretty
> efficient version from SPD5118 driver and make it generally available by
> putting it into the bitops header. As long as there is just one parity
> calculation helper, the creation of a distinct 'parity.h' header was
> discarded. Also, the usage of hweight8() for architectures having a
> popcnt instruction is postponed until a use case within hot paths is
> desired. The motivation for this patch is the frequent use of odd parity
> in the I3C specification and to simplify drivers there.
> 
> Changes compared to the original SPD5118 version are the addition of
> kernel documentation, switching the return type from bool to int, and
> renaming the argument of the function.
> 

Curious: Why not bool ?

Thanks,
Guenter

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Guenter Roeck]

On 12/14/24 06:19, Guenter Roeck wrote:
> On 12/14/24 00:58, Wolfram Sang wrote:
>> There are multiple open coded implementations for getting the parity of
>> a byte in the kernel, even using different approaches. Take the pretty
>> efficient version from SPD5118 driver and make it generally available by
>> putting it into the bitops header. As long as there is just one parity
>> calculation helper, the creation of a distinct 'parity.h' header was
>> discarded. Also, the usage of hweight8() for architectures having a
>> popcnt instruction is postponed until a use case within hot paths is
>> desired. The motivation for this patch is the frequent use of odd parity
>> in the I3C specification and to simplify drivers there.
>>
>> Changes compared to the original SPD5118 version are the addition of
>> kernel documentation, switching the return type from bool to int, and
>> renaming the argument of the function.
>>
> 
> Curious: Why not bool ?
> 

Never mind. It returns the parity, after all, not "parity is odd".

Thanks,
Guenter

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 138 bytes --]


> > Curious: Why not bool ?
> 
> Never mind. It returns the parity, after all, not "parity is odd".

Yes, this was my reasoning.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Guenter Roeck]

On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang wrote:
> There are multiple open coded implementations for getting the parity of
> a byte in the kernel, even using different approaches. Take the pretty
> efficient version from SPD5118 driver and make it generally available by
> putting it into the bitops header. As long as there is just one parity
> calculation helper, the creation of a distinct 'parity.h' header was
> discarded. Also, the usage of hweight8() for architectures having a
> popcnt instruction is postponed until a use case within hot paths is
> desired. The motivation for this patch is the frequent use of odd parity
> in the I3C specification and to simplify drivers there.
> 
> Changes compared to the original SPD5118 version are the addition of
> kernel documentation, switching the return type from bool to int, and
> renaming the argument of the function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index ba35bbf07798..4ed430934ffc 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -229,6 +229,37 @@ static inline int get_count_order_long(unsigned long l)
>  	return (int)fls_long(--l);
>  }
>  
> +/**
> + * get_parity8 - get the parity of an u8 value
> + * @value: the value to be examined
> + *
> + * Determine the parity of the u8 argument.
> + *
> + * Returns:
> + * 0 for even parity, 1 for odd parity
> + *
> + * Note: This function informs you about the current parity. Example to bail
> + * out when parity is odd:
> + *
> + *	if (get_parity8(val) == 1)
> + *		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 (get_parity8(val) == 0)
> + *		val |= BIT(7);
> + */
> +static inline int get_parity8(u8 val)
> +{
> +	/*
> +	 * One explanation of this algorithm:
> +	 * https://funloop.org/codex/problem/parity/README.html
> +	 */
> +	val ^= val >> 4;
> +	return (0x6996 >> (val & 0xf)) & 1;
> +}
> +
>  /**
>   * __ffs64 - find first set bit in a 64 bit word
>   * @word: The 64 bit word
> -- 
> 2.45.2
> 
> 

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Geert Uytterhoeven]

On Sat, Dec 14, 2024 at 9:59 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> There are multiple open coded implementations for getting the parity of
> a byte in the kernel, even using different approaches. Take the pretty
> efficient version from SPD5118 driver and make it generally available by
> putting it into the bitops header. As long as there is just one parity
> calculation helper, the creation of a distinct 'parity.h' header was
> discarded. Also, the usage of hweight8() for architectures having a
> popcnt instruction is postponed until a use case within hot paths is
> desired. The motivation for this patch is the frequent use of odd parity
> in the I3C specification and to simplify drivers there.
>
> Changes compared to the original SPD5118 version are the addition of
> kernel documentation, switching the return type from bool to int, and
> renaming the argument of the function.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Yury Norov]

On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang wrote:
> There are multiple open coded implementations for getting the parity of
> a byte in the kernel, even using different approaches. Take the pretty
> efficient version from SPD5118 driver and make it generally available by
> putting it into the bitops header. As long as there is just one parity
> calculation helper, the creation of a distinct 'parity.h' header was
> discarded. Also, the usage of hweight8() for architectures having a
> popcnt instruction is postponed until a use case within hot paths is
> desired. The motivation for this patch is the frequent use of odd parity
> in the I3C specification and to simplify drivers there.
> 
> Changes compared to the original SPD5118 version are the addition of
> kernel documentation, switching the return type from bool to int, and
> renaming the argument of the function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Yury Norov <yury.norov@gmail.com>

Would you like me to move this patch in bitmap-for-next?

> ---
>  include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index ba35bbf07798..4ed430934ffc 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -229,6 +229,37 @@ static inline int get_count_order_long(unsigned long l)
>  	return (int)fls_long(--l);
>  }
>  
> +/**
> + * get_parity8 - get the parity of an u8 value
> + * @value: the value to be examined
> + *
> + * Determine the parity of the u8 argument.
> + *
> + * Returns:
> + * 0 for even parity, 1 for odd parity
> + *
> + * Note: This function informs you about the current parity. Example to bail
> + * out when parity is odd:
> + *
> + *	if (get_parity8(val) == 1)
> + *		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 (get_parity8(val) == 0)
> + *		val |= BIT(7);
> + */
> +static inline int get_parity8(u8 val)
> +{
> +	/*
> +	 * One explanation of this algorithm:
> +	 * https://funloop.org/codex/problem/parity/README.html
> +	 */
> +	val ^= val >> 4;
> +	return (0x6996 >> (val & 0xf)) & 1;
> +}
> +
>  /**
>   * __ffs64 - find first set bit in a 64 bit word
>   * @word: The 64 bit word
> -- 
> 2.45.2

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Mon, Dec 16, 2024 at 06:24:43PM -0800, Yury Norov wrote:
> On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang wrote:
> > There are multiple open coded implementations for getting the parity of
> > a byte in the kernel, even using different approaches. Take the pretty
> > efficient version from SPD5118 driver and make it generally available by
> > putting it into the bitops header. As long as there is just one parity
> > calculation helper, the creation of a distinct 'parity.h' header was
> > discarded. Also, the usage of hweight8() for architectures having a
> > popcnt instruction is postponed until a use case within hot paths is
> > desired. The motivation for this patch is the frequent use of odd parity
> > in the I3C specification and to simplify drivers there.
> > 
> > Changes compared to the original SPD5118 version are the addition of
> > kernel documentation, switching the return type from bool to int, and
> > renaming the argument of the function.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Acked-by: Yury Norov <yury.norov@gmail.com>

Cool, thank you!

> Would you like me to move this patch in bitmap-for-next?

I hope that both patches can be applied in one go to avoid a dependency.
I'd think the hwmon-tree is a tad more suitable, but I am also fine with
bitmap as long as both patches go in. What do you maintainers thing?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]


> I hope that both patches can be applied in one go to avoid a dependency.
> I'd think the hwmon-tree is a tad more suitable, but I am also fine with
> bitmap as long as both patches go in. What do you maintainers thing?

Second thought, we can ask I3C to take it. Together with the cleanups
for the I3C drivers I would add then.

If this is not to your likings, then an immutable branch for I3C to pull
in would be helpful, though.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Guenter Roeck]

On 12/17/24 00:41, Wolfram Sang wrote:
> 
>> I hope that both patches can be applied in one go to avoid a dependency.
>> I'd think the hwmon-tree is a tad more suitable, but I am also fine with
>> bitmap as long as both patches go in. What do you maintainers thing?
> 
> Second thought, we can ask I3C to take it. Together with the cleanups
> for the I3C drivers I would add then.
> 
> If this is not to your likings, then an immutable branch for I3C to pull
> in would be helpful, though.
> 

Anything is fine with me. No need for an immutable branch from my perspective.

Guenter

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Kuan-Wei Chiu]

On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang wrote:
> There are multiple open coded implementations for getting the parity of
> a byte in the kernel, even using different approaches. Take the pretty
> efficient version from SPD5118 driver and make it generally available by
> putting it into the bitops header. As long as there is just one parity
> calculation helper, the creation of a distinct 'parity.h' header was
> discarded. Also, the usage of hweight8() for architectures having a
> popcnt instruction is postponed until a use case within hot paths is
> desired. The motivation for this patch is the frequent use of odd parity
> in the I3C specification and to simplify drivers there.
> 
> Changes compared to the original SPD5118 version are the addition of
> kernel documentation, switching the return type from bool to int, and
> renaming the argument of the function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Compare the results with hweight8(x) & 1 for correctness when x is in
the range [0, 255].

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>
Tested-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei

Re: [RFC PATCH 1/2] bitops: add generic parity calculation for u8

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 108 bytes --]


> Compare the results with hweight8(x) & 1 for correctness when x is in
> the range [0, 255].

Thank you!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 2/2] hwmon: (spd5118) Use generic parity calculation

[Wolfram Sang]

Make use of the new generic helper for calculating the parity.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/hwmon/spd5118.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 6cee48a3e5c3..07ab3a47b00c 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -291,12 +291,6 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
 	}
 }
 
-static inline bool spd5118_parity8(u8 w)
-{
-	w ^= w >> 4;
-	return (0x6996 >> (w & 0xf)) & 1;
-}
-
 /*
  * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
  * Vendor IDs 0 and 0x7f are invalid.
@@ -304,7 +298,7 @@ static inline bool spd5118_parity8(u8 w)
  */
 static bool spd5118_vendor_valid(u8 bank, u8 id)
 {
-	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
+	if (get_parity8(bank) == 0 || get_parity8(id) == 0)
 		return false;
 
 	id &= 0x7f;
-- 
2.45.2

Re: [RFC PATCH 2/2] hwmon: (spd5118) Use generic parity calculation

[Guenter Roeck]

On Sat, Dec 14, 2024 at 09:58:32AM +0100, Wolfram Sang wrote:
> Make use of the new generic helper for calculating the parity.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/hwmon/spd5118.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 6cee48a3e5c3..07ab3a47b00c 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -291,12 +291,6 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
>  	}
>  }
>  
> -static inline bool spd5118_parity8(u8 w)
> -{
> -	w ^= w >> 4;
> -	return (0x6996 >> (w & 0xf)) & 1;
> -}
> -
>  /*
>   * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
>   * Vendor IDs 0 and 0x7f are invalid.
> @@ -304,7 +298,7 @@ static inline bool spd5118_parity8(u8 w)
>   */
>  static bool spd5118_vendor_valid(u8 bank, u8 id)
>  {
> -	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
> +	if (get_parity8(bank) == 0 || get_parity8(id) == 0)
>  		return false;
>  
>  	id &= 0x7f;
> -- 
> 2.45.2
> 

Re: [RFC PATCH 2/2] hwmon: (spd5118) Use generic parity calculation

[Geert Uytterhoeven]

On Sat, Dec 14, 2024 at 9:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Make use of the new generic helper for calculating the parity.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/2] hwmon: (spd5118) Use generic parity calculation

[Kuan-Wei Chiu]

On Sat, Dec 14, 2024 at 09:58:32AM +0100, Wolfram Sang wrote:
> Make use of the new generic helper for calculating the parity.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Regards,
Kuan-Wei