From a40dc60f62a62caf12c402fff2a0756a7550a72e Mon Sep 17 00:00:00 2001 From: hselasky Date: Wed, 13 Feb 2013 12:35:17 +0000 Subject: Resolve a LOR after r246616. Protect control requests using the USB device enumeration lock. Make sure all callers of usbd_enum_lock() check the return value. Remove the control transfer specific lock. Bump the FreeBSD version number, hence external USB modules may need to be recompiled due to a USB device structure change. MFC after: 1 week --- sys/dev/usb/controller/usb_controller.c | 18 ++++++++++++------ sys/dev/usb/usb_dev.c | 11 ++++++----- sys/dev/usb/usb_dev.h | 1 + sys/dev/usb/usb_device.c | 4 ---- sys/dev/usb/usb_device.h | 1 - sys/dev/usb/usb_handle_request.c | 20 ++++++++++++++------ sys/dev/usb/usb_hub.c | 13 +++++++++---- sys/dev/usb/usb_request.c | 28 ++++++++++++---------------- 8 files changed, 54 insertions(+), 42 deletions(-) (limited to 'sys/dev/usb') diff --git a/sys/dev/usb/controller/usb_controller.c b/sys/dev/usb/controller/usb_controller.c index cf2ae7a..a28d612 100644 --- a/sys/dev/usb/controller/usb_controller.c +++ b/sys/dev/usb/controller/usb_controller.c @@ -427,6 +427,7 @@ usb_bus_suspend(struct usb_proc_msg *pm) struct usb_bus *bus; struct usb_device *udev; usb_error_t err; + uint8_t do_unlock; bus = ((struct usb_bus_msg *)pm)->bus; udev = bus->devices[USB_ROOT_HUB_ADDR]; @@ -447,7 +448,7 @@ usb_bus_suspend(struct usb_proc_msg *pm) bus_generic_shutdown(bus->bdev); - usbd_enum_lock(udev); + do_unlock = usbd_enum_lock(udev); err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX); if (err) @@ -464,7 +465,8 @@ usb_bus_suspend(struct usb_proc_msg *pm) if (bus->methods->set_hw_power_sleep != NULL) (bus->methods->set_hw_power_sleep) (bus, USB_HW_POWER_SUSPEND); - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_BUS_LOCK(bus); } @@ -480,6 +482,7 @@ usb_bus_resume(struct usb_proc_msg *pm) struct usb_bus *bus; struct usb_device *udev; usb_error_t err; + uint8_t do_unlock; bus = ((struct usb_bus_msg *)pm)->bus; udev = bus->devices[USB_ROOT_HUB_ADDR]; @@ -489,7 +492,7 @@ usb_bus_resume(struct usb_proc_msg *pm) USB_BUS_UNLOCK(bus); - usbd_enum_lock(udev); + do_unlock = usbd_enum_lock(udev); #if 0 DEVMETHOD(usb_take_controller, NULL); /* dummy */ #endif @@ -523,7 +526,8 @@ usb_bus_resume(struct usb_proc_msg *pm) "attach root HUB\n"); } - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_BUS_LOCK(bus); } @@ -539,6 +543,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm) struct usb_bus *bus; struct usb_device *udev; usb_error_t err; + uint8_t do_unlock; bus = ((struct usb_bus_msg *)pm)->bus; udev = bus->devices[USB_ROOT_HUB_ADDR]; @@ -550,7 +555,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm) bus_generic_shutdown(bus->bdev); - usbd_enum_lock(udev); + do_unlock = usbd_enum_lock(udev); err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX); if (err) @@ -567,7 +572,8 @@ usb_bus_shutdown(struct usb_proc_msg *pm) if (bus->methods->set_hw_power_sleep != NULL) (bus->methods->set_hw_power_sleep) (bus, USB_HW_POWER_SHUTDOWN); - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_BUS_LOCK(bus); } diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c index c4264c5..9bf430b 100644 --- a/sys/dev/usb/usb_dev.c +++ b/sys/dev/usb/usb_dev.c @@ -218,10 +218,10 @@ usb_ref_device(struct usb_cdev_privdata *cpd, mtx_unlock(&usb_ref_lock); /* - * We need to grab the sx-lock before grabbing the - * FIFO refs to avoid deadlock at detach! + * We need to grab the enumeration SX-lock before + * grabbing the FIFO refs to avoid deadlock at detach! */ - usbd_enum_lock(cpd->udev); + crd->do_unlock = usbd_enum_lock(cpd->udev); mtx_lock(&usb_ref_lock); @@ -282,9 +282,10 @@ usb_ref_device(struct usb_cdev_privdata *cpd, return (0); error: - if (crd->is_uref) { + if (crd->do_unlock) usbd_enum_unlock(cpd->udev); + if (crd->is_uref) { if (--(cpd->udev->refcount) == 0) { cv_signal(&cpd->udev->ref_cv); } @@ -336,7 +337,7 @@ usb_unref_device(struct usb_cdev_privdata *cpd, DPRINTFN(2, "cpd=%p is_uref=%d\n", cpd, crd->is_uref); - if (crd->is_uref) + if (crd->do_unlock) usbd_enum_unlock(cpd->udev); mtx_lock(&usb_ref_lock); diff --git a/sys/dev/usb/usb_dev.h b/sys/dev/usb/usb_dev.h index 3c75701..f87ba53 100644 --- a/sys/dev/usb/usb_dev.h +++ b/sys/dev/usb/usb_dev.h @@ -84,6 +84,7 @@ struct usb_cdev_refdata { uint8_t is_write; /* location has write access */ uint8_t is_uref; /* USB refcount decr. needed */ uint8_t is_usbfs; /* USB-FS is active */ + uint8_t do_unlock; /* USB enum unlock needed */ }; struct usb_fs_privdata { diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c index fdc734c..1c40815 100644 --- a/sys/dev/usb/usb_device.c +++ b/sys/dev/usb/usb_device.c @@ -1546,9 +1546,6 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus, return (NULL); } /* initialise our SX-lock */ - sx_init_flags(&udev->ctrl_sx, "USB device SX lock", SX_DUPOK); - - /* initialise our SX-lock */ sx_init_flags(&udev->enum_sx, "USB config SX lock", SX_DUPOK); sx_init_flags(&udev->sr_sx, "USB suspend and resume SX lock", SX_NOWITNESS); @@ -2119,7 +2116,6 @@ usb_free_device(struct usb_device *udev, uint8_t flag) &udev->cs_msg[0], &udev->cs_msg[1]); USB_BUS_UNLOCK(udev->bus); - sx_destroy(&udev->ctrl_sx); sx_destroy(&udev->enum_sx); sx_destroy(&udev->sr_sx); diff --git a/sys/dev/usb/usb_device.h b/sys/dev/usb/usb_device.h index 22a4652..b194256 100644 --- a/sys/dev/usb/usb_device.h +++ b/sys/dev/usb/usb_device.h @@ -181,7 +181,6 @@ union usb_device_scratch { struct usb_device { struct usb_clear_stall_msg cs_msg[2]; /* generic clear stall * messages */ - struct sx ctrl_sx; struct sx enum_sx; struct sx sr_sx; struct mtx device_mtx; diff --git a/sys/dev/usb/usb_handle_request.c b/sys/dev/usb/usb_handle_request.c index a775d67..7f3cdc6 100644 --- a/sys/dev/usb/usb_handle_request.c +++ b/sys/dev/usb/usb_handle_request.c @@ -149,6 +149,7 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no) { struct usb_device *udev = xfer->xroot->udev; usb_error_t err = 0; + uint8_t do_unlock; /* * We need to protect against other threads doing probe and @@ -156,7 +157,8 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no) */ USB_XFER_UNLOCK(xfer); - usbd_enum_lock(udev); + /* Prevent re-enumeration */ + do_unlock = usbd_enum_lock(udev); if (conf_no == USB_UNCONFIG_NO) { conf_no = USB_UNCONFIG_INDEX; @@ -179,7 +181,8 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no) goto done; } done: - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_XFER_LOCK(xfer); return (err); } @@ -221,6 +224,7 @@ usb_handle_iface_request(struct usb_xfer *xfer, int error; uint8_t iface_index; uint8_t temp_state; + uint8_t do_unlock; if ((req.bmRequestType & 0x1F) == UT_INTERFACE) { iface_index = req.wIndex[0]; /* unicast */ @@ -234,7 +238,8 @@ usb_handle_iface_request(struct usb_xfer *xfer, */ USB_XFER_UNLOCK(xfer); - usbd_enum_lock(udev); + /* Prevent re-enumeration */ + do_unlock = usbd_enum_lock(udev); error = ENXIO; @@ -350,17 +355,20 @@ tr_repeat: goto tr_stalled; } tr_valid: - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_XFER_LOCK(xfer); return (0); tr_short: - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_XFER_LOCK(xfer); return (USB_ERR_SHORT_XFER); tr_stalled: - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); USB_XFER_LOCK(xfer); return (USB_ERR_STALLED); } diff --git a/sys/dev/usb/usb_hub.c b/sys/dev/usb/usb_hub.c index 1d87a3f..a359ee2 100644 --- a/sys/dev/usb/usb_hub.c +++ b/sys/dev/usb/usb_hub.c @@ -243,7 +243,9 @@ uhub_explore_sub(struct uhub_softc *sc, struct usb_port *up) /* check if device should be re-enumerated */ if (child->flags.usb_mode == USB_MODE_HOST) { - usbd_enum_lock(child); + uint8_t do_unlock; + + do_unlock = usbd_enum_lock(child); if (child->re_enumerate_wait) { err = usbd_set_config_index(child, USB_UNCONFIG_INDEX); @@ -262,7 +264,8 @@ uhub_explore_sub(struct uhub_softc *sc, struct usb_port *up) child->re_enumerate_wait = 0; err = 0; } - usbd_enum_unlock(child); + if (do_unlock) + usbd_enum_unlock(child); } /* check if probe and attach should be done */ @@ -716,6 +719,7 @@ uhub_explore(struct usb_device *udev) usb_error_t err; uint8_t portno; uint8_t x; + uint8_t do_unlock; hub = udev->hub; sc = hub->hubsoftc; @@ -737,7 +741,7 @@ uhub_explore(struct usb_device *udev) * Make sure we don't race against user-space applications * like LibUSB: */ - usbd_enum_lock(udev); + do_unlock = usbd_enum_lock(udev); for (x = 0; x != hub->nports; x++) { up = hub->ports + x; @@ -817,7 +821,8 @@ uhub_explore(struct usb_device *udev) up->restartcnt = 0; } - usbd_enum_unlock(udev); + if (do_unlock) + usbd_enum_unlock(udev); /* initial status checked */ sc->sc_flags |= UHUB_FLAG_DID_EXPLORE; diff --git a/sys/dev/usb/usb_request.c b/sys/dev/usb/usb_request.c index 4db5b7d..1a366d6 100644 --- a/sys/dev/usb/usb_request.c +++ b/sys/dev/usb/usb_request.c @@ -389,9 +389,8 @@ usbd_get_hr_func(struct usb_device *udev) * than 30 seconds is treated like a 30 second timeout. This USB stack * does not allow control requests without a timeout. * - * NOTE: This function is thread safe. All calls to - * "usbd_do_request_flags" will be serialised by the use of an - * internal "sx_lock". + * NOTE: This function is thread safe. All calls to "usbd_do_request_flags" + * will be serialized by the use of the USB device enumeration lock. * * Returns: * 0: Success @@ -415,7 +414,7 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx, uint16_t length; uint16_t temp; uint16_t acttemp; - uint8_t enum_locked; + uint8_t do_unlock; if (timeout < 50) { /* timeout is too small */ @@ -427,8 +426,6 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx, } length = UGETW(req->wLength); - enum_locked = usbd_enum_is_locked(udev); - DPRINTFN(5, "udev=%p bmRequestType=0x%02x bRequest=0x%02x " "wValue=0x%02x%02x wIndex=0x%02x%02x wLength=0x%02x%02x\n", udev, req->bmRequestType, req->bRequest, @@ -459,17 +456,16 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx, } /* - * We need to allow suspend and resume at this point, else the - * control transfer will timeout if the device is suspended! + * Grab the USB device enumeration SX-lock serialization is + * achieved when multiple threads are involved: */ - if (enum_locked) - usbd_sr_unlock(udev); + do_unlock = usbd_enum_lock(udev); /* - * Grab the default sx-lock so that serialisation - * is achieved when multiple threads are involved: + * We need to allow suspend and resume at this point, else the + * control transfer will timeout if the device is suspended! */ - sx_xlock(&udev->ctrl_sx); + usbd_sr_unlock(udev); hr_func = usbd_get_hr_func(udev); @@ -713,10 +709,10 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx, USB_XFER_UNLOCK(xfer); done: - sx_xunlock(&udev->ctrl_sx); + usbd_sr_lock(udev); - if (enum_locked) - usbd_sr_lock(udev); + if (do_unlock) + usbd_enum_unlock(udev); if ((mtx != NULL) && (mtx != &Giant)) mtx_lock(mtx); -- cgit v1.1