On Mon, 10 Mar 2014, Poulain, Loic wrote:
> Despite the needs_rebind flag, the interface rebind was never
> done for the PM runtime resume. This patch fixes this issue
> by triggering the rebind in usb runtime resume.
>
> The rebind procedure needs to be called with the device lock.
> However, depending the call path (remote wakeup, local resume),
> the device lock may or may not already be held in usb runtime
> resume. So, use a work queue to take the lock unconditionally.
> Protect this work against any deadlock in the same way as
> reset_device.
>
> Signed-off-by: lpoulain <loic.poulain@xxxxxxxxx>
Although the basic idea is okay, this patch has a couple of weak spots.
> @@ -1662,6 +1662,52 @@ static void __usb_queue_reset_device(struct work_struct *ws)
> }
> }
>
> +/*
> + * Internal function to queue an interface rebind
> + *
> + * This is initialized into the workstruct in 'struct
> + * usb_device->rebind_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->rebind_running) because when we call
> + * the rebind, the interfaces will 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_rebind() doesn't
> + * try to do it.
> + */
> +static void __usb_queue_rebind_intf(struct work_struct *ws)
> +{
> + unsigned long jiffies_expire = jiffies + HZ;
> + struct usb_interface *iface =
> + container_of(ws, struct usb_interface, rebind_ws);
> + struct usb_device *udev = interface_to_usbdev(iface);
> +
> + /* Rather than sleeping to wait for the lock, poll repeatedly.
> + * This is to prevent any deadlock with usb_unbind_interface */
> + while (!usb_trylock_device(udev)) {
> + /* If we can't acquire the lock after waiting one second,
> + * we're probably deadlocked */
> + if (time_after(jiffies, jiffies_expire))
> + return;
> +
> + if (udev->state == USB_STATE_NOTATTACHED ||
> + iface->condition == USB_INTERFACE_UNBOUND)
> + return;
> + }
This loop is bad because it doesn't sleep. Can't you just call
usb_lock_device_for_reset() instead of basically rewriting it?
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -131,6 +131,10 @@ enum usb_interface_condition {
> * thread. See __usb_queue_reset_device().
> * @resetting_device: USB core reset the device, so use alt setting 0 as
> * current; needs bandwidth alloc after reset.
> + * @rebind_running: set to 1 if the interface is currently running a
> + * queued rebind so that usb_cancel_queued_rebind() doesn't try to
> + * remove from the workqueue when running inside the worker
> + * thread. See __usb_queue_rebind_device()
> *
> * USB device drivers attach to interfaces on a physical device. Each
> * interface encapsulates a single high level function, such as feeding
Where's the kerneldoc for rebind_ws?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
