Re: [PATCH v4 11/14] usb: introduce port status lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 7, 2014 at 2:11 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 31 Jan 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 port pm runtime operations.
>
> The nomenclature is a little confusing.  Let's agree that "port runtime
> PM operations" will refer to usb_port_runtime_suspend/resume, and "USB
> runtime PM operations" will refer to usb_port_suspend/resume.  So now
> this should say "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 becuase usb_port_resume() was in the middle of
>> modifying the port status.
>>
>> The lock ordering rules are now usb_lock_device() => usb_lock_port().
>
> Maybe you should mention the fact that you're introducing a new lock
> before describing the ordering rule.

Clarified the changelog accordingly...

>
>> 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-aquire
>> it when needing to take the device_lock.
>
> This patch should eliminate hub->busy_bits.  That field is unnecessary
> now that we have real mutual exlusion for port-status operations.

Might move that to a later cleanup, but it's probably small enough to
roll into this one.

>
>> @@ -4371,17 +4389,21 @@ hub_power_remaining (struct usb_hub *hub)
>>   *   usb_reset_and_verify_device() encounters changed descriptors (as from
>>   *           a firmware download)
>>   * caller already locked the hub
>> + *
>> + * It assumes the port is locked on entry and arranges for it to be
>> + * always unlocked on exit
>
> I don't really like this, but I guess it's okay.
>
>>   */
>> -static void hub_port_connect_change(struct usb_hub *hub, int port1,
>> -                                     u16 portstatus, u16 portchange)
>> +static void hub_port_connect_change_unlock(struct usb_hub *hub, int port1,
>> +                                        u16 portstatus, u16 portchange)
>
> You should add sparse annotations stating that this routine will unlock
> and lock port_dev in unexpected ways.  (If there's any way to do that;
> you may also have to tell sparse that usb_{un}lock_port is an
> {un}locking operation.)
>
>> @@ -4639,9 +4671,11 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
>>               /* TRSMRCY = 10 msec */
>>               msleep(10);
>>
>> +             usb_unlock_port(port_dev);
>>               usb_lock_device(udev);
>>               ret = usb_remote_wakeup(udev);
>>               usb_unlock_device(udev);
>> +             usb_lock_port(port_dev);
>
> These things mean hub_handle_remote_wakeup needs the sparse annotations
> too.
>
> Come to think of it, you can now move the device locking down into
> usb_remote_wakeup.  You could also move the port unlocking down there,
> if you passed port_dev as an additional argument.  (It would have to be
> NULL in the call from hcd_resume_work.)

I'll just do the usb_lock_device() push down.  Sparse does not like
conditional locking [1].

>
>> @@ -4731,7 +4765,13 @@ static void port_event(struct usb_hub *hub, int port1)
>>       /* take port actions that require the port to be powered on */
>>       pm_runtime_get_noresume(&port_dev->dev);
>>       pm_runtime_barrier(&port_dev->dev);
>> -     if (pm_runtime_active(&port_dev->dev)) {
>> +     usb_lock_port(port_dev);
>> +     do if (pm_runtime_active(&port_dev->dev)) {
>
> Please just use a normal "if" statement.

With the change below I also removed this.

>
>> +             /* re-read portstatus now that we are in-sync with
>> +              * usb_port_{suspend|resume}
>> +              */
>> +             if (hub_port_status(hub, port1, &portstatus, &portchange) < 0)
>> +                     break;
>
> For the most part, neither the pm_runtime_barrier nor the usb_lock_port
> will have to wait.  It seems unreasonable to read the port status a
> second time when there's no need.
>
> Why not put the pm_runtime_get_noresume, pm_runtime_barrier, and
> usb_lock_port calls at the start of this subroutine, just before the
> first call to hub_port_status?  Patch 9 could be altered accordingly.

I now wrap port_event() with the lock to resolve this.

>
>> @@ -4751,17 +4791,22 @@ 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);
>
> Sparse annotations.
>
> You forgot to acquire the port lock in usb_reset_device.  The lock and
> unlock calls should surround the call to usb_reset_and_verify_device.
> Both that routine and hub_port_init should be documented as requiring
> their caller to hold the port lock.

This might be a reason to move the lock to the usb_device itself.
Otherwise we'll end up with a conditional lock when resetting root hub
devices.  Sparse will not be able to statically verify such a lock
[1].  I had assumed that calliing usb_reset_device() on a root hub was
not allowed.

Hmm, maybe better to just up-level the udev->parent check to
usb_reset_device directly.

[1]: https://lwn.net/Articles/109066/
--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux