parisc: add barriers to mmio accessors

parisc: add barriers to mmio accessors

[Kyle McMartin]

Prevents GCC from reordering mmio accesses with regular memory
accesses.

They're in assembly since I felt like maybe using the strongly
ordered completer on the instructions, so they would be more obvious
in disassembly. (Yes, I know there's no weakly ordered PA
implementation...)


diff --git a/include/asm-parisc/io.h b/include/asm-parisc/io.h
index 55ddb18..e5ce0a9 100644
--- a/include/asm-parisc/io.h
+++ b/include/asm-parisc/io.h
@@ -148,36 +148,82 @@ extern void iounmap(const volatile void __iomem *addr);
 
 static inline unsigned char __raw_readb(const volatile void __iomem *addr)
 {
-	return (*(volatile unsigned char __force *) (addr));
+	unsigned char c;
+	asm volatile(
+	"	ldb	0(%0), %1\n"
+	: : "r"(addr), "r"(c) : "memory");
+	return c;
 }
+
 static inline unsigned short __raw_readw(const volatile void __iomem *addr)
 {
-	return *(volatile unsigned short __force *) addr;
+	unsigned short s;
+	asm volatile(
+	"	lds	0(%0), %1\n"
+	: : "r"(addr), "r"(s) : "memory");
+	return s;
 }
+
 static inline unsigned int __raw_readl(const volatile void __iomem *addr)
 {
-	return *(volatile unsigned int __force *) addr;
+	unsigned int i;
+	asm volatile(
+	"	ldw	0(%0), %1\n"
+	: : "r"(addr), "r"(i) : "memory");
+	return i;
 }
+
 static inline unsigned long long __raw_readq(const volatile void __iomem *addr)
 {
-	return *(volatile unsigned long long __force *) addr;
+	unsigned long long q;
+
+#ifdef CONFIG_64BIT
+	asm volatile(
+	"	ldq	0(%0), %1\n"
+	: : "r"(addr), "r"(q) : "memory");
+#else
+	unsigned int q_lo, q_hi;
+	q_hi = __raw_readl(addr);
+	q_lo = __raw_readl(addr+4);
+	q = (unsigned long long)(q_hi << 32) | (q_lo);
+#endif
+
+	return q;
 }
 
-static inline void __raw_writeb(unsigned char b, volatile void __iomem *addr)
+static inline void __raw_writeb(unsigned char c, volatile void __iomem *addr)
 {
-	*(volatile unsigned char __force *) addr = b;
+	asm volatile(
+	"	stb	%1, 0(%0)\n"
+	: : "r"(addr), "r"(c) : "memory");
 }
-static inline void __raw_writew(unsigned short b, volatile void __iomem *addr)
+
+static inline void __raw_writew(unsigned short s, volatile void __iomem *addr)
 {
-	*(volatile unsigned short __force *) addr = b;
+	asm volatile(
+	"	sts	%1, 0(%0)\n"
+	: : "r"(addr), "r"(s) : "memory");
 }
-static inline void __raw_writel(unsigned int b, volatile void __iomem *addr)
+
+static inline void __raw_writel(unsigned int i, volatile void __iomem *addr)
 {
-	*(volatile unsigned int __force *) addr = b;
+	asm volatile(
+	"	stw	%1, 0(%0)\n"
+	: : "r"(addr), "r"(i) : "memory");
 }
-static inline void __raw_writeq(unsigned long long b, volatile void __iomem *addr)
+
+static inline void __raw_writeq(unsigned long long q, volatile void __iomem *addr)
 {
-	*(volatile unsigned long long __force *) addr = b;
+#ifdef CONFIG_64BIT
+	asm volatile(
+	"	stq	%1, 0(%0)\n"
+	: : "r"(addr), "r"(q) : "memory");
+#else
+	unsigned int q_hi = (q >> 32) & ~0UL;
+	unsigned int q_lo = (q) & ~0UL;
+	__raw_writel(q_hi, addr);
+	__raw_writel(q_lo, addr+4);
+#endif
 }
 
 /* readb can never be const, so use __fswab instead of le*_to_cpu */

Re: parisc: add barriers to mmio accessors

[Carlos O'Donell]

On Tue, Jun 3, 2008 at 1:42 AM, Kyle McMartin <kyle@mcmartin.ca> wrote:
> Prevents GCC from reordering mmio accesses with regular memory
> accesses.
>
> They're in assembly since I felt like maybe using the strongly
> ordered completer on the instructions, so they would be more obvious
> in disassembly. (Yes, I know there's no weakly ordered PA
> implementation...)
> +#ifdef CONFIG_64BIT
> +       asm volatile(
> +       "       ldq     0(%0), %1\n"
> +       : : "r"(addr), "r"(q) : "memory");
> +#else
> +       unsigned int q_lo, q_hi;
> +       q_hi = __raw_readl(addr);
> +       q_lo = __raw_readl(addr+4);
> +       q = (unsigned long long)(q_hi << 32) | (q_lo);
> +#endif
> +
> +       return q;

Is there any reason the 32-bit version is uses plain C?

Cheers,
Carlos.

Re: parisc: add barriers to mmio accessors

[Carlos O'Donell]

On Tue, Jun 3, 2008 at 8:22 AM, Carlos O'Donell <carlos@systemhalted.org> wrote:
>> +#ifdef CONFIG_64BIT
>> +       asm volatile(
>> +       "       ldq     0(%0), %1\n"
>> +       : : "r"(addr), "r"(q) : "memory");
>> +#else
>> +       unsigned int q_lo, q_hi;
>> +       q_hi = __raw_readl(addr);
>> +       q_lo = __raw_readl(addr+4);
>> +       q = (unsigned long long)(q_hi << 32) | (q_lo);
>> +#endif
>> +
>> +       return q;
>
> Is there any reason the 32-bit version is uses plain C?

I see, you optimized this for the 64-bit case, that makes sense. I
should drink more coffee.

Cheers,
Carlos.

Re: parisc: add barriers to mmio accessors

[Matthew Wilcox]

On Tue, Jun 03, 2008 at 01:42:12AM -0400, Kyle McMartin wrote:
> +#ifdef CONFIG_64BIT
> +	asm volatile(
> +	"	ldq	0(%0), %1\n"
> +	: : "r"(addr), "r"(q) : "memory");
> +#else
> +	unsigned int q_lo, q_hi;
> +	q_hi = __raw_readl(addr);
> +	q_lo = __raw_readl(addr+4);
> +	q = (unsigned long long)(q_hi << 32) | (q_lo);
> +#endif

Are you sure this is correct?  Seems to me it should be:

	q = ((unsigned long long)q_hi << 32) | q_lo;

I would have thought GCC would complain about a shift exceeding the
width of the type.

> +static inline void __raw_writeq(unsigned long long q, volatile void __iomem *addr)
>  {
> -	*(volatile unsigned long long __force *) addr = b;
> +#ifdef CONFIG_64BIT
> +	asm volatile(
> +	"	stq	%1, 0(%0)\n"
> +	: : "r"(addr), "r"(q) : "memory");
> +#else
> +	unsigned int q_hi = (q >> 32) & ~0UL;
> +	unsigned int q_lo = (q) & ~0UL;
> +	__raw_writel(q_hi, addr);
> +	__raw_writel(q_lo, addr+4);
> +#endif

It feels a little funny to be masking with UL when assigning to an
unsigned int.  I'd personally use 0xffffffff or ~0U or nothing at all
since it'll be truncated anyway.  (I recognise the value of saying "I do
intend to truncate this value" explicitly though.)

A third thing is that you're doing this to the __raw_ variants which
don't have to be serialised.  How about we keep the current definitions of
__raw_readX/__raw_writeX, define the regular readX/writeX to be the inline
assembler you've just posted, and add new defines of __readX/__writeX
as byteswapping versions of __raw_readX/__raw_writeX?

[For those who aren't on linux-arch, there's just been a long thread
about the semantics of the different accessors and the above reflects my
understanding of that thread.]

Should we add memory clobbers to gsc_readX/gsc_writeX?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: parisc: add barriers to mmio accessors

[Kyle McMartin]

On Tue, Jun 03, 2008 at 06:42:29AM -0600, Matthew Wilcox wrote:
> Are you sure this is correct?  Seems to me it should be:
> 
> 	q = ((unsigned long long)q_hi << 32) | q_lo;
> 
> I would have thought GCC would complain about a shift exceeding the
> width of the type.
> 

I probably fat fingered the braces.

> A third thing is that you're doing this to the __raw_ variants which
> don't have to be serialised.  How about we keep the current definitions of
> __raw_readX/__raw_writeX, define the regular readX/writeX to be the inline
> assembler you've just posted, and add new defines of __readX/__writeX
> as byteswapping versions of __raw_readX/__raw_writeX?
> 

I was just going to define them all in terms of this... since, well,
performance is mostly irrelevant.

> [For those who aren't on linux-arch, there's just been a long thread
> about the semantics of the different accessors and the above reflects my
> understanding of that thread.]
> 
> Should we add memory clobbers to gsc_readX/gsc_writeX?
> 

Yeah, I think so (although, I really hope GCC won't reorder serializing
instructions like rsm/ssm. ;-)