>>> that not queuing the work after an urb fails with urb->status==true is
>>> safe -- what if some temporary error condition causes the rx queue to
>>> drain? Nothing will resubmit the urbs.
Here's an updated version, hopefully the extra commentary makes it
a bit more obvious. I added the allocate-in-irq fallback too.
Tested, including the fallback path, works.
The !priv->common.hw->workqueue path only triggers when freeing the urbs
after reading the eeprom (ie while killing/poisoning the urbs), hence it is
completely harmless and i would replace it w/ just:
if (priv->common.hw->workqueue)
queue_work(priv->common.hw->workqueue, &priv->rx_refill_work);
Other than that, patch seems ready.
Thanks,
artur
commit c93ede1be61a0db0a904424738afe3b75425a782
Author: Artur Skawina <art.08.09@xxxxxxxxx>
Date: Thu Jan 22 21:01:32 2009 +0100
Signed-off-by: Artur Skawina <art.08.09@xxxxxxxxx>
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 32534c1..872ace1 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -80,6 +80,15 @@ static struct usb_device_id p54u_table[] __devinitdata = {
MODULE_DEVICE_TABLE(usb, p54u_table);
+/*
+ * Every successfully submitted RX urb goes through this completion
+ * function. Each urb must be added to the refill list after being
+ * processed, this includes the failed/canceled ones (status!=0).
+ * p54u_rx_refill() will take care of resubmitting them later.
+ * Alternatively, an urb can be reanchored and resubmitted, it will
+ * then come back here again, after the I/O is done.
+ */
+
static void p54u_rx_cb(struct urb *urb)
{
struct sk_buff *skb = (struct sk_buff *) urb->context;
@@ -124,7 +133,7 @@ static void p54u_rx_cb(struct urb *urb)
WARN_ON(1);
urb->transfer_buffer = skb_tail_pointer(skb);
}
-
+resubmit_urb: /* Now both the urb and the skb are as good as new */
usb_anchor_urb(urb, &priv->submitted);
if (usb_submit_urb(urb, GFP_ATOMIC) == 0) {
skb_queue_tail(&priv->rx_queue, skb);
@@ -133,11 +142,32 @@ static void p54u_rx_cb(struct urb *urb)
usb_unanchor_urb(urb);
dev_kfree_skb_irq(skb);
}
+ } else {
+ /*
+ * This skb has been given away to the mac80211 layer.
+ * We should put the urb on the refill list, where it
+ * can be linked to a newly allocated skb later.
+ * Except, if the workqueue that does the refilling can't
+ * keep up, we'll try a bit harder and attempt to obtain
+ * a new skb right here.
+ */
+ if (skb_queue_len(&priv->rx_queue)<4) {
+ skb = dev_alloc_skb(priv->common.rx_mtu + 32);
+ if (skb) {
+ info = (struct p54u_rx_info *)skb->cb;
+ info->urb = urb;
+ info->dev = dev;
+ /* Update the urb to use the new skb */
+ urb->transfer_buffer = skb_tail_pointer(skb);
+ urb->context = skb;
+ goto resubmit_urb;
+ }
+ }
}
/*
* This skb CANNOT be reused.
- * Put the now unused urb into a list and do the refilled later on in
+ * Put the now unused urb into a list and do the refill later on in
* the less critical workqueue thread.
* This eases the memory pressure and prevents latency spikes.
*/
@@ -150,16 +180,12 @@ stash_urb:
list_add_tail(&urb->urb_list, &priv->rx_refill_list);
spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
- if (unlikely(urb->status)) {
- return;
- }
-
if (unlikely(!priv->common.hw->workqueue)) {
/*
* Huh? mac80211 isn't fully initialized yet?
* Please check your system, something bad is going on.
*/
- WARN_ON(1);
+ printk(KERN_WARNING "p54u_rx_cb(): workqueue missing\n");
return;
}
@@ -195,8 +221,21 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
+ usb_poison_anchored_urbs(&priv->submitted);
+ /*
+ * By now every RX urb has either finished or been cancelled;
+ * the p54u_rx_cb() completion has placed it on the refill
+ * list; any attempts to resubmit from p54u_rx_refill(),
+ * which could still be scheduled to run, will fail.
+ */
cancel_work_sync(&priv->rx_refill_work);
- usb_kill_anchored_urbs(&priv->submitted);
+ p54u_free_rx_refill_list(dev);
+ /*
+ * Unpoison the anchor itself; the old urbs are already gone,
+ * p54u_rx_cb() has moved them all to the refill list.
+ * Prevents new urbs from being poisoned when anchored.
+ */
+ usb_unpoison_anchored_urbs(&priv->submitted);
}
static int p54u_rx_refill(struct ieee80211_hw *dev)
@@ -205,6 +244,7 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
struct urb *entry, *tmp;
unsigned long flags;
unsigned int filled = 0;
+ int ret = 0;
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_for_each_entry_safe(entry, tmp, &priv->rx_refill_list, urb_list) {
@@ -237,7 +277,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
info->dev = dev;
usb_anchor_urb(entry, &priv->submitted);
- if (usb_submit_urb(entry, GFP_KERNEL)) {
+ ret=usb_submit_urb(entry, GFP_KERNEL);
+ if (ret) {
/*
* urb submittion failed.
* free the associated skb and put the urb back into
@@ -249,6 +290,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
kfree_skb(skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_add(&entry->urb_list, &priv->rx_refill_list);
+ if (ret==-EPERM) /* urb has been poisoned */
+ break; /* no point in trying to submit the rest */
} else {
skb_queue_tail(&priv->rx_queue, skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
@@ -280,9 +323,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
}
p54u_rx_refill(dev);
-
err:
- p54u_free_rx_refill_list(dev);
+ if (ret)
+ p54u_free_rx_refill_list(dev);
return ret;
}
@@ -979,7 +1022,7 @@ static int p54u_open(struct ieee80211_hw *dev)
return err;
}
- priv->common.open = p54u_rx_refill;
+ priv->common.open = p54u_init_urbs;
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
