linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 01/31] bitops: add parity functions
@ 2016-03-24  3:03 Zhaoxiu Zeng
  2016-03-24  8:38 ` Denys Vlasenko
  0 siblings, 1 reply; 15+ messages in thread
From: Zhaoxiu Zeng @ 2016-03-24  3:03 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Morton, Martin Kepplinger, Sasha Levin,
	Denys Vlasenko, Ingo Molnar, Yury Norov
  Cc: linux-kernel, linux-arch, Zeng Zhaoxiu

From: Zeng Zhaoxiu <zhaoxiu.zeng@gmail.com>

When I do "grep parity -r linux", I found many parity calculations distributed in many drivers.
These patches provide generic and architecture-specific parity calculations.

Signed-off-by: Zeng Zhaoxiu <zhaoxiu.zeng@gmail.com>
---
 include/asm-generic/bitops.h              |  1 +
 include/asm-generic/bitops/arch_parity.h  | 39 +++++++++++++++++++++++++++++++
 include/asm-generic/bitops/const_parity.h | 36 ++++++++++++++++++++++++++++
 include/asm-generic/bitops/parity.h       |  7 ++++++
 include/linux/bitops.h                    |  5 ++++
 5 files changed, 88 insertions(+)
 create mode 100644 include/asm-generic/bitops/arch_parity.h
 create mode 100644 include/asm-generic/bitops/const_parity.h
 create mode 100644 include/asm-generic/bitops/parity.h

diff --git a/include/asm-generic/bitops.h b/include/asm-generic/bitops.h
index dcdcacf..d85722f 100644
--- a/include/asm-generic/bitops.h
+++ b/include/asm-generic/bitops.h
@@ -27,6 +27,7 @@
 #include <asm-generic/bitops/sched.h>
 #include <asm-generic/bitops/ffs.h>
 #include <asm-generic/bitops/hweight.h>
+#include <asm-generic/bitops/parity.h>
 #include <asm-generic/bitops/lock.h>
 
 #include <asm-generic/bitops/atomic.h>
diff --git a/include/asm-generic/bitops/arch_parity.h b/include/asm-generic/bitops/arch_parity.h
new file mode 100644
index 0000000..8d51eb3
--- /dev/null
+++ b/include/asm-generic/bitops/arch_parity.h
@@ -0,0 +1,39 @@
+#ifndef _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
+#define _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
+
+#include <asm/types.h>
+
+/*
+ * Refrence to 'https://graphics.stanford.edu/~seander/bithacks.html#ParityParallel'.
+ */
+
+static inline unsigned int __arch_parity4(unsigned int w)
+{
+	w &= 0xf;
+	return (0x6996 >> w) & 1;
+}
+
+static inline unsigned int __arch_parity8(unsigned int w)
+{
+	w ^= w >> 4;
+	return __arch_parity4(w);
+}
+
+static inline unsigned int __arch_parity16(unsigned int w)
+{
+	w ^= w >> 8;
+	return __arch_parity8(w);
+}
+
+static inline unsigned int __arch_parity32(unsigned int w)
+{
+	w ^= w >> 16;
+	return __arch_parity16(w);
+}
+
+static inline unsigned int __arch_parity64(__u64 w)
+{
+	return __arch_parity32((unsigned int)(w >> 32) ^ (unsigned int)w);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_ARCH_PARITY_H_ */
diff --git a/include/asm-generic/bitops/const_parity.h b/include/asm-generic/bitops/const_parity.h
new file mode 100644
index 0000000..9590315
--- /dev/null
+++ b/include/asm-generic/bitops/const_parity.h
@@ -0,0 +1,36 @@
+#ifndef _ASM_GENERIC_BITOPS_CONST_PARITY_H_
+#define _ASM_GENERIC_BITOPS_CONST_PARITY_H_
+
+/*
+ * Compile time versions of __arch_parityN()
+ */
+#define __const_parity4(w)   ((0x6996 >> ((w) & 0xf)) & 1)
+#define __const_parity8(w)   (__const_parity4(w ^ (w >> 4)))
+#define __const_parity16(w)  (__const_parity8(w ^ (w >> 8)))
+#define __const_parity32(w)  (__const_parity16(w ^ (w >> 16)))
+#define __const_parity64(w)  (__const_parity32(w ^ (w >> 32)))
+
+/*
+ * Generic interface.
+ */
+#define parity4(w)   (__builtin_constant_p(w) ? __const_parity4(w)  : __arch_parity4(w))
+#define parity8(w)   (__builtin_constant_p(w) ? __const_parity8(w)  : __arch_parity8(w))
+#define parity16(w)  (__builtin_constant_p(w) ? __const_parity16(w) : __arch_parity16(w))
+#define parity32(w)  (__builtin_constant_p(w) ? __const_parity32(w) : __arch_parity32(w))
+#define parity64(w)  (__builtin_constant_p(w) ? __const_parity64(w) : __arch_parity64(w))
+
+/*
+ * Interface for known constant arguments
+ */
+#define PARITY4(w)   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity4(w))
+#define PARITY8(w)   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity8(w))
+#define PARITY16(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity16(w))
+#define PARITY32(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity32(w))
+#define PARITY64(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity64(w))
+
+/*
+ * Type invariant interface to the compile time constant parity functions.
+ */
+#define PARITY(w)    PARITY64((u64)w)
+
+#endif /* _ASM_GENERIC_BITOPS_CONST_PARITY_H_ */
diff --git a/include/asm-generic/bitops/parity.h b/include/asm-generic/bitops/parity.h
new file mode 100644
index 0000000..a91dce7
--- /dev/null
+++ b/include/asm-generic/bitops/parity.h
@@ -0,0 +1,7 @@
+#ifndef _ASM_GENERIC_BITOPS_PARITY_H_
+#define _ASM_GENERIC_BITOPS_PARITY_H_
+
+#include <asm-generic/bitops/arch_parity.h>
+#include <asm-generic/bitops/const_parity.h>
+
+#endif /* _ASM_GENERIC_BITOPS_PARITY_H_ */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index defeaac..f91cb44 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -80,6 +80,11 @@ static __always_inline unsigned long hweight_long(unsigned long w)
 	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
 }
 
+static __always_inline unsigned int parity_long(unsigned long w)
+{
+	return sizeof(w) == 4 ? parity32(w) : parity64(w);
+}
+
 /**
  * rol64 - rotate a 64-bit value left
  * @word: value to rotate
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-24  3:03 [PATCH 01/31] bitops: add parity functions Zhaoxiu Zeng
@ 2016-03-24  8:38 ` Denys Vlasenko
  2016-03-24 22:28   ` Andrew Morton
  2016-03-27  3:33   ` zhaoxiu.zeng
  0 siblings, 2 replies; 15+ messages in thread
From: Denys Vlasenko @ 2016-03-24  8:38 UTC (permalink / raw)
  To: Zhaoxiu Zeng, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov
  Cc: linux-kernel, linux-arch

On 03/24/2016 04:03 AM, Zhaoxiu Zeng wrote:
> +/*
> + * Type invariant interface to the compile time constant parity functions.
> + */
> +#define PARITY(w)    PARITY64((u64)w)

Can result in incorrect expansion of w. Should be PARITY64((u64)(w))

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-24  8:38 ` Denys Vlasenko
@ 2016-03-24 22:28   ` Andrew Morton
  2016-03-26 22:08     ` Martin Kepplinger
  2016-03-27  3:33   ` zhaoxiu.zeng
  1 sibling, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2016-03-24 22:28 UTC (permalink / raw)
  To: Denys Vlasenko
  Cc: Zhaoxiu Zeng, Arnd Bergmann, Martin Kepplinger, Sasha Levin,
	Ingo Molnar, Yury Norov, linux-kernel, linux-arch

On Thu, 24 Mar 2016 09:38:21 +0100 Denys Vlasenko <dvlasenk@redhat.com> wrote:

> On 03/24/2016 04:03 AM, Zhaoxiu Zeng wrote:
> > +/*
> > + * Type invariant interface to the compile time constant parity functions.
> > + */
> > +#define PARITY(w)    PARITY64((u64)w)
> 
> Can result in incorrect expansion of w. Should be PARITY64((u64)(w))

And we seem to be missing the other 30 patches.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-24 22:28   ` Andrew Morton
@ 2016-03-26 22:08     ` Martin Kepplinger
  2016-03-27  7:51       ` zhaoxiu.zeng
  0 siblings, 1 reply; 15+ messages in thread
From: Martin Kepplinger @ 2016-03-26 22:08 UTC (permalink / raw)
  To: Andrew Morton, Denys Vlasenko
  Cc: Zhaoxiu Zeng, Arnd Bergmann, Sasha Levin, Ingo Molnar,
	Yury Norov, linux-kernel, linux-arch

We do.

Am 24. März 2016 23:28:15 MEZ, schrieb Andrew Morton <akpm@linux-foundation.org>:
>On Thu, 24 Mar 2016 09:38:21 +0100 Denys Vlasenko <dvlasenk@redhat.com>
>wrote:
>
>> On 03/24/2016 04:03 AM, Zhaoxiu Zeng wrote:
>> > +/*
>> > + * Type invariant interface to the compile time constant parity
>functions.
>> > + */
>> > +#define PARITY(w)    PARITY64((u64)w)
>> 
>> Can result in incorrect expansion of w. Should be PARITY64((u64)(w))
>
>And we seem to be missing the other 30 patches.

-- 
Martin Kepplinger
http://martinkepplinger.com
sent from mobile

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-24  8:38 ` Denys Vlasenko
  2016-03-24 22:28   ` Andrew Morton
@ 2016-03-27  3:33   ` zhaoxiu.zeng
  2016-03-27 12:44     ` Sam Ravnborg
  2016-03-28  6:51     ` Sam Ravnborg
  1 sibling, 2 replies; 15+ messages in thread
From: zhaoxiu.zeng @ 2016-03-27  3:33 UTC (permalink / raw)
  To: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov
  Cc: linux-kernel, linux-arch

On 2016/3/24 16:38, Denys Vlasenko wrote:
> On 03/24/2016 04:03 AM, Zhaoxiu Zeng wrote:
>> +/*
>> + * Type invariant interface to the compile time constant parity functions.
>> + */
>> +#define PARITY(w)    PARITY64((u64)w)
> 
> Can result in incorrect expansion of w. Should be PARITY64((u64)(w))
> 
> .
> 
Thanks. The new version has been fixed.


Signed-off-by: Zeng Zhaoxiu <zhaoxiu.zeng@gmail.com>
---
 include/asm-generic/bitops.h              |  1 +
 include/asm-generic/bitops/arch_parity.h  | 39 +++++++++++++++++++++++++++++++
 include/asm-generic/bitops/const_parity.h | 36 ++++++++++++++++++++++++++++
 include/asm-generic/bitops/parity.h       |  7 ++++++
 include/linux/bitops.h                    |  5 ++++
 5 files changed, 88 insertions(+)
 create mode 100644 include/asm-generic/bitops/arch_parity.h
 create mode 100644 include/asm-generic/bitops/const_parity.h
 create mode 100644 include/asm-generic/bitops/parity.h

diff --git a/include/asm-generic/bitops.h b/include/asm-generic/bitops.h
index dcdcacf..d85722f 100644
--- a/include/asm-generic/bitops.h
+++ b/include/asm-generic/bitops.h
@@ -27,6 +27,7 @@
 #include <asm-generic/bitops/sched.h>
 #include <asm-generic/bitops/ffs.h>
 #include <asm-generic/bitops/hweight.h>
+#include <asm-generic/bitops/parity.h>
 #include <asm-generic/bitops/lock.h>
 
 #include <asm-generic/bitops/atomic.h>
diff --git a/include/asm-generic/bitops/arch_parity.h b/include/asm-generic/bitops/arch_parity.h
new file mode 100644
index 0000000..cddc555
--- /dev/null
+++ b/include/asm-generic/bitops/arch_parity.h
@@ -0,0 +1,39 @@
+#ifndef _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
+#define _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
+
+#include <asm/types.h>
+
+/*
+ * Refrence to 'https://graphics.stanford.edu/~seander/bithacks.html#ParityParallel'.
+ */
+
+static inline unsigned int __arch_parity4(unsigned int w)
+{
+	w &= 0xf;
+	return (0x6996 >> w) & 1;
+}
+
+static inline unsigned int __arch_parity8(unsigned int w)
+{
+	w ^= w >> 4;
+	return __arch_parity4(w);
+}
+
+static inline unsigned int __arch_parity16(unsigned int w)
+{
+	w ^= w >> 8;
+	return __arch_parity8(w);
+}
+
+static inline unsigned int __arch_parity32(unsigned int w)
+{
+	w ^= w >> 16;
+	return __arch_parity16(w);
+}
+
+static inline unsigned int __arch_parity64(__u64 w)
+{
+	return __arch_parity32((unsigned int)(w >> 32) ^ (unsigned int)w);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_ARCH_PARITY_H_ */
diff --git a/include/asm-generic/bitops/const_parity.h b/include/asm-generic/bitops/const_parity.h
new file mode 100644
index 0000000..6af7987
--- /dev/null
+++ b/include/asm-generic/bitops/const_parity.h
@@ -0,0 +1,36 @@
+#ifndef _ASM_GENERIC_BITOPS_CONST_PARITY_H_
+#define _ASM_GENERIC_BITOPS_CONST_PARITY_H_
+
+/*
+ * Compile time versions of __arch_parityN()
+ */
+#define __const_parity4(w)   ((0x6996 >> ((w) & 0xf)) & 1)
+#define __const_parity8(w)   (__const_parity4((w) ^ ((w) >> 4)))
+#define __const_parity16(w)  (__const_parity8((w) ^ ((w) >> 8)))
+#define __const_parity32(w)  (__const_parity16((w) ^ ((w) >> 16)))
+#define __const_parity64(w)  (__const_parity32((w) ^ ((w) >> 32)))
+
+/*
+ * Generic interface.
+ */
+#define parity4(w)   (__builtin_constant_p(w) ? __const_parity4(w)  : __arch_parity4(w))
+#define parity8(w)   (__builtin_constant_p(w) ? __const_parity8(w)  : __arch_parity8(w))
+#define parity16(w)  (__builtin_constant_p(w) ? __const_parity16(w) : __arch_parity16(w))
+#define parity32(w)  (__builtin_constant_p(w) ? __const_parity32(w) : __arch_parity32(w))
+#define parity64(w)  (__builtin_constant_p(w) ? __const_parity64(w) : __arch_parity64(w))
+
+/*
+ * Interface for known constant arguments
+ */
+#define PARITY4(w)   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity4(w))
+#define PARITY8(w)   (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity8(w))
+#define PARITY16(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity16(w))
+#define PARITY32(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity32(w))
+#define PARITY64(w)  (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_parity64(w))
+
+/*
+ * Type invariant interface to the compile time constant parity functions.
+ */
+#define PARITY(w)    PARITY64((u64)(w))
+
+#endif /* _ASM_GENERIC_BITOPS_CONST_PARITY_H_ */
diff --git a/include/asm-generic/bitops/parity.h b/include/asm-generic/bitops/parity.h
new file mode 100644
index 0000000..a91dce7
--- /dev/null
+++ b/include/asm-generic/bitops/parity.h
@@ -0,0 +1,7 @@
+#ifndef _ASM_GENERIC_BITOPS_PARITY_H_
+#define _ASM_GENERIC_BITOPS_PARITY_H_
+
+#include <asm-generic/bitops/arch_parity.h>
+#include <asm-generic/bitops/const_parity.h>
+
+#endif /* _ASM_GENERIC_BITOPS_PARITY_H_ */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index defeaac..8952f88 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -80,6 +80,11 @@ static __always_inline unsigned long hweight_long(unsigned long w)
 	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
 }
 
+static __always_inline unsigned int parity_long(unsigned long w)
+{
+	return sizeof(w) == 4 ? parity32(w) : parity64(w);
+}
+
 /**
  * rol64 - rotate a 64-bit value left
  * @word: value to rotate
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-26 22:08     ` Martin Kepplinger
@ 2016-03-27  7:51       ` zhaoxiu.zeng
  0 siblings, 0 replies; 15+ messages in thread
From: zhaoxiu.zeng @ 2016-03-27  7:51 UTC (permalink / raw)
  To: Martin Kepplinger, Andrew Morton, Denys Vlasenko
  Cc: Arnd Bergmann, Sasha Levin, Ingo Molnar, Yury Norov,
	linux-kernel, linux-arch

On 2016/3/27 6:08, Martin Kepplinger wrote:
> We do.
> 
> Am 24. März 2016 23:28:15 MEZ, schrieb Andrew Morton <akpm@linux-foundation.org>:
>> On Thu, 24 Mar 2016 09:38:21 +0100 Denys Vlasenko <dvlasenk@redhat.com>
>> wrote:
>>
>>> On 03/24/2016 04:03 AM, Zhaoxiu Zeng wrote:
>>>> +/*
>>>> + * Type invariant interface to the compile time constant parity
>> functions.
>>>> + */
>>>> +#define PARITY(w)    PARITY64((u64)w)
>>>
>>> Can result in incorrect expansion of w. Should be PARITY64((u64)(w))
>>
>> And we seem to be missing the other 30 patches.
> 

Sorry,I got some problems with my gmail.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27  3:33   ` zhaoxiu.zeng
@ 2016-03-27 12:44     ` Sam Ravnborg
  2016-03-27 13:38       ` zhaoxiu.zeng
  2016-03-28  6:51     ` Sam Ravnborg
  1 sibling, 1 reply; 15+ messages in thread
From: Sam Ravnborg @ 2016-03-27 12:44 UTC (permalink / raw)
  To: zhaoxiu.zeng
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch

Hi Zeng.

Looking through the arch specific implementations of __arch_parity().
Some architectures uses #defines, other uses inline static functions.

Any particular reason that you select one approach over the other
in the different cases?

ia64:
+#define __arch_parity32(x) ((unsigned int) __arch_parity64((x) & 0xfffffffful))
+#define __arch_parity16(x) ((unsigned int) __arch_parity64((x) & 0xfffful))
+#define __arch_parity8(x)  ((unsigned int) __arch_parity64((x) & 0xfful))
+#define __arch_parity4(x)  ((unsigned int) __arch_parity64((x) & 0xful))

tile:
+static inline unsigned int __arch_parity32(unsigned int w)
+{
+	return __builtin_popcount(w) & 1;
+}
+
+static inline unsigned int __arch_parity16(unsigned int w)
+{
+	return __arch_parity32(w & 0xffff);
+}
+
+static inline unsigned int __arch_parity8(unsigned int w)
+{
+	return __arch_parity32(w & 0xff);
+}
+
+static inline unsigned int __arch_parity4(unsigned int w)
+{
+	return __arch_parity32(w & 0xf);
+}

Just two examples.

Adding the parity helpers seems like veny nice simplifications.

A few comments to some of those I looked at.
(I am not subscribed to lkml, so you get it as comments here)

[PATCH 21/31] mtd: use parity16 in ssfdc.c
The original code semes to check that the parity equals the
value of first bit in the address.
This seems lost after the conversion.

[PATCH 20/31] scsi: use parity32 in isci/phy.c
+	if (parity32(phy_cap.all))
 		phy_cap.parity = 1;
Could be written like this - simpler IMO:
	phy_cap.parity = parity32(phy_cap.all);


	Sam

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27 12:44     ` Sam Ravnborg
@ 2016-03-27 13:38       ` zhaoxiu.zeng
  2016-03-27 17:56         ` Sam Ravnborg
  2016-03-28  2:15         ` Zeng Zhaoxiu
  0 siblings, 2 replies; 15+ messages in thread
From: zhaoxiu.zeng @ 2016-03-27 13:38 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch

On 2016/3/27 20:44, Sam Ravnborg wrote:
> Hi Zeng.
> 
> Looking through the arch specific implementations of __arch_parity().
> Some architectures uses #defines, other uses inline static functions.
> 
> Any particular reason that you select one approach over the other
> in the different cases?
> 
> ia64:
> +#define __arch_parity32(x) ((unsigned int) __arch_parity64((x) & 0xfffffffful))
> +#define __arch_parity16(x) ((unsigned int) __arch_parity64((x) & 0xfffful))
> +#define __arch_parity8(x)  ((unsigned int) __arch_parity64((x) & 0xfful))
> +#define __arch_parity4(x)  ((unsigned int) __arch_parity64((x) & 0xful))
> 
> tile:
> +static inline unsigned int __arch_parity32(unsigned int w)
> +{
> +	return __builtin_popcount(w) & 1;
> +}
> +
> +static inline unsigned int __arch_parity16(unsigned int w)
> +{
> +	return __arch_parity32(w & 0xffff);
> +}
> +
> +static inline unsigned int __arch_parity8(unsigned int w)
> +{
> +	return __arch_parity32(w & 0xff);
> +}
> +
> +static inline unsigned int __arch_parity4(unsigned int w)
> +{
> +	return __arch_parity32(w & 0xf);
> +}
> 

No particular reason, just like the architecture's __arch_hweightN.

> Just two examples.
> 
> Adding the parity helpers seems like veny nice simplifications.
> 
> A few comments to some of those I looked at.
> (I am not subscribed to lkml, so you get it as comments here)
> 

I think the conversion is simple and readable.

> [PATCH 21/31] mtd: use parity16 in ssfdc.c
> The original code semes to check that the parity equals the
> value of first bit in the address.
> This seems lost after the conversion.
> 

The original get_parity return 1 if the number is even, so
if block_address is valid, "block_address & 0x7ff" must be odd.

> [PATCH 20/31] scsi: use parity32 in isci/phy.c
> +	if (parity32(phy_cap.all))
>  		phy_cap.parity = 1;
> Could be written like this - simpler IMO:
> 	phy_cap.parity = parity32(phy_cap.all);
> 
> 
> 	Sam
> 
Yes. Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27 13:38       ` zhaoxiu.zeng
@ 2016-03-27 17:56         ` Sam Ravnborg
  2016-03-28  2:44           ` Zeng Zhaoxiu
  2016-03-28  2:15         ` Zeng Zhaoxiu
  1 sibling, 1 reply; 15+ messages in thread
From: Sam Ravnborg @ 2016-03-27 17:56 UTC (permalink / raw)
  To: zhaoxiu.zeng
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch

> > Any particular reason that you select one approach over the other
> > in the different cases?
> 
> No particular reason, just like the architecture's __arch_hweightN.

The general recommendatiosn these days are to use static inline
for code to get better type check.
And it would also be nice to be consistent across architectures.

	Sam

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27 13:38       ` zhaoxiu.zeng
  2016-03-27 17:56         ` Sam Ravnborg
@ 2016-03-28  2:15         ` Zeng Zhaoxiu
  1 sibling, 0 replies; 15+ messages in thread
From: Zeng Zhaoxiu @ 2016-03-28  2:15 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch

On 2016年03月27日 21:38, zhaoxiu.zeng wrote:
> On 2016/3/27 20:44, Sam Ravnborg wrote:
>> Hi Zeng.
>>
>> Looking through the arch specific implementations of __arch_parity().
>> Some architectures uses #defines, other uses inline static functions.
>>
>> Any particular reason that you select one approach over the other
>> in the different cases?
>>
>> ia64:
>> +#define __arch_parity32(x) ((unsigned int) __arch_parity64((x) & 0xfffffffful))
>> +#define __arch_parity16(x) ((unsigned int) __arch_parity64((x) & 0xfffful))
>> +#define __arch_parity8(x)  ((unsigned int) __arch_parity64((x) & 0xfful))
>> +#define __arch_parity4(x)  ((unsigned int) __arch_parity64((x) & 0xful))
>>
>> tile:
>> +static inline unsigned int __arch_parity32(unsigned int w)
>> +{
>> +	return __builtin_popcount(w) & 1;
>> +}
>> +
>> +static inline unsigned int __arch_parity16(unsigned int w)
>> +{
>> +	return __arch_parity32(w & 0xffff);
>> +}
>> +
>> +static inline unsigned int __arch_parity8(unsigned int w)
>> +{
>> +	return __arch_parity32(w & 0xff);
>> +}
>> +
>> +static inline unsigned int __arch_parity4(unsigned int w)
>> +{
>> +	return __arch_parity32(w & 0xf);
>> +}
>>
> No particular reason, just like the architecture's __arch_hweightN.
>
>> Just two examples.
>>
>> Adding the parity helpers seems like veny nice simplifications.
>>
>> A few comments to some of those I looked at.
>> (I am not subscribed to lkml, so you get it as comments here)
>>
> I think the conversion is simple and readable.
>
>> [PATCH 21/31] mtd: use parity16 in ssfdc.c
>> The original code semes to check that the parity equals the
>> value of first bit in the address.
>> This seems lost after the conversion.
>>
> The original get_parity return 1 if the number is even, so
> if block_address is valid, "block_address & 0x7ff" must be odd.
Make corrections:

The original get_parity return 1 if hweight of the input number is even, so
if block_address is valid, hweight of "block_address & 0x7ff" must be odd.

>
>> [PATCH 20/31] scsi: use parity32 in isci/phy.c
>> +	if (parity32(phy_cap.all))
>>   		phy_cap.parity = 1;
>> Could be written like this - simpler IMO:
>> 	phy_cap.parity = parity32(phy_cap.all);
>>
>>
>> 	Sam
>>
> Yes. Thanks!
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27 17:56         ` Sam Ravnborg
@ 2016-03-28  2:44           ` Zeng Zhaoxiu
  0 siblings, 0 replies; 15+ messages in thread
From: Zeng Zhaoxiu @ 2016-03-28  2:44 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch

OK, I will do the V2 patches soon.
In addition, the best is to provide asm version parity functions for 
powerpc, sparc, and x86.

在 2016年03月28日 01:56, Sam Ravnborg 写道:
>>> Any particular reason that you select one approach over the other
>>> in the different cases?
>> No particular reason, just like the architecture's __arch_hweightN.
> The general recommendatiosn these days are to use static inline
> for code to get better type check.
> And it would also be nice to be consistent across architectures.
>
> 	Sam

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-27  3:33   ` zhaoxiu.zeng
  2016-03-27 12:44     ` Sam Ravnborg
@ 2016-03-28  6:51     ` Sam Ravnborg
  2016-03-29  2:27       ` Zeng Zhaoxiu
  1 sibling, 1 reply; 15+ messages in thread
From: Sam Ravnborg @ 2016-03-28  6:51 UTC (permalink / raw)
  To: zhaoxiu.zeng
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch,
	David S. Miller

> diff --git a/include/asm-generic/bitops/arch_parity.h b/include/asm-generic/bitops/arch_parity.h
> new file mode 100644
> index 0000000..cddc555
> --- /dev/null
> +++ b/include/asm-generic/bitops/arch_parity.h
> @@ -0,0 +1,39 @@
> +#ifndef _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
> +#define _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
> +
> +#include <asm/types.h>
> +
> +/*
> + * Refrence to 'https://graphics.stanford.edu/~seander/bithacks.html#ParityParallel'.
> + */
> +
> +static inline unsigned int __arch_parity4(unsigned int w)
> +{
> +	w &= 0xf;
> +	return (0x6996 >> w) & 1;
> +}
> +
> +static inline unsigned int __arch_parity8(unsigned int w)
> +{
> +	w ^= w >> 4;
> +	return __arch_parity4(w);
> +}
> +
> +static inline unsigned int __arch_parity16(unsigned int w)
> +{
> +	w ^= w >> 8;
> +	return __arch_parity8(w);
> +}
> +
> +static inline unsigned int __arch_parity32(unsigned int w)
> +{
> +	w ^= w >> 16;
> +	return __arch_parity16(w);
> +}
> +
> +static inline unsigned int __arch_parity64(__u64 w)
> +{
> +	return __arch_parity32((unsigned int)(w >> 32) ^ (unsigned int)w);
> +}
Defining these as static inlines in asm-generic prevent an architecture
from selecting between a more optimal asm version or the generic version
at run-time.
sparc would benefit from this as only some sparc chips supports popc.
See how this is done for hweight*

	Sam

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-28  6:51     ` Sam Ravnborg
@ 2016-03-29  2:27       ` Zeng Zhaoxiu
  2016-03-29  2:56         ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Zeng Zhaoxiu @ 2016-03-29  2:27 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch,
	David S. Miller

在 2016年03月28日 14:51, Sam Ravnborg 写道:
>> diff --git a/include/asm-generic/bitops/arch_parity.h b/include/asm-generic/bitops/arch_parity.h
>> new file mode 100644
>> index 0000000..cddc555
>> --- /dev/null
>> +++ b/include/asm-generic/bitops/arch_parity.h
>> @@ -0,0 +1,39 @@
>> +#ifndef _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
>> +#define _ASM_GENERIC_BITOPS_ARCH_PARITY_H_
>> +
>> +#include <asm/types.h>
>> +
>> +/*
>> + * Refrence to 'https://graphics.stanford.edu/~seander/bithacks.html#ParityParallel'.
>> + */
>> +
>> +static inline unsigned int __arch_parity4(unsigned int w)
>> +{
>> +	w &= 0xf;
>> +	return (0x6996 >> w) & 1;
>> +}
>> +
>> +static inline unsigned int __arch_parity8(unsigned int w)
>> +{
>> +	w ^= w >> 4;
>> +	return __arch_parity4(w);
>> +}
>> +
>> +static inline unsigned int __arch_parity16(unsigned int w)
>> +{
>> +	w ^= w >> 8;
>> +	return __arch_parity8(w);
>> +}
>> +
>> +static inline unsigned int __arch_parity32(unsigned int w)
>> +{
>> +	w ^= w >> 16;
>> +	return __arch_parity16(w);
>> +}
>> +
>> +static inline unsigned int __arch_parity64(__u64 w)
>> +{
>> +	return __arch_parity32((unsigned int)(w >> 32) ^ (unsigned int)w);
>> +}
> Defining these as static inlines in asm-generic prevent an architecture
> from selecting between a more optimal asm version or the generic version
> at run-time.
> sparc would benefit from this as only some sparc chips supports popc.
> See how this is done for hweight*
>
> 	Sam

Thanks. I will try.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-29  2:27       ` Zeng Zhaoxiu
@ 2016-03-29  2:56         ` Joe Perches
  2016-03-29  2:56           ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2016-03-29  2:56 UTC (permalink / raw)
  To: Zeng Zhaoxiu, Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch,
	David S. Miller

On Tue, 2016-03-29 at 10:27 +0800, Zeng Zhaoxiu wrote:
> 在 2016年03月28日 14:51, Sam Ravnborg 写道:
[]
> > Defining these as static inlines in asm-generic prevent an
> > architecture
> > from selecting between a more optimal asm version or the generic version
> > at run-time.
> > sparc would benefit from this as only some sparc chips supports popc.
> > See how this is done for hweight*
> > 
> > 	Sam
> Thanks. I will try.

You might also try to describe in any commit message
and perhaps the internal documentation why using gcc's
__builtin_parity isn't appropriate.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 01/31] bitops: add parity functions
  2016-03-29  2:56         ` Joe Perches
@ 2016-03-29  2:56           ` Joe Perches
  0 siblings, 0 replies; 15+ messages in thread
From: Joe Perches @ 2016-03-29  2:56 UTC (permalink / raw)
  To: Zeng Zhaoxiu, Sam Ravnborg
  Cc: Denys Vlasenko, Arnd Bergmann, Andrew Morton, Martin Kepplinger,
	Sasha Levin, Ingo Molnar, Yury Norov, linux-kernel, linux-arch,
	David S. Miller

On Tue, 2016-03-29 at 10:27 +0800, Zeng Zhaoxiu wrote:
> 在 2016年03月28日 14:51, Sam Ravnborg 写道:
[]
> > Defining these as static inlines in asm-generic prevent an
> > architecture
> > from selecting between a more optimal asm version or the generic version
> > at run-time.
> > sparc would benefit from this as only some sparc chips supports popc.
> > See how this is done for hweight*
> > 
> > 	Sam
> Thanks. I will try.

You might also try to describe in any commit message
and perhaps the internal documentation why using gcc's
__builtin_parity isn't appropriate.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-03-29  3:01 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-24  3:03 [PATCH 01/31] bitops: add parity functions Zhaoxiu Zeng
2016-03-24  8:38 ` Denys Vlasenko
2016-03-24 22:28   ` Andrew Morton
2016-03-26 22:08     ` Martin Kepplinger
2016-03-27  7:51       ` zhaoxiu.zeng
2016-03-27  3:33   ` zhaoxiu.zeng
2016-03-27 12:44     ` Sam Ravnborg
2016-03-27 13:38       ` zhaoxiu.zeng
2016-03-27 17:56         ` Sam Ravnborg
2016-03-28  2:44           ` Zeng Zhaoxiu
2016-03-28  2:15         ` Zeng Zhaoxiu
2016-03-28  6:51     ` Sam Ravnborg
2016-03-29  2:27       ` Zeng Zhaoxiu
2016-03-29  2:56         ` Joe Perches
2016-03-29  2:56           ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).