Buffer allocation for USB transfers

Buffer allocation for USB transfers

[Christian Eggers]

In different drivers I've found several methods for allocating buffers 
transfered with usb_control_msg() or usb_submit_urb():

- usb_stor_msg_common() in "drivers/usb/storage/transport.c" uses buffers 
allocated with usb_buffer_alloc(). These buffers are used with 
URB_NO_xxx_DMA_MAP in urb->transfer_flags.

- asix_read_cmd() in "drivers/net/usb/asix.c" uses kmalloc(GFP_KERNEL).

- mcs7830_get_reg() in "drivers/net/usb/mcs7830.c" uses buffers from 
the stack.

At least the latter does not work on my SH-4 platform. It seems that other 
variables on the stack are overwritten after calling usb_control_msg(), probably as 
result of incorrect alignment.

For some reason the second example (kmalloc()) doesn't seem to cause problems (on 
my platform) but is there are guarantee that kmalloc() 
without GFP_DMA does always return a DMA capable buffer?

Shall all buffers used for usb_control_msg() and usb_submit_urb() be 
allocated with usb_buffer_alloc()? It seems that usb_control_msg() doesn't 
offer a way to set the URB_NO_xxx_DMA_MAP in urb->transfer_flags so that 
usb_buffer_alloc() can not be used here???

regards
Christian Eggers

Please CC to ceggers@gmx.de

Re: Buffer allocation for USB transfers

[Robert Hancock]

Christian Eggers wrote:
> In different drivers I've found several methods for allocating buffers 
> transfered with usb_control_msg() or usb_submit_urb():
> 
> - usb_stor_msg_common() in "drivers/usb/storage/transport.c" uses buffers 
> allocated with usb_buffer_alloc(). These buffers are used with 
> URB_NO_xxx_DMA_MAP in urb->transfer_flags.
> 
> - asix_read_cmd() in "drivers/net/usb/asix.c" uses kmalloc(GFP_KERNEL).
> 
> - mcs7830_get_reg() in "drivers/net/usb/mcs7830.c" uses buffers from 
> the stack.
> 
> At least the latter does not work on my SH-4 platform. It seems that other 
> variables on the stack are overwritten after calling usb_control_msg(), probably as 
> result of incorrect alignment.

Indeed, DMA to/from the stack is not allowed (on some platforms it may 
invalidate other data in the same cache line, etc. which is likely what 
you are seeing). Assuming that the USB core is DMAing directly to/from 
that memory, it's a bug in that mcs7830 driver.

> 
> For some reason the second example (kmalloc()) doesn't seem to cause problems (on 
> my platform) but is there are guarantee that kmalloc() 
> without GFP_DMA does always return a DMA capable buffer?

Yes, as long as the DMA mapping API is used properly to map the buffer 
(which I'm assuming it is, not being a USB core expert.) GFP_DMA is not 
intended for this, drivers should not be using that flag, unless maybe 
they are doing ISA transfers which is not the case with USB.

> 
> Shall all buffers used for usb_control_msg() and usb_submit_urb() be 
> allocated with usb_buffer_alloc()? It seems that usb_control_msg() doesn't 
> offer a way to set the URB_NO_xxx_DMA_MAP in urb->transfer_flags so that 
> usb_buffer_alloc() can not be used here???
> 
> regards
> Christian Eggers
> 
> Please CC to ceggers@gmx.de
> 

Re: Buffer allocation for USB transfers

[Oliver Neukum]

Am Wednesday 14 January 2009 19:58:44 schrieb Christian Eggers:
> For some reason the second example (kmalloc()) doesn't seem to cause problems (on 
> my platform) but is there are guarantee that kmalloc() 
> without GFP_DMA does always return a DMA capable buffer?

We depend on memory returned for GFP_KERNEL capable of DMA on PCI.
GFP_DMA is for ISA dma.

	Regards
		Oliver

Re: Buffer allocation for USB transfers

[Oliver Neukum]

Am Wednesday 14 January 2009 19:58:44 schrieb Christian Eggers:
> At least the latter does not work on my SH-4 platform. It seems that other 
> variables on the stack are overwritten after calling usb_control_msg(), probably as 
> result of incorrect alignment.

Does this make it run on SH-4?

	Regards
		Oliver

---

--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -181,12 +181,17 @@ static int mcs7830_read_phy(struct usbnet *dev, u8 index)
 {
 	int ret;
 	int i;
-	__le16 val;
-
-	u8 cmd[2] = {
-		HIF_REG_PHY_CMD1_READ | HIF_REG_PHY_CMD1_PHYADDR,
-		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | index,
-	};
+	__le16 *val;
+	u8 *cmd;
+
+	cmd = kmalloc(2, GFP_NOIO);
+	if (!cmd)
+		return -ENOMEM;
+	cmd[0] = HIF_REG_PHY_CMD1_READ | HIF_REG_PHY_CMD1_PHYADDR;
+	cmd[1] = HIF_REG_PHY_CMD2_PEND_FLAG_BIT | index;
+	val = kzalloc(sizeof(__le16), GFP_NOIO);
+	if (!val)
+		goto out2;
 
 	mutex_lock(&dev->phy_mutex);
 	/* write the MII command */
@@ -206,7 +211,7 @@ static int mcs7830_read_phy(struct usbnet *dev, u8 index)
 		goto out;
 
 	/* read actual register contents */
-	ret = mcs7830_get_reg(dev, HIF_REG_PHY_DATA, 2, &val);
+	ret = mcs7830_get_reg(dev, HIF_REG_PHY_DATA, 2, val);
 	if (ret < 0)
 		goto out;
 	ret = le16_to_cpu(val);
@@ -214,6 +219,9 @@ static int mcs7830_read_phy(struct usbnet *dev, u8 index)
 		index, val, i);
 out:
 	mutex_unlock(&dev->phy_mutex);
+	kfree(val);
+out2:
+	kfree(cmd);
 	return ret;
 }
 
@@ -221,18 +229,24 @@ static int mcs7830_write_phy(struct usbnet *dev, u8 index, u16 val)
 {
 	int ret;
 	int i;
-	__le16 le_val;
+	__le16 *le_val;
+	u8 *cmd;
+
+	cmd = kmalloc(2, GFP_NOIO);
+	if (!cmd)
+		return -ENOMEM;
+	cmd[0] = HIF_REG_PHY_CMD1_WRITE | HIF_REG_PHY_CMD1_PHYADDR;
+	cmd[1] = HIF_REG_PHY_CMD2_PEND_FLAG_BIT | (index & 0x1F);
 
-	u8 cmd[2] = {
-		HIF_REG_PHY_CMD1_WRITE | HIF_REG_PHY_CMD1_PHYADDR,
-		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | (index & 0x1F),
-	};
+	le_val = kmalloc(sizeof(__le16), GFP_NOIO);
+	if (!le_val)
+		goto out2;
 
 	mutex_lock(&dev->phy_mutex);
 
 	/* write the new register contents */
 	le_val = cpu_to_le16(val);
-	ret = mcs7830_set_reg(dev, HIF_REG_PHY_DATA, 2, &le_val);
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_DATA, 2, le_val);
 	if (ret < 0)
 		goto out;
 
@@ -257,6 +271,9 @@ static int mcs7830_write_phy(struct usbnet *dev, u8 index, u16 val)
 		index, val, i);
 out:
 	mutex_unlock(&dev->phy_mutex);
+	kfree(le_val);
+out2:
+	kfree(cmd);
 	return ret;
 }
 
@@ -289,9 +306,14 @@ static int mcs7830_set_autoneg(struct usbnet *dev, int ptrUserPhyMode)
  */
 static int mcs7830_get_rev(struct usbnet *dev)
 {
-	u8 dummy[2];
+	u8 *dummy;
 	int ret;
+
+	dummy = kmalloc(2, GFP_NOIO);
+	if (!dummy)
+		return 1;
 	ret = mcs7830_get_reg(dev, HIF_REG_22, 2, dummy);
+	kfree(dummy);
 	if (ret > 0)
 		return 2; /* Rev C or later */
 	return 1; /* earlier revision */
@@ -302,17 +324,22 @@ static int mcs7830_get_rev(struct usbnet *dev)
  */
 static void mcs7830_rev_C_fixup(struct usbnet *dev)
 {
-	u8 pause_threshold = HIF_REG_PAUSE_THRESHOLD_DEFAULT;
+	u8 *pause_threshold;
 	int retry;
 
+	pause_threshold = kmalloc(sizeof(u8), GFP_NOIO);
+	if (!pause_threshold)
+		return;
+	*pause_threshold = HIF_REG_PAUSE_THRESHOLD_DEFAULT;
 	for (retry = 0; retry < 2; retry++) {
 		if (mcs7830_get_rev(dev) == 2) {
 			dev_info(&dev->udev->dev, "applying rev.C fixup\n");
 			mcs7830_set_reg(dev, HIF_REG_PAUSE_THRESHOLD,
-					1, &pause_threshold);
+					1, pause_threshold);
 		}
 		msleep(1);
 	}
+	kfree(pause_threshold);
 }
 
 static int mcs7830_init_dev(struct usbnet *dev)

Re: Buffer allocation for USB transfers

[Christian Eggers]

[-- Attachment #1: Mail message body --]
[-- Type: text/plain, Size: 896 bytes --]

Am 15 Jan 2009 um 16:43 hat Oliver Neukum geschrieben:
> Am Wednesday 14 January 2009 19:58:44 schrieb Christian Eggers:
> > At least the latter does not work on my SH-4 platform. It seems that other 
> > variables on the stack are overwritten after calling usb_control_msg(), probably as 
> > result of incorrect alignment.
> 
> Does this make it run on SH-4?
> 
> 	Regards
> 		Oliver
> 

Sorry, you have been to fast... I've already prepared a solution for this: (see attached diff)

The driver asix.c has been fixed in a similar way some time ago:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-
2.6.git;a=commitdiff;h=51bf2976b55d07f9daae9697a0a3ac9f58abcedc

I've tested this change with 2.6.17 and 2.6.23 on a SH-4. The patch should compile with 
the current version. Could you please apply this for me? I've no routine in preparing 
patches for lkml.

Thanks
Christian Eggers



[-- Attachment #2: Attachment information. --]
[-- Type: text/plain, Size: 853 bytes --]

Der folgende Teil dieser Nachricht enthält einen Anhang im
sogenannten Internet MIME Nachrichtenformat.
Wenn Sie Pegasus Mail oder ein beliebiges anderes MIME-kompatibles
Email-System verwenden, sollte Sie den Anhang mit Ihrem Email-System
speichern oder anzeigen können. Anderenfalls fragen Sie Ihren Administrator.

The following section of this message contains a file attachment
prepared for transmission using the Internet MIME message format.
If you are using Pegasus Mail, or any another MIME-compliant system,
you should be able to save it or view it from within your mailer.
If you cannot, please ask your system administrator for assistance.

   ---- Datei Information/File information -----------
     Datei/File:  mcs7830.c.diff
     Datum/Date:  15 Jan 2009, 19:20
     Größe/Size:  1006 bytes.
     Typ/Type:    Unbekannt

[-- Attachment #3: mcs7830.c.diff --]
[-- Type: Application/Octet-stream, Size: 1006 bytes --]

--- mcs7830.c.org	2009-01-15 19:01:13.000000000 +0100
+++ mcs7830.c	2009-01-15 19:19:00.000000000 +0100
@@ -94,10 +94,18 @@
 {
 	struct usb_device *xdev = dev->udev;
 	int ret;
+	void *buffer;
+
+	buffer = kmalloc(size, GFP_NOIO);
+	if (buffer == NULL)
+		return -ENOMEM;
 
 	ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
-			      MCS7830_RD_BMREQ, 0x0000, index, data,
+			      MCS7830_RD_BMREQ, 0x0000, index, buffer,
 			      size, MCS7830_CTRL_TIMEOUT);
+	memcpy(data, buffer, size);
+	kfree(buffer);
+
 	return ret;
 }
 
@@ -105,10 +113,18 @@
 {
 	struct usb_device *xdev = dev->udev;
 	int ret;
+	void *buffer;
+
+	buffer = kmalloc(size, GFP_NOIO);
+	if (buffer == NULL)
+		return -ENOMEM;
+
+	memcpy(buffer, data, size);
 
 	ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
-			      MCS7830_WR_BMREQ, 0x0000, index, data,
+			      MCS7830_WR_BMREQ, 0x0000, index, buffer,
 			      size, MCS7830_CTRL_TIMEOUT);
+	kfree(buffer);
 	return ret;
 }