Re: [PATCH 3/3] drivers: net: usb: pegasus: fix control urbsubmission

From: Petko Manolov
Date: Fri Apr 26 2013 - 04:42:09 EST

From: Petko Manolov <petkan@xxxxxxxxxxxxx>

Pegasus driver used single callback for sync and async control URBs.
Special flags were employed to distinguish between both, but due to flawed
logic it didn't always work. As a result of this change
[get|set]_registers() are now much simpler. Async write is also leaner
and does not use single, statically allocated memory for usb_ctrlrequest,
which is another potential race when asynchronously submitting URBs.

Signed-off-by: Petko Manolov <petkan@xxxxxxxxxxxxx>
---
Diff made against latest net-next, not my git repository;

drivers/net/usb/pegasus.c | 285 +++++++++--------------------------
drivers/net/usb/pegasus.h | 8 +-
2 files changed, 76 insertions(+), 217 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 352b4dc..0969905 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1999-2005 Petko Manolov (petkan@xxxxxxxxxxxxxxxxxxxxx)
+ * Copyright (c) 1999-2013 Petko Manolov (petkan@xxxxxxxxxxxxx)
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -26,6 +26,9 @@
* v0.5.1 ethtool support added
* v0.5.5 rx socket buffers are in a pool and the their allocation
* is out of the interrupt routine.
+ * ...
+ * v0.9.3 simplified [get|set]_register(s), async update registers
+ * logic revisited, receive skb_pool removed.
*/

#include <linux/sched.h>
@@ -45,8 +48,8 @@
/*
* Version Information
*/
-#define DRIVER_VERSION "v0.6.14 (2006/09/27)"
-#define DRIVER_AUTHOR "Petko Manolov <petkan@xxxxxxxxxxxxxxxxxxxxx>"
+#define DRIVER_VERSION "v0.9.3 (2013/04/25)"
+#define DRIVER_AUTHOR "Petko Manolov <petkan@xxxxxxxxxxxxx>"
#define DRIVER_DESC "Pegasus/Pegasus II USB Ethernet driver"

static const char driver_name[] = "pegasus";
@@ -108,218 +111,90 @@ MODULE_PARM_DESC(msg_level, "Override default message level");
MODULE_DEVICE_TABLE(usb, pegasus_ids);
static const struct net_device_ops pegasus_netdev_ops;

-static int update_eth_regs_async(pegasus_t *);
-/* Aargh!!! I _really_ hate such tweaks */
-static void ctrl_callback(struct urb *urb)
+/*****/
+
+static void async_ctrl_callback(struct urb *urb)
{
- pegasus_t *pegasus = urb->context;
+ struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
int status = urb->status;

- if (!pegasus)
- return;
-
- switch (status) {
- case 0:
- if (pegasus->flags & ETH_REGS_CHANGE) {
- pegasus->flags &= ~ETH_REGS_CHANGE;
- pegasus->flags |= ETH_REGS_CHANGED;
- update_eth_regs_async(pegasus);
- return;
- }
- break;
- case -EINPROGRESS:
- return;
- case -ENOENT:
- break;
- default:
- if (net_ratelimit())
- netif_dbg(pegasus, drv, pegasus->net,
- "%s, status %d\n", __func__, status);
- break;
- }
- pegasus->flags &= ~ETH_REGS_CHANGED;
- wake_up(&pegasus->ctrl_wait);
+ if (status < 0)
+ dev_dbg(&urb->dev->dev, "%s failed with %d", __func__, status);
+ kfree(req);
+ usb_free_urb(urb);
}

-static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
- void *data)
+static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
{
int ret;
- char *buffer;
- DECLARE_WAITQUEUE(wait, current);
-
- buffer = kmalloc(size, GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
-
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
- while (pegasus->flags & ETH_REGS_CHANGED)
- schedule();
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_RUNNING);
-
- pegasus->dr.bRequestType = PEGASUS_REQT_READ;
- pegasus->dr.bRequest = PEGASUS_REQ_GET_REGS;
- pegasus->dr.wValue = cpu_to_le16(0);
- pegasus->dr.wIndex = cpu_to_le16(indx);
- pegasus->dr.wLength = cpu_to_le16(size);
- pegasus->ctrl_urb->transfer_buffer_length = size;
-
- usb_fill_control_urb(pegasus->ctrl_urb, pegasus->usb,
- usb_rcvctrlpipe(pegasus->usb, 0),
- (char *) &pegasus->dr,
- buffer, size, ctrl_callback, pegasus);
-
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
-
- /* using ATOMIC, we'd never wake up if we slept */
- if ((ret = usb_submit_urb(pegasus->ctrl_urb, GFP_ATOMIC))) {
- set_current_state(TASK_RUNNING);
- if (ret == -ENODEV)
- netif_device_detach(pegasus->net);
- if (net_ratelimit())
- netif_err(pegasus, drv, pegasus->net,
- "%s, status %d\n", __func__, ret);
- goto out;
- }
-
- schedule();
-out:
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- memcpy(data, buffer, size);
- kfree(buffer);

+ ret = usb_control_msg(pegasus->usb, usb_rcvctrlpipe(pegasus->usb, 0),
+ PEGASUS_REQ_GET_REGS, PEGASUS_REQT_READ, 0,
+ indx, data, size, 1000);
+ if (ret < 0)
+ netif_dbg(pegasus, drv, pegasus->net,
+ "%s returned %d\n", __func__, ret);
return ret;
}

-static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
- void *data)
+static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
{
int ret;
- char *buffer;
- DECLARE_WAITQUEUE(wait, current);
-
- buffer = kmemdup(data, size, GFP_KERNEL);
- if (!buffer) {
- netif_warn(pegasus, drv, pegasus->net,
- "out of memory in %s\n", __func__);
- return -ENOMEM;
- }
-
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
- while (pegasus->flags & ETH_REGS_CHANGED)
- schedule();
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_RUNNING);
-
- pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
- pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
- pegasus->dr.wValue = cpu_to_le16(0);
- pegasus->dr.wIndex = cpu_to_le16(indx);
- pegasus->dr.wLength = cpu_to_le16(size);
- pegasus->ctrl_urb->transfer_buffer_length = size;
-
- usb_fill_control_urb(pegasus->ctrl_urb, pegasus->usb,
- usb_sndctrlpipe(pegasus->usb, 0),
- (char *) &pegasus->dr,
- buffer, size, ctrl_callback, pegasus);
-
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
-
- if ((ret = usb_submit_urb(pegasus->ctrl_urb, GFP_ATOMIC))) {
- if (ret == -ENODEV)
- netif_device_detach(pegasus->net);
- netif_err(pegasus, drv, pegasus->net,
- "%s, status %d\n", __func__, ret);
- goto out;
- }
-
- schedule();
-out:
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- kfree(buffer);

+ ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
+ PEGASUS_REQ_SET_REGS, PEGASUS_REQT_WRITE, 0,
+ indx, data, size, 100);
+ if (ret < 0)
+ netif_dbg(pegasus, drv, pegasus->net,
+ "%s returned %d\n", __func__, ret);
return ret;
}

static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
{
int ret;
- char *tmp;
- DECLARE_WAITQUEUE(wait, current);
-
- tmp = kmemdup(&data, 1, GFP_KERNEL);
- if (!tmp) {
- netif_warn(pegasus, drv, pegasus->net,
- "out of memory in %s\n", __func__);
- return -ENOMEM;
- }
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
- while (pegasus->flags & ETH_REGS_CHANGED)
- schedule();
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_RUNNING);
-
- pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
- pegasus->dr.bRequest = PEGASUS_REQ_SET_REG;
- pegasus->dr.wValue = cpu_to_le16(data);
- pegasus->dr.wIndex = cpu_to_le16(indx);
- pegasus->dr.wLength = cpu_to_le16(1);
- pegasus->ctrl_urb->transfer_buffer_length = 1;
-
- usb_fill_control_urb(pegasus->ctrl_urb, pegasus->usb,
- usb_sndctrlpipe(pegasus->usb, 0),
- (char *) &pegasus->dr,
- tmp, 1, ctrl_callback, pegasus);
-
- add_wait_queue(&pegasus->ctrl_wait, &wait);
- set_current_state(TASK_UNINTERRUPTIBLE);
-
- if ((ret = usb_submit_urb(pegasus->ctrl_urb, GFP_ATOMIC))) {
- if (ret == -ENODEV)
- netif_device_detach(pegasus->net);
- if (net_ratelimit())
- netif_err(pegasus, drv, pegasus->net,
- "%s, status %d\n", __func__, ret);
- goto out;
- }
-
- schedule();
-out:
- remove_wait_queue(&pegasus->ctrl_wait, &wait);
- kfree(tmp);

+ ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
+ PEGASUS_REQ_SET_REG, PEGASUS_REQT_WRITE, data,
+ indx, &data, 1, 1000);
+ if (ret < 0)
+ netif_dbg(pegasus, drv, pegasus->net,
+ "%s returned %d\n", __func__, ret);
return ret;
}

static int update_eth_regs_async(pegasus_t *pegasus)
{
- int ret;
-
- pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
- pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
- pegasus->dr.wValue = cpu_to_le16(0);
- pegasus->dr.wIndex = cpu_to_le16(EthCtrl0);
- pegasus->dr.wLength = cpu_to_le16(3);
- pegasus->ctrl_urb->transfer_buffer_length = 3;
+ int ret = -ENOMEM;
+ struct urb *async_urb;
+ struct usb_ctrlrequest *req;

- usb_fill_control_urb(pegasus->ctrl_urb, pegasus->usb,
- usb_sndctrlpipe(pegasus->usb, 0),
- (char *) &pegasus->dr,
- pegasus->eth_regs, 3, ctrl_callback, pegasus);
+ req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_ATOMIC);
+ if (req == NULL)
+ return ret;

- if ((ret = usb_submit_urb(pegasus->ctrl_urb, GFP_ATOMIC))) {
+ async_urb = usb_alloc_urb(0, GFP_ATOMIC);
+ if (async_urb == NULL) {
+ kfree(req);
+ return ret;
+ }
+ req->bRequestType = PEGASUS_REQT_WRITE;
+ req->bRequest = PEGASUS_REQ_SET_REGS;
+ req->wValue = cpu_to_le16(0);
+ req->wIndex = cpu_to_le16(EthCtrl0);
+ req->wLength = cpu_to_le16(3);
+
+ usb_fill_control_urb(async_urb, pegasus->usb,
+ usb_sndctrlpipe(pegasus->usb, 0), (void *)req,
+ pegasus->eth_regs, 3, async_ctrl_callback, req);
+
+ ret = usb_submit_urb(async_urb, GFP_ATOMIC);
+ if (ret) {
if (ret == -ENODEV)
netif_device_detach(pegasus->net);
netif_err(pegasus, drv, pegasus->net,
- "%s, status %d\n", __func__, ret);
+ "%s returned %d\n", __func__, ret);
}
-
return ret;
}

@@ -899,7 +774,6 @@ static void free_all_urbs(pegasus_t *pegasus)
usb_free_urb(pegasus->intr_urb);
usb_free_urb(pegasus->tx_urb);
usb_free_urb(pegasus->rx_urb);
- usb_free_urb(pegasus->ctrl_urb);
}

static void unlink_all_urbs(pegasus_t *pegasus)
@@ -907,47 +781,42 @@ static void unlink_all_urbs(pegasus_t *pegasus)
usb_kill_urb(pegasus->intr_urb);
usb_kill_urb(pegasus->tx_urb);
usb_kill_urb(pegasus->rx_urb);
- usb_kill_urb(pegasus->ctrl_urb);
}

static int alloc_urbs(pegasus_t *pegasus)
{
- pegasus->ctrl_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!pegasus->ctrl_urb)
- return 0;
+ int res = -ENOMEM;
+
pegasus->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!pegasus->rx_urb) {
- usb_free_urb(pegasus->ctrl_urb);
- return 0;
+ return res;
}
pegasus->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!pegasus->tx_urb) {
usb_free_urb(pegasus->rx_urb);
- usb_free_urb(pegasus->ctrl_urb);
- return 0;
+ return res;
}
pegasus->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!pegasus->intr_urb) {
usb_free_urb(pegasus->tx_urb);
usb_free_urb(pegasus->rx_urb);
- usb_free_urb(pegasus->ctrl_urb);
- return 0;
+ return res;
}

- return 1;
+ return 0;
}

static int pegasus_open(struct net_device *net)
{
pegasus_t *pegasus = netdev_priv(net);
- int res;
+ int res=-ENOMEM;

if (pegasus->rx_skb == NULL)
pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net,
PEGASUS_MTU,
GFP_KERNEL);
if (!pegasus->rx_skb)
- return -ENOMEM;
+ goto exit;

res = set_registers(pegasus, EthID, 6, net->dev_addr);

@@ -973,7 +842,8 @@ static int pegasus_open(struct net_device *net)
usb_kill_urb(pegasus->rx_urb);
goto exit;
}
- if ((res = enable_net_traffic(net, pegasus->usb))) {
+ res = enable_net_traffic(net, pegasus->usb);
+ if (res < 0) {
netif_dbg(pegasus, ifup, net,
"can't enable_net_traffic() - %d\n", res);
res = -EIO;
@@ -1153,11 +1023,7 @@ static void pegasus_set_multicast(struct net_device *net)
pegasus->eth_regs[EthCtrl0] &= ~RX_MULTICAST;
pegasus->eth_regs[EthCtrl2] &= ~RX_PROMISCUOUS;
}
-
- pegasus->ctrl_urb->status = 0;
-
- pegasus->flags |= ETH_REGS_CHANGE;
- ctrl_callback(pegasus->ctrl_urb);
+ update_eth_regs_async(pegasus);
}

static __u8 mii_phy_probe(pegasus_t *pegasus)
@@ -1274,9 +1140,9 @@ static int pegasus_probe(struct usb_interface *intf,

pegasus = netdev_priv(net);
pegasus->dev_index = dev_index;
- init_waitqueue_head(&pegasus->ctrl_wait);

- if (!alloc_urbs(pegasus)) {
+ res = alloc_urbs(pegasus);
+ if (res < 0) {
dev_err(&intf->dev, "can't allocate %s\n", "urbs");
goto out1;
}
@@ -1326,12 +1192,9 @@ static int pegasus_probe(struct usb_interface *intf,
if (res)
goto out3;
queue_delayed_work(pegasus_workqueue, &pegasus->carrier_check,
- CARRIER_CHECK_DELAY);
-
- dev_info(&intf->dev, "%s, %s, %pM\n",
- net->name,
- usb_dev_id[dev_index].name,
- net->dev_addr);
+ CARRIER_CHECK_DELAY);
+ dev_info(&intf->dev, "%s, %s, %pM\n", net->name,
+ usb_dev_id[dev_index].name, net->dev_addr);
return 0;

out3:
diff --git a/drivers/net/usb/pegasus.h b/drivers/net/usb/pegasus.h
index 00d44e3..d156462 100644
--- a/drivers/net/usb/pegasus.h
+++ b/drivers/net/usb/pegasus.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1999-2003 Petko Manolov - Petkan (petkan@xxxxxxxxxxxxxxxxxxxxx)
+ * Copyright (c) 1999-2013 Petko Manolov (petkan@xxxxxxxxxxxxx)
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as published
@@ -33,8 +33,6 @@
#define CTRL_URB_SLEEP 0x00000020
#define PEGASUS_UNPLUG 0x00000040
#define PEGASUS_RX_URB_FAIL 0x00000080
-#define ETH_REGS_CHANGE 0x40000000
-#define ETH_REGS_CHANGED 0x80000000

#define RX_MULTICAST 2
#define RX_PROMISCUOUS 4
@@ -95,10 +93,8 @@ typedef struct pegasus {
int intr_interval;
struct tasklet_struct rx_tl;
struct delayed_work carrier_check;
- struct urb *ctrl_urb, *rx_urb, *tx_urb, *intr_urb;
+ struct urb *rx_urb, *tx_urb, *intr_urb;
struct sk_buff *rx_skb;
- struct usb_ctrlrequest dr;
- wait_queue_head_t ctrl_wait;
int chip;
unsigned char intr_buff[8];
__u8 tx_buff[PEGASUS_MTU];
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/