LinuxLists.cc - [PATCH 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

[permalink] [raw]

Subject: Re: [PATCH 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

From: hayeswang <[email protected]>
Date: Tue, 23 Jul 2013 17:26:04 +0800

> 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;

Your email client has corrupted these patches.

2013-07-25 07:59:41

[permalink] [raw]

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

2013-07-25 08:00:14

[permalink] [raw]

Subject: [PATCH v2 3/3] net/usb/r8152: adjust relative ocp function

- fix the conversion between cpu and __le32
- replace some pla_ocp and usb_ocp functions with generic_ocp function

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 66 +++++++++++++++++--------------------------------
1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ef033ab..11c51f2 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -514,37 +514,31 @@ int usb_ocp_write(struct r8152 *tp, u16 index, u16 byteen, u16 size, void *data)

static u32 ocp_read_dword(struct r8152 *tp, u16 type, u16 index)
{
- u32 data;
+ __le32 data;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(data), &data, type);

return __le32_to_cpu(data);
}

static void ocp_write_dword(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), &data);
- else
- usb_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), &data);
+ __le32 tmp = __cpu_to_le32(data);
+
+ generic_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(tmp), &tmp, type);
}

static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 2;

index &= ~3;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0xffff;

@@ -553,7 +547,8 @@ static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)

static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xffff;
+ u32 mask = 0xffff;
+ __le32 tmp;
u16 byen = BYTE_EN_WORD;
u8 shift = index & 2;

@@ -566,34 +561,25 @@ static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
index &= ~3;
}

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), &tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), &tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);

- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
}

static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 3;

index &= ~3;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0xff;

@@ -602,7 +588,8 @@ static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)

static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xff;
+ u32 mask = 0xff;
+ __le32 tmp;
u16 byen = BYTE_EN_BYTE;
u8 shift = index & 3;

@@ -615,19 +602,12 @@ static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
index &= ~3;
}

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), &tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), &tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);

- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
}

static void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
--
1.8.3.1

2013-07-25 08:00:19

[permalink] [raw]

Subject: [PATCH v2 2/3] net/usb/r8152: make sure the USB buffer is DMA-able

Allocate the required memory before calling usb_control_msg. And
the additional memory copy is necessary.

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 60 ++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee13f9e..ef033ab 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -344,17 +344,41 @@ static const int multicast_filter_limit = 32;
static
int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ memcpy(data, tmp, size);
+ kfree(tmp);
+
+ return ret;
}

static
int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy(tmp, data, size);
+
+ ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ kfree(tmp);
+ return ret;
}

static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
@@ -685,21 +709,14 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
static inline void set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;
- u8 *node_id;
+ u8 node_id[8] = {0};

- node_id = kmalloc(sizeof(u8) * 8, GFP_KERNEL);
- if (!node_id) {
- netif_err(tp, probe, dev, "out of memory");
- return;
- }
-
- if (pla_ocp_read(tp, PLA_IDR, sizeof(u8) * 8, node_id) < 0)
+ if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
netif_notice(tp, probe, dev, "inet addr fail\n");
else {
memcpy(dev->dev_addr, node_id, dev->addr_len);
memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
}
- kfree(node_id);
}

static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
@@ -882,15 +899,10 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
static void _rtl8152_set_rx_mode(struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- u32 tmp, *mc_filter; /* Multicast hash filter */
+ u32 mc_filter[2]; /* Multicast hash filter */
+ __le32 tmp[2];
u32 ocp_data;

- mc_filter = kmalloc(sizeof(u32) * 2, GFP_KERNEL);
- if (!mc_filter) {
- netif_err(tp, link, netdev, "out of memory");
- return;
- }
-
clear_bit(RTL8152_SET_RX_MODE, &tp->flags);
netif_stop_queue(netdev);
ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
@@ -918,14 +930,12 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
}
}

- tmp = mc_filter[0];
- mc_filter[0] = __cpu_to_le32(swab32(mc_filter[1]));
- mc_filter[1] = __cpu_to_le32(swab32(tmp));
+ tmp[0] = __cpu_to_le32(swab32(mc_filter[1]));
+ tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));

- pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(u32) * 2, mc_filter);
+ pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
netif_wake_queue(netdev);
- kfree(mc_filter);
}

static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
--
1.8.3.1

2013-07-28 03:21:10

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

From: Hayes Wang <[email protected]>
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 <[email protected]>

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.

2013-07-29 05:20:30

by Oliver Neukum

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Sat, 2013-07-27 at 20:21 -0700, David Miller wrote:
> From: Hayes Wang <[email protected]>
> 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 <[email protected]>
>
> 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

2013-07-29 07:46:05

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

From: Oliver Neukum <[email protected]>
Date: Mon, 29 Jul 2013 07:20:24 +0200

> On Sat, 2013-07-27 at 20:21 -0700, David Miller wrote:
>> From: Hayes Wang <[email protected]>
>> 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 <[email protected]>
>>
>> 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.

2013-07-30 08:29:30

[permalink] [raw]

Subject: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

Allocate the transfer buffer in probe(), and use the buffer for
usb control transfer.

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r815x.c | 117 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 90 insertions(+), 27 deletions(-)

diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 8523922..89ad63f 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -21,37 +21,52 @@
#define R815x_PHY_ID 32
#define REALTEK_VENDOR_ID 0x0bda

+struct r815x {
+ struct mutex transfer_mutex;
+ __le32 *transfer_buf;
+};

-static int pla_read_word(struct usb_device *udev, u16 index)
+static int pla_read_word(struct usbnet *dev, u16 index)
{
- int data, ret;
+ struct r815x *tp = dev->driver_priv;
+ struct usb_device *udev = dev->udev;
+ int ret;
u8 shift = index & 2;
- __le32 ocp_data;
+ __le32 *tmp;
+
+ mutex_lock(&tp->transfer_mutex);
+ tmp = tp->transfer_buf;

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:
+ mutex_unlock(&tp->transfer_mutex);
+ return ret;
}

-static int pla_write_word(struct usb_device *udev, u16 index, u32 data)
+static int pla_write_word(struct usbnet *dev, u16 index, u32 data)
{
- __le32 ocp_data;
+ struct r815x *tp = dev->driver_priv;
+ struct usb_device *udev = dev->udev;
+ __le32 *tmp;
u32 mask = 0xffff;
u16 byen = BYTE_EN_WORD;
u8 shift = index & 2;
int ret;

+ mutex_lock(&tp->transfer_mutex);
+ tmp = tp->transfer_buf;
+
data &= mask;

if (shift) {
@@ -63,19 +78,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:
+ mutex_unlock(&tp->transfer_mutex);
return ret;
}

@@ -85,12 +101,12 @@ static int ocp_reg_read(struct usbnet *dev, u16 addr)
int ret;

ocp_base = addr & 0xf000;
- ret = pla_write_word(dev->udev, OCP_BASE, ocp_base);
+ ret = pla_write_word(dev, OCP_BASE, ocp_base);
if (ret < 0)
goto out;

ocp_index = (addr & 0x0fff) | 0xb000;
- ret = pla_read_word(dev->udev, ocp_index);
+ ret = pla_read_word(dev, ocp_index);

out:
return ret;
@@ -102,12 +118,12 @@ static int ocp_reg_write(struct usbnet *dev, u16 addr, u16 data)
int ret;

ocp_base = addr & 0xf000;
- ret = pla_write_word(dev->udev, OCP_BASE, ocp_base);
+ ret = pla_write_word(dev, OCP_BASE, ocp_base);
if (ret < 0)
goto out1;

ocp_index = (addr & 0x0fff) | 0xb000;
- ret = pla_write_word(dev->udev, ocp_index, data);
+ ret = pla_write_word(dev, ocp_index, data);

out1:
return ret;
@@ -134,12 +150,59 @@ void r815x_mdio_write(struct net_device *netdev, int phy_id, int reg, int val)
ocp_reg_write(dev, BASE_MII + reg * 2, val);
}

-static int r8153_bind(struct usbnet *dev, struct usb_interface *intf)
+static int r815x_bind(struct usbnet *dev, struct usb_interface *intf)
{
+ struct r815x *tp;
int status;

+ tp = kmalloc(sizeof(*tp), GFP_KERNEL);
+ if (!tp)
+ return -ENOMEM;
+
+ memset(tp, 0, sizeof(*tp));
+
+ status = -ENOMEM;
+
+ tp->transfer_buf = kmalloc(sizeof(*tp->transfer_buf), GFP_KERNEL);
+ if (!tp->transfer_buf)
+ goto out1;
+
+ mutex_init (&tp->transfer_mutex);
+ dev->driver_priv = tp;
+
status = usbnet_cdc_bind(dev, intf);
if (status < 0)
+ goto out2;
+
+ return 0;
+
+out2:
+ kfree(tp->transfer_buf);
+out1:
+ kfree(tp);
+ return status;
+}
+
+static void r815x_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+ struct r815x *tp = dev->driver_priv;
+
+ usbnet_cdc_unbind(dev, intf);
+
+ if (tp) {
+ if (tp->transfer_buf)
+ kfree(tp->transfer_buf);
+ kfree(tp);
+ dev->driver_priv = NULL;
+ }
+}
+
+static int r8153_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+ int status;
+
+ status = r815x_bind(dev, intf);
+ if (status < 0)
return status;

dev->mii.dev = dev->net;
@@ -157,7 +220,7 @@ static int r8152_bind(struct usbnet *dev, struct usb_interface *intf)
{
int status;

- status = usbnet_cdc_bind(dev, intf);
+ status = r815x_bind(dev, intf);
if (status < 0)
return status;

@@ -176,7 +239,7 @@ static const struct driver_info r8152_info = {
.description = "RTL8152 ECM Device",
.flags = FLAG_ETHER | FLAG_POINTTOPOINT,
.bind = r8152_bind,
- .unbind = usbnet_cdc_unbind,
+ .unbind = r815x_unbind,
.status = usbnet_cdc_status,
.manage_power = usbnet_manage_power,
};
@@ -185,7 +248,7 @@ static const struct driver_info r8153_info = {
.description = "RTL8153 ECM Device",
.flags = FLAG_ETHER | FLAG_POINTTOPOINT,
.bind = r8153_bind,
- .unbind = usbnet_cdc_unbind,
+ .unbind = r815x_unbind,
.status = usbnet_cdc_status,
.manage_power = usbnet_manage_power,
};
--
1.8.3.1

2013-07-30 08:29:29

[permalink] [raw]

Subject: [PATCH v3 3/3] net/usb/r8152: adjust relative ocp function

- fix the conversion between cpu and __le32
- replace some pla_ocp and usb_ocp functions with generic_ocp function

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 66 +++++++++++++++++--------------------------------
1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 93a7baa..252b61b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -536,37 +536,31 @@ int usb_ocp_write(struct r8152 *tp, u16 index, u16 byteen, u16 size, void *data)

static u32 ocp_read_dword(struct r8152 *tp, u16 type, u16 index)
{
- u32 data;
+ __le32 data;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(data), &data, type);

return __le32_to_cpu(data);
}

static void ocp_write_dword(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), &data);
- else
- usb_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(data), &data);
+ __le32 tmp = __cpu_to_le32(data);
+
+ generic_ocp_write(tp, index, BYTE_EN_DWORD, sizeof(tmp), &tmp, type);
}

static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 2;

index &= ~3;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0xffff;

@@ -575,7 +569,8 @@ static u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)

static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xffff;
+ u32 mask = 0xffff;
+ __le32 tmp;
u16 byen = BYTE_EN_WORD;
u8 shift = index & 2;

@@ -588,34 +583,25 @@ static void ocp_write_word(struct r8152 *tp, u16 type, u16 index, u32 data)
index &= ~3;
}

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), &tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), &tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);

- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
}

static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)
{
u32 data;
+ __le32 tmp;
u8 shift = index & 3;

index &= ~3;

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(data), &data);
- else
- usb_ocp_read(tp, index, sizeof(data), &data);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- data = __le32_to_cpu(data);
+ data = __le32_to_cpu(tmp);
data >>= (shift * 8);
data &= 0xff;

@@ -624,7 +610,8 @@ static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)

static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
{
- u32 tmp, mask = 0xff;
+ u32 mask = 0xff;
+ __le32 tmp;
u16 byen = BYTE_EN_BYTE;
u8 shift = index & 3;

@@ -637,19 +624,12 @@ static void ocp_write_byte(struct r8152 *tp, u16 type, u16 index, u32 data)
index &= ~3;
}

- if (type == MCU_TYPE_PLA)
- pla_ocp_read(tp, index, sizeof(tmp), &tmp);
- else
- usb_ocp_read(tp, index, sizeof(tmp), &tmp);
+ generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);

- tmp = __le32_to_cpu(tmp) & ~mask;
- tmp |= data;
- tmp = __cpu_to_le32(tmp);
+ data |= __le32_to_cpu(tmp) & ~mask;
+ tmp = __cpu_to_le32(data);

- if (type == MCU_TYPE_PLA)
- pla_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
- else
- usb_ocp_write(tp, index, byen, sizeof(tmp), &tmp);
+ generic_ocp_write(tp, index, byen, sizeof(tmp), &tmp, type);
}

static void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
--
1.8.3.1

2013-07-30 08:29:28

[permalink] [raw]

Subject: [PATCH v3 2/3] net/usb/r8152: replace USB buffer from stack to DMA-able

Allocate the required transfer buffer for usb_control_msg.

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 91 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 66 insertions(+), 25 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee13f9e..93a7baa 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -282,6 +282,8 @@ enum rtl_register_content {
#define RTL8152_RMS (VLAN_ETH_FRAME_LEN + VLAN_HLEN)
#define RTL8152_TX_TIMEOUT (HZ)

+#define RTL8152_MAX_TRANSFER_SIZE 8
+
/* rtl8152 flags */
enum rtl8152_flags {
RTL8152_UNPLUG = 0,
@@ -324,8 +326,10 @@ struct r8152 {
struct sk_buff *tx_skb, *rx_skb;
struct delayed_work schedule;
struct mii_if_info mii;
+ struct mutex transfer_mutex;
u32 msg_enable;
u16 ocp_base;
+ u8 *transfer_buf;
u8 version;
u8 speed;
};
@@ -344,17 +348,59 @@ static const int multicast_filter_limit = 32;
static
int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ if (size > RTL8152_MAX_TRANSFER_SIZE || !tp->transfer_buf) {
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+ } else {
+ mutex_lock(&tp->transfer_mutex);
+ tmp = tp->transfer_buf;
+ }
+
+ ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ memcpy(data, tmp, size);
+
+ if (tmp == tp->transfer_buf)
+ mutex_unlock(&tp->transfer_mutex);
+ else
+ kfree(tmp);
+
+ return ret;
}

static
int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
{
- return usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
+ int ret;
+ void *tmp;
+
+ if (size > RTL8152_MAX_TRANSFER_SIZE || !tp->transfer_buf) {
+ tmp = kmalloc(size, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+ } else {
+ mutex_lock(&tp->transfer_mutex);
+ tmp = tp->transfer_buf;
+ }
+
+ memcpy(tmp, data, size);
+
+ ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
- value, index, data, size, 500);
+ value, index, tmp, size, 500);
+
+ if (tmp == tp->transfer_buf)
+ mutex_unlock(&tp->transfer_mutex);
+ else
+ kfree(tmp);
+
+ return ret;
}

static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
@@ -685,21 +731,14 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
static inline void set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;
- u8 *node_id;
-
- node_id = kmalloc(sizeof(u8) * 8, GFP_KERNEL);
- if (!node_id) {
- netif_err(tp, probe, dev, "out of memory");
- return;
- }
+ u8 node_id[8] = {0};

- if (pla_ocp_read(tp, PLA_IDR, sizeof(u8) * 8, node_id) < 0)
+ if (pla_ocp_read(tp, PLA_IDR, sizeof(node_id), node_id) < 0)
netif_notice(tp, probe, dev, "inet addr fail\n");
else {
memcpy(dev->dev_addr, node_id, dev->addr_len);
memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
}
- kfree(node_id);
}

static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
@@ -882,15 +921,10 @@ static void rtl8152_set_rx_mode(struct net_device *netdev)
static void _rtl8152_set_rx_mode(struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- u32 tmp, *mc_filter; /* Multicast hash filter */
+ u32 mc_filter[2]; /* Multicast hash filter */
+ __le32 tmp[2];
u32 ocp_data;

- mc_filter = kmalloc(sizeof(u32) * 2, GFP_KERNEL);
- if (!mc_filter) {
- netif_err(tp, link, netdev, "out of memory");
- return;
- }
-
clear_bit(RTL8152_SET_RX_MODE, &tp->flags);
netif_stop_queue(netdev);
ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
@@ -918,14 +952,12 @@ static void _rtl8152_set_rx_mode(struct net_device *netdev)
}
}

- tmp = mc_filter[0];
- mc_filter[0] = __cpu_to_le32(swab32(mc_filter[1]));
- mc_filter[1] = __cpu_to_le32(swab32(tmp));
+ tmp[0] = __cpu_to_le32(swab32(mc_filter[1]));
+ tmp[1] = __cpu_to_le32(swab32(mc_filter[0]));

- pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(u32) * 2, mc_filter);
+ pla_ocp_write(tp, PLA_MAR, BYTE_EN_DWORD, sizeof(tmp), tmp);
ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
netif_wake_queue(netdev);
- kfree(mc_filter);
}

static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
@@ -1644,6 +1676,7 @@ static int rtl8152_probe(struct usb_interface *intf,
tp = netdev_priv(netdev);
tp->msg_enable = 0x7FFF;

+ mutex_init (&tp->transfer_mutex);
tasklet_init(&tp->tl, rx_fixup, (unsigned long)tp);
INIT_DELAYED_WORK(&tp->schedule, rtl_work_func_t);

@@ -1655,6 +1688,12 @@ static int rtl8152_probe(struct usb_interface *intf,
SET_ETHTOOL_OPS(netdev, &ops);
tp->speed = 0;

+ tp->transfer_buf = kmalloc(RTL8152_MAX_TRANSFER_SIZE, GFP_KERNEL);
+ if (!tp->transfer_buf) {
+ netif_err(tp, probe, netdev, "out of memory");
+ goto out;
+ }
+
tp->mii.dev = netdev;
tp->mii.mdio_read = read_mii_word;
tp->mii.mdio_write = write_mii_word;
@@ -1728,6 +1767,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
free_all_urbs(tp);
if (tp->rx_skb)
dev_kfree_skb(tp->rx_skb);
+ if (tp->transfer_buf)
+ kfree(tp->transfer_buf);
free_netdev(tp->netdev);
}
}
--
1.8.3.1

2013-07-30 13:57:12

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

2013-07-30 13:59:44

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Tue, Jul 30, 2013 at 04:28:54PM +0800, Hayes Wang wrote:
> Allocate the transfer buffer in probe(), and use the buffer for
> usb control transfer.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r815x.c | 117 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 90 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
> index 8523922..89ad63f 100644
> --- a/drivers/net/usb/r815x.c
> +++ b/drivers/net/usb/r815x.c
> @@ -21,37 +21,52 @@
> #define R815x_PHY_ID 32
> #define REALTEK_VENDOR_ID 0x0bda
>
> +struct r815x {
> + struct mutex transfer_mutex;
> + __le32 *transfer_buf;
> +};

Really? Why not just allocate it the times you need to send the data?
Is it really all that often that you need to do this?

> -static int pla_read_word(struct usb_device *udev, u16 index)
> +static int pla_read_word(struct usbnet *dev, u16 index)
> {
> - int data, ret;
> + struct r815x *tp = dev->driver_priv;
> + struct usb_device *udev = dev->udev;
> + int ret;
> u8 shift = index & 2;
> - __le32 ocp_data;
> + __le32 *tmp;
> +
> + mutex_lock(&tp->transfer_mutex);
> + tmp = tp->transfer_buf;
>
> 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);

This call is so slow, you can afford to make a call to kmalloc for the
data, as it sure just did for other structures it needed :)

So just do a kmalloc call for the data before this function, same for
your other patches in this series, no need for a lock for your "transfer
data" buffer, and a global one at all.

thanks,

greg k-h

2013-07-30 14:00:36

[permalink] [raw]

Subject: Re: [PATCH v3 2/3] net/usb/r8152: replace USB buffer from stack to DMA-able

On Tue, Jul 30, 2013 at 04:28:55PM +0800, Hayes Wang wrote:
> Allocate the required transfer buffer for usb_control_msg.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 91 +++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 66 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ee13f9e..93a7baa 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -282,6 +282,8 @@ enum rtl_register_content {
> #define RTL8152_RMS (VLAN_ETH_FRAME_LEN + VLAN_HLEN)
> #define RTL8152_TX_TIMEOUT (HZ)
>
> +#define RTL8152_MAX_TRANSFER_SIZE 8
> +
> /* rtl8152 flags */
> enum rtl8152_flags {
> RTL8152_UNPLUG = 0,
> @@ -324,8 +326,10 @@ struct r8152 {
> struct sk_buff *tx_skb, *rx_skb;
> struct delayed_work schedule;
> struct mii_if_info mii;
> + struct mutex transfer_mutex;
> u32 msg_enable;
> u16 ocp_base;
> + u8 *transfer_buf;
> u8 version;
> u8 speed;
> };
> @@ -344,17 +348,59 @@ static const int multicast_filter_limit = 32;
> static
> int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data)
> {
> - return usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
> + int ret;
> + void *tmp;
> +
> + if (size > RTL8152_MAX_TRANSFER_SIZE || !tp->transfer_buf) {
> + tmp = kmalloc(size, GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> + } else {
> + mutex_lock(&tp->transfer_mutex);
> + tmp = tp->transfer_buf;
> + }
> +
> + ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
> RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> - value, index, data, size, 500);
> + value, index, tmp, size, 500);

Same here, no need for your "transfer buf" structure, just always
kmalloc the data, copy it in, and make the call. It's _really_ slow
anyway, you will never notice any speed overhead by doing this.

thanks,

greg k-h

2013-07-30 14:29:32

by Ming Lei

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Tue, Jul 30, 2013 at 4:28 PM, Hayes Wang <[email protected]> wrote:
> Allocate the transfer buffer in probe(), and use the buffer for
> usb control transfer.

Looks this is a usbnet device, so suggest to use usbnet command APIs
(usbnet_read_cmd/usbnet_write_cmd) to do that, another advantage is
you can avoid to access runtime-suspended device from ethtool.

Thanks,
--
Ming Lei

2013-07-30 15:18:03

by Joe Perches

[permalink] [raw]

Subject: [PATCH] MAINTAINERS: USB, remove F: drivers/net/usb/ pattern

On Tue, 2013-07-30 at 06:58 -0700, Greg KH wrote:

> In the future, you do not need to send drivers/net/usb/ patches to me,
> netdev and the linux-usb mailing lists should be sufficient.

Signed-off-by: Joe Perches <[email protected]>
---

This might help...

diff --git a/MAINTAINERS b/MAINTAINERS
index 339e798..9b5335a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8780,7 +8780,6 @@ W: http://www.linux-usb.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
S: Supported
F: Documentation/usb/
-F: drivers/net/usb/
F: drivers/usb/
F: include/linux/usb.h
F: include/linux/usb/

2013-07-30 18:33:31

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

From: Greg KH <[email protected]>
Date: Tue, 30 Jul 2013 07:00:59 -0700

> This call is so slow, you can afford to make a call to kmalloc for the
> data, as it sure just did for other structures it needed :)

I told him to implement things this way, to avoid calling kmalloc every
single register access.

Using kmalloc all the time makes the access fragile, since a badly timed
call during high memory pressure can fail.

I'd rather the potential failure happen at one time, probe time.

In any event, Ming Lei has suggested using usbnet_{read,write}_cmd()
instead, which sounds like a good solution to this problem.

2013-07-30 18:41:20

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Tue, 2013-07-30 at 11:33 -0700, David Miller wrote:
> From: Greg KH <[email protected]>
> Date: Tue, 30 Jul 2013 07:00:59 -0700
>
> > This call is so slow, you can afford to make a call to kmalloc for the
> > data, as it sure just did for other structures it needed :)
>
> I told him to implement things this way, to avoid calling kmalloc every
> single register access.
>
> Using kmalloc all the time makes the access fragile, since a badly timed
> call during high memory pressure can fail.
>
> I'd rather the potential failure happen at one time, probe time.
>
> In any event, Ming Lei has suggested using usbnet_{read,write}_cmd()
> instead, which sounds like a good solution to this problem.

Those do per-call allocs too.

2013-07-30 18:49:53

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

From: Joe Perches <[email protected]>
Date: Tue, 30 Jul 2013 11:41:17 -0700

> On Tue, 2013-07-30 at 11:33 -0700, David Miller wrote:
>> From: Greg KH <[email protected]>
>> Date: Tue, 30 Jul 2013 07:00:59 -0700
>>
>> > This call is so slow, you can afford to make a call to kmalloc for the
>> > data, as it sure just did for other structures it needed :)
>>
>> I told him to implement things this way, to avoid calling kmalloc every
>> single register access.
>>
>> Using kmalloc all the time makes the access fragile, since a badly timed
>> call during high memory pressure can fail.
>>
>> I'd rather the potential failure happen at one time, probe time.
>>
>> In any event, Ming Lei has suggested using usbnet_{read,write}_cmd()
>> instead, which sounds like a good solution to this problem.
>
> Those do per-call allocs too.

Sigh... Ok I won't fight this any longer then :-)

2013-07-30 18:58:14

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Tue, Jul 30, 2013 at 11:33:29AM -0700, David Miller wrote:
> From: Greg KH <[email protected]>
> Date: Tue, 30 Jul 2013 07:00:59 -0700
>
> > This call is so slow, you can afford to make a call to kmalloc for the
> > data, as it sure just did for other structures it needed :)
>
> I told him to implement things this way, to avoid calling kmalloc every
> single register access.
>
> Using kmalloc all the time makes the access fragile, since a badly timed
> call during high memory pressure can fail.
>
> I'd rather the potential failure happen at one time, probe time.

I agree, but the call usb_control_message() also does a tiny kmalloc(),
and is _very_ slow and if you have high memory pressure, doing USB
messages like this is not what you want to do anyway (the host
controller does it's own allocations and the like as well.)

USB isn't "fast" like most normal networking physical layers :)

thanks,

greg k-h

2013-07-31 09:22:00

[permalink] [raw]

Subject: [PATCH v4 2/5] net/usb/r815x: avoid to call mdio functions for runtime-suspended device

Don't replace the usb_control_msg() with usbnet_{read,write}_cmd()
which couldn't be called inside suspend/resume callback. Keep the
basic functions unlimited. Instead, using usb_autopm_get_interface()
and usb_autopm_put_interface() in r815x_mdio_{read,write}().

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r815x.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index e9b99ba..1a80e76 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -126,11 +126,18 @@ out1:
static int r815x_mdio_read(struct net_device *netdev, int phy_id, int reg)
{
struct usbnet *dev = netdev_priv(netdev);
+ int ret;

if (phy_id != R815x_PHY_ID)
return -EINVAL;

- return ocp_reg_read(dev, BASE_MII + reg * 2);
+ if (usb_autopm_get_interface(dev->intf) < 0)
+ return -ENODEV;
+
+ ret = ocp_reg_read(dev, BASE_MII + reg * 2);
+
+ usb_autopm_put_interface(dev->intf);
+ return ret;
}

static
@@ -141,7 +148,12 @@ void r815x_mdio_write(struct net_device *netdev, int phy_id, int reg, int val)
if (phy_id != R815x_PHY_ID)
return;

+ if (usb_autopm_get_interface(dev->intf) < 0)
+ return;
+
ocp_reg_write(dev, BASE_MII + reg * 2, val);
+
+ usb_autopm_put_interface(dev->intf);
}

static int r8153_bind(struct usbnet *dev, struct usb_interface *intf)
--
1.8.3.1

2013-07-31 09:22:02

[permalink] [raw]

Subject: [PATCH v4 3/5] net/usb/r815x: change the return value for bind functions

Replace 0 with the result from usbnet_cdc_bind().

Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r815x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r815x.c b/drivers/net/usb/r815x.c
index 1a80e76..2df2f4f 100644
--- a/drivers/net/usb/r815x.c
+++ b/drivers/net/usb/r815x.c
@@ -172,7 +172,7 @@ static int r8153_bind(struct usbnet *dev, struct usb_interface *intf)
dev->mii.phy_id = R815x_PHY_ID;
dev->mii.supports_gmii = 1;

- return 0;
+ return status;
}

static int r8152_bind(struct usbnet *dev, struct usb_interface *intf)
@@ -191,7 +191,7 @@ static int r8152_bind(struct usbnet *dev, struct usb_interface *intf)
dev->mii.phy_id = R815x_PHY_ID;
dev->mii.supports_gmii = 0;

- return 0;
+ return status;
}

static const struct driver_info r8152_info = {
--
1.8.3.1

2013-07-31 09:21:59

[permalink] [raw]

Subject: [PATCH v4 1/5] net/usb/r815x: replace USB buffer from stack to DMA-able

2013-07-31 09:22:51

[permalink] [raw]

Subject: [PATCH v4 5/5] net/usb/r8152: adjust relative ocp function

2013-07-31 09:23:20

[permalink] [raw]

Subject: [PATCH v4 4/5] net/usb/r8152: make sure the USB buffer is DMA-able

2013-07-31 10:29:50

by Ming Lei

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] net/usb/r815x: replace USB buffer from stack to DMA-able

On Wed, Jul 31, 2013 at 2:49 AM, David Miller <[email protected]> wrote:
> From: Joe Perches <[email protected]>
> Date: Tue, 30 Jul 2013 11:41:17 -0700
>
>> On Tue, 2013-07-30 at 11:33 -0700, David Miller wrote:
>>> From: Greg KH <[email protected]>
>>> Date: Tue, 30 Jul 2013 07:00:59 -0700
>>>
>>> > This call is so slow, you can afford to make a call to kmalloc for the
>>> > data, as it sure just did for other structures it needed :)
>>>
>>> I told him to implement things this way, to avoid calling kmalloc every
>>> single register access.
>>>
>>> Using kmalloc all the time makes the access fragile, since a badly timed
>>> call during high memory pressure can fail.
>>>
>>> I'd rather the potential failure happen at one time, probe time.
>>>
>>> In any event, Ming Lei has suggested using usbnet_{read,write}_cmd()
>>> instead, which sounds like a good solution to this problem.
>>
>> Those do per-call allocs too.
>
> Sigh... Ok I won't fight this any longer then :-)

It might not be a big deal because most of these commands are sent
during probe()/ethtool/... context, and not in tx/rx path.

Another choice is to reserve a big enough buffer during probe()
for read/write command, but one mutex is needed too for avoiding
concurrent calling.

Anyway, please call the command API to do such thing, so that we
can improve it easily in future.

Thanks,
--
Ming Lei

2013-07-31 12:52:37

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] net/usb/r815x: replace USB buffer from stack to DMA-able

On Wed, Jul 31, 2013 at 05:21:22PM +0800, Hayes Wang wrote:
> 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 <[email protected]>

Acked-by: Greg Kroah-Hartman <[email protected]>

2013-07-31 12:53:37

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] net/usb/r8152: make sure the USB buffer is DMA-able

On Wed, Jul 31, 2013 at 05:21:25PM +0800, Hayes Wang wrote:
> Allocate the required memory before calling usb_control_msg. And
> the additional memory copy is necessary.
>
> Signed-off-by: Hayes Wang <[email protected]>

Acked-by: Greg Kroah-Hartman <[email protected]>

2013-07-31 21:49:48

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] net/usb/r815x: replace USB buffer from stack to DMA-able

From: Hayes Wang <[email protected]>
Date: Wed, 31 Jul 2013 17:21:22 +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 <[email protected]>

Applied.

2013-07-31 21:49:54

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] net/usb/r815x: avoid to call mdio functions for runtime-suspended device

From: Hayes Wang <[email protected]>
Date: Wed, 31 Jul 2013 17:21:23 +0800

> Don't replace the usb_control_msg() with usbnet_{read,write}_cmd()
> which couldn't be called inside suspend/resume callback. Keep the
> basic functions unlimited. Instead, using usb_autopm_get_interface()
> and usb_autopm_put_interface() in r815x_mdio_{read,write}().
>
> Signed-off-by: Hayes Wang <[email protected]>

Applied.

2013-07-31 21:50:00

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] net/usb/r815x: change the return value for bind functions

From: Hayes Wang <[email protected]>
Date: Wed, 31 Jul 2013 17:21:24 +0800

> Replace 0 with the result from usbnet_cdc_bind().
>
> Signed-off-by: Hayes Wang <[email protected]>

Applied.

2013-07-31 21:50:09

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] net/usb/r8152: make sure the USB buffer is DMA-able

From: Hayes Wang <[email protected]>
Date: Wed, 31 Jul 2013 17:21:25 +0800

> Allocate the required memory before calling usb_control_msg. And
> the additional memory copy is necessary.
>
> Signed-off-by: Hayes Wang <[email protected]>

Applied.

2013-07-31 21:50:12