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.
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
