Skip to contentSkip to conversation

ARM: fix write*() I/O accessors

PATCH New Wolfgang Denk 3 participants 4 messages Feb 11, 2011 Arm (32-bit)

lore
Share & tools
lore

Wolfgang Denk

author
#0 lore raw

PATCH 1/1 ARM: fix write*() I/O accessors

Commit 3c0659b "ARM: Avoid compiler optimization for readb, writeb and friends." introduced I/O accessors with memory barriers. Unfortunately the new write*() accessors introduced a bug:

The problem is that the argument "v" gets evaluated twice. This breaks code like used here (from "drivers/net/dnet.c"):

	for (i = 0; i < wrsz; i++)
		writel(*bufp++, &dnet->regs->TX_DATA_FIFO);

Use auxiliary variables to avoid such problems.

Signed-off-by Wolfgang Denk <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Alexander Holler <[email protected]>
Cc Dirk Behme <[email protected]>

---

This patch fixes a pretty nasty problem. Everybody who has experienced strange failures on ARM lately should apply it ASAP and re-test. As far as I can tell at least the following drivers are affected:

drivers/net/dnet.c: writel(*bufp++, &dnet->regs->TX_DATA_FIFO);
drivers/usb/musb/musb_core.c: writeb(*data++, &musbr->fifox[ep]);
drivers/mmc/pxa_mmc.c: writel(*src++, MMC_TXFIFO);
drivers/mmc/mxcmmc.c: writel(*buf++, &host->base->buffer_access);
drivers/spi/davinci_spi.c: writel(data1_reg_val | *txp++, &ds->regs->dat1);

Albert, please apply ASAP!

 arch/arm/include/asm/io.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
arch/arm/include/asm/io.h +3 -3
index 3886f15..1fbc531 100644
@@ -133,9 +133,9 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)
 #define __iormb()	dmb()
 #define __iowmb()	dmb()
 
-#define writeb(v,c)	({ __iowmb(); __arch_putb(v,c); v; })
-#define writew(v,c)	({ __iowmb(); __arch_putw(v,c); v; })
-#define writel(v,c)	({ __iowmb(); __arch_putl(v,c); v; })
+#define writeb(v,c)	({ u8  __v = v; __iowmb(); __arch_putb(__v,c); __v; })
+#define writew(v,c)	({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; })
+#define writel(v,c)	({ u32 __v = v; __iowmb(); __arch_putl(__v,c); __v; })
 
 #define readb(c)	({ u8  __v = __arch_getb(c); __iormb(); __v; })
 #define readw(c)	({ u16 __v = __arch_getw(c); __iormb(); __v; })
Signature · 1 line
1.7.4
#1 lore raw

PATCH 1/1 ARM: fix write*() I/O accessors

Le 11/02/2011 23:25, Wolfgang Denk a ?crit :

Quoted 23 lines

Commit 3c0659b "ARM: Avoid compiler optimization for readb, writeb and friends." introduced I/O accessors with memory barriers. Unfortunately the new write*() accessors introduced a bug:

The problem is that the argument "v" gets evaluated twice. This breaks code like used here (from "drivers/net/dnet.c"):

	for (i = 0; i<  wrsz; i++)
		writel(*bufp++,&dnet->regs->TX_DATA_FIFO);

Use auxiliary variables to avoid such problems.

Signed-off-by Wolfgang Denk<[email protected]>
Cc Albert Aribaud<[email protected]>
Cc Alexander Holler<[email protected]>
Cc Dirk Behme<[email protected]>

---

This patch fixes a pretty nasty problem. Everybody who has experienced strange failures on ARM lately should apply it ASAP and re-test. As far as I can tell at least the following drivers are affected:

Quoted 7 lines

drivers/net/dnet.c: writel(*bufp++,&dnet->regs->TX_DATA_FIFO);
drivers/usb/musb/musb_core.c: writeb(*data++,&musbr->fifox[ep]);
drivers/mmc/pxa_mmc.c: writel(*src++, MMC_TXFIFO);
drivers/mmc/mxcmmc.c: writel(*buf++,&host->base->buffer_access);
drivers/spi/davinci_spi.c: writel(data1_reg_val | *txp++,&ds->regs->dat1);

Albert, please apply ASAP!

Applied, available on u-boot-arm/master.

Amicalement,

Signature · 1 line
Albert.
author
#2 lore raw

PATCH 1/1 ARM: fix write*() I/O accessors

Dear Albert ARIBAUD,

Quoted 12 lines · In message <[email protected]> you wrote:
Quoted 12 lines

This patch fixes a pretty nasty problem. Everybody who has experienced strange failures on ARM lately should apply it ASAP and re-test. As far as I can tell at least the following drivers are affected:

drivers/net/dnet.c: writel(*bufp++,&dnet->regs->TX_DATA_FIFO);
drivers/usb/musb/musb_core.c: writeb(*data++,&musbr->fifox[ep]);
drivers/mmc/pxa_mmc.c: writel(*src++, MMC_TXFIFO);
drivers/mmc/mxcmmc.c: writel(*buf++,&host->base->buffer_access);
drivers/spi/davinci_spi.c: writel(data1_reg_val | *txp++,&ds->regs->dat1);

Quoted 3 lines
Quoted 1 line

Albert, please apply ASAP!

Applied, available on u-boot-arm/master.

Thanks, highly appreciated.

Best regards,

Wolfgang Denk

Signature · 4 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
This is an unauthorized cybernetic announcement.
#3 lore raw

PATCH 1/1 ARM: fix write*() I/O accessors

In reply to Wolfgang Denk

Hello,

Am 11.02.2011 23:25, schrieb Wolfgang Denk:

arch/arm/include/asm/io.h extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) · line 138

Quoted 3 lines
arch/arm/include/asm/io.h +3 -0
+#define writeb(v,c)	({ u8  __v = v; __iowmb(); __arch_putb(__v,c); __v; })
+#define writew(v,c)	({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; })
+#define writel(v,c)	({ u32 __v = v; __iowmb(); __arch_putl(__v,c); __v; })

Thanks a lot, can I have the do {} while() back? I still don't like such macro-making-sense-only constructs. (Just kidding). ;)

Regards,

Alexander