Patchwork [v2,1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

login
register
mail settings
Submitter hayeswang
Date July 25, 2013, 7:59 a.m.
Message ID <1374739144-732-1-git-send-email-hayeswang@realtek.com>
Download mbox | patch
Permalink /patch/261606/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

hayeswang - July 25, 2013, 7:59 a.m.
Some USB buffers use stack which may not be DMA-able.
Use the buffers from kmalloc to replace those one.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r815x.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)
David Miller - July 28, 2013, 3:21 a.m.
From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 25 Jul 2013 15:59:02 +0800

> Some USB buffers use stack which may not be DMA-able.
> Use the buffers from kmalloc to replace those one.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

I don't think it's reasonable to kmalloc() a small integer every time
you want to use a USB message transfer to read or write chip
registers.

Instead, add a scratch buffer to struct r8152 which is allocated once
at driver attach time and which you can use for the transfers.

I think you only need an array of two u32's so something like:

	u32		transfer_buf[2];

ought to be sufficient.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum - July 29, 2013, 5:20 a.m.
On Sat, 2013-07-27 at 20:21 -0700, David Miller wrote:
> From: Hayes Wang <hayeswang@realtek.com>
> Date: Thu, 25 Jul 2013 15:59:02 +0800
> 
> > Some USB buffers use stack which may not be DMA-able.
> > Use the buffers from kmalloc to replace those one.
> > 
> > Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> 
> I don't think it's reasonable to kmalloc() a small integer every time
> you want to use a USB message transfer to read or write chip
> registers.
> 
> Instead, add a scratch buffer to struct r8152 which is allocated once
> at driver attach time and which you can use for the transfers.
> 
> I think you only need an array of two u32's so something like:
> 
> 	u32		transfer_buf[2];
> 
> ought to be sufficient.

We cannot do that. It would violate the rules about DMA coherency.
We must not touch the same cacheline while DMA is in operation.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 29, 2013, 7:46 a.m.
From: Oliver Neukum <oneukum@suse.de>
Date: Mon, 29 Jul 2013 07:20:24 +0200

> On Sat, 2013-07-27 at 20:21 -0700, David Miller wrote:
>> From: Hayes Wang <hayeswang@realtek.com>
>> Date: Thu, 25 Jul 2013 15:59:02 +0800
>> 
>> > Some USB buffers use stack which may not be DMA-able.
>> > Use the buffers from kmalloc to replace those one.
>> > 
>> > Signed-off-by: Hayes Wang <hayeswang@realtek.com>
>> 
>> I don't think it's reasonable to kmalloc() a small integer every time
>> you want to use a USB message transfer to read or write chip
>> registers.
>> 
>> Instead, add a scratch buffer to struct r8152 which is allocated once
>> at driver attach time and which you can use for the transfers.
>> 
>> I think you only need an array of two u32's so something like:
>> 
>> 	u32		transfer_buf[2];
>> 
>> ought to be sufficient.
> 
> We cannot do that. It would violate the rules about DMA coherency.
> We must not touch the same cacheline while DMA is in operation.

Good point, then it'll need to be:

	u32		*transfer_buf;

and allocated appropriately.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 8523922..e9b99ba 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -24,34 +24,43 @@ 
 
 static int pla_read_word(struct usb_device *udev, u16 index)
 {
-	int data, ret;
+	int ret;
 	u8 shift = index & 2;
-	__le32 ocp_data;
+	__le32 *tmp;
+
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
 
 	index &= ~3;
 
 	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 			      RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
-			      index, MCU_TYPE_PLA, &ocp_data, sizeof(ocp_data),
-			      500);
+			      index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
 	if (ret < 0)
-		return ret;
+		goto out2;
 
-	data = __le32_to_cpu(ocp_data);
-	data >>= (shift * 8);
-	data &= 0xffff;
+	ret = __le32_to_cpu(*tmp);
+	ret >>= (shift * 8);
+	ret &= 0xffff;
 
-	return data;
+out2:
+	kfree(tmp);
+	return ret;
 }
 
 static int pla_write_word(struct usb_device *udev, u16 index, u32 data)
 {
-	__le32 ocp_data;
+	__le32 *tmp;
 	u32 mask = 0xffff;
 	u16 byen = BYTE_EN_WORD;
 	u8 shift = index & 2;
 	int ret;
 
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
 	data &= mask;
 
 	if (shift) {
@@ -63,19 +72,20 @@  static int pla_write_word(struct usb_device *udev, u16 index, u32 data)
 
 	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 			      RTL815x_REQ_GET_REGS, RTL815x_REQT_READ,
-			      index, MCU_TYPE_PLA, &ocp_data, sizeof(ocp_data),
-			      500);
+			      index, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500);
 	if (ret < 0)
-		return ret;
+		goto out3;
 
-	data |= __le32_to_cpu(ocp_data) & ~mask;
-	ocp_data = __cpu_to_le32(data);
+	data |= __le32_to_cpu(*tmp) & ~mask;
+	*tmp = __cpu_to_le32(data);
 
 	ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 			      RTL815x_REQ_SET_REGS, RTL815x_REQT_WRITE,
-			      index, MCU_TYPE_PLA | byen, &ocp_data,
-			      sizeof(ocp_data), 500);
+			      index, MCU_TYPE_PLA | byen, tmp, sizeof(*tmp),
+			      500);
 
+out3:
+	kfree(tmp);
 	return ret;
 }