| Message ID | 20200726232457.9107-2-matthew.ruffell@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Reapply "usb: handle warm-reset port requests on hub resume" | expand |
On 27.07.20 01:24, Matthew Ruffell wrote: > From: Jan-Marek Glogowski <glogow@fbihome.de> > > BugLink: https://bugs.launchpad.net/bugs/1859873 > > On plug-in of my USB-C device, its USB_SS_PORT_LS_SS_INACTIVE > link state bit is set. Greping all the kernel for this bit shows > that the port status requests a warm-reset this way. > > This just happens, if its the only device on the root hub, the hub > therefore resumes and the HCDs status_urb isn't yet available. > If a warm-reset request is detected, this sets the hubs event_bits, > which will prevent any auto-suspend and allows the hubs workqueue > to warm-reset the port later in port_event. > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de> > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 4fdc1790e6a9ef22399c6bc6e63b80f4609f3b7e) > Signed-off-by: Matthew Ruffell <matthew.ruffell@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Ack for Bionic. For Eoan, the question is whether this would be useful. Eoan is EOL since Jul-17. The won't be a generic update there. We still have a few special projects that are on 5.3 but quickly being replaced and not sure whether those are using hw which benefits from the change. -Stefan > drivers/usb/core/hub.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index b99602789ca2..7a27237f6b67 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -107,6 +107,8 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem); > static void hub_release(struct kref *kref); > static int usb_reset_and_verify_device(struct usb_device *udev); > static int hub_port_disable(struct usb_hub *hub, int port1, int set_state); > +static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1, > + u16 portstatus); > > static inline char *portspeed(struct usb_hub *hub, int portstatus) > { > @@ -1117,6 +1119,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) > USB_PORT_FEAT_ENABLE); > } > > + /* Make sure a warm-reset request is handled by port_event */ > + if (type == HUB_RESUME && > + hub_port_warm_reset_required(hub, port1, portstatus)) > + set_bit(port1, hub->event_bits); > + > /* > * Add debounce if USB3 link is in polling/link training state. > * Link will automatically transition to Enabled state after >
On 27.07.20 01:24, Matthew Ruffell wrote: > From: Jan-Marek Glogowski <glogow@fbihome.de> > > BugLink: https://bugs.launchpad.net/bugs/1859873 > > On plug-in of my USB-C device, its USB_SS_PORT_LS_SS_INACTIVE > link state bit is set. Greping all the kernel for this bit shows > that the port status requests a warm-reset this way. > > This just happens, if its the only device on the root hub, the hub > therefore resumes and the HCDs status_urb isn't yet available. > If a warm-reset request is detected, this sets the hubs event_bits, > which will prevent any auto-suspend and allows the hubs workqueue > to warm-reset the port later in port_event. > > Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de> > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 4fdc1790e6a9ef22399c6bc6e63b80f4609f3b7e) > Signed-off-by: Matthew Ruffell <matthew.ruffell@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/usb/core/hub.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index b99602789ca2..7a27237f6b67 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -107,6 +107,8 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem); > static void hub_release(struct kref *kref); > static int usb_reset_and_verify_device(struct usb_device *udev); > static int hub_port_disable(struct usb_hub *hub, int port1, int set_state); > +static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1, > + u16 portstatus); > > static inline char *portspeed(struct usb_hub *hub, int portstatus) > { > @@ -1117,6 +1119,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) > USB_PORT_FEAT_ENABLE); > } > > + /* Make sure a warm-reset request is handled by port_event */ > + if (type == HUB_RESUME && > + hub_port_warm_reset_required(hub, port1, portstatus)) > + set_bit(port1, hub->event_bits); > + > /* > * Add debounce if USB3 link is in polling/link training state. > * Link will automatically transition to Enabled state after >
> > Ack for Bionic. For Eoan, the question is whether this would be useful. Eoan is > EOL since Jul-17. The won't be a generic update there. We still have a few > special projects that are on 5.3 but quickly being replaced and not sure whether > those are using hw which benefits from the change. > > -Stefan > Roger that. Eoan is unnecessary then, and I have changed the Launchpad bug to Wontfix for Eoan. I was just thinking Eoan would end up a lot like Disco, which got upstream stable patches and generic releases past its EOL date. Patching Bionic only is perfectly okay. Thanks, Matthew >> drivers/usb/core/hub.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index b99602789ca2..7a27237f6b67 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -107,6 +107,8 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem); >> static void hub_release(struct kref *kref); >> static int usb_reset_and_verify_device(struct usb_device *udev); >> static int hub_port_disable(struct usb_hub *hub, int port1, int set_state); >> +static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1, >> + u16 portstatus); >> >> static inline char *portspeed(struct usb_hub *hub, int portstatus) >> { >> @@ -1117,6 +1119,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) >> USB_PORT_FEAT_ENABLE); >> } >> >> + /* Make sure a warm-reset request is handled by port_event */ >> + if (type == HUB_RESUME && >> + hub_port_warm_reset_required(hub, port1, portstatus)) >> + set_bit(port1, hub->event_bits); >> + >> /* >> * Add debounce if USB3 link is in polling/link training state. >> * Link will automatically transition to Enabled state after >> > >
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index b99602789ca2..7a27237f6b67 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -107,6 +107,8 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem); static void hub_release(struct kref *kref); static int usb_reset_and_verify_device(struct usb_device *udev); static int hub_port_disable(struct usb_hub *hub, int port1, int set_state); +static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1, + u16 portstatus); static inline char *portspeed(struct usb_hub *hub, int portstatus) { @@ -1117,6 +1119,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) USB_PORT_FEAT_ENABLE); } + /* Make sure a warm-reset request is handled by port_event */ + if (type == HUB_RESUME && + hub_port_warm_reset_required(hub, port1, portstatus)) + set_bit(port1, hub->event_bits); + /* * Add debounce if USB3 link is in polling/link training state. * Link will automatically transition to Enabled state after