Well, here's a patch that makes USB a lot less sensitive to drivers that don't unlink all urbs in their disconnect() handlers: usbcore will now clean up after them, not (just some of) the HCDs.
It also reverts that "disconnect() may not unlink" patch that's been so troublesome (except in some test of Greg's)
This goes on top of the "unlink-0403.patch" I sent, and basically seems to work except that "rmmod ohci-hcd" wedges in what looks like sysfs (recent BK). I'm sending it around anyway since the changes are worth some review, and Greg wanted to see it.
- Adds a new disable() callback to usb_operations callback so
HCDs can scrub hardware and software state for given endpoint
during config changes or disconnect. It may sleep. This is needed for hardware (like EHCI and OHCI) that stores
endpoint state in the hardware QH/ED. (UHCI stores it in the
struct usb_device.) The deallocate() callback never worked
right for this since the LK 2.3 device refcounting changes. It will also finally allow us to safely implement the calls
to change device or interface configuration, and resolve a
bunch of FIXMEs and pray-we-never-see-this bugs -- later.- Implements that disable() for the HCD framework.
This necessarily adds a new endpoint_disable() callback to
the hc_driver. Not quite as necessarily, it replaces the
previous free_config() callback: that disabled N endpoints,
this new call just disables one. Net result: calling usb_operations->disable() asynchronously
unlinks any pending URBs (the HCD framework tracks them). Then
if the hardware driver wants it, it calls endpoint_disable();
when that returns, the unlinks finished and the endpoint state
is freed. - The EHCI and OHCI driver free_config() code got simplified
to only operate on one endpoint, and renamed to match its
new endpoint_disable() role. The two routines look similar... - usb_disconnect() now disables all endpoints before it returns.
This is done after all drivers have been disconnected from the
device's interfaces Drivers that correctly unlink all their URBs in disconnect()
will still work, but this CHANGES BEHAVIOR for drivers that
don't. Such drivers have (to date) been oopsably buggy in
various scenarios. The change should be an improvement. Previously, the hardware
handshake was done in the HCD's deallocate() routine ... which
such drivers would cause to be called in non-sleeping contexts,
so kablooie or other troubles. - Cleanup after HC death is a bit quicker too: all the state
gets cleaned up, it's not just marked as "dead". - Random fix, endpoint maxpacket values wouldn't be re-initted
to zero during a config change, as they should have been.Given that (sysfs?) wedge I'd not merge this yet, but it shouldn't be too far from a mergeable state. It'd be worth seeing if any USB drivers break rudely because of this.
- Dave
--- 1.92/drivers/usb/core/hcd.c Tue Mar 25 07:10:33 2003
+++ edited/drivers/usb/core/hcd.c Thu Apr 3 14:08:45 2003
@@ -32,6 +32,7 @@
#include <linux/version.h>
#include <linux/kernel.h>
#include <linux/slab.h>
+#include <linux/sched.h>
#include <linux/completion.h>
#include <linux/uts.h> /* for UTS_SYSNAME */
#include <linux/pci.h> /* for hcd->pdev and dma addressing */
@@ -931,6 +932,7 @@
if (dev == NULL)
return -ENOMEM;
memset (dev, 0, sizeof *dev);
+ dev->udev = udev;
INIT_LIST_HEAD (&dev->dev_list);
INIT_LIST_HEAD (&dev->urb_list);
@@ -1213,11 +1215,89 @@
/*-------------------------------------------------------------------------*/
-/* called by khubd, rmmod, apmd, or other thread for hcd-private cleanup */
+/* disables the endpoint: cancels any pending urbs, then synchronizes with
+ * the hcd to make sure all endpoint state is gone from hardware. use for
+ * set_configuration, set_interface, driver removal, physical disconnect.
+ * example of such state: a qh pointer stored in hcd_dev.ep[].
+ */
+static void hcd_endpoint_disable (struct usb_device *udev, int endpoint)
+{
+ unsigned long flags;
+ struct hcd_dev *dev;
+ struct usb_hcd *hcd;
+ struct urb *urb;
+ unsigned epnum = endpoint & USB_ENDPOINT_NUMBER_MASK;
+
+ spin_lock_irqsave (&hcd_data_lock, flags);
+ dev = udev->hcpriv;
+ hcd = udev->bus->hcpriv;
+
+rescan:
+ /* (re)block new requests, as best we can */
+ if (endpoint & USB_DIR_IN) {
+ usb_endpoint_halt (udev, epnum, 0);
+ udev->epmaxpacketin [epnum] = 0;
+ } else {
+ usb_endpoint_halt (udev, epnum, 1);
+ udev->epmaxpacketout [epnum] = 0;
+ }
-// FIXME: likely best to have explicit per-setting (config+alt)
-// setup primitives in the usbcore-to-hcd driver API, so nothing
-// is implicit. kernel 2.5 needs a bunch of config cleanup...
+ /* then kill any current requests */
+ list_for_each_entry (urb, &dev->urb_list, urb_list) {
+ int tmp;
+
+ /* ignore urbs for other endpoints */
+ if (usb_pipeendpoint (urb->pipe) != epnum)
+ continue;
+ if ((urb->pipe ^ endpoint) & USB_DIR_IN)
+ continue;
+
+ spin_lock (&urb->lock);
+ tmp = urb->status;
+ if (tmp == -EINPROGRESS)
+ urb->status = -ESHUTDOWN;
+ spin_unlock (&urb->lock);
+
+ /* kick hcd unless it's already returning this */
+ if (tmp != -EINPROGRESS)
+ continue;
+
+ tmp = urb->pipe;
+ dev_dbg (hcd->controller,
+ "kill urb %p ep%d%s%s\n",
+ urb, usb_pipeendpoint (tmp),
+ (tmp & USB_DIR_IN) ? "in" : "out",
+ ({ char *s; \
+ switch (usb_pipetype (urb->pipe)) { \
+ case PIPE_CONTROL: s = ""; break; \
+ case PIPE_BULK: s = "-bulk"; break; \
+ case PIPE_INTERRUPT: s = "-intr"; break; \
+ default: s = "-iso"; break; \
+ }; s;}));
+
+ spin_unlock (&hcd_data_lock);
+ unlink1 (hcd, urb);
+ spin_lock (&hcd_data_lock);
+
+ /* list contents may have changed */
+ goto rescan;
+ }
+ spin_unlock_irqrestore (&hcd_data_lock, flags);
+
+ /* synchronize with the hardware, so old configuration state
+ * clears out immediately (and will be freed).
+ */
+ might_sleep ();
+ if (hcd->driver->endpoint_disable)
+ hcd->driver->endpoint_disable (hcd, dev, endpoint);
+}
+
+/*-------------------------------------------------------------------------*/
+
+/* called by khubd, rmmod, apmd, or other thread for hcd-private cleanup.
+ * we're guaranteed that the device is fully quiesced. also, that each
+ * endpoint has been hcd_endpoint_disabled.
+ */
static int hcd_free_dev (struct usb_device *udev)
{
@@ -1238,14 +1318,11 @@
/* device driver problem with refcounts? */
if (!list_empty (&dev->urb_list)) {
- dev_dbg (hcd->controller, "free busy dev, %s devnum %d (bug!)\n",
- hcd->self.bus_name, udev->devnum);
+ dev_dbg (hcd->controller, "busy dev, path %s (bug!)\n",
+ udev->devpath);
return -EINVAL;
}
- if (hcd->driver->free_config)
- hcd->driver->free_config (hcd, udev);
-
spin_lock_irqsave (&hcd_data_lock, flags);
list_del (&dev->dev_list);
udev->hcpriv = NULL;
@@ -1270,6 +1347,7 @@
.deallocate = hcd_free_dev,
.buffer_alloc = hcd_buffer_alloc,
.buffer_free = hcd_buffer_free,
+ .disable = hcd_endpoint_disable,
};
EXPORT_SYMBOL (usb_hcd_operations);
@@ -1360,21 +1438,17 @@
*/
void usb_hc_died (struct usb_hcd *hcd)
{
- struct list_head *devlist, *urblist;
struct hcd_dev *dev;
struct urb *urb;
unsigned long flags;
+ int i;
/* flag every pending urb as done */
spin_lock_irqsave (&hcd_data_lock, flags);
- list_for_each (devlist, &hcd->dev_list) {
- dev = list_entry (devlist, struct hcd_dev, dev_list);
- list_for_each (urblist, &dev->urb_list) {
- urb = list_entry (urblist, struct urb, urb_list);
- dev_dbg (hcd->controller, "shutdown %s urb %p pipe %x, current
status %d\n",
- hcd->self.bus_name, urb, urb->pipe, urb->status);
- if (urb->status == -EINPROGRESS)
- urb->status = -ESHUTDOWN;
+ list_for_each_entry (dev, &hcd->dev_list, dev_list) {
+ for (i = 0; i < 15; i++) {
+ hcd_endpoint_disable (dev->udev, i);
+ hcd_endpoint_disable (dev->udev, USB_DIR_IN | i);
}
}
urb = (struct urb *) hcd->rh_timer.data;
--- 1.47/drivers/usb/core/hcd.h Tue Mar 4 21:09:52 2003
+++ edited/drivers/usb/core/hcd.h Thu Apr 3 13:30:09 2003
@@ -121,6 +121,7 @@
struct hcd_dev { /* usb_device.hcpriv points to this */
struct list_head dev_list; /* on this hcd */
struct list_head urb_list; /* pending on this dev */
+ struct usb_device *udev;
/* per-configuration HC/HCD state, such as QH or ED */
void *ep[32];
@@ -153,6 +154,8 @@
dma_addr_t *dma);
void (*buffer_free)(struct usb_bus *bus, size_t size,
void *addr, dma_addr_t dma);
+
+ void (*disable)(struct usb_device *udev, int bEndpointAddress);
};
/* each driver provides one of these, and hardware init support */
@@ -194,10 +197,9 @@
int mem_flags);
int (*urb_dequeue) (struct usb_hcd *hcd, struct urb *urb);
- // frees configuration resources -- allocated as needed during
- // urb_enqueue, and not freed by urb_dequeue
- void (*free_config) (struct usb_hcd *hcd,
- struct usb_device *dev);
+ /* hw synch, freeing endpoint resources that urb_dequeue can't */
+ void (*endpoint_disable)(struct usb_hcd *hcd,
+ struct hcd_dev *dev, int bEndpointAddress);
/* root hub support */
int (*hub_status_data) (struct usb_hcd *hcd, char *buf);
--- 1.39/drivers/usb/core/message.c Sat Mar 8 09:31:29 2003
+++ edited/drivers/usb/core/message.c Thu Apr 3 13:30:09 2003
@@ -672,6 +672,9 @@
{
int i, b;
+ for (i = 1; i < 15; i++)
+ dev->epmaxpacketout [i] = dev->epmaxpacketin [i] = 0;
+
for (i=0; i<dev->actconfig->desc.bNumInterfaces; i++) {
struct usb_interface *ifp = dev->actconfig->interface + i;
struct usb_host_interface *as = ifp->altsetting + ifp->act_altsetting;
--- 1.26/drivers/usb/core/urb.c Mon Mar 17 16:32:26 2003
+++ edited/drivers/usb/core/urb.c Thu Apr 3 14:08:45 2003
@@ -381,16 +381,7 @@
*/
int usb_unlink_urb(struct urb *urb)
{
- /* FIXME
- * We should not care about the state here, but the host controllers
- * die a horrible death if we unlink a urb for a device that has been
- * physically removed.
- */
- if (urb &&
- urb->dev &&
- (urb->dev->state >= USB_STATE_DEFAULT) &&
- urb->dev->bus &&
- urb->dev->bus->op)
+ if (urb && urb->dev && urb->dev->bus && urb->dev->bus->op)
return urb->dev->bus->op->unlink_urb(urb);
else
return -ENODEV;
--- 1.196/drivers/usb/core/usb.c Wed Mar 26 14:15:32 2003
+++ edited/drivers/usb/core/usb.c Thu Apr 3 13:30:09 2003
@@ -802,8 +802,12 @@
*/
void usb_disconnect(struct usb_device **pdev)
{
- struct usb_device * dev = *pdev;
- int i;
+ struct usb_device *dev = *pdev;
+ struct usb_bus *bus = dev->bus;
+ struct usb_operations *ops = bus->op;
+ int i;
+
+ might_sleep ();
if (!dev)
return;
@@ -824,13 +828,23 @@
usb_disconnect(child);
}
+ /* disconnect() drivers from interfaces (a key side effect) */
dev_dbg (&dev->dev, "unregistering interfaces\n");
if (dev->actconfig) {
for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
- struct usb_interface *interface =
&dev->actconfig->interface[i];
+ struct usb_interface *interface;
/* remove this interface */
+ interface = &dev->actconfig->interface[i];
device_unregister(&interface->dev);
+ }
+ }
+
+ /* deallocate hcd/hardware state */
+ if (ops->disable) {
+ for (i = 0; i < 15; i++) {
+ ops->disable (dev, i);
+ ops->disable (dev, USB_DIR_IN | i);
}
}
--- 1.77/drivers/usb/host/ehci-hcd.c Mon Mar 24 16:03:55 2003
+++ edited/drivers/usb/host/ehci-hcd.c Thu Apr 3 14:11:25 2003
@@ -870,80 +870,55 @@
// bulk qh holds the data toggle
-static void ehci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
+static void
+ehci_endpoint_disable (struct usb_hcd *hcd, struct hcd_dev *dev, int ep)
{
- struct hcd_dev *dev = (struct hcd_dev *)udev->hcpriv;
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
- int i;
+ int epnum;
unsigned long flags;
+ struct ehci_qh *qh;
- /* ASSERT: no requests/urbs are still linked (so no TDs) */
+ /* ASSERT: any requests/urbs are being unlinked */
/* ASSERT: nobody can be submitting urbs for this any more */
- ehci_dbg (ehci, "free_config %s devnum %d\n",
- udev->devpath, udev->devnum);
+ epnum = ep & USB_ENDPOINT_NUMBER_MASK;
+ if (epnum != 0 && (ep & USB_DIR_IN))
+ epnum |= 0x10;
+rescan:
spin_lock_irqsave (&ehci->lock, flags);
- for (i = 0; i < 32; i++) {
- if (dev->ep [i]) {
- struct ehci_qh *qh;
- char *why;
-
- /* dev->ep never has ITDs or SITDs */
- qh = (struct ehci_qh *) dev->ep [i];
-
- /* detect/report non-recoverable errors */
- if (in_interrupt ())
- why = "disconnect() didn't";
- else if ((qh->hw_info2 & cpu_to_le32 (0xffff)) != 0
- && qh->qh_state != QH_STATE_IDLE)
- why = "(active periodic)";
- else
- why = 0;
- if (why) {
- err ("dev %s-%s ep %d-%s error: %s",
- hcd_to_bus (hcd)->bus_name,
- udev->devpath,
- i & 0xf, (i & 0x10) ? "IN" : "OUT",
- why);
- BUG ();
- }
-
- dev->ep [i] = 0;
- if (qh->qh_state == QH_STATE_IDLE)
- goto idle;
- ehci_dbg (ehci, "free_config, async ep 0x%02x qh %p",
- i, qh);
-
- /* scan_async() empties the ring as it does its work,
- * using IAA, but doesn't (yet?) turn it off. if it
- * doesn't empty this qh, likely it's the last entry.
- */
- while (qh->qh_state == QH_STATE_LINKED
- && ehci->reclaim
- && HCD_IS_RUNNING (ehci->hcd.state)
- ) {
- spin_unlock_irqrestore (&ehci->lock, flags);
- /* wait_ms() won't spin, we're a thread;
- * and we know IRQ/timer/... can progress
- */
- wait_ms (1);
- spin_lock_irqsave (&ehci->lock, flags);
- }
- if (qh->qh_state == QH_STATE_LINKED)
- start_unlink_async (ehci, qh);
- while (qh->qh_state != QH_STATE_IDLE
- && ehci->hcd.state != USB_STATE_HALT) {
- spin_unlock_irqrestore (&ehci->lock, flags);
- wait_ms (1);
- spin_lock_irqsave (&ehci->lock, flags);
- }
-idle:
+ qh = (struct ehci_qh *) dev->ep [epnum];
+ if (!qh)
+ goto done;
+
+ ehci_vdbg (ehci, "dev %s ep %02x disable\n", dev->udev->devpath, ep);
+ if (!HCD_IS_RUNNING (ehci->hcd.state))
+ qh->qh_state = QH_STATE_IDLE;
+ switch (qh->qh_state) {
+ case QH_STATE_UNLINK: /* wait for hw to finish */
+ spin_unlock_irqrestore (&ehci->lock, flags);
+ set_current_state (TASK_UNINTERRUPTIBLE);
+ schedule_timeout (1);
+ goto rescan;
+ case QH_STATE_IDLE: /* fully unlinked */
+ if (list_empty (&qh->qtd_list)) {
qh_put (ehci, qh);
+ break;
}
+ /* else FALL THROUGH */
+ default:
+ /* caller was supposed to have unlinked any requests;
+ * that's not our job. just leak this qh and dummy.
+ */
+ ehci_err (ehci, "dev %s qh %p (#%d) state %d%s\n",
+ dev->udev->devpath, qh, epnum, qh->qh_state,
+ list_empty (&qh->qtd_list) ? "" : " (has qtds)");
+ break;
}
-
+ dev->ep [epnum] = 0;
+done:
spin_unlock_irqrestore (&ehci->lock, flags);
+ return;
}
/*-------------------------------------------------------------------------*/
@@ -978,7 +953,7 @@
*/
.urb_enqueue = ehci_urb_enqueue,
.urb_dequeue = ehci_urb_dequeue,
- .free_config = ehci_free_config,
+ .endpoint_disable = ehci_endpoint_disable,
/*
* scheduling support
--- 1.64/drivers/usb/host/ohci-hcd.c Thu Mar 27 23:45:41 2003
+++ edited/drivers/usb/host/ohci-hcd.c Thu Apr 3 14:12:49 2003
@@ -313,65 +313,55 @@
*/
static void
-ohci_free_config (struct usb_hcd *hcd, struct usb_device *udev)
+ohci_endpoint_disable (struct usb_hcd *hcd, struct hcd_dev *dev, int ep)
{
struct ohci_hcd *ohci = hcd_to_ohci (hcd);
- struct hcd_dev *dev = (struct hcd_dev *) udev->hcpriv;
- int i;
+ int epnum = ep & USB_ENDPOINT_NUMBER_MASK;
unsigned long flags;
+ struct ed *ed;
-rescan:
- /* free any eds, and dummy tds, still hanging around */
- spin_lock_irqsave (&ohci->lock, flags);
- for (i = 0; i < 32; i++) {
- struct ed *ed = dev->ep [i];
+ /* ASSERT: any requests/urbs are being unlinked */
+ /* ASSERT: nobody can be submitting urbs for this any more */
- if (!ed)
- continue;
+ epnum <<= 1;
+ if (epnum != 0 && !(ep & USB_DIR_IN))
+ epnum |= 1;
- if (ohci->disabled && ed->state != ED_IDLE)
- ed->state = ED_IDLE;
- switch (ed->state) {
- case ED_UNLINK: /* wait a frame? */
- goto do_rescan;
- case ED_IDLE: /* fully unlinked */
- td_free (ohci, ed->dummy);
+rescan:
+ spin_lock_irqsave (&ohci->lock, flags);
+ ed = dev->ep [epnum];
+ if (!ed)
+ goto done;
+
+ ohci_vdbg (ohci, "dev %s ep %02x disable\n", dev->udev->devpath, ep);
+ if (!HCD_IS_RUNNING (ohci->hcd.state) || ohci->disabled)
+ ed->state = ED_IDLE;
+ switch (ed->state) {
+ case ED_UNLINK: /* wait for hw to finish */
+ spin_unlock_irqrestore (&ohci->lock, flags);
+ set_current_state (TASK_UNINTERRUPTIBLE);
+ schedule_timeout (1);
+ goto rescan;
+ case ED_IDLE: /* fully unlinked */
+ if (list_empty (&ed->td_list)) {
+ ed_free (ohci, ed);
break;
- default:
- ohci_err (ohci,
- "dev %s ep%d-%s linked; disconnect() bug?\n",
- udev->devpath,
- (i >> 1) & 0x0f, (i & 1) ? "out" : "in");
-
- /* ED_OPER: some driver disconnect() is broken,
- * it didn't even start its unlinks much less wait
- * for their completions.
- * OTHERWISE: hcd bug, ed is garbage
- *
- * ... we can't recycle this memory in either case,
- * so just leak it to avoid oopsing.
- */
- continue;
}
- ed_free (ohci, ed);
+ /* else FALL THROUGH */
+ default:
+ /* caller was supposed to have unlinked any requests;
+ * that's not our job. just leak this ed.
+ */
+ ohci_err (ohci, "dev %s ed %p (#%d) state %d%s\n",
+ dev->udev->devpath, ed, epnum, ed->state,
+ list_empty (&ed->td_list) ? "" : " (has tds)");
+ break;
}
+ td_free (ohci, ed->dummy);
+ dev->ep [epnum] = 0;
+done:
spin_unlock_irqrestore (&ohci->lock, flags);
return;
-
-do_rescan:
-#ifdef DEBUG
- /* a driver->disconnect() returned before its unlinks completed? */
- if (in_interrupt ()) {
- ohci_warn (ohci,
- "driver disconnect() bug %s ep%d-%s\n",
- udev->devpath,
- (i >> 1) & 0x0f, (i & 1) ? "out" : "in");
- }
-#endif
-
- spin_unlock_irqrestore (&ohci->lock, flags);
- wait_ms (1);
- goto rescan;
}
static int ohci_get_frame (struct usb_hcd *hcd)
--- 1.16/drivers/usb/host/ohci-pci.c Mon Mar 17 16:32:26 2003
+++ edited/drivers/usb/host/ohci-pci.c Thu Apr 3 13:30:09 2003
@@ -334,7 +334,7 @@
*/
.urb_enqueue = ohci_urb_enqueue,
.urb_dequeue = ohci_urb_dequeue,
- .free_config = ohci_free_config,
+ .endpoint_disable = ohci_endpoint_disable,
/*
* scheduling support
--- 1.22/drivers/usb/host/ohci-sa1111.c Tue Mar 4 21:09:54 2003
+++ edited/drivers/usb/host/ohci-sa1111.c Thu Apr 3 13:30:09 2003
@@ -333,7 +333,7 @@
*/
.urb_enqueue = ohci_urb_enqueue,
.urb_dequeue = ohci_urb_dequeue,
- .free_config = ohci_free_config,
+ .endpoint_disable = ohci_endpoint_disable,
/*
* scheduling support
--- 1.44/drivers/usb/host/uhci-hcd.c Mon Mar 17 16:32:26 2003
+++ edited/drivers/usb/host/uhci-hcd.c Thu Apr 3 13:30:09 2003
@@ -2390,7 +2390,6 @@
.urb_enqueue = uhci_urb_enqueue,
.urb_dequeue = uhci_urb_dequeue,
- .free_config = NULL,
.get_frame_number = uhci_hcd_get_frame_number,
