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 <[email protected]>
---
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);
+ 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.
*/
--
2.37.2.672.g94769d06f0-goog
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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