On Wed, 30 Apr 2014, Dan Williams wrote:
> > 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);
I don't like the way you changed usb_reset_and_verify_device(). It
doesn't seem necessary when all we're worried about is hubs. And I
still think that PM operations on the ports should be avoided, if
possible, when a hub is being reset.
Here's my own effort (not tested). It doesn't clear any runtime PM
errors, but you can add that in if you want.
Index: usb-3.15/drivers/usb/core/hub.h
===================================================================
--- usb-3.15.orig/drivers/usb/core/hub.h
+++ usb-3.15/drivers/usb/core/hub.h
@@ -66,6 +66,7 @@ struct usb_hub {
unsigned limited_power:1;
unsigned quiescing:1;
unsigned disconnected:1;
+ unsigned in_reset:1;
unsigned quirk_check_port_auto_suspend:1;
Index: usb-3.15/drivers/usb/core/hub.c
===================================================================
--- usb-3.15.orig/drivers/usb/core/hub.c
+++ usb-3.15/drivers/usb/core/hub.c
@@ -1276,12 +1276,22 @@ static void hub_quiesce(struct usb_hub *
flush_work(&hub->tt.clear_work);
}
+static void hub_pm_barrier_for_all_ports(struct usb_hub *hub)
+{
+ int i;
+
+ for (i = 0; i < hub->hdev->maxchild; ++i)
+ pm_runtime_barrier(&hub->ports[i]->dev);
+}
+
/* caller has locked the hub device */
static int hub_pre_reset(struct usb_interface *intf)
{
struct usb_hub *hub = usb_get_intfdata(intf);
hub_quiesce(hub, HUB_PRE_RESET);
+ hub->in_reset = 1;
+ hub_pm_barrier_for_all_ports(hub);
return 0;
}
@@ -1290,6 +1300,8 @@ static int hub_post_reset(struct usb_int
{
struct usb_hub *hub = usb_get_intfdata(intf);
+ hub->in_reset = 0;
+ hub_pm_barrier_for_all_ports(hub);
hub_activate(hub, HUB_POST_RESET);
return 0;
}
Index: usb-3.15/drivers/usb/core/port.c
===================================================================
--- usb-3.15.orig/drivers/usb/core/port.c
+++ usb-3.15/drivers/usb/core/port.c
@@ -82,6 +82,11 @@ static int usb_port_runtime_resume(struc
if (!hub)
return -EINVAL;
+ if (hub->in_reset) {
+ port_dev->power_is_on = 1;
+ return 0;
+ }
+
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
@@ -122,6 +127,11 @@ static int usb_port_runtime_suspend(stru
== PM_QOS_FLAGS_ALL)
return -EAGAIN;
+ if (hub->in_reset) {
+ port_dev->power_is_on = 0;
+ return 0;
+ }
+
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
retval = usb_hub_set_port_power(hdev, hub, port1, false);
> > 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?
I don't care much where the flags go, so long as they all go in the
same place. :-)
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
