On Wed, 2014-04-30 at 15:51 -0400, Alan Stern wrote:
> 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.
Great point. Continuing the line of thinking I think we should also
clear any previous pm runtime errors in the ports when the hub is reset.
Hmm, I'm struggling to make the update of the "hub->in_reset" state
atomic with respect to in-flight runtime pm transitions. How about this
incremental addition to the previous suggestion where we:
1/ Still try to runtime resume all the ports
2/ Force any ports that didn't make the transition active
3/ Clear any runtime errors as a result of that forcing, and mark the
ports "on" for the purpose of hub_power_on().
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 02b0833b0a67..a662c7172f5b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5183,31 +5183,36 @@ static int descriptors_changed(struct usb_device *udev,
return changed;
}
-static void usb_hub_ports_pm_get_sync(struct usb_device *hdev)
+static void usb_hub_pm_prep_reset(struct usb_device *hdev)
{
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_port *port_dev;
int port1;
+ if (!hub)
+ return;
for (port1 = 1; port1 <= hdev->maxchild; port1++) {
- struct usb_port *port_dev = hub->ports[port1 - 1];
-
+ port_dev = hub->ports[port1 - 1];
pm_runtime_get_sync(&port_dev->dev);
+ pm_runtime_set_active(&port_dev->dev);
+ set_bit(port1, hub->power_bits);
}
}
-static void usb_hub_ports_pm_put(struct usb_device *hdev)
+static void usb_hub_pm_finish_reset(struct usb_device *hdev)
{
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_port *port_dev;
int port1;
+ if (!hub)
+ return;
for (port1 = 1; port1 <= hdev->maxchild; port1++) {
- struct usb_port *port_dev = hub->ports[port1 - 1];
-
+ 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)
@@ -5265,7 +5270,7 @@ 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);
+ usb_hub_pm_prep_reset(udev);
/* Disable USB2 hardware LPM.
* It will be re-enabled by the enumeration process.
@@ -5386,11 +5391,11 @@ done:
usb_enable_ltm(udev);
usb_release_bos_descriptor(udev);
udev->bos = bos;
- usb_hub_ports_pm_put(udev);
+ usb_hub_pm_finish_reset(udev);
return 0;
re_enumerate:
- usb_hub_ports_pm_put(udev);
+ usb_hub_pm_finish_reset(udev);
/* LPM state doesn't matter when we're about to destroy the device. */
hub_port_logical_disconnect(parent_hub, port1);
usb_release_bos_descriptor(udev);
>
> 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?
>
I did the conversion of putting them in struct usb_hub and aesthetically
I like the look of:
_bit(port1, hub->power_bits)
...better than:
_bit(USB_PORTDEV_POWER, &port_dev->flags)
So I'm inclined to keep them there regardless of the space argument.
Thoughts?
--
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
