svn commit: r277136 - in head/sys/dev/usb: . controller
Hans Petter Selasky
hselasky at FreeBSD.org
Tue Jan 13 16:37:45 UTC 2015
Author: hselasky
Date: Tue Jan 13 16:37:43 2015
New Revision: 277136
URL: https://svnweb.freebsd.org/changeset/base/277136
Log:
Resolve a special case deadlock: When two or more threads are
simultaneously detaching kernel drivers on the same USB device we can
get stuck in the "usb_wait_pending_ref_locked()" function because the
conditions needed for allowing detach are not met. The "destroy_dev()"
function waits for all system calls involving the given character
device to return. Character device system calls may lock the USB
enumeration lock, which is also held when "destroy_dev()" is
called. This can sometimes lead to a deadlock not noticed by
WITNESS. The current solution is to ensure the calling thread is the
only one holding the USB enumeration lock and prevent other threads
from getting refs while a USB device detach is ongoing. This turned
out not to be sufficient. To solve this deadlock we could use
"destroy_dev_sched()" to schedule the device destruction in the
background, but then we don't know when it is safe to free() the
private data of the character device. Instead a callback function is
executed by the USB explore process to kill off any leftover USB
character devices synchronously after the USB device explore code is
finished and the USB enumeration lock is no longer locked. This makes
porting easier and also ensures us that character devices must
eventually go away after a USB device detach.
While at it ensure that "flag_iserror" is only written when "priv_mtx"
is locked, which is protecting it.
MFC after: 5 days
Modified:
head/sys/dev/usb/controller/usb_controller.c
head/sys/dev/usb/usb_bus.h
head/sys/dev/usb/usb_dev.c
head/sys/dev/usb/usb_device.c
head/sys/dev/usb/usb_device.h
Modified: head/sys/dev/usb/controller/usb_controller.c
==============================================================================
--- head/sys/dev/usb/controller/usb_controller.c Tue Jan 13 16:18:31 2015 (r277135)
+++ head/sys/dev/usb/controller/usb_controller.c Tue Jan 13 16:37:43 2015 (r277136)
@@ -59,6 +59,7 @@
#include <dev/usb/usb_busdma.h>
#include <dev/usb/usb_dynamic.h>
#include <dev/usb/usb_device.h>
+#include <dev/usb/usb_dev.h>
#include <dev/usb/usb_hub.h>
#include <dev/usb/usb_controller.h>
@@ -219,6 +220,11 @@ usb_detach(device_t dev)
usb_proc_mwait(USB_BUS_EXPLORE_PROC(bus),
&bus->detach_msg[0], &bus->detach_msg[1]);
+#if USB_HAVE_UGEN
+ /* Wait for cleanup to complete */
+ usb_proc_mwait(USB_BUS_EXPLORE_PROC(bus),
+ &bus->cleanup_msg[0], &bus->cleanup_msg[1]);
+#endif
USB_BUS_UNLOCK(bus);
#if USB_HAVE_PER_BUS_PROCESS
@@ -631,6 +637,32 @@ usb_bus_shutdown(struct usb_proc_msg *pm
USB_BUS_LOCK(bus);
}
+/*------------------------------------------------------------------------*
+ * usb_bus_cleanup
+ *
+ * This function is used to cleanup leftover USB character devices.
+ *------------------------------------------------------------------------*/
+#if USB_HAVE_UGEN
+static void
+usb_bus_cleanup(struct usb_proc_msg *pm)
+{
+ struct usb_bus *bus;
+ struct usb_fs_privdata *pd;
+
+ bus = ((struct usb_bus_msg *)pm)->bus;
+
+ while ((pd = LIST_FIRST(&bus->pd_cleanup_list)) != NULL) {
+
+ LIST_REMOVE(pd, pd_next);
+ USB_BUS_UNLOCK(bus);
+
+ usb_destroy_dev_sync(pd);
+
+ USB_BUS_LOCK(bus);
+ }
+}
+#endif
+
static void
usb_power_wdog(void *arg)
{
@@ -813,6 +845,14 @@ usb_attach_sub(device_t dev, struct usb_
bus->shutdown_msg[1].hdr.pm_callback = &usb_bus_shutdown;
bus->shutdown_msg[1].bus = bus;
+#if USB_HAVE_UGEN
+ LIST_INIT(&bus->pd_cleanup_list);
+ bus->cleanup_msg[0].hdr.pm_callback = &usb_bus_cleanup;
+ bus->cleanup_msg[0].bus = bus;
+ bus->cleanup_msg[1].hdr.pm_callback = &usb_bus_cleanup;
+ bus->cleanup_msg[1].bus = bus;
+#endif
+
#if USB_HAVE_PER_BUS_PROCESS
/* Create USB explore and callback processes */
Modified: head/sys/dev/usb/usb_bus.h
==============================================================================
--- head/sys/dev/usb/usb_bus.h Tue Jan 13 16:18:31 2015 (r277135)
+++ head/sys/dev/usb/usb_bus.h Tue Jan 13 16:37:43 2015 (r277136)
@@ -27,6 +27,8 @@
#ifndef _USB_BUS_H_
#define _USB_BUS_H_
+struct usb_fs_privdata;
+
/*
* The following structure defines the USB explore message sent to the USB
* explore process.
@@ -83,6 +85,10 @@ struct usb_bus {
struct usb_bus_msg resume_msg[2];
struct usb_bus_msg reset_msg[2];
struct usb_bus_msg shutdown_msg[2];
+#if USB_HAVE_UGEN
+ struct usb_bus_msg cleanup_msg[2];
+ LIST_HEAD(,usb_fs_privdata) pd_cleanup_list;
+#endif
/*
* This mutex protects the USB hardware:
*/
Modified: head/sys/dev/usb/usb_dev.c
==============================================================================
--- head/sys/dev/usb/usb_dev.c Tue Jan 13 16:18:31 2015 (r277135)
+++ head/sys/dev/usb/usb_dev.c Tue Jan 13 16:37:43 2015 (r277136)
@@ -293,8 +293,8 @@ error:
usbd_enum_unlock(cpd->udev);
if (crd->is_uref) {
- cpd->udev->refcount--;
- cv_broadcast(&cpd->udev->ref_cv);
+ if (--(cpd->udev->refcount) == 0)
+ cv_broadcast(&cpd->udev->ref_cv);
}
mtx_unlock(&usb_ref_lock);
DPRINTFN(2, "fail\n");
@@ -365,8 +365,8 @@ usb_unref_device(struct usb_cdev_privdat
}
if (crd->is_uref) {
crd->is_uref = 0;
- cpd->udev->refcount--;
- cv_broadcast(&cpd->udev->ref_cv);
+ if (--(cpd->udev->refcount) == 0)
+ cv_broadcast(&cpd->udev->ref_cv);
}
mtx_unlock(&usb_ref_lock);
}
@@ -592,12 +592,12 @@ usb_fifo_free(struct usb_fifo *f)
/* decrease refcount */
f->refcount--;
- /* prevent any write flush */
- f->flag_iserror = 1;
/* need to wait until all callers have exited */
while (f->refcount != 0) {
mtx_unlock(&usb_ref_lock); /* avoid LOR */
mtx_lock(f->priv_mtx);
+ /* prevent write flush, if any */
+ f->flag_iserror = 1;
/* get I/O thread out of any sleep state */
if (f->flag_sleeping) {
f->flag_sleeping = 0;
Modified: head/sys/dev/usb/usb_device.c
==============================================================================
--- head/sys/dev/usb/usb_device.c Tue Jan 13 16:18:31 2015 (r277135)
+++ head/sys/dev/usb/usb_device.c Tue Jan 13 16:37:43 2015 (r277136)
@@ -448,68 +448,29 @@ usb_endpoint_foreach(struct usb_device *
return (NULL);
}
-#if USB_HAVE_UGEN
-static uint16_t
-usb_get_refcount(struct usb_device *udev)
-{
- if (usb_proc_is_called_from(USB_BUS_EXPLORE_PROC(udev->bus)) ||
- usb_proc_is_called_from(USB_BUS_CONTROL_XFER_PROC(udev->bus)))
- return (1);
- return (2);
-}
-#endif
-
/*------------------------------------------------------------------------*
- * usb_wait_pending_ref_locked
+ * usb_wait_pending_refs
*
* This function will wait for any USB references to go away before
- * returning and disable further USB device refcounting on the
- * specified USB device. This function is used when detaching a USB
- * device.
+ * returning. This function is used before freeing a USB device.
*------------------------------------------------------------------------*/
static void
-usb_wait_pending_ref_locked(struct usb_device *udev)
+usb_wait_pending_refs(struct usb_device *udev)
{
#if USB_HAVE_UGEN
- const uint16_t refcount = usb_get_refcount(udev);
-
- DPRINTF("Refcount = %d\n", (int)refcount);
+ DPRINTF("Refcount = %d\n", (int)udev->refcount);
+ mtx_lock(&usb_ref_lock);
+ udev->refcount--;
while (1) {
/* wait for any pending references to go away */
- mtx_lock(&usb_ref_lock);
- if (udev->refcount == refcount) {
- /* prevent further refs being taken */
+ if (udev->refcount == 0) {
+ /* prevent further refs being taken, if any */
udev->refcount = USB_DEV_REF_MAX;
- mtx_unlock(&usb_ref_lock);
break;
}
- usbd_enum_unlock(udev);
cv_wait(&udev->ref_cv, &usb_ref_lock);
- mtx_unlock(&usb_ref_lock);
- (void) usbd_enum_lock(udev);
}
-#endif
-}
-
-/*------------------------------------------------------------------------*
- * usb_ref_restore_locked
- *
- * This function will restore the reference count value after a call
- * to "usb_wait_pending_ref_locked()".
- *------------------------------------------------------------------------*/
-static void
-usb_ref_restore_locked(struct usb_device *udev)
-{
-#if USB_HAVE_UGEN
- const uint16_t refcount = usb_get_refcount(udev);
-
- DPRINTF("Refcount = %d\n", (int)refcount);
-
- /* restore reference count and wakeup waiters, if any */
- mtx_lock(&usb_ref_lock);
- udev->refcount = refcount;
- cv_broadcast(&udev->ref_cv);
mtx_unlock(&usb_ref_lock);
#endif
}
@@ -1186,9 +1147,6 @@ usb_detach_device(struct usb_device *ude
sx_assert(&udev->enum_sx, SA_LOCKED);
- /* wait for pending refs to go away */
- usb_wait_pending_ref_locked(udev);
-
/*
* First detach the child to give the child's detach routine a
* chance to detach the sub-devices in the correct order.
@@ -1215,8 +1173,6 @@ usb_detach_device(struct usb_device *ude
usb_detach_device_sub(udev, &iface->subdev,
&iface->pnpinfo, flag);
}
-
- usb_ref_restore_locked(udev);
}
/*------------------------------------------------------------------------*
@@ -2033,14 +1989,43 @@ usb_make_dev(struct usb_device *udev, co
}
void
+usb_destroy_dev_sync(struct usb_fs_privdata *pd)
+{
+ DPRINTFN(1, "Destroying device at ugen%d.%d\n",
+ pd->bus_index, pd->dev_index);
+
+ /*
+ * Destroy character device synchronously. After this
+ * all system calls are returned. Can block.
+ */
+ destroy_dev(pd->cdev);
+
+ free(pd, M_USBDEV);
+}
+
+void
usb_destroy_dev(struct usb_fs_privdata *pd)
{
+ struct usb_bus *bus;
+
if (pd == NULL)
return;
- destroy_dev(pd->cdev);
+ mtx_lock(&usb_ref_lock);
+ bus = devclass_get_softc(usb_devclass_ptr, pd->bus_index);
+ mtx_unlock(&usb_ref_lock);
- free(pd, M_USBDEV);
+ if (bus == NULL) {
+ usb_destroy_dev_sync(pd);
+ return;
+ }
+
+ USB_BUS_LOCK(bus);
+ LIST_INSERT_HEAD(&bus->pd_cleanup_list, pd, pd_next);
+ /* get cleanup going */
+ usb_proc_msignal(USB_BUS_EXPLORE_PROC(bus),
+ &bus->cleanup_msg[0], &bus->cleanup_msg[1]);
+ USB_BUS_UNLOCK(bus);
}
static void
@@ -2191,6 +2176,9 @@ usb_free_device(struct usb_device *udev,
&udev->cs_msg[0], &udev->cs_msg[1]);
USB_BUS_UNLOCK(udev->bus);
+ /* wait for all references to go away */
+ usb_wait_pending_refs(udev);
+
sx_destroy(&udev->enum_sx);
sx_destroy(&udev->sr_sx);
@@ -2676,14 +2664,8 @@ usb_fifo_free_wrap(struct usb_device *ud
/* no need to free this FIFO */
continue;
}
- /* wait for pending refs to go away */
- usb_wait_pending_ref_locked(udev);
-
/* free this FIFO */
usb_fifo_free(f);
-
- /* restore refcount */
- usb_ref_restore_locked(udev);
}
}
#endif
Modified: head/sys/dev/usb/usb_device.h
==============================================================================
--- head/sys/dev/usb/usb_device.h Tue Jan 13 16:18:31 2015 (r277135)
+++ head/sys/dev/usb/usb_device.h Tue Jan 13 16:37:43 2015 (r277136)
@@ -293,6 +293,7 @@ struct usb_device *usb_alloc_device(devi
struct usb_fs_privdata *usb_make_dev(struct usb_device *, const char *,
int, int, int, uid_t, gid_t, int);
void usb_destroy_dev(struct usb_fs_privdata *);
+void usb_destroy_dev_sync(struct usb_fs_privdata *);
#endif
usb_error_t usb_probe_and_attach(struct usb_device *udev,
uint8_t iface_index);
More information about the svn-src-head
mailing list