Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 8 Jun 2014 20:10:29 +0000 (UTC)
From:      Hans Petter Selasky <hselasky@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r267240 - in head/sys/dev/usb: . controller
Message-ID:  <201406082010.s58KATIT002757@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: hselasky
Date: Sun Jun  8 20:10:29 2014
New Revision: 267240
URL: http://svnweb.freebsd.org/changeset/base/267240

Log:
  Resolve a deadlock setting the USB configuration index from userspace
  on USB HUBs by moving the code into the USB explore threads. The
  deadlock happens because child devices of the USB HUB don't have the
  expected reference count when called from outside the explore
  thread. Only the HUB device itself, which the IOCTL interface locks,
  gets the correct reference count.
  
  MFC after:	3 days

Modified:
  head/sys/dev/usb/controller/usb_controller.c
  head/sys/dev/usb/usb_dev.c
  head/sys/dev/usb/usb_device.h
  head/sys/dev/usb/usb_generic.c
  head/sys/dev/usb/usb_hub.c
  head/sys/dev/usb/usb_hub.h
  head/sys/dev/usb/usbdi.h

Modified: head/sys/dev/usb/controller/usb_controller.c
==============================================================================
--- head/sys/dev/usb/controller/usb_controller.c	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/controller/usb_controller.c	Sun Jun  8 20:10:29 2014	(r267240)
@@ -367,7 +367,13 @@ usb_bus_explore(struct usb_proc_msg *pm)
 	if (bus->no_explore != 0)
 		return;
 
-	if (udev && udev->hub) {
+	if (udev != NULL) {
+		USB_BUS_UNLOCK(bus);
+		uhub_explore_handle_re_enumerate(udev);
+		USB_BUS_LOCK(bus);
+	}
+
+	if (udev != NULL && udev->hub != NULL) {
 
 		if (bus->do_probe) {
 			bus->do_probe = 0;

Modified: head/sys/dev/usb/usb_dev.c
==============================================================================
--- head/sys/dev/usb/usb_dev.c	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usb_dev.c	Sun Jun  8 20:10:29 2014	(r267240)
@@ -1116,9 +1116,14 @@ usb_ioctl(struct cdev *dev, u_long cmd, 
 
 		usb_pause_mtx(NULL, hz / 128);
 
-		if (usb_ref_device(cpd, &refs, 1 /* need uref */)) {
-			err = ENXIO;
-			goto done;
+		while (usb_ref_device(cpd, &refs, 1 /* need uref */)) {
+			if (usb_ref_device(cpd, &refs, 0)) {
+				/* device no longer exits */
+				err = ENXIO;
+				goto done;
+			}
+			usb_unref_device(cpd, &refs);
+			usb_pause_mtx(NULL, hz / 128);
 		}
 	}
 

Modified: head/sys/dev/usb/usb_device.h
==============================================================================
--- head/sys/dev/usb/usb_device.h	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usb_device.h	Sun Jun  8 20:10:29 2014	(r267240)
@@ -228,6 +228,7 @@ struct usb_device {
 	uint8_t	address;		/* device addess */
 	uint8_t	device_index;		/* device index in "bus->devices" */
 	uint8_t	controller_slot_id;	/* controller specific value */
+	uint8_t next_config_index;	/* used by USB_RE_ENUM_SET_CONFIG */
 	uint8_t	curr_config_index;	/* current configuration index */
 	uint8_t	curr_config_no;		/* current configuration number */
 	uint8_t	depth;			/* distance from root HUB */
@@ -241,6 +242,7 @@ struct usb_device {
 #define	USB_RE_ENUM_DONE	0
 #define	USB_RE_ENUM_START	1
 #define	USB_RE_ENUM_PWR_OFF	2
+#define	USB_RE_ENUM_SET_CONFIG	3
 	uint8_t ifaces_max;		/* number of interfaces present */
 	uint8_t endpoints_max;		/* number of endpoints present */
 

Modified: head/sys/dev/usb/usb_generic.c
==============================================================================
--- head/sys/dev/usb/usb_generic.c	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usb_generic.c	Sun Jun  8 20:10:29 2014	(r267240)
@@ -616,24 +616,17 @@ ugen_set_config(struct usb_fifo *f, uint
 		/* not possible in device side mode */
 		return (ENOTTY);
 	}
-	if (f->udev->curr_config_index == index) {
-		/* no change needed */
-		return (0);
-	}
+
 	/* make sure all FIFO's are gone */
 	/* else there can be a deadlock */
 	if (ugen_fs_uninit(f)) {
 		/* ignore any errors */
 		DPRINTFN(6, "no FIFOs\n");
 	}
-	/* change setting - will free generic FIFOs, if any */
-	if (usbd_set_config_index(f->udev, index)) {
-		return (EIO);
-	}
-	/* probe and attach */
-	if (usb_probe_and_attach(f->udev, USB_IFACE_INDEX_ANY)) {
+
+	if (usbd_start_set_config(f->udev, index) != 0)
 		return (EIO);
-	}
+
 	return (0);
 }
 
@@ -970,11 +963,6 @@ ugen_re_enumerate(struct usb_fifo *f)
 		DPRINTFN(6, "device mode\n");
 		return (ENOTTY);
 	}
-	if (udev->parent_hub == NULL) {
-		/* the root HUB cannot be re-enumerated */
-		DPRINTFN(6, "cannot reset root HUB\n");
-		return (EINVAL);
-	}
 	/* make sure all FIFO's are gone */
 	/* else there can be a deadlock */
 	if (ugen_fs_uninit(f)) {

Modified: head/sys/dev/usb/usb_hub.c
==============================================================================
--- head/sys/dev/usb/usb_hub.c	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usb_hub.c	Sun Jun  8 20:10:29 2014	(r267240)
@@ -427,6 +427,86 @@ done:
 	return (retval);
 }
 
+void
+uhub_explore_handle_re_enumerate(struct usb_device *child)
+{
+	uint8_t do_unlock;
+	usb_error_t err;
+
+	/* check if device should be re-enumerated */
+	if (child->flags.usb_mode != USB_MODE_HOST)
+		return;
+
+	do_unlock = usbd_enum_lock(child);
+	switch (child->re_enumerate_wait) {
+	case USB_RE_ENUM_START:
+		err = usbd_set_config_index(child,
+		    USB_UNCONFIG_INDEX);
+		if (err != 0) {
+			DPRINTF("Unconfigure failed: %s: Ignored.\n",
+			    usbd_errstr(err));
+		}
+		if (child->parent_hub == NULL) {
+			/* the root HUB cannot be re-enumerated */
+			DPRINTFN(6, "cannot reset root HUB\n");
+			err = 0;
+		} else {
+			err = usbd_req_re_enumerate(child, NULL);
+		}
+		if (err == 0)
+			err = usbd_set_config_index(child, 0);
+		if (err == 0) {
+			err = usb_probe_and_attach(child,
+			    USB_IFACE_INDEX_ANY);
+		}
+		child->re_enumerate_wait = USB_RE_ENUM_DONE;
+		break;
+
+	case USB_RE_ENUM_PWR_OFF:
+		/* get the device unconfigured */
+		err = usbd_set_config_index(child,
+		    USB_UNCONFIG_INDEX);
+		if (err) {
+			DPRINTFN(0, "Could not unconfigure "
+			    "device (ignored)\n");
+		}
+		if (child->parent_hub == NULL) {
+			/* the root HUB cannot be re-enumerated */
+			DPRINTFN(6, "cannot set port feature\n");
+			err = 0;
+		} else {
+			/* clear port enable */
+			err = usbd_req_clear_port_feature(child->parent_hub,
+			    NULL, child->port_no, UHF_PORT_ENABLE);
+			if (err) {
+				DPRINTFN(0, "Could not disable port "
+				    "(ignored)\n");
+			}
+		}
+		child->re_enumerate_wait = USB_RE_ENUM_DONE;
+		break;
+
+	case USB_RE_ENUM_SET_CONFIG:
+		err = usbd_set_config_index(child,
+		    child->next_config_index);
+		if (err != 0) {
+			DPRINTF("Configure failed: %s: Ignored.\n",
+			    usbd_errstr(err));
+		} else {
+			err = usb_probe_and_attach(child,
+			    USB_IFACE_INDEX_ANY);
+		}
+		child->re_enumerate_wait = USB_RE_ENUM_DONE;
+		break;
+
+	default:
+		child->re_enumerate_wait = USB_RE_ENUM_DONE;
+		break;
+	}
+	if (do_unlock)
+		usbd_enum_unlock(child);
+}
+
 /*------------------------------------------------------------------------*
  *	uhub_explore_sub - subroutine
  *
@@ -455,59 +535,7 @@ uhub_explore_sub(struct uhub_softc *sc, 
 		goto done;
 	}
 
-	/* check if device should be re-enumerated */
-
-	if (child->flags.usb_mode == USB_MODE_HOST) {
-		uint8_t do_unlock;
-		
-		do_unlock = usbd_enum_lock(child);
-		switch (child->re_enumerate_wait) {
-		case USB_RE_ENUM_START:
-			err = usbd_set_config_index(child,
-			    USB_UNCONFIG_INDEX);
-			if (err != 0) {
-				DPRINTF("Unconfigure failed: "
-				    "%s: Ignored.\n",
-				    usbd_errstr(err));
-			}
-			err = usbd_req_re_enumerate(child, NULL);
-			if (err == 0)
-				err = usbd_set_config_index(child, 0);
-			if (err == 0) {
-				err = usb_probe_and_attach(child,
-				    USB_IFACE_INDEX_ANY);
-			}
-			child->re_enumerate_wait = USB_RE_ENUM_DONE;
-			err = 0;
-			break;
-
-		case USB_RE_ENUM_PWR_OFF:
-			/* get the device unconfigured */
-			err = usbd_set_config_index(child,
-			    USB_UNCONFIG_INDEX);
-			if (err) {
-				DPRINTFN(0, "Could not unconfigure "
-				    "device (ignored)\n");
-			}
-
-			/* clear port enable */
-			err = usbd_req_clear_port_feature(child->parent_hub,
-			    NULL, child->port_no, UHF_PORT_ENABLE);
-			if (err) {
-				DPRINTFN(0, "Could not disable port "
-				    "(ignored)\n");
-			}
-			child->re_enumerate_wait = USB_RE_ENUM_DONE;
-			err = 0;
-			break;
-
-		default:
-			child->re_enumerate_wait = USB_RE_ENUM_DONE;
-			break;
-		}
-		if (do_unlock)
-			usbd_enum_unlock(child);
-	}
+	uhub_explore_handle_re_enumerate(child);
 
 	/* check if probe and attach should be done */
 
@@ -2799,3 +2827,31 @@ usbd_start_re_enumerate(struct usb_devic
 		usb_needs_explore(udev->bus, 0);
 	}
 }
+
+/*-----------------------------------------------------------------------*
+ *	usbd_start_set_config
+ *
+ * This function starts setting a USB configuration. This function
+ * does not need to be called BUS-locked. This function does not wait
+ * until the set USB configuratino is completed.
+ *------------------------------------------------------------------------*/
+usb_error_t
+usbd_start_set_config(struct usb_device *udev, uint8_t index)
+{
+	if (udev->re_enumerate_wait == USB_RE_ENUM_DONE) {
+		if (udev->curr_config_index == index) {
+			/* no change needed */
+			return (0);
+		}
+		udev->next_config_index = index;
+		udev->re_enumerate_wait = USB_RE_ENUM_SET_CONFIG;
+		usb_needs_explore(udev->bus, 0);
+		return (0);
+	} else if (udev->re_enumerate_wait == USB_RE_ENUM_SET_CONFIG) {
+		if (udev->next_config_index == index) {
+			/* no change needed */
+			return (0);
+		}
+	}
+	return (USB_ERR_PENDING_REQUESTS);
+}

Modified: head/sys/dev/usb/usb_hub.h
==============================================================================
--- head/sys/dev/usb/usb_hub.h	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usb_hub.h	Sun Jun  8 20:10:29 2014	(r267240)
@@ -75,5 +75,6 @@ void	usb_bus_power_update(struct usb_bus
 void	usb_bus_powerd(struct usb_bus *bus);
 void	uhub_root_intr(struct usb_bus *, const uint8_t *, uint8_t);
 usb_error_t uhub_query_info(struct usb_device *, uint8_t *, uint8_t *);
+void	uhub_explore_handle_re_enumerate(struct usb_device *);
 
 #endif					/* _USB_HUB_H_ */

Modified: head/sys/dev/usb/usbdi.h
==============================================================================
--- head/sys/dev/usb/usbdi.h	Sun Jun  8 19:01:37 2014	(r267239)
+++ head/sys/dev/usb/usbdi.h	Sun Jun  8 20:10:29 2014	(r267240)
@@ -586,6 +586,8 @@ void	usbd_m_copy_in(struct usb_page_cach
 void	usbd_frame_zero(struct usb_page_cache *cache, usb_frlength_t offset,
 	    usb_frlength_t len);
 void	usbd_start_re_enumerate(struct usb_device *udev);
+usb_error_t
+	usbd_start_set_config(struct usb_device *, uint8_t);
 
 int	usb_fifo_attach(struct usb_device *udev, void *priv_sc,
 	    struct mtx *priv_mtx, struct usb_fifo_methods *pm,



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201406082010.s58KATIT002757>