[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [rfc/patch] usb: Introduce usb_queue_reset() to do resets from atomic contexts v3
From:       Inaky Perez-Gonzalez <inaky () linux ! intel ! com>
Date:       2008-10-31 22:05:09
Message-ID: 200810311505.10691.inaky () linux ! intel ! com
[Download RAW message or body]

On Friday 31 October 2008, Alan Stern wrote:

> > Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>
>
> You attached two copies of the patch: an old copy followed by the new
> copy.

Ops sorry -- -ETOOMANYTHREADS

> > @@ -239,6 +253,7 @@ static int usb_probe_interface(struct device *dev)
> >
> >  		error = driver->probe(intf, id);
> >  		if (error) {
> > +			usb_cancel_queued_reset(intf);
> >  			mark_quiesced(intf);
> >  			intf->needs_remote_wakeup = 0;
> >  			intf->condition = USB_INTERFACE_UNBOUND;
>
> It would be better to call usb_cancel_queued_reset() after
> iface->condition is set back to USB_CONDITION_UNBOUND.  Then the
> cancel won't have to wait for usb_lock_device_for_reset() to time out.

ack

> > @@ -378,7 +394,8 @@ void usb_driver_release_interface(struct usb_driver
> > *driver,
> >  	if (device_is_registered(dev)) {
> >  		iface->condition = USB_INTERFACE_UNBINDING;
> >  		device_release_driver(dev);
> > -	}
> > +	} else
> > +		usb_cancel_queued_reset(iface);
> >
> >  	dev->driver = NULL;
> >  	usb_set_intfdata(iface, NULL);
>
> Similarly, here it would be better to set iface->condition back to
> USB_CONDITION_UNBOUND before calling usb_cancel_queued_reset().

Hummm, how do we go about this? currently it is set to unbound at the end 
of the function, but with the pmlock taken. So where would you suggest
I insert the cancel queued reset? at the end (>>1) and remove the call 
when !device_is_registered() (<<2)? This would mean that in the case of 
the device being registered, there will be to consecutive calls to
usb_cancel_queued_reset() unless we save the result of device is 
registered (using 'was_registered').

	int was_registered;
	...
	/* don't release if the interface hasn't been added yet */
	if ((was_registered = device_is_registered(dev))) {
		iface->condition = USB_INTERFACE_UNBINDING;
		device_release_driver(dev);
<<2	} else
<<2		usb_cancel_queued_reset(iface);

	dev->driver = NULL;
	usb_set_intfdata(iface, NULL);

	usb_pm_lock(udev);
	iface->condition = USB_INTERFACE_UNBOUND;
	mark_quiesced(iface);
	iface->needs_remote_wakeup = 0;
	usb_pm_unlock(udev);
>>1	if (was_registered)
>>1             usb_cancel_queued_reset(iface);

> > +void __usb_queue_reset_device(struct work_struct *ws)
> > +{
> > ...
 
> This version doesn't need any memory barriers.  The lock and unlock
> calls provide barriers of their own.

Uh, right; still it has to be called only when setting to zero if the
usb_unlock_device() is not called.

Ok, here goes v5

---

[usb] Introduce usb_queue_reset() to do resets from atomic contexts v5

This patch introduces a new call to be able to do a USB reset from an
atomic contect. This is quite helpful in USB callbacks to handle
errors (when the only thing that can be done is to do a device
reset). The Wireless USB code for using wire adaptors needs this
functionality, so does the WiMAX driver which is going to be submitted
RSN.

It is done queuing a work struct that will do the actual reset.

Per request from the list, changed the implementation to attach to a
USB interface instead of a USB device; the workstruct is attached to
the 'struct usb_interface' and before interface destruction, we make
sure that all work on it is cancelled. Likewise before a driver is
unbound.

The call flow then becomes:

usb_queue_reset_device()
  __usb_queue_reset_device() [workqueue]
    usb_reset_device()

usb_probe_interface()
  usb_cancel_queue_reset()      [error path]

usb_unbind_interface()
  usb_cancel_queue_reset()

usb_driver_release_interface()
  usb_cancel_queue_reset()

Note usb_cancel_queue_reset() needs smarts to try not to unqueue when
it is actually being executed. This happens when we run the reset that
unbinds the devices and interfaces. On interface unbind time,
usb_cancel_queue_reset() is called. So in this case, it shall not try
to cancel_work_sync() or we'd deadlock. usb_intf->reset_running is
used for that.

Changes since v4

 - (from Alan) usb_[un]lock_device*() provides barriers; do wmb() only
   if usb_unlock_device() is not being called.

 - (from Alan) call usb_cancel_queued_reset() after setting the
   interface to UNBOUND.

Changes since v3

 - (from Alan) do reset only inside usb_lock_device_for_reset(), don't
   rely on interface->status.

Changes since v2

 - (many) link to interfaces, not USB devices.

Changes since v1

 - (from Alan) don't do usb_put/get_device() refcounting as the
   deferred call is always bound to the lifetime of the 'struct
   usb_device' and thus is not neccessary.

 - (from Alan) lock the device before calling usb_reset_device() in
   __usb_queue_reset_device(). Still not all that clear what to do if
   the locking fails [as for example what would happen during .probe
   or .disconnect where the lock is already taken]. Current
   implementation looks dirty to me.

 - Misc fixes (from Alan): move __usb_queue_reset_device() from hub.c
   to usb.c and made it static; remove left over 'struct workstruct
   reset_ws' from 'struct usb_bus'; fix /it's/its/; improve
   documentation clarifying the behaviour in corner cases.

Patch is against 2.6.28-rc2

Signed-off-by: Inaky Perez-Gonzalez <inaky@linux.intel.com>

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 3d7793d..cff871b 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -184,6 +184,20 @@ static int usb_unbind_device(struct device *dev)
 	return 0;
 }
 
+/*
+ * Cancel any pending scheduled resets
+ *
+ * [see usb_queue_reset_device()]
+ *
+ * Called after unconfiguring / when releasing interfaces. See
+ * comments in __usb_queue_reset_device() regarding
+ * udev->reset_running.
+ */
+static void usb_cancel_queued_reset(struct usb_interface *iface)
+{
+	if (iface->reset_running == 0)
+		cancel_work_sync(&iface->reset_ws);
+}
 
 /* called from driver core with dev locked */
 static int usb_probe_interface(struct device *dev)
@@ -242,6 +256,7 @@ static int usb_probe_interface(struct device *dev)
 			mark_quiesced(intf);
 			intf->needs_remote_wakeup = 0;
 			intf->condition = USB_INTERFACE_UNBOUND;
+			usb_cancel_queued_reset(intf);
 		} else
 			intf->condition = USB_INTERFACE_BOUND;
 
@@ -272,6 +287,7 @@ static int usb_unbind_interface(struct device *dev)
 		usb_disable_interface(udev, intf);
 
 	driver->disconnect(intf);
+	usb_cancel_queued_reset(intf);
 
 	/* Reset other interface state.
 	 * We cannot do a Set-Interface if the device is suspended or
@@ -365,6 +381,7 @@ void usb_driver_release_interface(struct usb_driver 
*driver,
 {
 	struct device *dev = &iface->dev;
 	struct usb_device *udev = interface_to_usbdev(iface);
+	int was_registered;
 
 	/* this should never happen, don't release something that's not ours */
 	if (!dev->driver || dev->driver != &driver->drvwrap.driver)
@@ -375,7 +392,8 @@ void usb_driver_release_interface(struct usb_driver 
*driver,
 		return;
 
 	/* don't release if the interface hasn't been added yet */
-	if (device_is_registered(dev)) {
+	was_registered = device_is_registered(dev);
+	if (was_registered) {
 		iface->condition = USB_INTERFACE_UNBINDING;
 		device_release_driver(dev);
 	}
@@ -388,6 +406,10 @@ void usb_driver_release_interface(struct usb_driver 
*driver,
 	mark_quiesced(iface);
 	iface->needs_remote_wakeup = 0;
 	usb_pm_unlock(udev);
+	/* Cancel the reset here, so it doesn't have to wait for the
+	 * interface to be unbound */
+	if (was_registered)
+		usb_cancel_queued_reset(iface);
 }
 EXPORT_SYMBOL_GPL(usb_driver_release_interface);
 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 9b3f16b..f68bdfc 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3513,3 +3513,47 @@ int usb_reset_device(struct usb_device *udev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_reset_device);
+
+
+/**
+ * Reset a USB device from an atomic context
+ *
+ * @udev: USB device to reset
+ *
+ * This function can be used to reset a USB device from an atomic
+ * context, where usb_reset_device() won't work (as it blocks).
+ *
+ * Doing a reset via this method is functionally equivalent to calling
+ * usb_reset_device(), except for the fact that it is delayed to a
+ * workqueue. This means that any drivers bound to other interfaces
+ * might be unbound, as well as users from usbfs in user space.
+ *
+ * Corner cases:
+ *
+ * - Scheduling two resets at the same time from two different drivers
+ *   attached to two different interfaces of the same device is
+ *   possible; depending on how the driver attached to each interface
+ *   handles ->pre_reset(), the second reset might happen or not.
+ *
+ * - If a driver is unbound and it had a pending reset, the reset will
+ *   be cancelled.
+ *
+ * - This function can be called during .probe() or .disconnect()
+ *   times. On return from .disconnect(), any pending resets will be
+ *   cancelled.
+ *
+ * There is no no need to lock/unlock the @reset_ws as schedule_work()
+ * does its own.
+ *
+ * NOTE: We don't do any reference count tracking because it is not
+ *     needed. The lifecycle of the work_struct is tied to the
+ *     usb_interface. Before destroying the interface we cancel the
+ *     work_struct, so the fact that work_struct is queued and or
+ *     running means the interface (and thus, the device) exist and
+ *     are referenced.
+ */
+void usb_queue_reset_device(struct usb_interface *iface)
+{
+	schedule_work(&iface->reset_ws);
+}
+EXPORT_SYMBOL_GPL(usb_queue_reset_device);
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8877385..a18ca60 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1440,6 +1440,49 @@ static struct usb_interface_assoc_descriptor 
*find_iad(struct usb_device *dev,
 	return retval;
 }
 
+
+/*
+ * Internal function to queue a device reset
+ *
+ * This is initialized into the workstruct in 'struct
+ * usb_device->reset_ws' that is launched by
+ * message.c:usb_set_configuration() when initializing each 'struct
+ * usb_interface'.
+ *
+ * It is safe to get the USB device without reference counts because
+ * the life cycle of @iface is bound to the life cycle of @udev. Then,
+ * this function will be ran only if @iface is alive (and before
+ * freeing it any scheduled instances of it will have been cancelled).
+ *
+ * We need to set a flag (usb_dev->reset_running) because when we call
+ * the reset, the interfaces might be unbound. The current interface
+ * cannot try to remove the queued work as it would cause a deadlock
+ * (you cannot remove your work from within your executing
+ * workqueue). This flag lets it know, so that
+ * usb_cancel_queued_reset() doesn't try to do it.
+ *
+ * See usb_queue_reset_device() for more details
+ */
+void __usb_queue_reset_device(struct work_struct *ws)
+{
+	int rc;
+	struct usb_interface *iface =
+		container_of(ws, struct usb_interface, reset_ws);
+	struct usb_device *udev = interface_to_usbdev(iface);
+
+	rc = usb_lock_device_for_reset(udev, iface);
+	if (rc >= 0) {
+		iface->reset_running = 1;
+		usb_reset_device(udev);
+		iface->reset_running = 0;
+		if (rc > 0)
+			usb_unlock_device(udev);
+		else
+			wmb();
+	}
+}
+
+
 /*
  * usb_set_configuration - Makes a particular device setting be current
  * @dev: the device whose configuration is being updated
@@ -1610,6 +1653,7 @@ free_interfaces:
 		intf->dev.type = &usb_if_device_type;
 		intf->dev.groups = usb_interface_groups;
 		intf->dev.dma_mask = dev->dev.dma_mask;
+		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
 		device_initialize(&intf->dev);
 		mark_quiesced(intf);
 		dev_set_name(&intf->dev, "%d-%s:%d.%d",
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 8fa973b..370d611 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -119,6 +119,11 @@ enum usb_interface_condition {
  *	to the sysfs representation for that device.
  * @pm_usage_cnt: PM usage counter for this interface; autosuspend is not
  *	allowed unless the counter is 0.
+ * @reset_ws: Used for scheduling resets from atomic context.
+ * @reset_running: set to 1 if the interface is currently running a
+ *      queued reset so that usb_cancel_queued_reset() doesn't try to
+ *      remove from the workqueue when running inside the worker
+ *      thread. See __usb_queue_reset_device().
  *
  * USB device drivers attach to interfaces on a physical device.  Each
  * interface encapsulates a single high level function, such as feeding
@@ -166,10 +171,12 @@ struct usb_interface {
 	unsigned needs_remote_wakeup:1;	/* driver requires remote wakeup */
 	unsigned needs_altsetting0:1;	/* switch to altsetting 0 is pending */
 	unsigned needs_binding:1;	/* needs delayed unbind/rebind */
+	unsigned reset_running:1;
 
 	struct device dev;		/* interface specific device info */
 	struct device *usb_dev;
 	int pm_usage_cnt;		/* usage counter for autosuspend */
+	struct work_struct reset_ws;	/* for resets in atomic context */
 };
 #define	to_usb_interface(d) container_of(d, struct usb_interface, dev)
 #define	interface_to_usbdev(intf) \
@@ -503,6 +510,7 @@ extern int usb_lock_device_for_reset(struct usb_device 
*udev,
 
 /* USB port reset for device reinitialization */
 extern int usb_reset_device(struct usb_device *dev);
+extern void usb_queue_reset_device(struct usb_interface *dev);
 
 extern struct usb_device *usb_find_device(u16 vendor_id, u16 product_id);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]