On Saturday 24 January 2009 05:18:07 Artur Skawina wrote:
> Christian Lamparter wrote:
> > 1) urb_poison_anchored_urbs gets called
> > 1) poison anchor structure
> > 2) poison & killing every single urb
> > 2) the usb_hcd_giveback_urb is called
> > 1) >>unanchores<< the urb form anchor_list
> > 2) calles urb->complete (urb)
> > 3) p54u_rx_cb -here- but nothing interesting there
> > 3) ... [time goes by]
> > 4) urb_unpoison_anchored_urb is called
> > 1) unpoison the anchor structure
> > 2) tries to unpoison the anchored urbs... But there's not a single one in the anchor_list,
> > since step 2.1 (usb_hcd_giveback_urb) killed them off.
>
> I assume that's just how it's supposed to be. You could always anchor
> the urbs to another anchor in the completion_. Or free any buffers and
> drop the last ref before leaving the completion. (in fact, the former
> is basically what you're doing, just using a list instead)
true! In fact --- behold the clean up patch which uses another anchor instead of the list ---
> >> I'm curious why you keep the urbs around in the stopped state?
> > well, in most cases the "stopped state" is very short and most wlan-adapters are always connected.
> > So, why throw them away when we need them again in a few seconds?
> > (usually wpa_supplicant/NM has the bad habit of doing a interface up/down dance... sometimes)
>
> ok. (i don't know about most wlans being always up, but it seems a
> reasonable compromise. still, that's 100k+ wasted ram in the down
> state.)
??? We don't waste 100k+.
We recycle the skbs, so the only thing left is 32 urb structures.
> > well, we don't schedule the workqueue if we canceling the urbs now,
> > ( that's what the urb->status switch is supposed to do/( or in this context )stop...)
>
> yep, noticed that later, see below.
>
> > Another maybe related thing: ( a bit above)
> > * In order to prevent a loop, we put the URB
> > * back at the _front_ of the list, so we can
> > * march on, in out-of-memory situations.
> >
> > I guess this could be true for -EPERM as well?
> > As far as I know list_for_each_entry_* iterates until it hits (head)
> > and since we insert the -EPERM "urb" with list_add (_head),
> > we will never do more than 32 iterations?! (since list_add put the elements in (head)->next )
> >
> > But if we cancel on -EPERM, we should bail out on -ENODEV
> > (or -ECONNRESET, what ever says that the device is unavailable ) as well...
>
> I'm not sure I follow.. Ah, the only reason I bailed out on -EPERM
> is that usb_submit_urb() will return -EPERM for poisoned urbs and
> i didn't want to retry this call for every other urb as they would
> all fail. Each try involves a useless skb alloc and free...
> [My version schedules the work for every urb, even the poisoned ones]
well, there's now a hard limit... no change of a endless loop now.
> >>> + if (skb_queue_len(&priv->rx_queue) != 32) {
> >>> + dev_err(&priv->udev->dev, "Not enough useable transfer buffers "
> >>> + "available to initialize the device.");
> >>> + return -ENOMEM;
> >> Why 32 urbs?
> > Well, that's the firmware/hardware limit for all prism54 chips
> > (doesn't matter if usb/pci fullmac/softmac etc...)
> > all have 32 rx and tx slots in the "normal priority" queue/ring-buffer.
> >
> >> And why should open() fail if, say, only 28 got successfully allocated?
> >> Shouldn't the device function nonetheless?
> > Well, what's the point of supporting a system that has problems finding 32 pages with GFP_KERNEL?
> > you know "one allocation on device init isn't worth avoiding." :-p
>
> ok. that's not something this patch changes anyway ;)
>
>
> I looked at your v2 briefly yesterday and even wrote a reply, but
> didn't send it. I really liked your v1 much better, the new version
> makes the code much harder to follow, and still can stall the device
> after a few consecutive urb completion or submission (this is new)
> errors happen. Uhm, i probably should shut up now ;)
be prepared to write the reply again ;-)
Yeah, it's always easier to follow your own code, however its sometimes
harder to find bugs, because you assumed you did everything right in the first place...
---
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 9539ddc..64da6cb 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -89,54 +89,102 @@ static void p54u_rx_cb(struct urb *urb)
skb_unlink(skb, &priv->rx_queue);
- if (unlikely(urb->status)) {
- dev_kfree_skb_irq(skb);
- return;
- }
-
- skb_put(skb, urb->actual_length);
+ switch (urb->status) {
+ case 0:
+ /*
+ * The device sent us a packet for processing.
+ */
+ skb_put(skb, urb->actual_length);
+
+ /*
+ * remove firmware/device specific headers in
+ * front of the frame.
+ */
+ if (priv->hw_type == P54U_NET2280)
+ skb_pull(skb, priv->common.tx_hdr_len);
+ if (priv->common.fw_interface == FW_LM87) {
+ skb_pull(skb, 4);
+ skb_put(skb, 4);
+ }
- if (priv->hw_type == P54U_NET2280)
- skb_pull(skb, priv->common.tx_hdr_len);
- if (priv->common.fw_interface == FW_LM87) {
- skb_pull(skb, 4);
- skb_put(skb, 4);
- }
+ if (p54_rx(dev, skb)) {
+ /*
+ * This skb has been accepted by library and now
+ * belongs 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->context = skb;
+ break;
+ }
+ }
- if (p54_rx(dev, skb)) {
- skb = dev_alloc_skb(priv->common.rx_mtu + 32);
- if (unlikely(!skb)) {
- /* TODO check rx queue length and refill *somewhere* */
- return;
+ usb_anchor_urb(urb, &priv->urb_pool);
+ queue_work(priv->common.hw->workqueue,
+ &priv->rx_refill_work);
+ return ;
}
- info = (struct p54u_rx_info *) skb->cb;
- info->urb = urb;
- info->dev = dev;
- urb->transfer_buffer = skb_tail_pointer(skb);
- urb->context = skb;
- } else {
+ /* Reverse all modifications, it must look like new. */
if (priv->hw_type == P54U_NET2280)
skb_push(skb, priv->common.tx_hdr_len);
if (priv->common.fw_interface == FW_LM87) {
skb_push(skb, 4);
skb_put(skb, 4);
}
- skb_reset_tail_pointer(skb);
- skb_trim(skb, 0);
- if (urb->transfer_buffer != skb_tail_pointer(skb)) {
- /* this should not happen */
- WARN_ON(1);
- urb->transfer_buffer = skb_tail_pointer(skb);
- }
+
+ break;
+
+ case -ENOENT:
+ case -ECONNRESET:
+ case -ENODEV:
+ case -ESHUTDOWN:
+ /*
+ * The device has been stopped or disconnected.
+ * Free the skb and put the URBs into the refill_list.
+ */
+
+ usb_anchor_urb(urb, &priv->urb_pool);
+ dev_kfree_skb_irq(skb);
+ return;
+
+ default:
+ /*
+ * USB error
+ * This frame is lost, but we can resubmit URB + skb and
+ * wait for a successful retry.
+ */
+ break;
}
- skb_queue_tail(&priv->rx_queue, skb);
+
+ /*
+ * Reuse the URB and its associated skb.
+ * Reset all data pointers into their original state and resubmit it.
+ */
+ skb_reset_tail_pointer(skb);
+ skb_trim(skb, 0);
+ urb->transfer_buffer = skb_tail_pointer(skb);
+
usb_anchor_urb(urb, &priv->submitted);
if (usb_submit_urb(urb, GFP_ATOMIC)) {
- skb_unlink(skb, &priv->rx_queue);
usb_unanchor_urb(urb);
dev_kfree_skb_irq(skb);
- }
+ usb_anchor_urb(urb, &priv->urb_pool);
+ } else
+ skb_queue_tail(&priv->rx_queue, skb);
+
+ return ;
}
static void p54u_tx_cb(struct urb *urb)
@@ -150,59 +198,146 @@ static void p54u_tx_cb(struct urb *urb)
static void p54u_tx_dummy_cb(struct urb *urb) { }
-static void p54u_free_urbs(struct ieee80211_hw *dev)
+static void p54u_cancel_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
- usb_kill_anchored_urbs(&priv->submitted);
+
+ usb_poison_anchored_urbs(&priv->submitted);
+
+ /*
+ * By now every RX URB has either finished or been canceled;
+ * 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);
+
+ /*
+ * 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);
+
+ /*
+ * Unpoison all unused URBs in the pool, in case we want to reuse them.
+ */
+ usb_unpoison_anchored_urbs(&priv->urb_pool);
}
-static int p54u_init_urbs(struct ieee80211_hw *dev)
+static int p54u_rx_refill(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
- struct urb *entry = NULL;
- struct sk_buff *skb;
- struct p54u_rx_info *info;
- int ret = 0;
+ struct urb *entry;
+ unsigned int refilled_urbs = 0, cnt = 0;
+ int err = -EINVAL;
+
+ while (cnt++ != 32 && (entry = usb_get_from_anchor(&priv->urb_pool))) {
+ struct p54u_rx_info *info;
+ struct sk_buff *skb;
- while (skb_queue_len(&priv->rx_queue) < 32) {
skb = __dev_alloc_skb(priv->common.rx_mtu + 32, GFP_KERNEL);
- if (!skb) {
- ret = -ENOMEM;
- goto err;
- }
- entry = usb_alloc_urb(0, GFP_KERNEL);
- if (!entry) {
- ret = -ENOMEM;
- goto err;
+
+ if (unlikely(!skb)) {
+ /*
+ * In order to prevent a loop, we put the URB
+ * back at the _front_ of the list, so we can
+ * march on, in out-of-memory situations.
+ */
+
+ usb_anchor_urb(entry, &priv->urb_pool);
+ usb_free_urb(entry);
+ err = -ENOMEM;
+ continue;
}
usb_fill_bulk_urb(entry, priv->udev,
usb_rcvbulkpipe(priv->udev, P54U_PIPE_DATA),
skb_tail_pointer(skb),
priv->common.rx_mtu + 32, p54u_rx_cb, skb);
+
info = (struct p54u_rx_info *) skb->cb;
info->urb = entry;
info->dev = dev;
- skb_queue_tail(&priv->rx_queue, skb);
usb_anchor_urb(entry, &priv->submitted);
- ret = usb_submit_urb(entry, GFP_KERNEL);
- if (ret) {
- skb_unlink(skb, &priv->rx_queue);
+ err = usb_submit_urb(entry, GFP_KERNEL);
+ if (err) {
+ /*
+ * URB submission failed.
+ * Free the associated skb and put the URB back into
+ * the front of the refill list, so we can try our luck
+ * next time.
+ */
+
+ dev_err(&priv->udev->dev, "failed to resubmit %p\n",
+ entry);
+
usb_unanchor_urb(entry);
- goto err;
+ kfree_skb(skb);
+ usb_anchor_urb(entry, &priv->urb_pool);
+ } else {
+ skb_queue_tail(&priv->rx_queue, skb);
+ refilled_urbs++;
}
usb_free_urb(entry);
- entry = NULL;
+ }
+
+ return refilled_urbs ? 0 : err;
+}
+
+static int p54u_open(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ int err;
+
+ err = p54u_rx_refill(dev);
+ if (err)
+ return err;
+
+ if (skb_queue_len(&priv->rx_queue) != 32) {
+ dev_err(&priv->udev->dev, "Not enough useable transfer buffers "
+ "available to initialize the device.");
+ p54u_cancel_urbs(dev);
+ return -ENOMEM;
}
return 0;
+}
- err:
- usb_free_urb(entry);
- kfree_skb(skb);
- p54u_free_urbs(dev);
- return ret;
+static void p54u_drain_urb_pool(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ struct urb *entry;
+
+ while ((entry = usb_get_from_anchor(&priv->urb_pool))) {
+ usb_free_urb(entry);
+ usb_free_urb(entry);
+ }
+}
+
+
+static int p54u_init_urbs(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ struct urb *entry;
+ int err = 0;
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ entry = usb_alloc_urb(0, GFP_KERNEL);
+ if (!entry) {
+ err = -ENOMEM;
+ break;
+ }
+
+ usb_anchor_urb(entry, &priv->urb_pool);
+ }
+
+ if (err)
+ p54u_drain_urb_pool(dev);
+
+ return err;
}
static void p54u_tx_3887(struct ieee80211_hw *dev, struct sk_buff *skb)
@@ -880,28 +1015,12 @@ static int p54u_upload_firmware_net2280(struct ieee80211_hw *dev)
return err;
}
-static int p54u_open(struct ieee80211_hw *dev)
+static void p54u_rx_refill_work(struct work_struct *work)
{
- struct p54u_priv *priv = dev->priv;
- int err;
-
- err = p54u_init_urbs(dev);
- if (err) {
- return err;
- }
-
- priv->common.open = p54u_init_urbs;
-
- return 0;
-}
+ struct p54u_priv *priv = container_of(work, struct p54u_priv,
+ rx_refill_work);
-static void p54u_stop(struct ieee80211_hw *dev)
-{
- /* TODO: figure out how to reliably stop the 3887 and net2280 so
- the hardware is still usable next time we want to start it.
- until then, we just stop listening to the hardware.. */
- p54u_free_urbs(dev);
- return;
+ p54u_rx_refill(priv->common.hw);
}
static int __devinit p54u_probe(struct usb_interface *intf,
@@ -928,6 +1047,8 @@ static int __devinit p54u_probe(struct usb_interface *intf,
priv->intf = intf;
skb_queue_head_init(&priv->rx_queue);
init_usb_anchor(&priv->submitted);
+ init_usb_anchor(&priv->urb_pool);
+ INIT_WORK(&priv->rx_refill_work, p54u_rx_refill_work);
usb_get_dev(udev);
@@ -949,8 +1070,7 @@ static int __devinit p54u_probe(struct usb_interface *intf,
recognized_pipes++;
}
}
- priv->common.open = p54u_open;
- priv->common.stop = p54u_stop;
+
if (recognized_pipes < P54U_PIPE_NUMBER) {
priv->hw_type = P54U_3887;
err = p54u_upload_firmware_3887(dev);
@@ -970,12 +1090,19 @@ static int __devinit p54u_probe(struct usb_interface *intf,
if (err)
goto err_free_dev;
- p54u_open(dev);
+ err = p54u_init_urbs(dev);
+ if (err)
+ goto err_free_dev;
+ err = p54u_open(dev);
+ if (err)
+ goto err_free_dev;
err = p54_read_eeprom(dev);
- p54u_stop(dev);
+ p54u_cancel_urbs(dev);
if (err)
goto err_free_dev;
+ priv->common.open = p54u_open;
+ priv->common.stop = p54u_cancel_urbs;
err = ieee80211_register_hw(dev);
if (err) {
dev_err(&udev->dev, "(p54usb) Cannot register netdevice\n");
@@ -999,9 +1126,12 @@ static void __devexit p54u_disconnect(struct usb_interface *intf)
if (!dev)
return;
+ priv = dev->priv;
+
+ p54u_drain_urb_pool(dev);
+
ieee80211_unregister_hw(dev);
- priv = dev->priv;
usb_put_dev(interface_to_usbdev(intf));
p54_free_common(dev);
ieee80211_free_hw(dev);
diff --git a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
index 8bc5898..4a5d8b2 100644
--- a/drivers/net/wireless/p54/p54usb.h
+++ b/drivers/net/wireless/p54/p54usb.h
@@ -132,9 +132,10 @@ struct p54u_priv {
P54U_3887
} hw_type;
- spinlock_t lock;
struct sk_buff_head rx_queue;
+ struct work_struct rx_refill_work;
struct usb_anchor submitted;
+ struct usb_anchor urb_pool;
};
#endif /* P54USB_H */
--
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
