Re: DMA debug trace pointing to rtl8187

Re: DMA debug trace pointing to rtl8187

[Greg KH]

On Wed, May 06, 2009 at 08:35:52AM +0200, Eric Valette wrote:
> FUJITA Tomonori wrote:
> > CC'ed linux-usb,
> > 
> > The ehci_hcd driver uses buffers on the stack for DMA?
> > 
> > On Sun, 03 May 2009 17:36:24 +0200
> > Eric Valette <eric.valette@free.fr> wrote:
> > 
> >> ------------[ cut here ]------------
> >>
> >> WARNING: at /usr/src/linux-2.6.22.9/lib/dma-debug.c:609
> > 
> > Hmm, the kernel version is wired. lib/dma-debug.c was added in
> > 2.6.30-rc.
> 
> No that's the file path.  I use ketchup to apply patches...
> 
> > 
> >> check_for_stack+0x6b/0x8b()
> >> Hardware name: P5W DH Deluxe
> >>
> >> ehci_hcd 0000:00:1d.7: DMA-API: device driver maps memory fromstack
> >> [addr=ffff88007fa79968]
> >> Modules linked in:
> >>
> >> Pid: 297, comm: khubd Not tainted 2.6.30-rc4-git1 #32
> 
> Here is the real version.

The problem is in the rtl8187 driver.

They are calling usb_control_msg and passing a pointer to a buffer on
the stack.  See drivers/net/wireless/rtl818x/rtl8187.h for where the
problem happens in numerous places.

Also it looks like rtl8225_write_8051() is incorrect.  You are passing a
pointer to a variable that was passed as an argument.  I don't know
where that is supposed to be on, somewhere on the stack I guess.

Larry, care to fix this up?

thanks,

greg k-h

[RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[John W. Linville]

This definitely needs to fail more gracefully in the event of a
kmalloc failure...

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/rtl818x/rtl8187.h         |   72 ++++++++++++++++++++----
 drivers/net/wireless/rtl818x/rtl8187_rtl8225.c |   11 +++-
 2 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187.h
index 622196d..600ae34 100644
--- a/drivers/net/wireless/rtl818x/rtl8187.h
+++ b/drivers/net/wireless/rtl818x/rtl8187.h
@@ -134,13 +134,21 @@ void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
 static inline u8 rtl818x_ioread8_idx(struct rtl8187_priv *priv,
 				     u8 *addr, u8 idx)
 {
-	u8 val;
+	u8 val, *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
 
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
+			(unsigned long)addr, idx & 0x03, buf,
 			sizeof(val), HZ / 2);
 
+	val = *buf;
+	kfree(buf);
+
 	return val;
 }
 
@@ -152,13 +160,21 @@ static inline u8 rtl818x_ioread8(struct rtl8187_priv *priv, u8 *addr)
 static inline u16 rtl818x_ioread16_idx(struct rtl8187_priv *priv,
 				       __le16 *addr, u8 idx)
 {
-	__le16 val;
+	__le16 val, *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
 
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
+			(unsigned long)addr, idx & 0x03, buf,
 			sizeof(val), HZ / 2);
 
+	val = *buf;
+	kfree(buf);
+
 	return le16_to_cpu(val);
 }
 
@@ -170,13 +186,21 @@ static inline u16 rtl818x_ioread16(struct rtl8187_priv *priv, __le16 *addr)
 static inline u32 rtl818x_ioread32_idx(struct rtl8187_priv *priv,
 				       __le32 *addr, u8 idx)
 {
-	__le32 val;
+	__le32 val, *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
 
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
+			(unsigned long)addr, idx & 0x03, buf,
 			sizeof(val), HZ / 2);
 
+	val = *buf;
+	kfree(buf);
+
 	return le32_to_cpu(val);
 }
 
@@ -188,10 +212,20 @@ static inline u32 rtl818x_ioread32(struct rtl8187_priv *priv, __le32 *addr)
 static inline void rtl818x_iowrite8_idx(struct rtl8187_priv *priv,
 					u8 *addr, u8 val, u8 idx)
 {
+	u8 *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
+	*buf = val;
+
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &val,
+			(unsigned long)addr, idx & 0x03, buf,
 			sizeof(val), HZ / 2);
+
+	kfree(buf);
 }
 
 static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
@@ -202,12 +236,20 @@ static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
 static inline void rtl818x_iowrite16_idx(struct rtl8187_priv *priv,
 					 __le16 *addr, u16 val, u8 idx)
 {
-	__le16 buf = cpu_to_le16(val);
+	__le16 *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
+	*buf = cpu_to_le16(val);
 
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
+			(unsigned long)addr, idx & 0x03, buf, sizeof(*buf),
 			HZ / 2);
+
+	kfree(buf);
 }
 
 static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
@@ -219,12 +261,20 @@ static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
 static inline void rtl818x_iowrite32_idx(struct rtl8187_priv *priv,
 					 __le32 *addr, u32 val, u8 idx)
 {
-	__le32 buf = cpu_to_le32(val);
+	__le32 *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
+	*buf = cpu_to_le32(val);
 
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
+			(unsigned long)addr, idx & 0x03, buf, sizeof(*buf),
 			HZ / 2);
+
+	kfree(buf);
 }
 
 static inline void rtl818x_iowrite32(struct rtl8187_priv *priv, __le32 *addr,
diff --git a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
index 78df281..6c6dba2 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
@@ -70,6 +70,13 @@ static void rtl8225_write_8051(struct ieee80211_hw *dev, u8 addr, __le16 data)
 {
 	struct rtl8187_priv *priv = dev->priv;
 	u16 reg80, reg82, reg84;
+	__le16 *buf;
+
+	/* Use "DMA-aware" buffer. */
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		BUG(); /* TODO -- handle this more gracefully */
+	*buf = data;
 
 	reg80 = rtl818x_ioread16(priv, &priv->map->RFPinsOutput);
 	reg82 = rtl818x_ioread16(priv, &priv->map->RFPinsEnable);
@@ -90,13 +97,15 @@ static void rtl8225_write_8051(struct ieee80211_hw *dev, u8 addr, __le16 data)
 
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			addr, 0x8225, &data, sizeof(data), HZ / 2);
+			addr, 0x8225, buf, sizeof(*buf), HZ / 2);
 
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80 | (1 << 2));
 	udelay(10);
 
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80 | (1 << 2));
 	rtl818x_iowrite16(priv, &priv->map->RFPinsSelect, reg84);
+
+	kfree(buf);
 }
 
 static void rtl8225_write(struct ieee80211_hw *dev, u8 addr, u16 data)
-- 
1.6.0.6

Re: DMA debug trace pointing to rtl8187

[John W. Linville]

On Tue, May 05, 2009 at 11:45:13PM -0700, Greg KH wrote:
> On Wed, May 06, 2009 at 08:35:52AM +0200, Eric Valette wrote:
> > FUJITA Tomonori wrote:
> > > CC'ed linux-usb,
> > > 
> > > The ehci_hcd driver uses buffers on the stack for DMA?
> > > 
> > > On Sun, 03 May 2009 17:36:24 +0200
> > > Eric Valette <eric.valette@free.fr> wrote:
> > > 
> > >> ------------[ cut here ]------------
> > >>
> > >> WARNING: at /usr/src/linux-2.6.22.9/lib/dma-debug.c:609
> > > 
> > > Hmm, the kernel version is wired. lib/dma-debug.c was added in
> > > 2.6.30-rc.
> > 
> > No that's the file path.  I use ketchup to apply patches...
> > 
> > > 
> > >> check_for_stack+0x6b/0x8b()
> > >> Hardware name: P5W DH Deluxe
> > >>
> > >> ehci_hcd 0000:00:1d.7: DMA-API: device driver maps memory fromstack
> > >> [addr=ffff88007fa79968]
> > >> Modules linked in:
> > >>
> > >> Pid: 297, comm: khubd Not tainted 2.6.30-rc4-git1 #32
> > 
> > Here is the real version.
> 
> The problem is in the rtl8187 driver.
> 
> They are calling usb_control_msg and passing a pointer to a buffer on
> the stack.  See drivers/net/wireless/rtl818x/rtl8187.h for where the
> problem happens in numerous places.
> 
> Also it looks like rtl8225_write_8051() is incorrect.  You are passing a
> pointer to a variable that was passed as an argument.  I don't know
> where that is supposed to be on, somewhere on the stack I guess.
> 
> Larry, care to fix this up?

I just sent '[RFT] rtl8187: use DMA-aware buffers with usb_control_msg'
as a parallel reply to this message.  It is pretty ugly w.r.t. kmalloc
failures, but it might be worth testing just to see if it helps the
problem at hand...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Hin-Tak Leung]

On Wed, May 6, 2009 at 7:02 PM, John W. Linville <linville@tuxdriver.com> wrote:
> This definitely needs to fail more gracefully in the event of a
> kmalloc failure...
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

I am  using this patch with compat-wireless with
2.6.29.2-134.fc11.x86_64 - presumably one needs to be using 2.6.30-rcX
to see any problems?

We mostly inherited that code concerned (and its style) from the vendor driver.

On Wed, May 6, 2009 at 7:45 AM, Greg KH <greg@kroah.com> wrote:

> Also it looks like rtl8225_write_8051() is incorrect.  You are passing a
> pointer to a variable that was passed as an argument.  I don't know
> where that is supposed to be on, somewhere on the stack I guess.
>
> Larry, care to fix this up?

I think John's patch resolves this issue to the extent it can -
rtl8225_write_8051() is called by rtl8225_write() which in turns gets
its input mostly in the form of magic contants (except a few
instances). So allocating a buffer and copying the magic contants in,
just before calling usb_control_msg() seems an acceptable way forward?

Hin-Tak

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Eric Valette]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

Hin-Tak Leung wrote:
> On Wed, May 6, 2009 at 7:02 PM, John W. Linville <linville@tuxdriver.com> wrote:
>> This definitely needs to fail more gracefully in the event of a
>> kmalloc failure...
>>
>> Signed-off-by: John W. Linville <linville@tuxdriver.com>

OK guys. Thanks for your support and sorry for the delay: I add to
struggle with drm git to compile with 2.6.30-rc4-git3 (due to various
non yet integraed things my automated rebuild procedure does git fetch
for drm, xf86-video-nouveau, ...) , leading to no X server which for me
is more annoying than a trace in a USB wireless driver that I seldom use
only to test some new wireless feature (two GE port onboard).

The patch fix the DMA warning and the driver seems to work (just
associated it) but I must say that the allocation failure handling path
and the fact that we use now kmalloc for allocating a few bytes in such
a routine makes me worry about possible negative performance impact
unless theses routines are used only in a slow configuration path (did
no took time to red the code due to many other problems).

BTW if someone know who I should send this attached patch for DRM git, I
would gladly forward it.

-- eric



[-- Attachment #2: patch-drm --]
[-- Type: text/plain, Size: 1174 bytes --]

diff --git a/linux-core/drm_os_linux.h b/linux-core/drm_os_linux.h
index f58296b..b47420e 100644
--- a/linux-core/drm_os_linux.h
+++ b/linux-core/drm_os_linux.h
@@ -32,11 +32,6 @@
 /** IRQ handler arguments and return type and values */
 #define DRM_IRQ_ARGS		int irq, void *arg
 /** backwards compatibility with old irq return values */
-#ifndef IRQ_HANDLED
-typedef void irqreturn_t;
-#define IRQ_HANDLED		/* nothing */
-#define IRQ_NONE		/* nothing */
-#endif
 
 /** AGP types */
 #if __OS_HAS_AGP
diff --git a/linux-core/drm_sysfs.c b/linux-core/drm_sysfs.c
index 6de9367..637f5c2 100644
--- a/linux-core/drm_sysfs.c
+++ b/linux-core/drm_sysfs.c
@@ -162,14 +162,14 @@ int drm_sysfs_device_add(struct drm_minor *minor)
 	int err;
 	int i, j;
 	char *minor_str;
-
+	
 	minor->kdev.parent = &minor->dev->pdev->dev;
 	minor->kdev.class = drm_class;
 	minor->kdev.release = drm_sysfs_device_release;
 	minor->kdev.devt = minor->device;
-	minor_str = "card%d";
-	
-	snprintf(minor->kdev.bus_id, BUS_ID_SIZE, minor_str, minor->index);
+        minor_str = "card%d";
+
+	dev_set_name(&minor->kdev, minor_str, minor->index);
 
 	err = device_register(&minor->kdev);
 	if (err) {

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Greg KH]

On Sat, May 09, 2009 at 11:38:32AM +0200, Eric Valette wrote:
> The patch fix the DMA warning and the driver seems to work (just
> associated it) but I must say that the allocation failure handling path
> and the fact that we use now kmalloc for allocating a few bytes in such
> a routine makes me worry about possible negative performance impact
> unless theses routines are used only in a slow configuration path (did
> no took time to red the code due to many other problems).

usb_control messages are slow and should not be on the "fast path" of
any data being sent through the device.  Any overhead of the
kmalloc/kfree is totally eaten up by the actual transmission turn around
time of the message itself, so you don't have to worry about the
performance impact.

thanks for testing.

> BTW if someone know who I should send this attached patch for DRM git, I
> would gladly forward it.

To the drm maintainer?

thanks,

greg k-h

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[John W. Linville]

On Sat, May 09, 2009 at 06:57:31AM -0700, Greg KH wrote:
> On Sat, May 09, 2009 at 11:38:32AM +0200, Eric Valette wrote:
> > The patch fix the DMA warning and the driver seems to work (just
> > associated it) but I must say that the allocation failure handling path
> > and the fact that we use now kmalloc for allocating a few bytes in such
> > a routine makes me worry about possible negative performance impact
> > unless theses routines are used only in a slow configuration path (did
> > no took time to red the code due to many other problems).
> 
> usb_control messages are slow and should not be on the "fast path" of
> any data being sent through the device.  Any overhead of the
> kmalloc/kfree is totally eaten up by the actual transmission turn around
> time of the message itself, so you don't have to worry about the
> performance impact.

Yeah, I don't like the original version either.  Even if the kmalloc's
aren't a big performance hit, the failure path sucks.  I've included a
new version below, but unfortunately I haven't had a chance to test it.
Please give it a try if you get a chance?

> thanks for testing.

Ditto!

---

>From 73f58f111f3ec5009c603bb4abf86e5ef4c5503f Mon Sep 17 00:00:00 2001
From: John W. Linville <linville@tuxdriver.com>
Date: Wed, 6 May 2009 13:57:27 -0400
Subject: [PATCH] rtl8187: use DMA-aware buffers with usb_control_msg

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/rtl818x/rtl8187.h         |   57 ++++++++++++++++++------
 drivers/net/wireless/rtl818x/rtl8187_dev.c     |    8 +++
 drivers/net/wireless/rtl818x/rtl8187_rtl8225.c |    8 +++-
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187.h
index 622196d..c09bfef 100644
--- a/drivers/net/wireless/rtl818x/rtl8187.h
+++ b/drivers/net/wireless/rtl818x/rtl8187.h
@@ -127,6 +127,12 @@ struct rtl8187_priv {
 		__le64 buf;
 		struct sk_buff_head queue;
 	} b_tx_status; /* This queue is used by both -b and non-b devices */
+	struct mutex io_mutex;
+	union {
+		u8 bits8;
+		__le16 bits16;
+		__le32 bits32;
+	} *io_dmabuf;
 };
 
 void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
@@ -136,10 +142,14 @@ static inline u8 rtl818x_ioread8_idx(struct rtl8187_priv *priv,
 {
 	u8 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits8, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits8;
+	mutex_unlock(&priv->io_mutex);
 
 	return val;
 }
@@ -154,10 +164,14 @@ static inline u16 rtl818x_ioread16_idx(struct rtl8187_priv *priv,
 {
 	__le16 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits16, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits16;
+	mutex_unlock(&priv->io_mutex);
 
 	return le16_to_cpu(val);
 }
@@ -172,10 +186,14 @@ static inline u32 rtl818x_ioread32_idx(struct rtl8187_priv *priv,
 {
 	__le32 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits32, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits32;
+	mutex_unlock(&priv->io_mutex);
 
 	return le32_to_cpu(val);
 }
@@ -188,10 +206,15 @@ static inline u32 rtl818x_ioread32(struct rtl8187_priv *priv, __le32 *addr)
 static inline void rtl818x_iowrite8_idx(struct rtl8187_priv *priv,
 					u8 *addr, u8 val, u8 idx)
 {
+	mutex_lock(&priv->io_mutex);
+
+	priv->io_dmabuf->bits8 = val;
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits8, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
@@ -202,12 +225,15 @@ static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
 static inline void rtl818x_iowrite16_idx(struct rtl8187_priv *priv,
 					 __le16 *addr, u16 val, u8 idx)
 {
-	__le16 buf = cpu_to_le16(val);
+	mutex_lock(&priv->io_mutex);
 
+	priv->io_dmabuf->bits16 = cpu_to_le16(val);
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
-			HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits16, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
@@ -219,12 +245,15 @@ static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
 static inline void rtl818x_iowrite32_idx(struct rtl8187_priv *priv,
 					 __le32 *addr, u32 val, u8 idx)
 {
-	__le32 buf = cpu_to_le32(val);
+	mutex_lock(&priv->io_mutex);
 
+	priv->io_dmabuf->bits32 = cpu_to_le32(val);
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
-			HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits32, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite32(struct rtl8187_priv *priv, __le32 *addr,
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 158827e..a492abd 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1326,6 +1326,14 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 	priv = dev->priv;
 	priv->is_rtl8187b = (id->driver_info == DEVICE_RTL8187B);
 
+	/* allocate "DMA aware" buffer for register accesses */
+	priv->io_dmabuf = kmalloc(sizeof(*priv->io_dmabuf), GFP_KERNEL);
+	if (!priv->io_dmabuf) {
+		err = -ENOMEM;
+		goto err_free_dev;
+	}
+	mutex_init(&priv->io_mutex);
+
 	SET_IEEE80211_DEV(dev, &intf->dev);
 	usb_set_intfdata(intf, dev);
 	priv->udev = udev;
diff --git a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
index 78df281..a098193 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
@@ -88,9 +88,15 @@ static void rtl8225_write_8051(struct ieee80211_hw *dev, u8 addr, __le16 data)
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80);
 	udelay(10);
 
+	mutex_lock(&priv->io_mutex);
+
+	priv->io_dmabuf->bits16 = data;
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			addr, 0x8225, &data, sizeof(data), HZ / 2);
+			addr, 0x8225, &priv->io_dmabuf->bits16, sizeof(data),
+			HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80 | (1 << 2));
 	udelay(10);
-- 
1.6.0.6

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Greg KH]

On Sat, May 09, 2009 at 11:50:14AM -0400, John W. Linville wrote:
> > usb_control messages are slow and should not be on the "fast path" of
> > any data being sent through the device.  Any overhead of the
> > kmalloc/kfree is totally eaten up by the actual transmission turn around
> > time of the message itself, so you don't have to worry about the
> > performance impact.
> 
> Yeah, I don't like the original version either.  Even if the kmalloc's
> aren't a big performance hit, the failure path sucks.  I've included a
> new version below, but unfortunately I haven't had a chance to test it.
> Please give it a try if you get a chance?

Looks good to me, a bit cleaner even.

thanks,

greg k-h

Re: DMA debug trace pointing to rtl8187

[Larry Finger]

Greg KH wrote:
> 
> The problem is in the rtl8187 driver.
> 
> They are calling usb_control_msg and passing a pointer to a buffer on
> the stack.  See drivers/net/wireless/rtl818x/rtl8187.h for where the
> problem happens in numerous places.
> 
> Also it looks like rtl8225_write_8051() is incorrect.  You are passing a
> pointer to a variable that was passed as an argument.  I don't know
> where that is supposed to be on, somewhere on the stack I guess.
> 
> Larry, care to fix this up?

Yes, I'll try to fix it. I'm currently traveling and have intermittent Internet
access.

I think there is a second problem that John's fix does not treat. Although the
buffer is removed from the stack, there is no assurance that the buffer obtained
with kmalloc() is reachable by DMA. This case will be triggered if the USB
adapter does 32-bit DMA and the system has more than 4 GB RAM.

Please let me know if my analysis is wrong. If so, then John's patch will be
fine, although the error handling should be improved. The severity should be
that of a warning rather than a bug. If I'm correct, my fix would be to allocate
a DMA-reachable buffer in the initialization and keep a pointer to it in the
private area.

I just saw John's version 2 that looks more like what I was thinking about. I
will be testing soon.

Thanks,

Larry

Re: DMA debug trace pointing to rtl8187

[Eric Valette]

Larry Finger wrote:

> I just saw John's version 2 that looks more like what I was thinking about. I
> will be testing soon.

Should find time to compile, test and browse the WEB a little using WiFi
at 9PM (France = UTC +2 right now).

I'll report ASAP.

--eric

Re: DMA debug trace pointing to rtl8187

[Eric Valette]

Eric Valette wrote:
> Larry Finger wrote:
> 
>> I just saw John's version 2 that looks more like what I was thinking about. I
>> will be testing soon.
> 
> Should find time to compile, test and browse the WEB a little using WiFi
> at 9PM (France = UTC +2 right now).

Well is 9:20 PM and the new patch has been tested on 2.6.30-rc5. This
mail will be sent using my wlan0 interface.

Would like to say thanks to all of you who helped analysing and fixing it.

Whish you all a nice sunday.

-- eric

Re: DMA debug trace pointing to rtl8187

[Greg KH]

On Sat, May 09, 2009 at 12:29:27PM -0500, Larry Finger wrote:
> I think there is a second problem that John's fix does not treat. Although the
> buffer is removed from the stack, there is no assurance that the buffer obtained
> with kmalloc() is reachable by DMA. This case will be triggered if the USB
> adapter does 32-bit DMA and the system has more than 4 GB RAM.

Memory returned by kmalloc will always be able to be DMAable.  If not,
we have lots of problems :)

thanks,

greg k-h

Re: DMA debug trace pointing to rtl8187

[Michael Buesch]

On Saturday 09 May 2009 21:29:59 Greg KH wrote:
> On Sat, May 09, 2009 at 12:29:27PM -0500, Larry Finger wrote:
> > I think there is a second problem that John's fix does not treat. Although the
> > buffer is removed from the stack, there is no assurance that the buffer obtained
> > with kmalloc() is reachable by DMA. This case will be triggered if the USB
> > adapter does 32-bit DMA and the system has more than 4 GB RAM.

In practice this does not hit, because such systems' kmalloc does not return
memory above 4G (i386) _or_ the DMA mapping functions take care of bounce buffering
or I/O-remapping (should be true for all other arches).
So if the device is able to do DMA with addresses >=32bit it should be fine, provided
it correctly sets the DMA mask.

> Memory returned by kmalloc will always be able to be DMAable.  If not,
> we have lots of problems :)

True for sane devices.
False for devices like Broadcom HND-DMA, which should only be used to slap thy hw engineers.

-- 
Greetings, Michael.

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Larry Finger]

John W. Linville wrote:
> 
> Yeah, I don't like the original version either.  Even if the kmalloc's
> aren't a big performance hit, the failure path sucks.  I've included a
> new version below, but unfortunately I haven't had a chance to test it.
> Please give it a try if you get a chance?

This one tests just fine here running on the latest pull of wireless testing
(v2.6.30-rc4-22732-gd879ac6). I think there is a leak of priv->io_dmabuf when
the driver is unloaded, and kfree(priv->io_dmabuf) should be added to
rtl8187_disconnect(). Otherwise ACK.

Larry

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[John W. Linville]

On Sat, May 09, 2009 at 04:24:21PM -0500, Larry Finger wrote:
> John W. Linville wrote:
> > 
> > Yeah, I don't like the original version either.  Even if the kmalloc's
> > aren't a big performance hit, the failure path sucks.  I've included a
> > new version below, but unfortunately I haven't had a chance to test it.
> > Please give it a try if you get a chance?
> 
> This one tests just fine here running on the latest pull of wireless testing
> (v2.6.30-rc4-22732-gd879ac6). I think there is a leak of priv->io_dmabuf when
> the driver is unloaded, and kfree(priv->io_dmabuf) should be added to
> rtl8187_disconnect(). Otherwise ACK.

Doh!

---

>From dbc8e19329b52c53832cbb03eea76646e44aab07 Mon Sep 17 00:00:00 2001
From: John W. Linville <linville@tuxdriver.com>
Date: Wed, 6 May 2009 13:57:27 -0400
Subject: [PATCH] rtl8187: use DMA-aware buffers with usb_control_msg

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/rtl818x/rtl8187.h         |   57 ++++++++++++++++++------
 drivers/net/wireless/rtl818x/rtl8187_dev.c     |   13 +++++-
 drivers/net/wireless/rtl818x/rtl8187_rtl8225.c |    8 +++-
 3 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187.h
index 622196d..c09bfef 100644
--- a/drivers/net/wireless/rtl818x/rtl8187.h
+++ b/drivers/net/wireless/rtl818x/rtl8187.h
@@ -127,6 +127,12 @@ struct rtl8187_priv {
 		__le64 buf;
 		struct sk_buff_head queue;
 	} b_tx_status; /* This queue is used by both -b and non-b devices */
+	struct mutex io_mutex;
+	union {
+		u8 bits8;
+		__le16 bits16;
+		__le32 bits32;
+	} *io_dmabuf;
 };
 
 void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
@@ -136,10 +142,14 @@ static inline u8 rtl818x_ioread8_idx(struct rtl8187_priv *priv,
 {
 	u8 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits8, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits8;
+	mutex_unlock(&priv->io_mutex);
 
 	return val;
 }
@@ -154,10 +164,14 @@ static inline u16 rtl818x_ioread16_idx(struct rtl8187_priv *priv,
 {
 	__le16 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits16, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits16;
+	mutex_unlock(&priv->io_mutex);
 
 	return le16_to_cpu(val);
 }
@@ -172,10 +186,14 @@ static inline u32 rtl818x_ioread32_idx(struct rtl8187_priv *priv,
 {
 	__le32 val;
 
+	mutex_lock(&priv->io_mutex);
 	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
 			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits32, sizeof(val), HZ / 2);
+
+	val = priv->io_dmabuf->bits32;
+	mutex_unlock(&priv->io_mutex);
 
 	return le32_to_cpu(val);
 }
@@ -188,10 +206,15 @@ static inline u32 rtl818x_ioread32(struct rtl8187_priv *priv, __le32 *addr)
 static inline void rtl818x_iowrite8_idx(struct rtl8187_priv *priv,
 					u8 *addr, u8 val, u8 idx)
 {
+	mutex_lock(&priv->io_mutex);
+
+	priv->io_dmabuf->bits8 = val;
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &val,
-			sizeof(val), HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits8, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
@@ -202,12 +225,15 @@ static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val)
 static inline void rtl818x_iowrite16_idx(struct rtl8187_priv *priv,
 					 __le16 *addr, u16 val, u8 idx)
 {
-	__le16 buf = cpu_to_le16(val);
+	mutex_lock(&priv->io_mutex);
 
+	priv->io_dmabuf->bits16 = cpu_to_le16(val);
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
-			HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits16, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
@@ -219,12 +245,15 @@ static inline void rtl818x_iowrite16(struct rtl8187_priv *priv, __le16 *addr,
 static inline void rtl818x_iowrite32_idx(struct rtl8187_priv *priv,
 					 __le32 *addr, u32 val, u8 idx)
 {
-	__le32 buf = cpu_to_le32(val);
+	mutex_lock(&priv->io_mutex);
 
+	priv->io_dmabuf->bits32 = cpu_to_le32(val);
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			(unsigned long)addr, idx & 0x03, &buf, sizeof(buf),
-			HZ / 2);
+			(unsigned long)addr, idx & 0x03,
+			&priv->io_dmabuf->bits32, sizeof(val), HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 }
 
 static inline void rtl818x_iowrite32(struct rtl8187_priv *priv, __le32 *addr,
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 158827e..6499ccc 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1326,6 +1326,14 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 	priv = dev->priv;
 	priv->is_rtl8187b = (id->driver_info == DEVICE_RTL8187B);
 
+	/* allocate "DMA aware" buffer for register accesses */
+	priv->io_dmabuf = kmalloc(sizeof(*priv->io_dmabuf), GFP_KERNEL);
+	if (!priv->io_dmabuf) {
+		err = -ENOMEM;
+		goto err_free_dev;
+	}
+	mutex_init(&priv->io_mutex);
+
 	SET_IEEE80211_DEV(dev, &intf->dev);
 	usb_set_intfdata(intf, dev);
 	priv->udev = udev;
@@ -1489,7 +1497,7 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 	err = ieee80211_register_hw(dev);
 	if (err) {
 		printk(KERN_ERR "rtl8187: Cannot register device\n");
-		goto err_free_dev;
+		goto err_free_dmabuf;
 	}
 	mutex_init(&priv->conf_mutex);
 	skb_queue_head_init(&priv->b_tx_status.queue);
@@ -1506,6 +1514,8 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 
 	return 0;
 
+ err_free_dmabuf:
+	kfree(priv->io_dmabuf);
  err_free_dev:
 	ieee80211_free_hw(dev);
 	usb_set_intfdata(intf, NULL);
@@ -1529,6 +1539,7 @@ static void __devexit rtl8187_disconnect(struct usb_interface *intf)
 	priv = dev->priv;
 	usb_reset_device(priv->udev);
 	usb_put_dev(interface_to_usbdev(intf));
+	kfree(priv->io_dmabuf);
 	ieee80211_free_hw(dev);
 }
 
diff --git a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
index 78df281..a098193 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_rtl8225.c
@@ -88,9 +88,15 @@ static void rtl8225_write_8051(struct ieee80211_hw *dev, u8 addr, __le16 data)
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80);
 	udelay(10);
 
+	mutex_lock(&priv->io_mutex);
+
+	priv->io_dmabuf->bits16 = data;
 	usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
 			RTL8187_REQ_SET_REG, RTL8187_REQT_WRITE,
-			addr, 0x8225, &data, sizeof(data), HZ / 2);
+			addr, 0x8225, &priv->io_dmabuf->bits16, sizeof(data),
+			HZ / 2);
+
+	mutex_unlock(&priv->io_mutex);
 
 	rtl818x_iowrite16(priv, &priv->map->RFPinsOutput, reg80 | (1 << 2));
 	udelay(10);
-- 
1.6.0.6

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [RFT] rtl8187: use DMA-aware buffers with usb_control_msg

[Hin-Tak Leung]

On Mon, May 11, 2009 at 2:20 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Sat, May 09, 2009 at 04:24:21PM -0500, Larry Finger wrote:
>> John W. Linville wrote:
>> >
>> > Yeah, I don't like the original version either. =A0Even if the kma=
lloc's
>> > aren't a big performance hit, the failure path sucks. =A0I've incl=
uded a
>> > new version below, but unfortunately I haven't had a chance to tes=
t it.
>> > Please give it a try if you get a chance?
>>
>> This one tests just fine here running on the latest pull of wireless=
 testing
>> (v2.6.30-rc4-22732-gd879ac6). I think there is a leak of priv->io_dm=
abuf when
>> the driver is unloaded, and kfree(priv->io_dmabuf) should be added t=
o
>> rtl8187_disconnect(). Otherwise ACK.
>
> Doh!
>
> ---
>
> From dbc8e19329b52c53832cbb03eea76646e44aab07 Mon Sep 17 00:00:00 200=
1
> From: John W. Linville <linville@tuxdriver.com>
> Date: Wed, 6 May 2009 13:57:27 -0400
> Subject: [PATCH] rtl8187: use DMA-aware buffers with usb_control_msg
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

This looks so much cleaner. ACK - I see you already checked this in to
wireless-testing. Tried to give it a go just now, but got bitten by
the wext key management change, I think.

Hin-Tak
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html