| Message ID | 20220908104019.3080989-1-raychi@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: core: stop USB enumeration if too many retries | expand |
On Thu, Sep 08, 2022 at 06:40:19PM +0800, Ray Chi wrote: > When a broken USB accessory connects to a USB host, usbcore might > keep doing enumeration retries. If the host has a watchdog mechanism, > the kernel panic will happen on the host. > > The patch provide a attribute to limit the numbers of retries and > timeout lengths. This is per-port, right? Please say that here. > In addition, it also adds a function to stop the > port initialization and ignore events on the port if the broken > accessory is still connected. So this should be 2 patches? Remember, only do one logical thing per patch please. > > Signed-off-by: Ray Chi <raychi@google.com> > --- > Changes since v2: > - replace the quirk with the attribute > - Document the attribute > - modify hub_port_stop_enumerate() position in port_event() > - modify the changelog > > Changes since v1: > - remove usb_hub_set_port_power() > - add a variable ignore_connect into struct port_dev > - modify hub_port_stop_enumerate() and set ignore_connect in > this function > - avoid calling hub_port_connect_change() in port_event() > --- > Documentation/ABI/testing/sysfs-bus-usb | 9 ++++++ > drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++ > drivers/usb/core/hub.h | 4 +++ > drivers/usb/core/port.c | 23 +++++++++++++++ > 4 files changed, 75 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > index 568103d3376e..d44c8aaef929 100644 > --- a/Documentation/ABI/testing/sysfs-bus-usb > +++ b/Documentation/ABI/testing/sysfs-bus-usb > @@ -264,6 +264,15 @@ Description: > attached to the port will not be detected, initialized, > or enumerated. > > +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init > +Date: Sep 2022 > +Contact: Ray Chi <raychi@google.com> > +Description: > + Some USB hosts have some watchdog mechanisms so that the device > + may enter ramdump if it takes a long time during port initialization. > + This attribute limits the numbers of retries and timeout lengths once > + an initialization of the port meets failures. The text here is very odd, maybe it's just a translation issue. A port can not "meet" a failure. And this really has nothing to do with any watchdog, it's just a "we want to fail broken devices quickly instead of giving them lots of time to connect" type of issue, right? And we already have different init schemes, "quick" is an odd way to phrase this, but I can't think of anything else at the moment. > + > What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout > Date: May 2013 > Contact: Mathias Nyman <mathias.nyman@linux.intel.com> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index d4b1e70d1498..f22caa133274 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -3081,6 +3081,29 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > return status; > } > > +/* Check whether a hub would stop enumeration or ignore events on the port. */ Based on what? Retries? Time of day? Phase of moon? :) > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > +{ > + struct usb_port *port_dev = hub->ports[port1 - 1]; > + > + if (port_dev->quick_init) { > + if (port_dev->ignore_connect) > + return true; > + > + /* > + * Since some normal devices will be timeout in the first attempt, > + * set the condition to half of the retries > + */ > + if (retries < (PORT_INIT_TRIES - 1) / 2) > + return false; > + > + port_dev->ignore_connect = true; > + } else > + port_dev->ignore_connect = false; So this function modifies a port_dev value, AND returns it to the caller? Why not document that somewhere? Why do both? And if quick_init is false then it always set ignore_connect to false and returns false always? > + > + return port_dev->ignore_connect; > +} > + > /* Check if a port is power on */ > int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus) > { > @@ -4855,6 +4878,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, > buf->bMaxPacketSize0; > kfree(buf); > > + if (r < 0 && port_dev->quick_init) { > + retval = r; > + goto fail; > + } > + > retval = hub_port_reset(hub, port1, udev, delay, false); > if (retval < 0) /* error or disconnect */ > goto fail; > @@ -5387,6 +5415,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"); > @@ -5614,6 +5645,9 @@ static void port_event(struct usb_hub *hub, int port1) > if (!pm_runtime_active(&port_dev->dev)) > return; > > + if (hub_port_stop_enumerate(hub, port1, 0)) > + return; > + > if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange)) > connect_change = 1; > > @@ -5934,6 +5968,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 +6059,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/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > index b2925856b4cb..57995ec3af58 100644 > --- a/drivers/usb/core/hub.h > +++ b/drivers/usb/core/hub.h > @@ -90,6 +90,8 @@ struct usb_hub { > * @is_superspeed cache super-speed status > * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted. > * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted. > + * @ignore_connect: ignore the connection or not. Which connection? Any future connection? Or the one that is currently connected? > + * @quick_init: limit the numbers of retries for port initialization Limit it to what? > */ > struct usb_port { > struct usb_device *child; > @@ -103,6 +105,8 @@ struct usb_port { > u32 over_current_count; > u8 portnum; > u32 quirks; > + bool ignore_connect; > + bool quick_init; Why are these bool, when: > unsigned int is_superspeed:1; > unsigned int usb3_lpm_u1_permit:1; > unsigned int usb3_lpm_u2_permit:1; Those are all bits? Be consistent please. thanks, greg k-h
Sorry I forgot about this patch. On Thu, Sep 22, 2022 at 02:06:51PM +0200, Greg KH wrote: > On Thu, Sep 08, 2022 at 06:40:19PM +0800, Ray Chi wrote: > > When a broken USB accessory connects to a USB host, usbcore might > > keep doing enumeration retries. If the host has a watchdog mechanism, > > the kernel panic will happen on the host. > > > > The patch provide a attribute to limit the numbers of retries and > > timeout lengths. > > This is per-port, right? Please say that here. Also, the patch does not limit any timeout lengths -- just the number of retries. Maybe you want to make it limit the timeouts also. > > In addition, it also adds a function to stop the > > port initialization and ignore events on the port if the broken > > accessory is still connected. > > So this should be 2 patches? Remember, only do one logical thing per > patch please. The "adding a function" part is unimportant; we can see the function simply by reading the patch. The main points here, which you have not explained clearly, are: 1. If a port is marked with the quick_init attribute, unsuccessful connection attempts will fail quickly. 2. Once a quick_init port has failed to initialize, it will ignore all future connections until the quick_init attribute is turned off. > > Signed-off-by: Ray Chi <raychi@google.com> > > --- > > Changes since v2: > > - replace the quirk with the attribute > > - Document the attribute > > - modify hub_port_stop_enumerate() position in port_event() > > - modify the changelog > > > > Changes since v1: > > - remove usb_hub_set_port_power() > > - add a variable ignore_connect into struct port_dev > > - modify hub_port_stop_enumerate() and set ignore_connect in > > this function > > - avoid calling hub_port_connect_change() in port_event() > > --- > > Documentation/ABI/testing/sysfs-bus-usb | 9 ++++++ > > drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++ > > drivers/usb/core/hub.h | 4 +++ > > drivers/usb/core/port.c | 23 +++++++++++++++ > > 4 files changed, 75 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > > index 568103d3376e..d44c8aaef929 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-usb > > +++ b/Documentation/ABI/testing/sysfs-bus-usb > > @@ -264,6 +264,15 @@ Description: > > attached to the port will not be detected, initialized, > > or enumerated. > > > > +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init > > +Date: Sep 2022 > > +Contact: Ray Chi <raychi@google.com> > > +Description: > > + Some USB hosts have some watchdog mechanisms so that the device > > + may enter ramdump if it takes a long time during port initialization. > > + This attribute limits the numbers of retries and timeout lengths once > > + an initialization of the port meets failures. > > The text here is very odd, maybe it's just a translation issue. A port > can not "meet" a failure. And this really has nothing to do with any > watchdog, it's just a "we want to fail broken devices quickly instead of > giving them lots of time to connect" type of issue, right? > > And we already have different init schemes, "quick" is an odd way to > phrase this, but I can't think of anything else at the moment. "fail-fast" gives the idea better, but it's more awkward. > > + > > What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout > > Date: May 2013 > > Contact: Mathias Nyman <mathias.nyman@linux.intel.com> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index d4b1e70d1498..f22caa133274 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -3081,6 +3081,29 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > > return status; > > } > > > > +/* Check whether a hub would stop enumeration or ignore events on the port. */ > > Based on what? Retries? Time of day? Phase of moon? :) > > > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > > +{ > > + struct usb_port *port_dev = hub->ports[port1 - 1]; > > + > > + if (port_dev->quick_init) { > > + if (port_dev->ignore_connect) > > + return true; > > + > > + /* > > + * Since some normal devices will be timeout in the first attempt, > > + * set the condition to half of the retries > > + */ > > + if (retries < (PORT_INIT_TRIES - 1) / 2) > > + return false; This is not a good test. You shouldn't rely on PORT_INIT_TRIES having any particular value or being used in any particular way. You should just something like: /* * We want unsuccessful attempts to fail quickly. * Therefore allow three tries but no more. */ if (retries < 3) > > + > > + port_dev->ignore_connect = true; > > + } else > > + port_dev->ignore_connect = false; > > So this function modifies a port_dev value, AND returns it to the > caller? Why not document that somewhere? Why do both? > > And if quick_init is false then it always set ignore_connect to false > and returns false always? As mentioned above, the goal is to limit the number of retries if quick_init is set and prevent all connection attempts once one has failed, without changing anything if quick_init is not set. > > > + > > + return port_dev->ignore_connect; > > +} > > + > > /* Check if a port is power on */ > > int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus) > > { > > @@ -4855,6 +4878,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, > > buf->bMaxPacketSize0; > > kfree(buf); > > > > + if (r < 0 && port_dev->quick_init) { > > + retval = r; > > + goto fail; I still think you should test hub_port_stop_enumeration() here. It doesn't make sense that you allow a few retries for other types of errors but not for this one. Alan Stern > > + } > > + > > retval = hub_port_reset(hub, port1, udev, delay, false); > > if (retval < 0) /* error or disconnect */ > > goto fail; > > @@ -5387,6 +5415,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"); > > @@ -5614,6 +5645,9 @@ static void port_event(struct usb_hub *hub, int port1) > > if (!pm_runtime_active(&port_dev->dev)) > > return; > > > > + if (hub_port_stop_enumerate(hub, port1, 0)) > > + return; > > + > > if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange)) > > connect_change = 1; > > > > @@ -5934,6 +5968,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 +6059,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/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > > index b2925856b4cb..57995ec3af58 100644 > > --- a/drivers/usb/core/hub.h > > +++ b/drivers/usb/core/hub.h > > @@ -90,6 +90,8 @@ struct usb_hub { > > * @is_superspeed cache super-speed status > > * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted. > > * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted. > > + * @ignore_connect: ignore the connection or not. > > Which connection? Any future connection? Or the one that is currently > connected? > > > + * @quick_init: limit the numbers of retries for port initialization > > Limit it to what? > > > */ > > struct usb_port { > > struct usb_device *child; > > @@ -103,6 +105,8 @@ struct usb_port { > > u32 over_current_count; > > u8 portnum; > > u32 quirks; > > + bool ignore_connect; > > + bool quick_init; > > Why are these bool, when: > > > unsigned int is_superspeed:1; > > unsigned int usb3_lpm_u1_permit:1; > > unsigned int usb3_lpm_u2_permit:1; > > Those are all bits? > > Be consistent please. > > thanks, > > greg k-h
On Thu, Sep 22, 2022 at 11:15 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > Sorry I forgot about this patch. > > On Thu, Sep 22, 2022 at 02:06:51PM +0200, Greg KH wrote: > > On Thu, Sep 08, 2022 at 06:40:19PM +0800, Ray Chi wrote: > > > When a broken USB accessory connects to a USB host, usbcore might > > > keep doing enumeration retries. If the host has a watchdog mechanism, > > > the kernel panic will happen on the host. > > > > > > The patch provide a attribute to limit the numbers of retries and > > > timeout lengths. > > > > This is per-port, right? Please say that here. > > Also, the patch does not limit any timeout lengths -- just the number of > retries. Maybe you want to make it limit the timeouts also. I modified the changelog. > > > > In addition, it also adds a function to stop the > > > port initialization and ignore events on the port if the broken > > > accessory is still connected. > > > > So this should be 2 patches? Remember, only do one logical thing per > > patch please. > > The "adding a function" part is unimportant; we can see the function > simply by reading the patch. The main points here, which you have not > explained clearly, are: > > 1. If a port is marked with the quick_init attribute, unsuccessful > connection attempts will fail quickly. > > 2. Once a quick_init port has failed to initialize, it will ignore > all future connections until the quick_init attribute is turned > off. > The changelog is modified. > > > Signed-off-by: Ray Chi <raychi@google.com> > > > --- > > > Changes since v2: > > > - replace the quirk with the attribute > > > - Document the attribute > > > - modify hub_port_stop_enumerate() position in port_event() > > > - modify the changelog > > > > > > Changes since v1: > > > - remove usb_hub_set_port_power() > > > - add a variable ignore_connect into struct port_dev > > > - modify hub_port_stop_enumerate() and set ignore_connect in > > > this function > > > - avoid calling hub_port_connect_change() in port_event() > > > --- > > > Documentation/ABI/testing/sysfs-bus-usb | 9 ++++++ > > > drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++ > > > drivers/usb/core/hub.h | 4 +++ > > > drivers/usb/core/port.c | 23 +++++++++++++++ > > > 4 files changed, 75 insertions(+) > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb > > > index 568103d3376e..d44c8aaef929 100644 > > > --- a/Documentation/ABI/testing/sysfs-bus-usb > > > +++ b/Documentation/ABI/testing/sysfs-bus-usb > > > @@ -264,6 +264,15 @@ Description: > > > attached to the port will not be detected, initialized, > > > or enumerated. > > > > > > +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init > > > +Date: Sep 2022 > > > +Contact: Ray Chi <raychi@google.com> > > > +Description: > > > + Some USB hosts have some watchdog mechanisms so that the device > > > + may enter ramdump if it takes a long time during port initialization. > > > + This attribute limits the numbers of retries and timeout lengths once > > > + an initialization of the port meets failures. > > > > The text here is very odd, maybe it's just a translation issue. A port > > can not "meet" a failure. And this really has nothing to do with any > > watchdog, it's just a "we want to fail broken devices quickly instead of > > giving them lots of time to connect" type of issue, right? > > > > And we already have different init schemes, "quick" is an odd way to > > phrase this, but I can't think of anything else at the moment. > > "fail-fast" gives the idea better, but it's more awkward. > The documentation is modified and I rename the sysfs node from quick_init to early_stop. I think it would be better. > > > + > > > What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout > > > Date: May 2013 > > > Contact: Mathias Nyman <mathias.nyman@linux.intel.com> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > > index d4b1e70d1498..f22caa133274 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -3081,6 +3081,29 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > > > return status; > > > } > > > > > > +/* Check whether a hub would stop enumeration or ignore events on the port. */ > > > > Based on what? Retries? Time of day? Phase of moon? :) > > I added more comments to explain the details. > > > +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) > > > +{ > > > + struct usb_port *port_dev = hub->ports[port1 - 1]; > > > + > > > + if (port_dev->quick_init) { > > > + if (port_dev->ignore_connect) > > > + return true; > > > + > > > + /* > > > + * Since some normal devices will be timeout in the first attempt, > > > + * set the condition to half of the retries > > > + */ > > > + if (retries < (PORT_INIT_TRIES - 1) / 2) > > > + return false; > > This is not a good test. You shouldn't rely on PORT_INIT_TRIES having > any particular value or being used in any particular way. You should > just something like: > > /* > * We want unsuccessful attempts to fail quickly. > * Therefore allow three tries but no more. > */ > if (retries < 3) > It is modified. > > > + > > > + port_dev->ignore_connect = true; > > > + } else > > > + port_dev->ignore_connect = false; > > > > So this function modifies a port_dev value, AND returns it to the > > caller? Why not document that somewhere? Why do both? > > I add the comments to explain the details. > > And if quick_init is false then it always set ignore_connect to false > > and returns false always? > > As mentioned above, the goal is to limit the number of retries if > quick_init is set and prevent all connection attempts once one has > failed, without changing anything if quick_init is not set. > > > > > > + > > > + return port_dev->ignore_connect; > > > +} > > > + > > > /* Check if a port is power on */ > > > int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus) > > > { > > > @@ -4855,6 +4878,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, > > > buf->bMaxPacketSize0; > > > kfree(buf); > > > > > > + if (r < 0 && port_dev->quick_init) { > > > + retval = r; > > > + goto fail; > > I still think you should test hub_port_stop_enumeration() here. It > doesn't make sense that you allow a few retries for other types of > errors but not for this one. > > Alan Stern > It is modified. > > > + } > > > + > > > retval = hub_port_reset(hub, port1, udev, delay, false); > > > if (retval < 0) /* error or disconnect */ > > > goto fail; > > > @@ -5387,6 +5415,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"); > > > @@ -5614,6 +5645,9 @@ static void port_event(struct usb_hub *hub, int port1) > > > if (!pm_runtime_active(&port_dev->dev)) > > > return; > > > > > > + if (hub_port_stop_enumerate(hub, port1, 0)) > > > + return; > > > + > > > if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange)) > > > connect_change = 1; > > > > > > @@ -5934,6 +5968,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 +6059,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/drivers/usb/core/hub.h b/drivers/usb/core/hub.h > > > index b2925856b4cb..57995ec3af58 100644 > > > --- a/drivers/usb/core/hub.h > > > +++ b/drivers/usb/core/hub.h > > > @@ -90,6 +90,8 @@ struct usb_hub { > > > * @is_superspeed cache super-speed status > > > * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted. > > > * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted. > > > + * @ignore_connect: ignore the connection or not. > > > > Which connection? Any future connection? Or the one that is currently > > connected? > > > > > + * @quick_init: limit the numbers of retries for port initialization > > > > Limit it to what? > > > > > */ > > > struct usb_port { > > > struct usb_device *child; > > > @@ -103,6 +105,8 @@ struct usb_port { > > > u32 over_current_count; > > > u8 portnum; > > > u32 quirks; > > > + bool ignore_connect; > > > + bool quick_init; > > > > Why are these bool, when: > > > > > unsigned int is_superspeed:1; > > > unsigned int usb3_lpm_u1_permit:1; > > > unsigned int usb3_lpm_u2_permit:1; > > > > Those are all bits? > > > > Be consistent please. > > > > thanks, > > > > greg k-h BTW, I found a new mail tree is created after the patch v4 is uploaded, but the patch is downloaded by mbox. What do I do if I want to keep the original mail tree? Thanks, Ray
On Wed, Sep 28, 2022 at 12:37:55AM +0800, Ray Chi wrote: > BTW, I found a new mail tree is created after the patch v4 is > uploaded, but the patch is downloaded by mbox. I do not understand what you mean by this. > What do I do if I want to keep the original mail tree? Why would you want to do that? You can keep the threading if you use the correct option to git-send-email but you really don't want to do that. thanks, greg k-h
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb index 568103d3376e..d44c8aaef929 100644 --- a/Documentation/ABI/testing/sysfs-bus-usb +++ b/Documentation/ABI/testing/sysfs-bus-usb @@ -264,6 +264,15 @@ Description: attached to the port will not be detected, initialized, or enumerated. +What: /sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init +Date: Sep 2022 +Contact: Ray Chi <raychi@google.com> +Description: + Some USB hosts have some watchdog mechanisms so that the device + may enter ramdump if it takes a long time during port initialization. + This attribute limits the numbers of retries and timeout lengths once + an initialization of the port meets failures. + What: /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout Date: May 2013 Contact: Mathias Nyman <mathias.nyman@linux.intel.com> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index d4b1e70d1498..f22caa133274 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -3081,6 +3081,29 @@ static int hub_port_reset(struct usb_hub *hub, int port1, return status; } +/* Check whether a hub would stop enumeration or ignore events on the port. */ +static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries) +{ + struct usb_port *port_dev = hub->ports[port1 - 1]; + + if (port_dev->quick_init) { + if (port_dev->ignore_connect) + return true; + + /* + * Since some normal devices will be timeout in the first attempt, + * set the condition to half of the retries + */ + if (retries < (PORT_INIT_TRIES - 1) / 2) + return false; + + port_dev->ignore_connect = true; + } else + port_dev->ignore_connect = false; + + return port_dev->ignore_connect; +} + /* Check if a port is power on */ int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus) { @@ -4855,6 +4878,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, buf->bMaxPacketSize0; kfree(buf); + if (r < 0 && port_dev->quick_init) { + retval = r; + goto fail; + } + retval = hub_port_reset(hub, port1, udev, delay, false); if (retval < 0) /* error or disconnect */ goto fail; @@ -5387,6 +5415,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"); @@ -5614,6 +5645,9 @@ static void port_event(struct usb_hub *hub, int port1) if (!pm_runtime_active(&port_dev->dev)) return; + if (hub_port_stop_enumerate(hub, port1, 0)) + return; + if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange)) connect_change = 1; @@ -5934,6 +5968,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 +6059,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/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index b2925856b4cb..57995ec3af58 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -90,6 +90,8 @@ struct usb_hub { * @is_superspeed cache super-speed status * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted. * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted. + * @ignore_connect: ignore the connection or not. + * @quick_init: limit the numbers of retries for port initialization */ struct usb_port { struct usb_device *child; @@ -103,6 +105,8 @@ struct usb_port { u32 over_current_count; u8 portnum; u32 quirks; + bool ignore_connect; + bool quick_init; unsigned int is_superspeed:1; unsigned int usb3_lpm_u1_permit:1; unsigned int usb3_lpm_u2_permit:1; diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 38c1a4f4fdea..7d5acd3ff353 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -17,6 +17,28 @@ static int usb_port_block_power_off; static const struct attribute_group *port_dev_group[]; +static ssize_t quick_init_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct usb_port *port_dev = to_usb_port(dev); + + return sysfs_emit(buf, "%s\n", port_dev->quick_init ? "yes" : "no"); +} + +static ssize_t quick_init_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct usb_port *port_dev = to_usb_port(dev); + bool value; + + if (kstrtobool(buf, &value)) + return -EINVAL; + + port_dev->quick_init = value; + return count; +} +static DEVICE_ATTR_RW(quick_init); + static ssize_t disable_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -236,6 +258,7 @@ static struct attribute *port_dev_attrs[] = { &dev_attr_quirks.attr, &dev_attr_over_current_count.attr, &dev_attr_disable.attr, + &dev_attr_quick_init.attr, NULL, };
When a broken USB accessory connects to a USB host, usbcore might keep doing enumeration retries. If the host has a watchdog mechanism, the kernel panic will happen on the host. The patch provide a attribute to limit the numbers of retries and timeout lengths. In addition, it also adds a function to stop the port initialization and ignore events on the port if the broken accessory is still connected. Signed-off-by: Ray Chi <raychi@google.com> --- Changes since v2: - replace the quirk with the attribute - Document the attribute - modify hub_port_stop_enumerate() position in port_event() - modify the changelog Changes since v1: - remove usb_hub_set_port_power() - add a variable ignore_connect into struct port_dev - modify hub_port_stop_enumerate() and set ignore_connect in this function - avoid calling hub_port_connect_change() in port_event() --- Documentation/ABI/testing/sysfs-bus-usb | 9 ++++++ drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++ drivers/usb/core/hub.h | 4 +++ drivers/usb/core/port.c | 23 +++++++++++++++ 4 files changed, 75 insertions(+)