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 [email protected]
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 [email protected]
>
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
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)