From 78fc800f06a72c25842e585fd747fa6a98f3f0e5 Mon Sep 17 00:00:00 2001 From: Jussi Kivilinna Date: Mon, 31 Jan 2011 20:47:08 +0200 Subject: [PATCH] zd1211rw: use urb anchors for tx and fix tx-queue disabling When stress testing AP-mode I hit OOPS when unpluging or rmmodding driver. It appears that when tx-queue is disabled, tx-urbs might be left pending. These can cause ehci to call non-existing tx_urb_complete() (after rmmod) or uninitialized/reseted private structure (after disconnect()). Add skb queue for submitted packets and unlink pending urbs on zd_usb_disable_tx(). Part of the problem seems to be usb->free_urb_list that isn't always working as it should, causing machine freeze when trying to free the list in zd_usb_disable_tx(). Caching free urbs isn't what other drivers seem to be doing (usbnet for example) so strip free_usb_list. Patch makes tx-urb handling saner with use of urb anchors. Signed-off-by: Jussi Kivilinna Signed-off-by: John W. Linville --- drivers/net/wireless/zd1211rw/zd_usb.c | 108 +++++++++---------------- drivers/net/wireless/zd1211rw/zd_usb.h | 8 +- 2 files changed, 41 insertions(+), 75 deletions(-) diff --git a/drivers/net/wireless/zd1211rw/zd_usb.c b/drivers/net/wireless/zd1211rw/zd_usb.c index 06041cb1c422..c32a2472eb44 100644 --- a/drivers/net/wireless/zd1211rw/zd_usb.c +++ b/drivers/net/wireless/zd1211rw/zd_usb.c @@ -779,19 +779,20 @@ void zd_usb_disable_tx(struct zd_usb *usb) { struct zd_usb_tx *tx = &usb->tx; unsigned long flags; - struct list_head *pos, *n; + + atomic_set(&tx->enabled, 0); + + /* kill all submitted tx-urbs */ + usb_kill_anchored_urbs(&tx->submitted); spin_lock_irqsave(&tx->lock, flags); - list_for_each_safe(pos, n, &tx->free_urb_list) { - list_del(pos); - usb_free_urb(list_entry(pos, struct urb, urb_list)); - } - tx->enabled = 0; + WARN_ON(tx->submitted_urbs != 0); tx->submitted_urbs = 0; + spin_unlock_irqrestore(&tx->lock, flags); + /* The stopped state is ignored, relying on ieee80211_wake_queues() * in a potentionally following zd_usb_enable_tx(). */ - spin_unlock_irqrestore(&tx->lock, flags); } /** @@ -807,63 +808,13 @@ void zd_usb_enable_tx(struct zd_usb *usb) struct zd_usb_tx *tx = &usb->tx; spin_lock_irqsave(&tx->lock, flags); - tx->enabled = 1; + atomic_set(&tx->enabled, 1); tx->submitted_urbs = 0; ieee80211_wake_queues(zd_usb_to_hw(usb)); tx->stopped = 0; spin_unlock_irqrestore(&tx->lock, flags); } -/** - * alloc_tx_urb - provides an tx URB - * @usb: a &struct zd_usb pointer - * - * Allocates a new URB. If possible takes the urb from the free list in - * usb->tx. - */ -static struct urb *alloc_tx_urb(struct zd_usb *usb) -{ - struct zd_usb_tx *tx = &usb->tx; - unsigned long flags; - struct list_head *entry; - struct urb *urb; - - spin_lock_irqsave(&tx->lock, flags); - if (list_empty(&tx->free_urb_list)) { - urb = usb_alloc_urb(0, GFP_ATOMIC); - goto out; - } - entry = tx->free_urb_list.next; - list_del(entry); - urb = list_entry(entry, struct urb, urb_list); -out: - spin_unlock_irqrestore(&tx->lock, flags); - return urb; -} - -/** - * free_tx_urb - frees a used tx URB - * @usb: a &struct zd_usb pointer - * @urb: URB to be freed - * - * Frees the transmission URB, which means to put it on the free URB - * list. - */ -static void free_tx_urb(struct zd_usb *usb, struct urb *urb) -{ - struct zd_usb_tx *tx = &usb->tx; - unsigned long flags; - - spin_lock_irqsave(&tx->lock, flags); - if (!tx->enabled) { - usb_free_urb(urb); - goto out; - } - list_add(&urb->urb_list, &tx->free_urb_list); -out: - spin_unlock_irqrestore(&tx->lock, flags); -} - static void tx_dec_submitted_urbs(struct zd_usb *usb) { struct zd_usb_tx *tx = &usb->tx; @@ -905,6 +856,16 @@ static void tx_urb_complete(struct urb *urb) struct sk_buff *skb; struct ieee80211_tx_info *info; struct zd_usb *usb; + struct zd_usb_tx *tx; + + skb = (struct sk_buff *)urb->context; + info = IEEE80211_SKB_CB(skb); + /* + * grab 'usb' pointer before handing off the skb (since + * it might be freed by zd_mac_tx_to_dev or mac80211) + */ + usb = &zd_hw_mac(info->rate_driver_data[0])->chip.usb; + tx = &usb->tx; switch (urb->status) { case 0: @@ -922,20 +883,15 @@ static void tx_urb_complete(struct urb *urb) goto resubmit; } free_urb: - skb = (struct sk_buff *)urb->context; - /* - * grab 'usb' pointer before handing off the skb (since - * it might be freed by zd_mac_tx_to_dev or mac80211) - */ - info = IEEE80211_SKB_CB(skb); - usb = &zd_hw_mac(info->rate_driver_data[0])->chip.usb; zd_mac_tx_to_dev(skb, urb->status); - free_tx_urb(usb, urb); + usb_free_urb(urb); tx_dec_submitted_urbs(usb); return; resubmit: + usb_anchor_urb(urb, &tx->submitted); r = usb_submit_urb(urb, GFP_ATOMIC); if (r) { + usb_unanchor_urb(urb); dev_dbg_f(urb_dev(urb), "error resubmit urb %p %d\n", urb, r); goto free_urb; } @@ -958,8 +914,14 @@ int zd_usb_tx(struct zd_usb *usb, struct sk_buff *skb) int r; struct usb_device *udev = zd_usb_to_usbdev(usb); struct urb *urb; + struct zd_usb_tx *tx = &usb->tx; - urb = alloc_tx_urb(usb); + if (!atomic_read(&tx->enabled)) { + r = -ENOENT; + goto out; + } + + urb = usb_alloc_urb(0, GFP_ATOMIC); if (!urb) { r = -ENOMEM; goto out; @@ -968,13 +930,16 @@ int zd_usb_tx(struct zd_usb *usb, struct sk_buff *skb) usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, EP_DATA_OUT), skb->data, skb->len, tx_urb_complete, skb); + usb_anchor_urb(urb, &tx->submitted); r = usb_submit_urb(urb, GFP_ATOMIC); - if (r) + if (r) { + usb_unanchor_urb(urb); goto error; + } tx_inc_submitted_urbs(usb); return 0; error: - free_tx_urb(usb, urb); + usb_free_urb(urb); out: return r; } @@ -1005,9 +970,9 @@ static inline void init_usb_tx(struct zd_usb *usb) { struct zd_usb_tx *tx = &usb->tx; spin_lock_init(&tx->lock); - tx->enabled = 0; + atomic_set(&tx->enabled, 0); tx->stopped = 0; - INIT_LIST_HEAD(&tx->free_urb_list); + init_usb_anchor(&tx->submitted); tx->submitted_urbs = 0; } @@ -1240,6 +1205,7 @@ static void disconnect(struct usb_interface *intf) ieee80211_unregister_hw(hw); /* Just in case something has gone wrong! */ + zd_usb_disable_tx(usb); zd_usb_disable_rx(usb); zd_usb_disable_int(usb); diff --git a/drivers/net/wireless/zd1211rw/zd_usb.h b/drivers/net/wireless/zd1211rw/zd_usb.h index 1b1655cb7cb4..233ce825b71c 100644 --- a/drivers/net/wireless/zd1211rw/zd_usb.h +++ b/drivers/net/wireless/zd1211rw/zd_usb.h @@ -184,18 +184,18 @@ struct zd_usb_rx { /** * struct zd_usb_tx - structure used for transmitting frames + * @enabled: atomic enabled flag, indicates whether tx is enabled * @lock: lock for transmission - * @free_urb_list: list of free URBs, contains all the URBs, which can be used + * @submitted: anchor for URBs sent to device * @submitted_urbs: atomic integer that counts the URBs having sent to the * device, which haven't been completed - * @enabled: enabled flag, indicates whether tx is enabled * @stopped: indicates whether higher level tx queues are stopped */ struct zd_usb_tx { + atomic_t enabled; spinlock_t lock; - struct list_head free_urb_list; + struct usb_anchor submitted; int submitted_urbs; - int enabled; int stopped; }; -- GitLab