On Wed, Mar 26, 2014 at 1:46 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> 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.willi...@intel.com>
>
> 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?
Not sure. This was a case of programming by permutation until sparse
both reported the locking imbalance caused by port_event() and then
quieted the warning by annotating port_event(). Without these
annotations sparse on my system did not complain that port_event()
dropped the lock before taking it.
>
>> +}
>> +
>> +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.
Ok.
>
>> @@ -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.
Ok.
>
>> 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.
Ok, I'll take a second look before deleting.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
