On Wed, 19 Mar 2014, Dan Williams wrote: > If a port is powered-off, or in the process of being powered-off, prevent > khubd from operating on it. Otherwise, the following sequence of events > leading to an unintended disconnect may occur: > > Events: > (0) <set pm_qos_no_poweroff to '0' for port1> > (1) hub 2-2:1.0: hub_resume > (2) hub 2-2:1.0: port 1: status 0301 change 0000 > (3) hub 2-2:1.0: state 7 ports 4 chg 0002 evt 0000 > (4) hub 2-2:1.0: port 1, power off status 0000, change 0000, 12 Mb/s > (5) usb 2-2.1: USB disconnect, device number 5 > > Description: > (1) hub is resumed before sending a ClearPortFeature request > (2) hub_activate() notices the port is connected and sets > hub->change_bits for the port > (3) hub_events() starts, but at the same time the port suspends > (4) hub_connect_change() sees the disabled port and triggers disconnect > > Most of the code thrash here is just indenting the portions of > port_event() that require the port to be runtime pm active. > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > --- > drivers/usb/core/hub.c | 91 +++++++++++++++++++++++++++--------------------- > 1 files changed, 51 insertions(+), 40 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index e0c593ea7a0e..10f420626204 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -4761,51 +4761,56 @@ static void port_event(struct usb_hub *hub, int port1) > USB_PORT_FEAT_C_PORT_CONFIG_ERROR); > } > > - if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange)) > - connect_change = 1; > - > - /* > - * Warm reset a USB3 protocol port if it's in > - * SS.Inactive state. > - */ > - if (hub_port_warm_reset_required(hub, portstatus)) { > - int status; > + /* take port actions that require the port to be powered on */ > + if (pm_runtime_active(&port_dev->dev)) { How about avoiding all the churn by doing this? + /* The following actions aren't needed if the port is powered off */ + if (!pm_runtime_active(&port_dev->dev)) + return; + > @@ -4892,11 +4897,17 @@ static void hub_events(void) > > /* deal with port status changes */ > for (i = 1; i <= hdev->maxchild; i++) { > + struct usb_port *port_dev = hub->ports[i - 1]; > + > if (!test_bit(i, hub->busy_bits) > && (test_and_clear_bit(i, hub->event_bits) > || test_bit(i, hub->change_bits) > - || test_bit(i, hub->wakeup_bits))) > + || test_bit(i, hub->wakeup_bits))) { Please add a comment here, explaining that this is to prevent any runtime suspends from powering-off the port while we're handling the events. > + pm_runtime_get_noresume(&port_dev->dev); > + pm_runtime_barrier(&port_dev->dev); > port_event(hub, i); > + pm_runtime_put_sync(&port_dev->dev); > + } > } 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