Commit 76730c9fb39242f4f46f36ed07024deeaf29cedd

Authored by Alan Stern
Committed by Greg Kroah-Hartman
1 parent 482b4b7132

USB: don't cancel queued resets when unbinding drivers

commit 524134d422316a59d5464ccbc12036bbe90c5563 upstream.

The USB stack provides a mechanism for drivers to request an
asynchronous device reset (usb_queue_reset_device()).  The mechanism
uses a work item (reset_ws) embedded in the usb_interface structure
used by the driver, and the reset is carried out by a work queue
routine.

The asynchronous reset can race with driver unbinding.  When this
happens, we try to cancel the queued reset before unbinding the
driver, on the theory that the driver won't care about any resets once
it is unbound.

However, thanks to the fact that lockdep now tracks work queue
accesses, this can provoke a lockdep warning in situations where the
device reset causes another interface's driver to be unbound; see

	http://marc.info/?l=linux-usb&m=141893165203776&w=2

for an example.  The reason is that the work routine for reset_ws in
one interface calls cancel_queued_work() for the reset_ws in another
interface.  Lockdep thinks this might lead to a work routine trying to
cancel itself.  The simplest solution is not to cancel queued resets
when unbinding drivers.

This means we now need to acquire a reference to the usb_interface
when queuing a reset_ws work item and to drop the reference when the
work routine finishes.  We also need to make sure that the
usb_interface structure doesn't outlive its parent usb_device; this
means acquiring and dropping a reference when the interface is created
and destroyed.

In addition, cancelling a queued reset can fail (if the device is in
the middle of an earlier reset), and this can cause usb_reset_device()
to try to rebind an interface that has been deallocated (see
http://marc.info/?l=linux-usb&m=142175717016628&w=2 for details).
Acquiring the extra references prevents this failure.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Russell King - ARM Linux <linux@arm.linux.org.uk>
Reported-by: Olivier Sobrie <olivier@sobrie.be>
Tested-by: Olivier Sobrie <olivier@sobrie.be>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Showing 4 changed files with 12 additions and 58 deletions Side-by-side Diff

drivers/usb/core/driver.c
... ... @@ -275,21 +275,6 @@
275 275 return 0;
276 276 }
277 277  
278   -/*
279   - * Cancel any pending scheduled resets
280   - *
281   - * [see usb_queue_reset_device()]
282   - *
283   - * Called after unconfiguring / when releasing interfaces. See
284   - * comments in __usb_queue_reset_device() regarding
285   - * udev->reset_running.
286   - */
287   -static void usb_cancel_queued_reset(struct usb_interface *iface)
288   -{
289   - if (iface->reset_running == 0)
290   - cancel_work_sync(&iface->reset_ws);
291   -}
292   -
293 278 /* called from driver core with dev locked */
294 279 static int usb_probe_interface(struct device *dev)
295 280 {
... ... @@ -380,7 +365,6 @@
380 365 usb_set_intfdata(intf, NULL);
381 366 intf->needs_remote_wakeup = 0;
382 367 intf->condition = USB_INTERFACE_UNBOUND;
383   - usb_cancel_queued_reset(intf);
384 368  
385 369 /* If the LPM disable succeeded, balance the ref counts. */
386 370 if (!lpm_disable_error)
... ... @@ -425,7 +409,6 @@
425 409 usb_disable_interface(udev, intf, false);
426 410  
427 411 driver->disconnect(intf);
428   - usb_cancel_queued_reset(intf);
429 412  
430 413 /* Free streams */
431 414 for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
drivers/usb/core/hub.c
... ... @@ -5591,26 +5591,19 @@
5591 5591 * possible; depending on how the driver attached to each interface
5592 5592 * handles ->pre_reset(), the second reset might happen or not.
5593 5593 *
5594   - * - If a driver is unbound and it had a pending reset, the reset will
5595   - * be cancelled.
  5594 + * - If the reset is delayed so long that the interface is unbound from
  5595 + * its driver, the reset will be skipped.
5596 5596 *
5597   - * - This function can be called during .probe() or .disconnect()
5598   - * times. On return from .disconnect(), any pending resets will be
5599   - * cancelled.
5600   - *
5601   - * There is no no need to lock/unlock the @reset_ws as schedule_work()
5602   - * does its own.
5603   - *
5604   - * NOTE: We don't do any reference count tracking because it is not
5605   - * needed. The lifecycle of the work_struct is tied to the
5606   - * usb_interface. Before destroying the interface we cancel the
5607   - * work_struct, so the fact that work_struct is queued and or
5608   - * running means the interface (and thus, the device) exist and
5609   - * are referenced.
  5597 + * - This function can be called during .probe(). It can also be called
  5598 + * during .disconnect(), but doing so is pointless because the reset
  5599 + * will not occur. If you really want to reset the device during
  5600 + * .disconnect(), call usb_reset_device() directly -- but watch out
  5601 + * for nested unbinding issues!
5610 5602 */
5611 5603 void usb_queue_reset_device(struct usb_interface *iface)
5612 5604 {
5613   - schedule_work(&iface->reset_ws);
  5605 + if (schedule_work(&iface->reset_ws))
  5606 + usb_get_intf(iface);
5614 5607 }
5615 5608 EXPORT_SYMBOL_GPL(usb_queue_reset_device);
5616 5609  
drivers/usb/core/message.c
... ... @@ -1551,6 +1551,7 @@
1551 1551 altsetting_to_usb_interface_cache(intf->altsetting);
1552 1552  
1553 1553 kref_put(&intfc->ref, usb_release_interface_cache);
  1554 + usb_put_dev(interface_to_usbdev(intf));
1554 1555 kfree(intf);
1555 1556 }
1556 1557  
... ... @@ -1626,24 +1627,6 @@
1626 1627  
1627 1628 /*
1628 1629 * Internal function to queue a device reset
1629   - *
1630   - * This is initialized into the workstruct in 'struct
1631   - * usb_device->reset_ws' that is launched by
1632   - * message.c:usb_set_configuration() when initializing each 'struct
1633   - * usb_interface'.
1634   - *
1635   - * It is safe to get the USB device without reference counts because
1636   - * the life cycle of @iface is bound to the life cycle of @udev. Then,
1637   - * this function will be ran only if @iface is alive (and before
1638   - * freeing it any scheduled instances of it will have been cancelled).
1639   - *
1640   - * We need to set a flag (usb_dev->reset_running) because when we call
1641   - * the reset, the interfaces might be unbound. The current interface
1642   - * cannot try to remove the queued work as it would cause a deadlock
1643   - * (you cannot remove your work from within your executing
1644   - * workqueue). This flag lets it know, so that
1645   - * usb_cancel_queued_reset() doesn't try to do it.
1646   - *
1647 1630 * See usb_queue_reset_device() for more details
1648 1631 */
1649 1632 static void __usb_queue_reset_device(struct work_struct *ws)
1650 1633  
1651 1634  
... ... @@ -1655,11 +1638,10 @@
1655 1638  
1656 1639 rc = usb_lock_device_for_reset(udev, iface);
1657 1640 if (rc >= 0) {
1658   - iface->reset_running = 1;
1659 1641 usb_reset_device(udev);
1660   - iface->reset_running = 0;
1661 1642 usb_unlock_device(udev);
1662 1643 }
  1644 + usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */
1663 1645 }
1664 1646  
1665 1647  
... ... @@ -1854,6 +1836,7 @@
1854 1836 dev_set_name(&intf->dev, "%d-%s:%d.%d",
1855 1837 dev->bus->busnum, dev->devpath,
1856 1838 configuration, alt->desc.bInterfaceNumber);
  1839 + usb_get_dev(dev);
1857 1840 }
1858 1841 kfree(new_interfaces);
1859 1842  
... ... @@ -127,10 +127,6 @@
127 127 * to the sysfs representation for that device.
128 128 * @pm_usage_cnt: PM usage counter for this interface
129 129 * @reset_ws: Used for scheduling resets from atomic context.
130   - * @reset_running: set to 1 if the interface is currently running a
131   - * queued reset so that usb_cancel_queued_reset() doesn't try to
132   - * remove from the workqueue when running inside the worker
133   - * thread. See __usb_queue_reset_device().
134 130 * @resetting_device: USB core reset the device, so use alt setting 0 as
135 131 * current; needs bandwidth alloc after reset.
136 132 *
... ... @@ -181,7 +177,6 @@
181 177 unsigned needs_remote_wakeup:1; /* driver requires remote wakeup */
182 178 unsigned needs_altsetting0:1; /* switch to altsetting 0 is pending */
183 179 unsigned needs_binding:1; /* needs delayed unbind/rebind */
184   - unsigned reset_running:1;
185 180 unsigned resetting_device:1; /* true: bandwidth alloc after reset */
186 181  
187 182 struct device dev; /* interface specific device info */