Date: Wed, 9 Sep 2009 23:19:38 GMT From: Hans Petter Selasky <hselasky@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 168387 for review Message-ID: <200909092319.n89NJcST025645@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=168387 Change 168387 by hselasky@hselasky_laptop001 on 2009/09/09 23:19:08 USB CORE: - 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. Affected files ... .. //depot/projects/usb/src/sys/dev/usb/controller/usb_controller.c#32 edit .. //depot/projects/usb/src/sys/dev/usb/usb_device.c#55 edit .. //depot/projects/usb/src/sys/dev/usb/usb_device.h#31 edit .. //depot/projects/usb/src/sys/dev/usb/usb_hub.c#33 edit .. //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#166 edit Differences ... ==== //depot/projects/usb/src/sys/dev/usb/controller/usb_controller.c#32 (text+ko) ==== @@ -270,11 +270,9 @@ 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 */ ==== //depot/projects/usb/src/sys/dev/usb/usb_device.c#55 (text+ko) ==== @@ -494,7 +494,7 @@ usbd_enum_lock(udev); } - usb_unconfigure(udev, USB_UNCFG_FLAG_FREE_SUBDEV); + usb_unconfigure(udev, 0); if (index == USB_UNCONFIG_INDEX) { /* @@ -598,7 +598,7 @@ 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); @@ -1005,18 +1005,13 @@ 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 " @@ -1821,7 +1816,7 @@ } 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 */ @@ -1846,10 +1841,10 @@ #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); @@ -1969,9 +1964,10 @@ /*------------------------------------------------------------------------* * 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) @@ -2025,7 +2021,7 @@ } /* 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); ==== //depot/projects/usb/src/sys/dev/usb/usb_device.h#31 (text+ko) ==== @@ -41,7 +41,6 @@ /* "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 { ==== //depot/projects/usb/src/sys/dev/usb/usb_hub.c#33 (text+ko) ==== @@ -316,12 +316,13 @@ 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 @@ 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 @@ 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,16 +905,13 @@ if (child == NULL) { continue; } + /* - * Subdevices are not freed, because the caller of - * uhub_detach() will do that. + * Free USB device and all subdevices, if any. */ - usb_free_device(child, - USB_UNCFG_FLAG_FREE_EP0); + usb_free_device(child, 0); } - usbd_transfer_unsetup(sc->sc_xfer, UHUB_N_TRANSFER); - free(hub, M_USBDEV); sc->sc_udev->hub = NULL; @@ -984,10 +985,19 @@ 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 @@ 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) { ==== //depot/projects/usb/src/sys/dev/usb/usb_transfer.c#166 (text+ko) ==== @@ -2123,6 +2123,9 @@ 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); } @@ -2492,9 +2495,6 @@ 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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200909092319.n89NJcST025645>