| Message ID | 20220826075839.292615-1-raychi@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: core: stop USB enumeration if too many retries | expand |
On Fri, Aug 26, 2022 at 03:58:39PM +0800, Ray Chi wrote: > If a broken accessory connected to a USB host, usbcore might > keep doing enumeration retries and it will take a long time to > cause system unstable. > > This patch provides a quirk to specific USB ports of the hub to > stop USB enumeration if needed. Why only to specific ports? > Signed-off-by: Ray Chi <raychi@google.com> > --- > drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ > include/linux/usb.h | 3 +++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 2633acde7ac1..0f4097440ffb 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -3081,6 +3081,28 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > return status; > } > > +/* Stop enumerate if the port met errors and quirk is set */ > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > +{ > + struct usb_port *port_dev = hub->ports[port1 - 1]; > + struct usb_device *hdev = hub->hdev; > + > + if (retries < (PORT_INIT_TRIES - 1) / 2) > + return false; > + > + /* > + * Some USB hosts can't take a long time to keep doing enumeration > + * retry. After doing half of the retries, we would turn off the port > + * power to stop enumeration if the quirk is set. > + */ > + if (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM) { > + usb_hub_set_port_power(hdev, hub, port1, false); Why turn the port power off? Aren't there better ways to stop the enumeration attempts? When will the power ever get turned back on? Why not use the initial_descriptor_timeout module parameter for this purpose? That's the sort of thing it was meant for. Alan Stern
On Fri, Aug 26, 2022 at 10:55 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Aug 26, 2022 at 03:58:39PM +0800, Ray Chi wrote: > > If a broken accessory connected to a USB host, usbcore might > > keep doing enumeration retries and it will take a long time to > > cause system unstable. > > > > This patch provides a quirk to specific USB ports of the hub to > > stop USB enumeration if needed. > > Why only to specific ports? The specific port means it is connected to a broken accessory. > > > Signed-off-by: Ray Chi <raychi@google.com> > > --- > > drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ > > include/linux/usb.h | 3 +++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 2633acde7ac1..0f4097440ffb 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -3081,6 +3081,28 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > > return status; > > } > > > > +/* Stop enumerate if the port met errors and quirk is set */ > > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > > +{ > > + struct usb_port *port_dev = hub->ports[port1 - 1]; > > + struct usb_device *hdev = hub->hdev; > > + > > + if (retries < (PORT_INIT_TRIES - 1) / 2) > > + return false; > > + > > + /* > > + * Some USB hosts can't take a long time to keep doing enumeration > > + * retry. After doing half of the retries, we would turn off the port > > + * power to stop enumeration if the quirk is set. > > + */ > > + if (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM) { > > + usb_hub_set_port_power(hdev, hub, port1, false); > > Why turn the port power off? Aren't there better ways to stop the > enumeration attempts? When will the power ever get turned back on? > Because the broken accessory is still connected to the port. Even if we stop the enumeration attempts, the port change event will issue a new port initialization. The implementation is used for dual-role devices, the port power could be turned on when the host mode restarts again. > Why not use the initial_descriptor_timeout module parameter for this > purpose? That's the sort of thing it was meant for. > As I mentioned above, the usbcore driver will keep doing enumeration attempts if the broken accessory is still connected. It never stops. This is why I want to turn off the port to stop enumeration. > Alan Stern Thanks, Ray
On Sat, Aug 27, 2022 at 01:53:45AM +0800, Ray Chi wrote: > On Fri, Aug 26, 2022 at 10:55 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Aug 26, 2022 at 03:58:39PM +0800, Ray Chi wrote: > > > If a broken accessory connected to a USB host, usbcore might > > > keep doing enumeration retries and it will take a long time to > > > cause system unstable. > > > > > > This patch provides a quirk to specific USB ports of the hub to > > > stop USB enumeration if needed. > > > > Why only to specific ports? > > The specific port means it is connected to a broken accessory. Then this patch should be about connections to broken accessories, not too many retries. The quirk should make the hub driver permanently ignore the port; whether the power is on or off doesn't matter. Also, you might want to check whether the port connection is fixed rather than hot-unpluggable. If the broken accessory can be unplugged from the port then you don't want to disable the port. > > > Signed-off-by: Ray Chi <raychi@google.com> > > > --- > > > drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ > > > include/linux/usb.h | 3 +++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > > index 2633acde7ac1..0f4097440ffb 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -3081,6 +3081,28 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > > > return status; > > > } > > > > > > +/* Stop enumerate if the port met errors and quirk is set */ > > > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > > > +{ > > > + struct usb_port *port_dev = hub->ports[port1 - 1]; > > > + struct usb_device *hdev = hub->hdev; > > > + > > > + if (retries < (PORT_INIT_TRIES - 1) / 2) > > > + return false; > > > + > > > + /* > > > + * Some USB hosts can't take a long time to keep doing enumeration > > > + * retry. After doing half of the retries, we would turn off the port > > > + * power to stop enumeration if the quirk is set. > > > + */ > > > + if (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM) { > > > + usb_hub_set_port_power(hdev, hub, port1, false); > > > > Why turn the port power off? Aren't there better ways to stop the > > enumeration attempts? When will the power ever get turned back on? > > > > Because the broken accessory is still connected to the port. Even if we stop > the enumeration attempts, the port change event will issue a new port > initialization. If you know the accessory is broken then there's no reason to pay attention to the port change events in the first place. You can simply make port_event() avoid calling hub_port_connect_change() if the quirk is set. Alan Stern > The implementation is used for dual-role devices, the port power could > be turned on > when the host mode restarts again. > > > Why not use the initial_descriptor_timeout module parameter for this > > purpose? That's the sort of thing it was meant for. > > > > As I mentioned above, the usbcore driver will keep doing enumeration attempts if > the broken accessory is still connected. It never stops. This is why I > want to turn > off the port to stop enumeration. > > > Alan Stern > > Thanks, > Ray
On Fri, Aug 26, 2022 at 03:58:39PM +0800, Ray Chi wrote: > If a broken accessory connected to a USB host, usbcore might > keep doing enumeration retries and it will take a long time to > cause system unstable. > > This patch provides a quirk to specific USB ports of the hub to > stop USB enumeration if needed. Where does it ever allow the port to handle new devices in the future if the device is removed and then a new one is added back? Or is the port just now dead for forever? thanks, greg k-h
On Tue, Aug 30, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Aug 26, 2022 at 03:58:39PM +0800, Ray Chi wrote: > > If a broken accessory connected to a USB host, usbcore might > > keep doing enumeration retries and it will take a long time to > > cause system unstable. > > > > This patch provides a quirk to specific USB ports of the hub to > > stop USB enumeration if needed. > > Where does it ever allow the port to handle new devices in the future if > the device is removed and then a new one is added back? Or is the port > just now dead for forever? > I modified the patch according to Alan's suggestion, so the port will be working again after clearing the quirk with the v2 patch. > thanks, > > greg k-h Thanks, Ray
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 2633acde7ac1..0f4097440ffb 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -3081,6 +3081,28 @@ static int hub_port_reset(struct usb_hub *hub, int port1, return status; } +/* Stop enumerate if the port met errors and quirk is set */ +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) +{ + struct usb_port *port_dev = hub->ports[port1 - 1]; + struct usb_device *hdev = hub->hdev; + + if (retries < (PORT_INIT_TRIES - 1) / 2) + return false; + + /* + * Some USB hosts can't take a long time to keep doing enumeration + * retry. After doing half of the retries, we would turn off the port + * power to stop enumeration if the quirk is set. + */ + if (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM) { + usb_hub_set_port_power(hdev, hub, port1, false); + return true; + } + + return false; +} + /* Check if a port is power on */ int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus) { @@ -4855,6 +4877,9 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, buf->bMaxPacketSize0; kfree(buf); + if (r < 0 && (port_dev->quirks & USB_PORT_QUIRK_STOP_ENUM)) + goto fail; + retval = hub_port_reset(hub, port1, udev, delay, false); if (retval < 0) /* error or disconnect */ goto fail; @@ -5387,6 +5412,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, if ((status == -ENOTCONN) || (status == -ENOTSUPP)) break; + if (hub_port_stop_enumerate(hub, port1, i)) + break; + /* When halfway through our retry count, power-cycle the port */ if (i == (PORT_INIT_TRIES - 1) / 2) { dev_info(&port_dev->dev, "attempt power cycle\n"); @@ -5934,6 +5962,9 @@ static int usb_reset_and_verify_device(struct usb_device *udev) ret = hub_port_init(parent_hub, udev, port1, i); if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV) break; + + if (hub_port_stop_enumerate(parent_hub, port1, i)) + goto stop_enumerate; } mutex_unlock(hcd->address0_mutex); @@ -6022,6 +6053,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev) udev->bos = bos; return 0; +stop_enumerate: + mutex_unlock(hcd->address0_mutex); re_enumerate: usb_release_bos_descriptor(udev); udev->bos = bos; diff --git a/include/linux/usb.h b/include/linux/usb.h index f7a9914fc97f..fc0fef58c706 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -490,6 +490,9 @@ enum usb_port_connect_type { /* Decrease TRSTRCY to 10ms during device enumeration. */ #define USB_PORT_QUIRK_FAST_ENUM BIT(1) +/* Stop the enumeration for the given port if there are too many failures*/ +#define USB_PORT_QUIRK_STOP_ENUM BIT(2) + /* * USB 2.0 Link Power Management (LPM) parameters. */
If a broken accessory connected to a USB host, usbcore might keep doing enumeration retries and it will take a long time to cause system unstable. This patch provides a quirk to specific USB ports of the hub to stop USB enumeration if needed. Signed-off-by: Ray Chi <raychi@google.com> --- drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ include/linux/usb.h | 3 +++ 2 files changed, 36 insertions(+)