Re: [RFC][RFT][PATCH] p54usb: rx refill revamp

[Artur Skawina <art.08.09@gmail.com>]

>>> 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@gmail.com>
Date:   Thu Jan 22 21:01:32 2009 +0100

    Signed-off-by: Artur Skawina <art.08.09@gmail.com>

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