d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Resolve a special case deadlock: When two or more threads are

Browse Source 
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
This commit is contained in:
Hans Petter Selasky 2015-01-13 16:37:43 +00:00
parent e503548810
commit b78e84d132
5 changed files with 96 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
sys/dev/usb/controller/usb_controller.c
View File

@ -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 *bus)
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 */

6
sys/dev/usb/usb_bus.h
View File

@ -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:
*/

12
sys/dev/usb/usb_dev.c
View File

@ -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_privdata *cpd,
}
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;

104
sys/dev/usb/usb_device.c
View File

@ -448,68 +448,29 @@ usb_endpoint_foreach(struct usb_device *udev, struct usb_endpoint *ep)
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 *udev, uint8_t iface_index,
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 *udev, uint8_t iface_index,
usb_detach_device_sub(udev, &iface->subdev,
&iface->pnpinfo, flag);
}
usb_ref_restore_locked(udev);
}
/*------------------------------------------------------------------------*
@ -2033,16 +1989,45 @@ usb_make_dev(struct usb_device *udev, const char *devname, int ep,
}
void
usb_destroy_dev(struct usb_fs_privdata *pd)
usb_destroy_dev_sync(struct usb_fs_privdata *pd)
{
if (pd == NULL)
return;
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;
mtx_lock(&usb_ref_lock);
bus = devclass_get_softc(usb_devclass_ptr, pd->bus_index);
mtx_unlock(&usb_ref_lock);
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
usb_cdev_create(struct usb_device *udev)
{
@ -2191,6 +2176,9 @@ usb_free_device(struct usb_device *udev, uint8_t flag)
&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 *udev,
/* 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

1
sys/dev/usb/usb_device.h
View File

@ -293,6 +293,7 @@ struct usb_device *usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
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);