diff mbox series

[2/5] include: add setbits32/clrbits32/clrsetbits32/setbits64/clrbits64/clrsetbits64 in linux/setbits.h

Message ID 1536349307-20714-3-git-send-email-clabbe@baylibre.com (mailing list archive)
State Not Applicable
Headers show
Series introduce setbits32/clrbits32/clrsetbits32/setbits64/clrbits64/clrsetbits64 functions | expand

Commit Message

Corentin LABBE Sept. 7, 2018, 7:41 p.m. UTC 
This patch adds setbits32/clrbits32/clrsetbits32 and
setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 include/linux/setbits.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 include/linux/setbits.h

Comments

Crystal Wood Sept. 7, 2018, 8 p.m. UTC | #1 
On Fri, 2018-09-07 at 19:41 +0000, Corentin Labbe wrote:
> This patch adds setbits32/clrbits32/clrsetbits32 and
> setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  include/linux/setbits.h | 55
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 include/linux/setbits.h
> 
> diff --git a/include/linux/setbits.h b/include/linux/setbits.h
> new file mode 100644
> index 000000000000..3e1e273551bb
> --- /dev/null
> +++ b/include/linux/setbits.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_SETBITS_H
> +#define __LINUX_SETBITS_H
> +
> +#include <linux/io.h>
> +
> +#define __setbits(readfunction, writefunction, addr, set) \
> +	writefunction((readfunction(addr) | (set)), addr)
> +#define __clrbits(readfunction, writefunction, addr, mask) \
> +	writefunction((readfunction(addr) & ~(mask)), addr)
> +#define __clrsetbits(readfunction, writefunction, addr, mask, set) \
> +	writefunction(((readfunction(addr) & ~(mask)) | (set)), addr)
> +#define __setclrbits(readfunction, writefunction, addr, mask, set) \
> +	writefunction(((readfunction(addr) | (seti)) & ~(mask)), addr)
> +
> +#define setbits32(addr, set) __setbits(readl, writel, addr, set)
> +#define setbits32_relaxed(addr, set) __setbits(readl_relaxed,
> writel_relaxed, \
> +					       addr, set)
> +
> +#define clrbits32(addr, mask) __clrbits(readl, writel, addr, mask)
> +#define clrbits32_relaxed(addr, mask) __clrbits(readl_relaxed,
> writel_relaxed, \
> +						addr, mask)

So now setbits32/clrbits32 is implicitly little-endian?  Introducing new
implicit-endian accessors is probably a bad thing in general, but doing it
with a name that until this patchset was implicitly big-endian (at least on
powerpc) seems worse.  Why not setbits32_le()?


> +
> +#define clrsetbits32(addr, mask, set) __clrsetbits(readl, writel, addr,
> mask, set)
> +#define clrsetbits32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
> +							   writel_relaxed,
> \
> +							   addr, mask, set)
> +
> +#define setclrbits32(addr, mask, set) __setclrbits(readl, writel, addr,
> mask, set)
> +#define setclrbits32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
> +							   writel_relaxed,
> \
> +							   addr, mask, set)

What's the use case for setclrbits?  I don't see it used anywhere in this
patchset (not even in the coccinelle patterns), it doesn't seem like it would
be a common pattern, and it could easily get confused with clrsetbits.

-Scott
Christophe Leroy Sept. 10, 2018, 5:22 a.m. UTC | #2 
Le 07/09/2018 à 21:41, Corentin Labbe a écrit :
> This patch adds setbits32/clrbits32/clrsetbits32 and
> setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.

So you changed the name of setbits32() ... to setbits32_be() and now you 
are adding new functions called setbits32() ... which do something 
different ?

What will happen if any file has been forgotten during the conversion, 
or if anybody has outoftree drivers and missed this change ?
They will silently successfully compile without any error or warning, 
and the result will be crap buggy.

And why would it be more legitim to have setbits32() be implicitely LE 
instead of implicitely BE ?

I really think those new functions should be called something like 
setbits_le32() ...

Christophe

> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>   include/linux/setbits.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>   create mode 100644 include/linux/setbits.h
> 
> diff --git a/include/linux/setbits.h b/include/linux/setbits.h
> new file mode 100644
> index 000000000000..3e1e273551bb
> --- /dev/null
> +++ b/include/linux/setbits.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_SETBITS_H
> +#define __LINUX_SETBITS_H
> +
> +#include <linux/io.h>
> +
> +#define __setbits(readfunction, writefunction, addr, set) \
> +	writefunction((readfunction(addr) | (set)), addr)
> +#define __clrbits(readfunction, writefunction, addr, mask) \
> +	writefunction((readfunction(addr) & ~(mask)), addr)
> +#define __clrsetbits(readfunction, writefunction, addr, mask, set) \
> +	writefunction(((readfunction(addr) & ~(mask)) | (set)), addr)
> +#define __setclrbits(readfunction, writefunction, addr, mask, set) \
> +	writefunction(((readfunction(addr) | (seti)) & ~(mask)), addr)
> +
> +#define setbits32(addr, set) __setbits(readl, writel, addr, set)
> +#define setbits32_relaxed(addr, set) __setbits(readl_relaxed, writel_relaxed, \
> +					       addr, set)
> +
> +#define clrbits32(addr, mask) __clrbits(readl, writel, addr, mask)
> +#define clrbits32_relaxed(addr, mask) __clrbits(readl_relaxed, writel_relaxed, \
> +						addr, mask)
> +
> +#define clrsetbits32(addr, mask, set) __clrsetbits(readl, writel, addr, mask, set)
> +#define clrsetbits32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
> +							   writel_relaxed, \
> +							   addr, mask, set)
> +
> +#define setclrbits32(addr, mask, set) __setclrbits(readl, writel, addr, mask, set)
> +#define setclrbits32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
> +							   writel_relaxed, \
> +							   addr, mask, set)
> +
> +/* We cannot use CONFIG_64BIT as some x86 drivers use writeq */
> +#if defined(writeq) && defined(readq)
> +#define setbits64(addr, set) __setbits(readq, writeq, addr, set)
> +#define setbits64_relaxed(addr, set) __setbits(readq_relaxed, writeq_relaxed, \
> +					       addr, set)
> +
> +#define clrbits64(addr, mask) __clrbits(readq, writeq, addr, mask)
> +#define clrbits64_relaxed(addr, mask) __clrbits(readq_relaxed, writeq_relaxed, \
> +						addr, mask)
> +
> +#define clrsetbits64(addr, mask, set) __clrsetbits(readq, writeq, addr, mask, set)
> +#define clrsetbits64_relaxed(addr, mask, set) __clrsetbits(readq_relaxed, \
> +							   writeq_relaxed, \
> +							   addr, mask, set)
> +
> +#define setclrbits64(addr, mask, set) __setclrbits(readq, writeq, addr, mask, set)
> +#define setclrbits64_relaxed(addr, mask, set) __setclrbits(readq_relaxed, \
> +							   writeq_relaxed, \
> +							   addr, mask, set)
> +#endif /* writeq/readq */
> +
> +#endif /* __LINUX_SETBITS_H */
>
Corentin LABBE Sept. 10, 2018, 6:49 p.m. UTC | #3 
On Fri, Sep 07, 2018 at 03:00:40PM -0500, Scott Wood wrote:
> On Fri, 2018-09-07 at 19:41 +0000, Corentin Labbe wrote:
> > This patch adds setbits32/clrbits32/clrsetbits32 and
> > setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.
> > 
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> >  include/linux/setbits.h | 55
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 include/linux/setbits.h
> > 
> > diff --git a/include/linux/setbits.h b/include/linux/setbits.h
> > new file mode 100644
> > index 000000000000..3e1e273551bb
> > --- /dev/null
> > +++ b/include/linux/setbits.h
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __LINUX_SETBITS_H
> > +#define __LINUX_SETBITS_H
> > +
> > +#include <linux/io.h>
> > +
> > +#define __setbits(readfunction, writefunction, addr, set) \
> > +	writefunction((readfunction(addr) | (set)), addr)
> > +#define __clrbits(readfunction, writefunction, addr, mask) \
> > +	writefunction((readfunction(addr) & ~(mask)), addr)
> > +#define __clrsetbits(readfunction, writefunction, addr, mask, set) \
> > +	writefunction(((readfunction(addr) & ~(mask)) | (set)), addr)
> > +#define __setclrbits(readfunction, writefunction, addr, mask, set) \
> > +	writefunction(((readfunction(addr) | (seti)) & ~(mask)), addr)
> > +
> > +#define setbits32(addr, set) __setbits(readl, writel, addr, set)
> > +#define setbits32_relaxed(addr, set) __setbits(readl_relaxed,
> > writel_relaxed, \
> > +					       addr, set)
> > +
> > +#define clrbits32(addr, mask) __clrbits(readl, writel, addr, mask)
> > +#define clrbits32_relaxed(addr, mask) __clrbits(readl_relaxed,
> > writel_relaxed, \
> > +						addr, mask)
> 
> So now setbits32/clrbits32 is implicitly little-endian?  Introducing new
> implicit-endian accessors is probably a bad thing in general, but doing it
> with a name that until this patchset was implicitly big-endian (at least on
> powerpc) seems worse.  Why not setbits32_le()?
> 

I believed that writel/readl was endian agnostic, but It seems that I was wrong.
So I will use _le32.

> 
> > +
> > +#define clrsetbits32(addr, mask, set) __clrsetbits(readl, writel, addr,
> > mask, set)
> > +#define clrsetbits32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
> > +							   writel_relaxed,
> > \
> > +							   addr, mask, set)
> > +
> > +#define setclrbits32(addr, mask, set) __setclrbits(readl, writel, addr,
> > mask, set)
> > +#define setclrbits32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
> > +							   writel_relaxed,
> > \
> > +							   addr, mask, set)
> 
> What's the use case for setclrbits?  I don't see it used anywhere in this
> patchset (not even in the coccinelle patterns), it doesn't seem like it would
> be a common pattern, and it could easily get confused with clrsetbits.
> 

It is absent from the coccinelle script due to copy/paste error.
And absent from patchset since it is only two possible example that I can test.

If you run the next fixed coccinelle script, you will find some setclrbits.
Since I fear that mask and set could have some common bits sometimes, I prefer to keep separate clrsetbits and setclrbits.

Regards
Corentin LABBE Sept. 10, 2018, 6:53 p.m. UTC | #4 
On Mon, Sep 10, 2018 at 07:22:04AM +0200, Christophe LEROY wrote:
> 
> 
> Le 07/09/2018 à 21:41, Corentin Labbe a écrit :
> > This patch adds setbits32/clrbits32/clrsetbits32 and
> > setbits64/clrbits64/clrsetbits64 in linux/setbits.h header.
> 
> So you changed the name of setbits32() ... to setbits32_be() and now you 
> are adding new functions called setbits32() ... which do something 
> different ?
> 
> What will happen if any file has been forgotten during the conversion, 
> or if anybody has outoftree drivers and missed this change ?
> They will silently successfully compile without any error or warning, 
> and the result will be crap buggy.
> 
> And why would it be more legitim to have setbits32() be implicitely LE 
> instead of implicitely BE ?
> 
> I really think those new functions should be called something like 
> setbits_le32() ...
> 

I believed that writel/readl was endian agnostic so it explain my mistake.

I will use xxxbits_le32 as you requests.

Thanks
Regards
diff mbox series

Patch

diff --git a/include/linux/setbits.h b/include/linux/setbits.h
new file mode 100644
index 000000000000..3e1e273551bb
--- /dev/null
+++ b/include/linux/setbits.h
@@ -0,0 +1,55 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_SETBITS_H
+#define __LINUX_SETBITS_H
+
+#include <linux/io.h>
+
+#define __setbits(readfunction, writefunction, addr, set) \
+	writefunction((readfunction(addr) | (set)), addr)
+#define __clrbits(readfunction, writefunction, addr, mask) \
+	writefunction((readfunction(addr) & ~(mask)), addr)
+#define __clrsetbits(readfunction, writefunction, addr, mask, set) \
+	writefunction(((readfunction(addr) & ~(mask)) | (set)), addr)
+#define __setclrbits(readfunction, writefunction, addr, mask, set) \
+	writefunction(((readfunction(addr) | (seti)) & ~(mask)), addr)
+
+#define setbits32(addr, set) __setbits(readl, writel, addr, set)
+#define setbits32_relaxed(addr, set) __setbits(readl_relaxed, writel_relaxed, \
+					       addr, set)
+
+#define clrbits32(addr, mask) __clrbits(readl, writel, addr, mask)
+#define clrbits32_relaxed(addr, mask) __clrbits(readl_relaxed, writel_relaxed, \
+						addr, mask)
+
+#define clrsetbits32(addr, mask, set) __clrsetbits(readl, writel, addr, mask, set)
+#define clrsetbits32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
+							   writel_relaxed, \
+							   addr, mask, set)
+
+#define setclrbits32(addr, mask, set) __setclrbits(readl, writel, addr, mask, set)
+#define setclrbits32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
+							   writel_relaxed, \
+							   addr, mask, set)
+
+/* We cannot use CONFIG_64BIT as some x86 drivers use writeq */
+#if defined(writeq) && defined(readq)
+#define setbits64(addr, set) __setbits(readq, writeq, addr, set)
+#define setbits64_relaxed(addr, set) __setbits(readq_relaxed, writeq_relaxed, \
+					       addr, set)
+
+#define clrbits64(addr, mask) __clrbits(readq, writeq, addr, mask)
+#define clrbits64_relaxed(addr, mask) __clrbits(readq_relaxed, writeq_relaxed, \
+						addr, mask)
+
+#define clrsetbits64(addr, mask, set) __clrsetbits(readq, writeq, addr, mask, set)
+#define clrsetbits64_relaxed(addr, mask, set) __clrsetbits(readq_relaxed, \
+							   writeq_relaxed, \
+							   addr, mask, set)
+
+#define setclrbits64(addr, mask, set) __setclrbits(readq, writeq, addr, mask, set)
+#define setclrbits64_relaxed(addr, mask, set) __setclrbits(readq_relaxed, \
+							   writeq_relaxed, \
+							   addr, mask, set)
+#endif /* writeq/readq */
+
+#endif /* __LINUX_SETBITS_H */