Moan: usage of __iormb() and __iowmb() outside of asm/io.h

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Russell King - ARM Linux]

Guys,

I notice that we have users of __iormb() and __iowmb() outside of asm/io.h:

drivers/clocksource/timer-keystone.c:   __iowmb();
drivers/dma/cppi41.c:                   __iormb();
drivers/dma/cppi41.c:   __iowmb();
drivers/dma/cppi41.c:           __iowmb();
drivers/dma/cppi41.c:                   __iormb();
drivers/soc/ti/knav_qmss_queue.c:       __iowmb();

These are not official kernel barriers - the only reason they exist in
asm/io.h is purely to provide a barrier implementation _for_ creating
the accessors _in_ asm/io.h, which are macros, and therefore these
macros need to stay around for the same scope as those accessors.

As with all details which are an architecture matter, they are subject
to the whims of the architecture maintainer to provide whatever semantics
for them that the architecture maintainer deems fit: there is no official
requirement for anything of that nature to do anything, and no guarantee
that anything such a detail does today it will do so tomorrow.

This is why only official interfaces should be used, and if they do not
satisfy the requirements, then new official interfaces need to be 
proposed.  Don't ever poke about with stuff that's an architecture
implementation detail.

We've been here before with some of the cache flushing code - and people
have been burnt by it.

I do wish that people would see the difference between stuff which is
implemented to facilitate the implementation of an architecture detail
vs something which is provided for everyone's use.

I'm working on a patch which will completely remove these from view.
I would strongly suggest that these uses are removed from the above
code as a matter of urgency.

(Thankfully, it looks like it's only TI OMAP related code which has the
issue right now.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Sebastian Andrzej Siewior]

* Russell King - ARM Linux | 2015-06-08 19:47:01 [+0100]:

>These are not official kernel barriers - the only reason they exist in
>asm/io.h is purely to provide a barrier implementation _for_ creating
>the accessors _in_ asm/io.h, which are macros, and therefore these
>macros need to stay around for the same scope as those accessors.

I'm sorry for that. Are those _relaxed() accessors okay? They don't show
up in Documentation/ so I ask here before I do it wrong again?

On the other hand if Tony or someone else things that this is not worth
it at all I could stick with readl/writel and drop the barries. It is
just that musb and releated code used those __raw accessors and I ended
up with them, too (I'm not looking for an excuse just saying that I do
not have any numbers to show which say that this is good in terms of
performance and worth keeping).

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -257,12 +257,12 @@ static struct cppi41_channel *desc_to_chan(struct cppi41_dd *cdd, u32 desc)
 
 static void cppi_writel(u32 val, void *__iomem *mem)
 {
-	__raw_writel(val, mem);
+	writel_relaxed(val, mem);
 }
 
 static u32 cppi_readl(void *__iomem *mem)
 {
-	return __raw_readl(mem);
+	return readl_relaxed(mem);
 }
 
 static u32 pd_trans_len(u32 val)
@@ -308,7 +308,7 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 		}
 
 		if (val)
-			__iormb();
+			rmb();
 
 		while (val) {
 			u32 desc, len;
@@ -410,14 +410,7 @@ static void cppi41_dma_issue_pending(struct dma_chan *chan)
 		reg |= c->q_comp_num;
 	}
 
-	cppi_writel(reg, c->gcr_reg);
-
-	/*
-	 * We don't use writel() but __raw_writel() so we have to make sure
-	 * that the DMA descriptor in coherent memory made to the main memory
-	 * before starting the dma engine.
-	 */
-	__iowmb();
+	writel(reg, c->gcr_reg);
 	push_desc_queue(c);
 }
 
@@ -546,7 +539,7 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
 
 	if (!c->td_queued) {
 		cppi41_compute_td_desc(td);
-		__iowmb();
+		wmb();
 
 		reg = (sizeof(struct cppi41_desc) - 24) / 4;
 		reg |= td_desc_phys;
@@ -577,7 +570,7 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
 		} else if (desc_phys == td_desc_phys) {
 			u32 pd0;
 
-			__iormb();
+			rmb();
 			pd0 = td->pd0;
 			WARN_ON((pd0 >> DESC_TYPE) != DESC_TYPE_TEARD);
 			WARN_ON(!c->is_tx && !(pd0 & TD_DESC_IS_RX));

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Murali Karicheri]

On 06/08/2015 02:47 PM, Russell King - ARM Linux wrote:
> Guys,
>
> I notice that we have users of __iormb() and __iowmb() outside of asm/io.h:
>
> drivers/clocksource/timer-keystone.c:   __iowmb();
> drivers/dma/cppi41.c:                   __iormb();
> drivers/dma/cppi41.c:   __iowmb();
> drivers/dma/cppi41.c:           __iowmb();
> drivers/dma/cppi41.c:                   __iormb();
> drivers/soc/ti/knav_qmss_queue.c:       __iowmb();
>
> These are not official kernel barriers - the only reason they exist in
> asm/io.h is purely to provide a barrier implementation _for_ creating
> the accessors _in_ asm/io.h, which are macros, and therefore these
> macros need to stay around for the same scope as those accessors.
>
> As with all details which are an architecture matter, they are subject
> to the whims of the architecture maintainer to provide whatever semantics
> for them that the architecture maintainer deems fit: there is no official
> requirement for anything of that nature to do anything, and no guarantee
> that anything such a detail does today it will do so tomorrow.
>
> This is why only official interfaces should be used, and if they do not
> satisfy the requirements, then new official interfaces need to be
> proposed.  Don't ever poke about with stuff that's an architecture
> implementation detail.
>
> We've been here before with some of the cache flushing code - and people
> have been burnt by it.
>
> I do wish that people would see the difference between stuff which is
> implemented to facilitate the implementation of an architecture detail
> vs something which is provided for everyone's use.
>
> I'm working on a patch which will completely remove these from view.
> I would strongly suggest that these uses are removed from the above
> code as a matter of urgency.
>
> (Thankfully, it looks like it's only TI OMAP related code which has the
> issue right now.)
>

Adding Santosh as well.

-- 
Murali Karicheri
Linux Kernel, Keystone

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Will Deacon]

Hi Russell,

On Mon, Jun 08, 2015 at 07:47:01PM +0100, Russell King - ARM Linux wrote:
> I notice that we have users of __iormb() and __iowmb() outside of asm/io.h:
> 
> drivers/clocksource/timer-keystone.c:   __iowmb();
> drivers/dma/cppi41.c:                   __iormb();
> drivers/dma/cppi41.c:   __iowmb();
> drivers/dma/cppi41.c:           __iowmb();
> drivers/dma/cppi41.c:                   __iormb();
> drivers/soc/ti/knav_qmss_queue.c:       __iowmb();
> 
> These are not official kernel barriers - the only reason they exist in
> asm/io.h is purely to provide a barrier implementation _for_ creating
> the accessors _in_ asm/io.h, which are macros, and therefore these
> macros need to stay around for the same scope as those accessors.
> 
> As with all details which are an architecture matter, they are subject
> to the whims of the architecture maintainer to provide whatever semantics
> for them that the architecture maintainer deems fit: there is no official
> requirement for anything of that nature to do anything, and no guarantee
> that anything such a detail does today it will do so tomorrow.
> 
> This is why only official interfaces should be used, and if they do not
> satisfy the requirements, then new official interfaces need to be 
> proposed.  Don't ever poke about with stuff that's an architecture
> implementation detail.
> 
> We've been here before with some of the cache flushing code - and people
> have been burnt by it.
> 
> I do wish that people would see the difference between stuff which is
> implemented to facilitate the implementation of an architecture detail
> vs something which is provided for everyone's use.
> 
> I'm working on a patch which will completely remove these from view.
> I would strongly suggest that these uses are removed from the above
> code as a matter of urgency.

I agree to removing these from view; we already have plenty of barrier
macros and we don't want these to proliferate outside of the arch code.

Any chance you could do a similar change for arm64, please (we have the
same macros there)?

Will

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Russell King - ARM Linux]

On Wed, Jun 10, 2015 at 12:18:20PM +0100, Will Deacon wrote:
> Hi Russell,
> 
> On Mon, Jun 08, 2015 at 07:47:01PM +0100, Russell King - ARM Linux wrote:
> > I notice that we have users of __iormb() and __iowmb() outside of asm/io.h:
> > 
> > drivers/clocksource/timer-keystone.c:   __iowmb();
> > drivers/dma/cppi41.c:                   __iormb();
> > drivers/dma/cppi41.c:   __iowmb();
> > drivers/dma/cppi41.c:           __iowmb();
> > drivers/dma/cppi41.c:                   __iormb();
> > drivers/soc/ti/knav_qmss_queue.c:       __iowmb();
> > 
> > These are not official kernel barriers - the only reason they exist in
> > asm/io.h is purely to provide a barrier implementation _for_ creating
> > the accessors _in_ asm/io.h, which are macros, and therefore these
> > macros need to stay around for the same scope as those accessors.
> > 
> > As with all details which are an architecture matter, they are subject
> > to the whims of the architecture maintainer to provide whatever semantics
> > for them that the architecture maintainer deems fit: there is no official
> > requirement for anything of that nature to do anything, and no guarantee
> > that anything such a detail does today it will do so tomorrow.
> > 
> > This is why only official interfaces should be used, and if they do not
> > satisfy the requirements, then new official interfaces need to be 
> > proposed.  Don't ever poke about with stuff that's an architecture
> > implementation detail.
> > 
> > We've been here before with some of the cache flushing code - and people
> > have been burnt by it.
> > 
> > I do wish that people would see the difference between stuff which is
> > implemented to facilitate the implementation of an architecture detail
> > vs something which is provided for everyone's use.
> > 
> > I'm working on a patch which will completely remove these from view.
> > I would strongly suggest that these uses are removed from the above
> > code as a matter of urgency.
> 
> I agree to removing these from view; we already have plenty of barrier
> macros and we don't want these to proliferate outside of the arch code.
> 
> Any chance you could do a similar change for arm64, please (we have the
> same macros there)?

Yes - as you're aware, removing them from sight does cause us to decend
into macro-hell in asm/io.h, but I think that's better than having people
get their grubby fingers on arch internal stuff they shouldn't be touching.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Russell King - ARM Linux]

On Wed, Jun 10, 2015 at 12:24:34PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 10, 2015 at 12:18:20PM +0100, Will Deacon wrote:
> > I agree to removing these from view; we already have plenty of barrier
> > macros and we don't want these to proliferate outside of the arch code.
> > 
> > Any chance you could do a similar change for arm64, please (we have the
> > same macros there)?
> 
> Yes - as you're aware, removing them from sight does cause us to decend
> into macro-hell in asm/io.h, but I think that's better than having people
> get their grubby fingers on arch internal stuff they shouldn't be touching.

Here's what I'm proposing for ARM.  As I say, it's macro-hell...  It's
also not perfect yet (since the __LINUX_ARM_ARCH__ < 6 case isn't
properly handled yet.)

The down-side is that we end up with a load of __arm_(read|write)*
functions, which people could start using.

I _guess_ a solution to that would be to have these macros be used to
generate inline functions with the correct accessor name as required.

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index addfb3dd095f..6566ef9f69f8 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -31,6 +31,16 @@
 #include <asm-generic/pci_iomap.h>
 #include <xen/xen.h>
 
+/* IO barriers */
+#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
+#include <asm/barrier.h>
+#define __iormb()		rmb()
+#define __iowmb()		wmb()
+#else
+#define __iormb()		do { } while (0)
+#define __iowmb()		do { } while (0)
+#endif
+
 /*
  * ISA I/O bus memory addresses are 1:1 with the physical address.
  */
@@ -56,6 +66,61 @@ void __raw_readsb(const volatile void __iomem *addr, void *data, int bytelen);
 void __raw_readsw(const volatile void __iomem *addr, void *data, int wordlen);
 void __raw_readsl(const volatile void __iomem *addr, void *data, int longlen);
 
+/*
+ * Macros used to define the raw IO accessors.
+ * This generates:
+ *  __arm_read[bwl]()
+ *  __arm_read[bwl]_rmb()
+ *  __arm_read[bwl]_le()
+ *  __arm_read[bwl]_be()
+ *  __arm_read[bwl]_le_rmb()
+ *  __arm_read[bwl]_be_rmb()
+ *  __arm_write[bwl]()
+ *  __arm_write[bwl]_rmb()
+ *  __arm_write[bwl]_le()
+ *  __arm_write[bwl]_be()
+ *  __arm_write[bwl]_le_rmb()
+ *  __arm_write[bwl]_be_rmb()
+ */
+#define DEFINE_IOR_OP(size, suffix, endian, instr, constraint, conv, barrier) \
+static inline u##size __arm_read##suffix(const volatile avoid __iomem *addr) \
+{									\
+	endian##size val;						\
+	asm volatile(instr						\
+		     : "=r" (val)					\
+		     : constraint (*(volatile endian##size __force *)addr)); \
+	barrier;							\
+	return conv(val);						\
+}
+
+#define DEFINE_IOW_OP(size, suffix, endian, instr, constraint, conv, barrier) \
+static inline void __arm_write##suffix(u##size val, volatile void __iomem *addr) \
+{									\
+	barrier;							\
+	asm volatile(instr						\
+		     : : "r" (conv(val)),				\
+			 constraint (*(volatile endian##size __force *)addr)); \
+}
+
+#define DEFINE_IOR_OP_B(size, suffix, endian, instr, constraint, conv)	\
+DEFINE_IOR_OP(size, suffix, endian, instr, constraint, conv, )		\
+DEFINE_IOR_OP(size, suffix##_rmb, endian, instr, constraint, conv, __iormb())
+
+#define DEFINE_IOW_OP_B(size, suffix, endian, instr, constraint, conv)	\
+DEFINE_IOW_OP(size, suffix, endian, instr, constraint, conv, )		\
+DEFINE_IOW_OP(size, suffix##_wmb, endian, instr, constraint, conv, __iowmb())
+
+#define DEFINE_IO_OP(size, suffix, rinstr, winstr, constraint)		\
+DEFINE_IOR_OP_B(size, suffix, u, rinstr, constraint,)			\
+DEFINE_IOW_OP_B(size, suffix, u, winstr, constraint,)
+
+#define DEFINE_IO_LE_OP(size, suffix, rinstr, winstr, constraint)	\
+DEFINE_IO_OP(size, suffix, rinstr, winstr, constraint)		\
+DEFINE_IOR_OP_B(size, suffix##_le, le, rinstr, constraint, cpu_to_le##size) \
+DEFINE_IOW_OP_B(size, suffix##_le, le, winstr, constraint, le##size##_to_cpu) \
+DEFINE_IOR_OP_B(size, suffix##_be, be, rinstr, constraint, cpu_to_be##size) \
+DEFINE_IOW_OP_B(size, suffix##_be, be, winstr, constraint, be##size##_to_cpu)
+
 #if __LINUX_ARM_ARCH__ < 6
 /*
  * Half-word accesses are problematic with RiscPC due to limitations of
@@ -70,57 +135,30 @@ void __raw_readsl(const volatile void __iomem *addr, void *data, int longlen);
  * writeback addressing modes as these incur a significant performance
  * overhead (the address generation must be emulated in software).
  */
-#define __raw_writew __raw_writew
-static inline void __raw_writew(u16 val, volatile void __iomem *addr)
-{
-	asm volatile("strh %1, %0"
-		     : : "Q" (*(volatile u16 __force *)addr), "r" (val));
-}
+#define __raw_writew __arm_writew
+#define __raw_readw __arm_readw
+DEFINE_IO_LE_OP(16, w, "ldrh %0, %1", "strh %0, %1", "Q")
 
-#define __raw_readw __raw_readw
-static inline u16 __raw_readw(const volatile void __iomem *addr)
-{
-	u16 val;
-	asm volatile("ldrh %0, %1"
-		     : "=r" (val)
-		     : "Q" (*(volatile u16 __force *)addr));
-	return val;
-}
 #endif
 
-#define __raw_writeb __raw_writeb
-static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
-{
-	asm volatile("strb %1, %0"
-		     : : "Qo" (*(volatile u8 __force *)addr), "r" (val));
-}
+#define __raw_writeb __arm_writeb
+#define __raw_readb __arm_readb
+DEFINE_IO_OP(8, b, "ldrb %0, %1", "strb %0, %1", "Qo")
 
-#define __raw_writel __raw_writel
-static inline void __raw_writel(u32 val, volatile void __iomem *addr)
-{
-	asm volatile("str %1, %0"
-		     : : "Qo" (*(volatile u32 __force *)addr), "r" (val));
-}
+#define __raw_writel __arm_writel
+#define __raw_readl __arm_readl
+DEFINE_IO_LE_OP(32, l, "ldr %0, %1", "str %0, %1", "Qo")
 
-#define __raw_readb __raw_readb
-static inline u8 __raw_readb(const volatile void __iomem *addr)
-{
-	u8 val;
-	asm volatile("ldrb %0, %1"
-		     : "=r" (val)
-		     : "Qo" (*(volatile u8 __force *)addr));
-	return val;
-}
+#undef DEFINE_IO_OP
+#undef DEFINE_IOW_OP
+#undef DEFINE_IOW_OP_B
+#undef DEFINE_IOR_OP
+#undef DEFINE_IOR_OP_B
+#undef DEFINE_IO_LE_OP
 
-#define __raw_readl __raw_readl
-static inline u32 __raw_readl(const volatile void __iomem *addr)
-{
-	u32 val;
-	asm volatile("ldr %0, %1"
-		     : "=r" (val)
-		     : "Qo" (*(volatile u32 __force *)addr));
-	return val;
-}
+/* Don't let people use these as another barrier in their code. */
+#undef __iowmb
+#undef __iormb
 
 /*
  * Architecture ioremap implementation.
@@ -170,16 +208,6 @@ static inline void __iomem *__typesafe_io(unsigned long addr)
 
 #define IOMEM(x)	((void __force __iomem *)(x))
 
-/* IO barriers */
-#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
-#include <asm/barrier.h>
-#define __iormb()		rmb()
-#define __iowmb()		wmb()
-#else
-#define __iormb()		do { } while (0)
-#define __iowmb()		do { } while (0)
-#endif
-
 /* PCI fixed i/o mapping */
 #define PCI_IO_VIRT_BASE	0xfee00000
 #define PCI_IOBASE		((void __iomem *)PCI_IO_VIRT_BASE)
@@ -250,17 +278,13 @@ extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
  * The {in,out}[bwl] macros are for emulating x86-style PCI/ISA IO space.
  */
 #ifdef __io
-#define outb(v,p)	({ __iowmb(); __raw_writeb(v,__io(p)); })
-#define outw(v,p)	({ __iowmb(); __raw_writew((__force __u16) \
-					cpu_to_le16(v),__io(p)); })
-#define outl(v,p)	({ __iowmb(); __raw_writel((__force __u32) \
-					cpu_to_le32(v),__io(p)); })
-
-#define inb(p)	({ __u8 __v = __raw_readb(__io(p)); __iormb(); __v; })
-#define inw(p)	({ __u16 __v = le16_to_cpu((__force __le16) \
-			__raw_readw(__io(p))); __iormb(); __v; })
-#define inl(p)	({ __u32 __v = le32_to_cpu((__force __le32) \
-			__raw_readl(__io(p))); __iormb(); __v; })
+#define outb(v,p)		__arm_writeb_wmb(v,__io(p))
+#define outw(v,p)		__arm_writew_le_wmb(v,__io(p))
+#define outl(v,p)		__arm_writel_le_wmb(v,__io(p))
+
+#define inb(p)			__arm_readb_rmb(__io(p))
+#define inw(p)			__arm_readw_le_rmb(__io(p))
+#define inl(p)			__arm_readl_le_rmb(__io(p))
 
 #define outsb(p,d,l)		__raw_writesb(__io(p),d,l)
 #define outsw(p,d,l)		__raw_writesw(__io(p),d,l)
@@ -291,23 +315,21 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
  * IO port primitives for more information.
  */
 #ifndef readl
-#define readb_relaxed(c) ({ u8  __r = __raw_readb(c); __r; })
-#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
-					__raw_readw(c)); __r; })
-#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
-					__raw_readl(c)); __r; })
+#define readb_relaxed(c)	__arm_readb(c)
+#define readw_relaxed(c)	__arm_readw_le(c)
+#define readl_relaxed(c)	__arm_readl_le(c)
 
-#define writeb_relaxed(v,c)	__raw_writeb(v,c)
-#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
-#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)
+#define writeb_relaxed(v,c)	__arm_writeb(v,c)
+#define writew_relaxed(v,c)	__arm_writew_le(v,c)
+#define writel_relaxed(v,c)	__arm_writel_le(v,c)
 
-#define readb(c)		({ u8  __v = readb_relaxed(c); __iormb(); __v; })
-#define readw(c)		({ u16 __v = readw_relaxed(c); __iormb(); __v; })
-#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(); __v; })
+#define readb(c)		__arm_readb_rmb(c)
+#define readw(c)		__arm_readw_le_rmb(c)
+#define readl(c)		__arm_readl_le_rmb(c)
 
-#define writeb(v,c)		({ __iowmb(); writeb_relaxed(v,c); })
-#define writew(v,c)		({ __iowmb(); writew_relaxed(v,c); })
-#define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })
+#define writeb(v,c)		__arm_writeb_wmb(v,c)
+#define writew(v,c)		__arm_writew_le_wmb(v,c)
+#define writel(v,c)		__arm_writel_le_wmb(v,c)
 
 #define readsb(p,d,l)		__raw_readsb(p,d,l)
 #define readsw(p,d,l)		__raw_readsw(p,d,l)
@@ -363,11 +385,11 @@ static inline void memcpy_toio(volatile void __iomem *to, const void *from,
 /*
  * io{read,write}{16,32}be() macros
  */
-#define ioread16be(p)		({ __u16 __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
-#define ioread32be(p)		({ __u32 __v = be32_to_cpu((__force __be32)__raw_readl(p)); __iormb(); __v; })
+#define ioread16be(p)			__arm_readw_be_rmb(p)
+#define ioread32be(p)			__arm_readl_be_rmb(p)
 
-#define iowrite16be(v,p)	({ __iowmb(); __raw_writew((__force __u16)cpu_to_be16(v), p); })
-#define iowrite32be(v,p)	({ __iowmb(); __raw_writel((__force __u32)cpu_to_be32(v), p); })
+#define iowrite16be(v,p)		__arm_writew_be_wmb(v,p)
+#define iowrite32be(v,p)		__arm_writel_be_wmb(v,p)
 
 #ifndef ioport_map
 #define ioport_map ioport_map


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Catalin Marinas]

On Wed, Jun 10, 2015 at 01:30:40PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 10, 2015 at 12:24:34PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 10, 2015 at 12:18:20PM +0100, Will Deacon wrote:
> > > I agree to removing these from view; we already have plenty of barrier
> > > macros and we don't want these to proliferate outside of the arch code.
> > > 
> > > Any chance you could do a similar change for arm64, please (we have the
> > > same macros there)?
> > 
> > Yes - as you're aware, removing them from sight does cause us to decend
> > into macro-hell in asm/io.h, but I think that's better than having people
> > get their grubby fingers on arch internal stuff they shouldn't be touching.
> 
> Here's what I'm proposing for ARM.  As I say, it's macro-hell...  It's
> also not perfect yet (since the __LINUX_ARM_ARCH__ < 6 case isn't
> properly handled yet.)

IMO, it makes the code pretty unreadable, it breaks searching/ctags. It
may be easier to just randomly change the number of underscores in front
of "io*mb" every few kernel releases (e.g. twice a year), so we identify
the users outside asm/io.h.

-- 
Catalin

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Will Deacon]

On Wed, Jun 10, 2015 at 05:53:24PM +0100, Catalin Marinas wrote:
> On Wed, Jun 10, 2015 at 01:30:40PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 10, 2015 at 12:24:34PM +0100, Russell King - ARM Linux wrote:
> > > On Wed, Jun 10, 2015 at 12:18:20PM +0100, Will Deacon wrote:
> > > > I agree to removing these from view; we already have plenty of barrier
> > > > macros and we don't want these to proliferate outside of the arch code.
> > > > 
> > > > Any chance you could do a similar change for arm64, please (we have the
> > > > same macros there)?
> > > 
> > > Yes - as you're aware, removing them from sight does cause us to decend
> > > into macro-hell in asm/io.h, but I think that's better than having people
> > > get their grubby fingers on arch internal stuff they shouldn't be touching.
> > 
> > Here's what I'm proposing for ARM.  As I say, it's macro-hell...  It's
> > also not perfect yet (since the __LINUX_ARM_ARCH__ < 6 case isn't
> > properly handled yet.)
> 
> IMO, it makes the code pretty unreadable, it breaks searching/ctags. It
> may be easier to just randomly change the number of underscores in front
> of "io*mb" every few kernel releases (e.g. twice a year), so we identify
> the users outside asm/io.h.

Or generate static line functions from the macros, as Russell suggested
previously. Then we can #undef the macros at the end of the file (I have
patches doing this for our cmpxchg implementation).

Will

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[santosh shilimkar]

On 6/10/2015 3:59 AM, Murali Karicheri wrote:
> On 06/08/2015 02:47 PM, Russell King - ARM Linux wrote:
>> Guys,
>>
>> I notice that we have users of __iormb() and __iowmb() outside of
>> asm/io.h:
>>
>> drivers/clocksource/timer-keystone.c:   __iowmb();
>> drivers/dma/cppi41.c:                   __iormb();
>> drivers/dma/cppi41.c:   __iowmb();
>> drivers/dma/cppi41.c:           __iowmb();
>> drivers/dma/cppi41.c:                   __iormb();
>> drivers/soc/ti/knav_qmss_queue.c:       __iowmb();
>>
Looks like CPPI abuse got carry forwarded. I also missed these because
of oversight. Should have insisted to use the standard accessors.

>> These are not official kernel barriers - the only reason they exist in
>> asm/io.h is purely to provide a barrier implementation _for_ creating
>> the accessors _in_ asm/io.h, which are macros, and therefore these
>> macros need to stay around for the same scope as those accessors.
>>
>> As with all details which are an architecture matter, they are subject
>> to the whims of the architecture maintainer to provide whatever semantics
>> for them that the architecture maintainer deems fit: there is no official
>> requirement for anything of that nature to do anything, and no guarantee
>> that anything such a detail does today it will do so tomorrow.
>>
>> This is why only official interfaces should be used, and if they do not
>> satisfy the requirements, then new official interfaces need to be
>> proposed.  Don't ever poke about with stuff that's an architecture
>> implementation detail.
>>
>> We've been here before with some of the cache flushing code - and people
>> have been burnt by it.
>>
>> I do wish that people would see the difference between stuff which is
>> implemented to facilitate the implementation of an architecture detail
>> vs something which is provided for everyone's use.
>>
>> I'm working on a patch which will completely remove these from view.
>> I would strongly suggest that these uses are removed from the above
>> code as a matter of urgency.
>>
Sorry for that Russell and thanks for addressing it advance.

Regards,
Santosh

Moan: usage of __iormb() and __iowmb() outside of asm/io.h

[Russell King - ARM Linux]

On Wed, Jun 10, 2015 at 05:59:04PM +0100, Will Deacon wrote:
> Or generate static line functions from the macros, as Russell suggested
> previously. Then we can #undef the macros at the end of the file (I have
> patches doing this for our cmpxchg implementation).

Or something like this, which probably improves the ctags stuff - but
at the expense if not using macros, so the chances of there being
something wrong is _considerably_ greater.

Macros have their place: when there's lots of repetitive code with only
a few changes between, which is very much the case with these IO accessor
macros.  Also look at what happens to the line count when we have an
inline function for every damn accessor which has to exist in its own
individual right just for ctags.

IMHO, this is not "better" in any regard - it's just a different trade-off
between ctags-compatibility, and making sure the code is correct.

 arch/arm/include/asm/io.h | 248 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 195 insertions(+), 53 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index addfb3dd095f..9b1f89918bf0 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -56,70 +56,90 @@ void __raw_readsb(const volatile void __iomem *addr, void *data, int bytelen);
 void __raw_readsw(const volatile void __iomem *addr, void *data, int wordlen);
 void __raw_readsl(const volatile void __iomem *addr, void *data, int longlen);
 
+/*
+ * Macros used to define the raw assembly IO accessors.
+ */
+#define __IOR_OP(type, instr, constraint, addr)				\
+	({								\
+		type ior_val;						\
+		asm volatile(instr					\
+			     : "=r" (ior_val)				\
+			     : constraint (*(volatile type __force *)addr)); \
+		ior_val;						\
+	})
+
+#define __IOW_OP(type, instr, constraint, addr, val)			\
+	({								\
+		asm volatile(instr					\
+			     : : "r" (val),				\
+				 constraint (*(volatile type __force *)addr)); \
+	})
+
 #if __LINUX_ARM_ARCH__ < 6
 /*
  * Half-word accesses are problematic with RiscPC due to limitations of
  * the bus. Rather than special-case the machine, just let the compiler
  * generate the access for CPUs prior to ARMv6.
  */
-#define __raw_readw(a)         (__chk_io_ptr(a), *(volatile unsigned short __force *)(a))
-#define __raw_writew(v,a)      ((void)(__chk_io_ptr(a), *(volatile unsigned short __force *)(a) = (v)))
+#define __IOR16(endian, addr)		\
+	*(volatile unsigned short __force *)(addr))
+#define __IOW16(endian, addr, val)	\
+	((void)*(volatile unsigned short __force *)(addr) = (val))
 #else
 /*
  * When running under a hypervisor, we want to avoid I/O accesses with
  * writeback addressing modes as these incur a significant performance
  * overhead (the address generation must be emulated in software).
  */
+#define __IOR16(endian, addr) \
+	__IOR_OP(endian##16, "ldrh %0, %1", "Q", addr)
+#define __IOW16(endian, addr, val) \
+	__IOW_OP(endian##16, "strh %0, %1", "Q", addr, val)
+#endif
+
+#define __IOR8(endian, addr) \
+	__IOR_OP(u8, "ldrb %0, %1", "Qo", addr)
+#define __IOR32(endian, addr) \
+	__IOR_OP(endian##32, "ldr %0, %1", "Qo", addr)
+#define __IOW8(endian, addr, val) \
+	__IOW_OP(u8, "strb %0, %1", "Qo", addr, val)
+#define __IOW32(endian, addr, val) \
+	__IOW_OP(endian##32, "str %0, %1", "Qo", addr, val)
+
 #define __raw_writew __raw_writew
 static inline void __raw_writew(u16 val, volatile void __iomem *addr)
 {
-	asm volatile("strh %1, %0"
-		     : : "Q" (*(volatile u16 __force *)addr), "r" (val));
+	__IOW16(u, addr, val);
 }
 
 #define __raw_readw __raw_readw
 static inline u16 __raw_readw(const volatile void __iomem *addr)
 {
-	u16 val;
-	asm volatile("ldrh %0, %1"
-		     : "=r" (val)
-		     : "Q" (*(volatile u16 __force *)addr));
-	return val;
+	return __IOR16(u, addr);
 }
-#endif
 
 #define __raw_writeb __raw_writeb
 static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
 {
-	asm volatile("strb %1, %0"
-		     : : "Qo" (*(volatile u8 __force *)addr), "r" (val));
+	__IOW8(u, addr, val);
 }
 
 #define __raw_writel __raw_writel
 static inline void __raw_writel(u32 val, volatile void __iomem *addr)
 {
-	asm volatile("str %1, %0"
-		     : : "Qo" (*(volatile u32 __force *)addr), "r" (val));
+	__IOW32(u, addr, val);
 }
 
 #define __raw_readb __raw_readb
 static inline u8 __raw_readb(const volatile void __iomem *addr)
 {
-	u8 val;
-	asm volatile("ldrb %0, %1"
-		     : "=r" (val)
-		     : "Qo" (*(volatile u8 __force *)addr));
-	return val;
+	return __IOR8(u, addr);
 }
 
 #define __raw_readl __raw_readl
 static inline u32 __raw_readl(const volatile void __iomem *addr)
 {
-	u32 val;
-	asm volatile("ldr %0, %1"
-		     : "=r" (val)
-		     : "Qo" (*(volatile u32 __force *)addr));
-	return val;
+	return __IOR32(u, addr);
 }
 
 /*
@@ -250,17 +270,53 @@ extern int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr);
  * The {in,out}[bwl] macros are for emulating x86-style PCI/ISA IO space.
  */
 #ifdef __io
-#define outb(v,p)	({ __iowmb(); __raw_writeb(v,__io(p)); })
-#define outw(v,p)	({ __iowmb(); __raw_writew((__force __u16) \
-					cpu_to_le16(v),__io(p)); })
-#define outl(v,p)	({ __iowmb(); __raw_writel((__force __u32) \
-					cpu_to_le32(v),__io(p)); })
-
-#define inb(p)	({ __u8 __v = __raw_readb(__io(p)); __iormb(); __v; })
-#define inw(p)	({ __u16 __v = le16_to_cpu((__force __le16) \
-			__raw_readw(__io(p))); __iormb(); __v; })
-#define inl(p)	({ __u32 __v = le32_to_cpu((__force __le32) \
-			__raw_readl(__io(p))); __iormb(); __v; })
+static inline void outb(u8 val, unsigned int port)
+{
+	void __iomem *addr = __io(port);
+
+	__iowmb();
+	__IOW8(u, addr, val);
+}
+
+static inline void outw(u16 val, unsigned int port)
+{
+	void __iomem *addr = __io(port);
+
+	__iowmb();
+	__IOW16(le, addr, cpu_to_le16(val));
+}
+
+static inline void outl(u32 val, unsigned int port)
+{
+	void __iomem *addr = __io(port);
+
+	__iowmb();
+	__IOW32(le, addr, cpu_to_le32(val));
+}
+
+static inline u8 inb(unsigned int port)
+{
+	void __iomem *addr = __io(port);
+	u8 val = __IOR8(u, addr);
+	__iormb();
+	return val;
+}
+
+static inline u16 inw(unsigned int port)
+{
+	void __iomem *addr = __io(port);
+	le16 val = __IOR16(le, addr);
+	__iormb();
+	return le16_to_cpu(val);
+}
+
+static inline u32 inl(unsigned int port)
+{
+	void __iomem *addr = __io(port);
+	le32 val = __IOR32(le, addr);
+	__iormb();
+	return le32_to_cpu(val);
+}
 
 #define outsb(p,d,l)		__raw_writesb(__io(p),d,l)
 #define outsw(p,d,l)		__raw_writesw(__io(p),d,l)
@@ -291,23 +347,74 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
  * IO port primitives for more information.
  */
 #ifndef readl
-#define readb_relaxed(c) ({ u8  __r = __raw_readb(c); __r; })
-#define readw_relaxed(c) ({ u16 __r = le16_to_cpu((__force __le16) \
-					__raw_readw(c)); __r; })
-#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
-					__raw_readl(c)); __r; })
+static inline u8 readb_relaxed(const volatile void __iomem *c)
+{
+	return __IOR8(u, c);
+}
 
-#define writeb_relaxed(v,c)	__raw_writeb(v,c)
-#define writew_relaxed(v,c)	__raw_writew((__force u16) cpu_to_le16(v),c)
-#define writel_relaxed(v,c)	__raw_writel((__force u32) cpu_to_le32(v),c)
+static inline u16 readw_relaxed(const volatile void __iomem *c)
+{
+	return le16_to_cpu(__IOR16(le, c));
+}
 
-#define readb(c)		({ u8  __v = readb_relaxed(c); __iormb(); __v; })
-#define readw(c)		({ u16 __v = readw_relaxed(c); __iormb(); __v; })
-#define readl(c)		({ u32 __v = readl_relaxed(c); __iormb(); __v; })
+static inline u32 readl_relaxed(const volatile void __iomem *c)
+{
+	return le32_to_cpu(__IOR32(le, c));
+}
 
-#define writeb(v,c)		({ __iowmb(); writeb_relaxed(v,c); })
-#define writew(v,c)		({ __iowmb(); writew_relaxed(v,c); })
-#define writel(v,c)		({ __iowmb(); writel_relaxed(v,c); })
+static inline void writeb_relaxed(u8 val, volatile void __iomem *c)
+{
+	__IOW8(u, c, val);
+}
+
+static inline void writew_relaxed(u16 val, volatile void __iomem *c)
+{
+	__IOW16(le, c, cpu_to_le16(val));
+}
+
+static inline void writel_relaxed(u32 val, volatile void __iomem *c)
+{
+	__IOW32(le, c, cpu_to_le32(val));
+}
+
+static inline u8 readb(const volatile void __iomem *c)
+{
+	u8 val = __IOR8(u, c);
+	__iormb();
+	return val;
+}
+
+static inline u16 readw(const volatile void __iomem *c)
+{
+	le16 val = __IOR16(le, c);
+	__iormb();
+	return le16_to_cpu(val);
+}
+
+static inline u32 readl(const volatile void __iomem *c)
+{
+	le32 val = __IOR32(le, c);
+	__iormb();
+	return le32_to_cpu(val);
+}
+
+static inline void writeb(u8 val, volatile void __iomem *c)
+{
+	__iowmb();
+	__IOW8(u, c, val);
+}
+
+static inline void writew(u16 val, volatile void __iomem *c)
+{
+	__iowmb();
+	__IOW16(le, c, cpu_to_le16(val));
+}
+
+static inline void writel(u32 val, volatile void __iomem *c)
+{
+	__iowmb();
+	__IOW32(le, c, cpu_to_le32(val));
+}
 
 #define readsb(p,d,l)		__raw_readsb(p,d,l)
 #define readsw(p,d,l)		__raw_readsw(p,d,l)
@@ -363,11 +470,46 @@ static inline void memcpy_toio(volatile void __iomem *to, const void *from,
 /*
  * io{read,write}{16,32}be() macros
  */
-#define ioread16be(p)		({ __u16 __v = be16_to_cpu((__force __be16)__raw_readw(p)); __iormb(); __v; })
-#define ioread32be(p)		({ __u32 __v = be32_to_cpu((__force __be32)__raw_readl(p)); __iormb(); __v; })
+static inline u16 ioread16be(const volatile void __iomem *c)
+{
+	be16 val = __IOR16(be, c);
+	__iormb();
+	return be16_to_cpu(val);
+}
 
-#define iowrite16be(v,p)	({ __iowmb(); __raw_writew((__force __u16)cpu_to_be16(v), p); })
-#define iowrite32be(v,p)	({ __iowmb(); __raw_writel((__force __u32)cpu_to_be32(v), p); })
+static inline u32 ioread32be(const volatile void __iomem *c)
+{
+	be32 val = __IOR32(be, c);
+	__iormb();
+	return be32_to_cpu(val);
+}
+
+static void iowrite16be(u16 val, volatile void __iomem *c)
+{
+	__iowmb();
+	__IOW16(be, c, cpu_to_be16(val));
+}
+
+static void iowrite32be(u32 val, volatile void __iomem *c)
+{
+	__iowmb();
+	__IOW32(be, c, cpu_to_be32(val));
+}
+
+/*
+ * Don't let people use these macros in their code - they're an arch
+ * implementation detail.
+ */
+#undef __iowmb
+#undef __iormb
+#undef __IOW8
+#undef __IOW16
+#undef __IOW32
+#undef __IOW_OP
+#undef __IOR8
+#undef __IOR16
+#undef __IOR32
+#undef __IOR_OP
 
 #ifndef ioport_map
 #define ioport_map ioport_map


-- 
FTTC broadband for 0.8mile line: currently@10.5Mbps down 400kbps up
according to speedtest.net.