On Wed, 30 Apr 2014, Dan Williams wrote:
> On Wed, 2014-04-30 at 10:04 -0400, Alan Stern wrote:
> > On Tue, 29 Apr 2014, Dan Williams wrote:
> >
> > > > What happens if a thread tries to resume or suspend a port while the
> > > > hub is being reset? With nothing to prevent it, the request sent to
> > > > the hub will fail and the port may end up in a runtime PM error state.
> > > >
> > >
> > > I'm expected to be protected by:
> > >
> > > /* Prevent autosuspend during the reset */
> > > usb_autoresume_device(udev);
> > >
> > > ...in usb_reset_device() and usb_reset_and_verify_device()'s
> > > requirement that the device in question not be suspended. The hub
> > > should pin it's parent port active during the reset.
> >
> > My question wasn't clear enough. What happens if, while hub H is being
> > reset, a thread tries to resume a port on hub H (not on H's parent)?
> >
>
> Ah, yes, I think we need something like the following. I'll fold it
> into the re-submit.
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 943366345beb..02b0833b0a67 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5183,6 +5183,31 @@ static int descriptors_changed(struct usb_device *udev,
> return changed;
> }
>
> +static void usb_hub_ports_pm_get_sync(struct usb_device *hdev)
> +{
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + int port1;
> +
> + for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> + struct usb_port *port_dev = hub->ports[port1 - 1];
> +
> + pm_runtime_get_sync(&port_dev->dev);
> + }
> +}
> +
> +static void usb_hub_ports_pm_put(struct usb_device *hdev)
> +{
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + int port1;
> +
> + for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> + struct usb_port *port_dev = hub->ports[port1 - 1];
> +
> + pm_runtime_put(&port_dev->dev);
> + }
> +}
> +
> +
> /**
> * usb_reset_and_verify_device - perform a USB port reset to reinitialize a device
> * @udev: device to reset (not in SUSPENDED or NOTATTACHED state)
> @@ -5239,6 +5264,9 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
>
> parent_hub = usb_hub_to_struct_hub(parent_hdev);
>
> + /* Disable usb_port power management */
> + usb_hub_ports_pm_get_sync(udev);
I'm kind of doubtful about this. Remember, if the hub is being reset
then it probably isn't working correctly. There's a good chance that
asking it to change a port's power level will fail.
It would be better if the port suspend and resume routines knew when a
hub reset was in progress. If it was, they could set the power_is_on
flag appropriately and immediately return 0. When the reset finished,
hub_activate() would then automatically apply power to the right ports.
Alan Stern
PS: Regarding those atomic flags... Maybe the best way to save space
is to move the existing atomic flags in usb_hub down into the usb_port
structures. Along with the two flags that are already there, we'd end
up with one long per port rather than 6 longs per hub. What do you
think?
--
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
