On Mon, May 12, 2014 at 7:22 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 9 May 2014, Dan Williams wrote:
>
>> Testing looks good. In my objection to this approach I neglected to
>> consider that the port_status_lock now protects us from port_event()
>> usb_port_{suspend_resume} collisions. I have updated the changelog
>> accordingly.
>>
>> 8<-----------
>> Subject: usb: resume (wakeup) child device when port is powered on
>>
>> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>>
>> Unconditionally wake up the child device when the power session is
>> recovered.
>>
>> This addresses the following scenarios:
>>
>> 1/ The device may need a reset on power-session loss, without this
>> change port power-on recovery exposes khubd to scenarios that
>> usb_port_resume() is set to handle. Prior to port power control the
>> only time a power session would be lost is during dpm_suspend of the
>> hub. In that scenario usb_port_resume() is guaranteed to be called
>> prior to khubd running for that port. With this change we wakeup the
>> child device as soon as possible (prior to khubd running again for this
>> port).
>>
>> Although khubd has facilities to wake a child device it will only do
>> so if the portstatus / portchange indicates a suspend state. In the
>> case of port power control we are not coming from a hub-port-suspend
>> state. This implementation extends usb_wake_notification() for the
>> port rpm_resume case.
>
> Actually it doesn't any more (extend usb_wake_notification(), that is).
>
>> @@ -2057,9 +2057,11 @@ static void hub_free_dev(struct usb_device *udev)
>> */
>> void usb_disconnect(struct usb_device **pdev)
>> {
>> - struct usb_device *udev = *pdev;
>> - struct usb_hub *hub = usb_hub_to_struct_hub(udev);
>> - int i;
>> + int i;
>> + struct usb_device *udev = *pdev;
>> + int port1 = udev->portnum;
>> + struct usb_port *port_dev = NULL;
>> + struct usb_hub *hub = usb_hub_to_struct_hub(udev);
>>
>> /* mark the device as inactive, so any further urb submissions for
>> * this device (and any of its children) will fail immediately.
>> @@ -2086,15 +2088,18 @@ void usb_disconnect(struct usb_device **pdev)
>> usb_hcd_synchronize_unlinks(udev);
>>
>> if (udev->parent) {
>> - int port1 = udev->portnum;
>> struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
>> - struct usb_port *port_dev = hub->ports[port1 - 1];
>>
>> + port_dev = hub->ports[port1 - 1];
>> sysfs_remove_link(&udev->dev.kobj, "port");
>> sysfs_remove_link(&port_dev->dev.kobj, "device");
>>
>> - if (test_and_clear_bit(port1, hub->child_usage_bits))
>> - pm_runtime_put(&port_dev->dev);
>> + /*
>> + * As usb_port_runtime_resume() de-references udev, make
>> + * sure no resumes occur during removal
>> + */
>> + if (!test_and_set_bit(port1, hub->child_usage_bits))
>> + pm_runtime_get_sync(&port_dev->dev);
>> }
>>
>> usb_remove_ep_devs(&udev->ep0);
>> @@ -2116,6 +2121,9 @@ void usb_disconnect(struct usb_device **pdev)
>> *pdev = NULL;
>> spin_unlock_irq(&device_state_lock);
>>
>> + if (port_dev && test_and_clear_bit(port1, hub->child_usage_bits))
>> + pm_runtime_put(&port_dev->dev);
>> +
>
> There's a nasty bug here. I'll let you figure it out for yourself. :-)
> Hint: Hiding a variable by declaring another local variable with the
> same name in an inner block often leads to trouble.
Hmm, it seems it wasn't the mistake I thought I had made with a
botched rebase and keeping port_dev defined in two locations. Great
catch!
--
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
