MFp4 @ 168387

- clean up USB detach logic. There seems to be some problems detaching multiple
  USB HUBs connected in series from the root.

- after this patch the rule is:
  1) Always use device_detach() on the USB HUB first.
  2) Never just device_delete_child() on the USB HUB, because that function
  will traverse to all the device leaves and free them first, and then the USB
  stack will free the devices twice which doesn't work very well.

- make sure the did DMA delay gets set after the timeout has elapsed to make
  logic more clear. There is no functional difference.

Submitted by:	Hans Petter Selasky

sys/dev/usb/controller/usb_controller.c

@@ -270,11 +270,9 @@ usb_bus_detach(struct usb_proc_msg *pm)
 	mtx_unlock(&Giant);
 
 	/*
-	 * Free USB Root device, but not any sub-devices, hence they
-	 * are freed by the caller of this function:
+	 * Free USB device and all subdevices, if any.
 	 */
-	usb_free_device(udev,
-	    USB_UNCFG_FLAG_FREE_EP0);
+	usb_free_device(udev, 0);
 
 	USB_BUS_LOCK(bus);
 	/* clear bdev variable last */

sys/dev/usb/usb_device.c

@@ -478,7 +478,7 @@ usbd_set_config_index(struct usb_device *udev, uint8_t index)
 		usbd_enum_lock(udev);
 	}
 
-	usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_SUBDEV);
+	usb_unconfigure(udev, 0);
 
 	if (index == USB_UNCONFIG_INDEX) {
 		/*
@@ -582,7 +582,7 @@ usbd_set_config_index(struct usb_device *udev, uint8_t index)
 done:
 	DPRINTF("error=%s\n", usbd_errstr(err));
 	if (err) {
-		usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_SUBDEV);
+		usb_unconfigure(udev, 0);
 	}
 	if (do_unlock)
 		usbd_enum_unlock(udev);
@@ -989,18 +989,13 @@ usb_detach_device_sub(struct usb_device *udev, device_t *ppdev,
 	device_t dev;
 	int err;
 
-	if (!(flag & USB_UNCFG_FLAG_FREE_SUBDEV)) {
-
-		*ppdev = NULL;
-
-	} else if (*ppdev) {
-
+	dev = *ppdev;
+	if (dev) {
 		/*
 		 * NOTE: It is important to clear "*ppdev" before deleting
 		 * the child due to some device methods being called late
 		 * during the delete process !
 		 */
-		dev = *ppdev;
 		*ppdev = NULL;
 
 		device_printf(dev, "at %s, port %d, addr %d "
@@ -1778,7 +1773,7 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 		} else if (usb_test_huawei_autoinst_p(udev, &uaa) == 0) {
 			DPRINTFN(0, "Found Huawei auto-install disk!\n");
 			/* leave device unconfigured */
-			usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_SUBDEV);
+			usb_unconfigure(udev, 0);
 		}
 	} else {
 		err = 0;		/* set success */
@@ -1803,10 +1798,10 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 #endif
 done:
 	if (err) {
-		/* free device  */
-		usb_free_device(udev,
-		    USB_UNCFG_FLAG_FREE_SUBDEV |
-		    USB_UNCFG_FLAG_FREE_EP0);
+		/*
+		 * Free USB device and all subdevices, if any.
+		 */
+		usb_free_device(udev, 0);
 		udev = NULL;
 	}
 	return (udev);
@@ -1926,9 +1921,10 @@ usb_cdev_cleanup(void* arg)
 /*------------------------------------------------------------------------*
  *	usb_free_device
  *
- * This function is NULL safe and will free an USB device.
+ * This function is NULL safe and will free an USB device and its
+ * children devices, if any.
  *
- * Flag values, see "USB_UNCFG_FLAG_XXX".
+ * Flag values: Reserved, set to zero.
  *------------------------------------------------------------------------*/
 void
 usb_free_device(struct usb_device *udev, uint8_t flag)
@@ -1982,7 +1978,7 @@ usb_free_device(struct usb_device *udev, uint8_t flag)
 	}
 
 	/* the following will get the device unconfigured in software */
-	usb_unconfigure(udev, flag);
+	usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_EP0);
 
 	/* unsetup any leftover default USB transfers */
 	usbd_transfer_unsetup(udev->default_xfer, USB_DEFAULT_XFER_MAX);

sys/dev/usb/usb_device.h

@@ -41,7 +41,6 @@ struct usb_device;		/* linux compat */
 /* "usb_unconfigure()" flags */
 
 #define	USB_UNCFG_FLAG_NONE 0x00
-#define	USB_UNCFG_FLAG_FREE_SUBDEV 0x01		/* subdevices are freed */
 #define	USB_UNCFG_FLAG_FREE_EP0	0x02		/* endpoint zero is freed */
 
 struct usb_clear_stall_msg {

sys/dev/usb/usb_hub.c

@@ -316,12 +316,13 @@ uhub_reattach_port(struct uhub_softc *sc, uint8_t portno)
 	if (err) {
 		goto error;
 	}
-	/* detach any existing devices */
+	/* check if there is a child */
 
-	if (child) {
-		usb_free_device(child,
-		    USB_UNCFG_FLAG_FREE_SUBDEV |
-		    USB_UNCFG_FLAG_FREE_EP0);
+	if (child != NULL) {
+		/*
+		 * Free USB device and all subdevices, if any.
+		 */
+		usb_free_device(child, 0);
 		child = NULL;
 	}
 	/* get fresh status */
@@ -438,10 +439,11 @@ uhub_reattach_port(struct uhub_softc *sc, uint8_t portno)
 	return (0);			/* success */
 
 error:
-	if (child) {
-		usb_free_device(child,
-		    USB_UNCFG_FLAG_FREE_SUBDEV |
-		    USB_UNCFG_FLAG_FREE_EP0);
+	if (child != NULL) {
+		/*
+		 * Free USB device and all subdevices, if any.
+		 */
+		usb_free_device(child, 0);
 		child = NULL;
 	}
 	if (err == 0) {
@@ -888,12 +890,14 @@ uhub_detach(device_t dev)
 	struct usb_device *child;
 	uint8_t x;
 
-	/* detach all children first */
-	bus_generic_detach(dev);
-
 	if (hub == NULL) {		/* must be partially working */
 		return (0);
 	}
+
+	/* Make sure interrupt transfer is gone. */
+	usbd_transfer_unsetup(sc->sc_xfer, UHUB_N_TRANSFER);
+
+	/* Detach all ports */
 	for (x = 0; x != hub->nports; x++) {
 
 		child = usb_bus_port_get_device(sc->sc_udev->bus, hub->ports + x);
@@ -901,15 +905,12 @@ uhub_detach(device_t dev)
 		if (child == NULL) {
 			continue;
 		}
-		/*
-		 * Subdevices are not freed, because the caller of
-		 * uhub_detach() will do that.
-		 */
-		usb_free_device(child,
-		    USB_UNCFG_FLAG_FREE_EP0);
-	}
 
-	usbd_transfer_unsetup(sc->sc_xfer, UHUB_N_TRANSFER);
+		/*
+		 * Free USB device and all subdevices, if any.
+		 */
+		usb_free_device(child, 0);
+	}
 
 	free(hub, M_USBDEV);
 	sc->sc_udev->hub = NULL;
@@ -984,10 +985,19 @@ static int
 uhub_child_location_string(device_t parent, device_t child,
     char *buf, size_t buflen)
 {
-	struct uhub_softc *sc = device_get_softc(parent);
-	struct usb_hub *hub = sc->sc_udev->hub;
+	struct uhub_softc *sc;
+	struct usb_hub *hub;
 	struct hub_result res;
 
+	if (!device_is_attached(parent)) {
+		if (buflen)
+			buf[0] = 0;
+		return (0);
+	}
+
+	sc = device_get_softc(parent);
+	hub = sc->sc_udev->hub;
+
 	mtx_lock(&Giant);
 	uhub_find_iface_index(hub, child, &res);
 	if (!res.udev) {
@@ -1009,11 +1019,20 @@ static int
 uhub_child_pnpinfo_string(device_t parent, device_t child,
     char *buf, size_t buflen)
 {
-	struct uhub_softc *sc = device_get_softc(parent);
-	struct usb_hub *hub = sc->sc_udev->hub;
+	struct uhub_softc *sc;
+	struct usb_hub *hub;
 	struct usb_interface *iface;
 	struct hub_result res;
 
+	if (!device_is_attached(parent)) {
+		if (buflen)
+			buf[0] = 0;
+		return (0);
+	}
+
+	sc = device_get_softc(parent);
+	hub = sc->sc_udev->hub;
+
 	mtx_lock(&Giant);
 	uhub_find_iface_index(hub, child, &res);
 	if (!res.udev) {

sys/dev/usb/usb_transfer.c

@@ -2121,6 +2121,9 @@ usb_dma_delay_done_cb(void *arg)
 
 	DPRINTFN(3, "Completed %p\n", xfer);
 
+	/* only delay once */
+	xfer->flags_int.did_dma_delay = 1;
+
 	/* queue callback for execution, again */
 	usbd_transfer_done(xfer, 0);
 }
@@ -2488,9 +2491,6 @@ usbd_callback_wrapper_sub(struct usb_xfer *xfer)
 
 		usb_timeout_t temp;
 
-		/* only delay once */
-		xfer->flags_int.did_dma_delay = 1;
-
 		/* we can not cancel this delay */
 		xfer->flags_int.can_cancel_immed = 0;