Re: [PATCH v7 14/16] usb: resume (wakeup) child device when port is powered on

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

 



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