usb: core: stop USB enumeration if too many retries

PATCH Superseded Ray Chi 3 participants 5 messages Sep 8, 2022 – Sep 28, 2022 USB

Share & tools

Ray Chi

author

2022-09-08 10:41 UTC #0 lore raw

PATCH v3 1/1 usb: core: stop USB enumeration if too many retries

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 <[email protected]>

---
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(+)

Documentation/ABI/testing/sysfs-bus-usb +9 -0

index 568103d3376e..d44c8aaef929 100644
@@ -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 <[email protected]>
+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 <[email protected]>

drivers/usb/core/hub.c +39 -0

index d4b1e70d1498..f22caa133274 100644
@@ -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;

drivers/usb/core/hub.h +4 -0

index b2925856b4cb..57995ec3af58 100644
@@ -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;

drivers/usb/core/port.c +23 -0

index 38c1a4f4fdea..7d5acd3ff353 100644
@@ -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,
 };

Signature · 1 line

2.37.2.789.g6183377224-goog

Greg KH

maintainer

2022-09-22 12:07 UTC #1 lore raw

Quoted 6 lines · Ray Chi

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.

Quoted 3 lines

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.

Quoted 28 lines

Signed-off-by Ray Chi <[email protected]>
---
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(+)
Documentation/ABI/testing/sysfs-bus-usb +0 -0
index 568103d3376e..d44c8aaef929 100644
@@ -264,6 +264,15 @@ Description:
 		attached to the port will not be detected, initialized,

Documentation/ABI/testing/sysfs-bus-usb Description: · line 274

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-usb +8 -0

 		or enumerated.
 
+What:		/sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init
+Date:		Sep 2022
+Contact:	Ray Chi <[email protected]>
+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.

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3084

Quoted 13 lines

Documentation/ABI/testing/sysfs-bus-usb +1 -0

+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <[email protected]>

drivers/usb/core/hub.c +1 -0

index d4b1e70d1498..f22caa133274 100644
@@ -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? :)

Quoted 8 lines

drivers/usb/core/hub.c +8 -0

+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;
+

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3102

Quoted 10 lines

drivers/usb/core/hub.c +10 -0

+		/*
+		 * 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?

Quoted 63 lines

drivers/usb/core/hub.c +20 -0

+
+	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;

drivers/usb/core/hub.h +0 -0

index b2925856b4cb..57995ec3af58 100644

drivers/usb/core/hub.h struct usb_hub { · line 93

Quoted 4 lines

drivers/usb/core/hub.h +1 -0

@@ -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?

drivers/usb/core/hub.h struct usb_hub { · line 94

Quoted 1 line

drivers/usb/core/hub.h +1 -0

+ * @quick_init: limit the numbers of retries for port initialization

Limit it to what?

drivers/usb/core/hub.h struct usb_port { · line 109

Quoted 9 lines

drivers/usb/core/hub.h +2 -0

  */
 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:

drivers/usb/core/hub.h struct usb_port { · line 112

Quoted 3 lines

drivers/usb/core/hub.h +0 -0

 	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

Alan Stern

2022-09-22 15:15 UTC #2 lore raw

Sorry I forgot about this patch.

Quoted 9 lines · Greg KH

Quoted 6 lines · Ray Chi
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 43 lines

Quoted 27 lines
Signed-off-by Ray Chi <[email protected]>
---
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(+)
Documentation/ABI/testing/sysfs-bus-usb +0 -0
index 568103d3376e..d44c8aaef929 100644
@@ -264,6 +264,15 @@ Description:
 		attached to the port will not be detected, initialized,
Documentation/ABI/testing/sysfs-bus-usb Description: · line 274
Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-usb +8 -0
 		or enumerated.
 
+What:		/sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init
+Date:		Sep 2022
+Contact:	Ray Chi <[email protected]>
+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?

Quoted 2 lines

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.

Quoted 16 lines

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3084
Quoted 13 lines
Documentation/ABI/testing/sysfs-bus-usb +1 -0
+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <[email protected]>
drivers/usb/core/hub.c +1 -0
index d4b1e70d1498..f22caa133274 100644
@@ -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? :)

Quoted 14 lines

Quoted 4 lines

drivers/usb/core/hub.c +4 -0

+static bool hub_port_stop_enumerate(struct usb_hub *hub, int port1, int retries)
+{
+	struct usb_port *port_dev = hub->ports[port1 - 1];
+

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3098

Quoted 10 lines

drivers/usb/core/hub.c +10 -0

+	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)

Quoted 10 lines

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3102
Quoted 4 lines
drivers/usb/core/hub.c +4 -0
+
+		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.

Quoted 12 lines

drivers/usb/core/hub.c +4 -0

+
+	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);

Quoted 3 lines

drivers/usb/core/hub.c hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, · line 4883
Quoted 3 lines
drivers/usb/core/hub.c +3 -0
+			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

Quoted 84 lines

Quoted 49 lines

drivers/usb/core/hub.c +13 -0

+			}
+
 			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;

drivers/usb/core/hub.h +0 -0

index b2925856b4cb..57995ec3af58 100644

drivers/usb/core/hub.h struct usb_hub { · line 93

Quoted 4 lines

drivers/usb/core/hub.h +1 -0

@@ -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?

drivers/usb/core/hub.h struct usb_hub { · line 94

Quoted 1 line

drivers/usb/core/hub.h +1 -0

+ * @quick_init: limit the numbers of retries for port initialization

Limit it to what?

drivers/usb/core/hub.h struct usb_port { · line 109

Quoted 9 lines

drivers/usb/core/hub.h +2 -0

  */
 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:

drivers/usb/core/hub.h struct usb_port { · line 112

Quoted 3 lines

drivers/usb/core/hub.h +0 -0

 	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

Ray Chi

author

2022-09-27 16:38 UTC #3 lore raw

Quoted 9 lines · Alan Stern

Sorry I forgot about this patch.
Quoted 5 lines · Greg KH
Quoted 4 lines · Ray Chi
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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.

Quoted 12 lines

Quoted 7 lines
Quoted 4 lines
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:

Quoted 7 lines

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.

Quoted 43 lines

Quoted 43 lines
Quoted 27 lines
Signed-off-by Ray Chi <[email protected]>
---
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(+)
Documentation/ABI/testing/sysfs-bus-usb +0 -0
index 568103d3376e..d44c8aaef929 100644
@@ -264,6 +264,15 @@ Description:
            attached to the port will not be detected, initialized,
Documentation/ABI/testing/sysfs-bus-usb Description: · line 274
Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-usb +8 -0
            or enumerated.
 
+What:              /sys/bus/usb/devices/.../<hub_interface>/port<X>/quick_init
+Date:              Sep 2022
+Contact:   Ray Chi <[email protected]>
+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?

Quoted 5 lines

Quoted 2 lines
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.

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-usb +1 -0

+
 What:              /sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:              May 2013
 Contact:   Mathias Nyman <[email protected]>

drivers/usb/core/hub.c +0 -0

index d4b1e70d1498..f22caa133274 100644
@@ -3081,6 +3081,29 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
    return status;
 }

Quoted 4 lines

Quoted 4 lines
drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3084
Quoted 1 line
drivers/usb/core/hub.c +1 -0
+/* 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.

Quoted 19 lines

Quoted 6 lines

drivers/usb/core/hub.c +6 -0

+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)

Quoted 8 lines

drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3098

Quoted 8 lines

drivers/usb/core/hub.c +8 -0

+                   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:

Quoted 6 lines

/*
* We want unsuccessful attempts to fail quickly.
* Therefore allow three tries but no more. */
if (retries < 3)

It is modified.

Quoted 8 lines

Quoted 8 lines
drivers/usb/core/hub.c static int hub_port_reset(struct usb_hub *hub, int port1, · line 3102
Quoted 4 lines
drivers/usb/core/hub.c +4 -0
+
+           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.

Quoted 27 lines

Quoted 2 lines
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.
Quoted 12 lines
Quoted 12 lines
drivers/usb/core/hub.c +4 -0
+
+   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);
 
Quoted 3 lines
drivers/usb/core/hub.c hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, · line 4883
Quoted 3 lines
drivers/usb/core/hub.c +3 -0
+                   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.

Quoted 2 lines

Alan Stern

It is modified.

Quoted 77 lines

Quoted 49 lines

drivers/usb/core/hub.c +13 -0

+                   }
+
                    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;

drivers/usb/core/hub.h +0 -0

index b2925856b4cb..57995ec3af58 100644

drivers/usb/core/hub.h struct usb_hub { · line 93

Quoted 4 lines

drivers/usb/core/hub.h +1 -0

@@ -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?

drivers/usb/core/hub.h struct usb_hub { · line 94

Quoted 1 line

drivers/usb/core/hub.h +1 -0

+ * @quick_init: limit the numbers of retries for port initialization

Limit it to what?

drivers/usb/core/hub.h struct usb_port { · line 109

Quoted 9 lines

drivers/usb/core/hub.h +2 -0

  */
 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:

Quoted 3 lines

drivers/usb/core/hub.h +0 -0

    unsigned int is_superspeed:1;
    unsigned int usb3_lpm_u1_permit:1;
    unsigned int usb3_lpm_u2_permit:1;

Quoted 7 lines

Quoted 7 lines
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

Greg KH

maintainer

2022-09-28 06:57 UTC #4 lore raw

Quoted 2 lines · Ray Chi

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.

Quoted 1 line

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