On Wed, 19 Mar 2014, Dan Williams wrote:
> In general we do not want khubd to act on port status changes that are
> the result of in progress resets or USB runtime PM operations.
> Specifically port power control testing has been able to trigger an
> unintended disconnect in hub_port_connect_change(), paraphrasing:
>
> if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
> udev->state != USB_STATE_NOTATTACHED) {
> if (portstatus & USB_PORT_STAT_ENABLE) {
> /* Nothing to do */
> } else if (udev->state == USB_STATE_SUSPENDED &&
> udev->persist_enabled) {
> ...
> } else {
> /* Don't resuscitate */;
> }
> }
>
> ...by falling to the "Don't resuscitate" path or missing
> USB_PORT_STAT_CONNECTION because usb_port_resume() was in the middle of
> modifying the port status.
>
> So, we want a new lock to hold off khubd for a given port while the
> child device is being suspended, resumed, or reset. The lock ordering
> rules are now usb_lock_device() => usb_lock_port(). This is mandated by
> the device core which may hold the device_lock on the usb_device before
> invoking usb_port_{suspend|resume} which in turn take the status_lock on
> the usb_port. We attempt to hold the status_lock for the duration of a
> port_event() run, and drop/re-acquire it when needing to take the
> device_lock. The lock is also dropped/re-acquired during
> hub_port_reconnect().
>
> This patch also deletes hub->busy_bits as all use cases are now covered
> by port PM runtime synchronization or the port->status_lock and it
> pushes down usb_device_lock() into usb_remote_wakeup().
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Minor suggestions...
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2764,6 +2764,20 @@ static int port_is_power_on(struct usb_hub *hub, unsigned portstatus)
> return ret;
> }
>
> +static void usb_lock_port(struct usb_port *port_dev)
> + __acquires(&port_dev->status_lock)
> +{
> + mutex_lock(&port_dev->status_lock);
> + __acquire(&port_dev->status_lock);
I don't know exactly what this line does. Is it needed?
> +}
> +
> +static void usb_unlock_port(struct usb_port *port_dev)
> + __releases(&port_dev->status_lock)
> +{
> + mutex_unlock(&port_dev->status_lock);
> + __release(&port_dev->status_lock);
And likewise.
> @@ -4633,26 +4656,29 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
> /* For a suspended device, treat this as a
> * remote wakeup event.
> */
> - usb_lock_device(udev);
> + usb_unlock_port(port_dev);
> status = usb_remote_wakeup(udev);
> - usb_unlock_device(udev);
> + usb_lock_port(port_dev);
> #endif
> } else {
> /* Don't resuscitate */;
> }
> -
> }
> clear_bit(port1, hub->change_bits);
>
> + /* successfully revalidated the connection */
> if (status == 0)
> return;
These two changes should be moved into the cleanup patch.
> @@ -4782,9 +4809,11 @@ static void port_event(struct usb_hub *hub, int port1)
> if (status < 0)
> hub_port_disable(hub, port1, 1);
> } else {
> + usb_unlock_port(port_dev);
> usb_lock_device(udev);
> status = usb_reset_device(udev);
> usb_unlock_device(udev);
> + usb_lock_port(port_dev);
> connect_change = 0;
> }
> /*
Yet another reason to combine this code with the stuff immediately
following. You left out the unlock/lock calls for that part.
> @@ -5143,15 +5173,18 @@ static int descriptors_changed(struct usb_device *udev,
> * if the reset wasn't even attempted.
> *
> * Note:
> - * The caller must own the device lock. For example, it's safe to use
> - * this from a driver probe() routine after downloading new firmware.
> - * For calls that might not occur during probe(), drivers should lock
> - * the device using usb_lock_device_for_reset().
> + * The caller must own the device lock and the port lock, the latter is
> + * taken by usb_reset_device(). For example, it's safe to use
This is slightly awkward grammatically and a little confusing. I
suggest putting the new phrase inside parentheses instead of setting it
off with a comma.
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index dba7bf3fabc5..77a8f3d7779a 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -113,6 +113,12 @@ static inline int usb_autoresume_device(struct usb_device *udev)
>
> static inline int usb_remote_wakeup(struct usb_device *udev)
> {
> + /*
> + * In the PM_RUNTIME=n case we still bounce the lock to keep
> + * usb_remote_wakeup() as a flush for locked device operations
> + */
> + usb_lock_device(udev);
> + usb_unlock_device(udev);
> return 0;
> }
I'm pretty sure this isn't needed because we never call
usb_remote_wakeup if CONFIG_PM_RUNTIME isn't enabled. Any wakeup
requests arriving during system suspend will effectively be swallowed
up by usb_port_resume before khubd can see them.
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
