Skip to contentSkip to conversation

usb: core: stop USB enumeration if too many retries

PATCH RESEND Superseded Ray Chi 2 participants 2 messages Oct 28, 2022 USB

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Ray Chi

author
#0 lore raw

PATCH RESEND v4 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.

This patch provides an attribute early_stop to limit the numbers of retries for each port of a hub. If a port was marked with early_stop attribute, unsuccessful connection attempts will fail quickly. In addition, if an early_stop port has failed to initialize, it will ignore all future connection events until early_stop attribute is clear.

Signed-off-by Ray Chi <[email protected]>

---

Changes since v3:
- rename the attribute from quick_init to early_stop
- rename the variable of port_dev from ignore_connect to ignore_event
- modify changelog, documentation
- add more comments for hub_port_stop_enumerate()

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 | 11 +++++
 drivers/usb/core/hub.c                  | 59 +++++++++++++++++++++++++
 drivers/usb/core/hub.h                  |  4 ++
 drivers/usb/core/port.c                 | 27 +++++++++++
 4 files changed, 101 insertions(+)
Documentation/ABI/testing/sysfs-bus-usb +11 -0
index 568103d3376e..545c2dd97ed0 100644
@@ -264,6 +264,17 @@ Description:
 		attached to the port will not be detected, initialized,
 		or enumerated.
 
+What:		/sys/bus/usb/devices/.../<hub_interface>/port<X>/early_stop
+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 allows each port just has two attempts so that the
+		port initialization will be failed quickly. In addition, if a port
+		which is marked with early_stop has failed to initialize, it will ignore
+		all future connections until this attribute is clear.
+
 What:		/sys/bus/usb/devices/.../power/usb2_lpm_l1_timeout
 Date:		May 2013
 Contact:	Mathias Nyman <[email protected]>
drivers/usb/core/hub.c +59 -0

95 lines not on this page · raw message

drivers/usb/core/hub.h +4 -0
index b2925856b4cb..e23833562e4f 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.
+ * @early_stop: whether port initialization will be stopped earlier.
+ * @ignore_event: whether events of the port are ignored.
  */
 struct usb_port {
 	struct usb_device *child;

@@ -103,6 +105,8 @@ struct usb_port {
 	u32 over_current_count;
 	u8 portnum;
 	u32 quirks;
+	unsigned int early_stop:1;
+	unsigned int ignore_event:1;
 	unsigned int is_superspeed:1;
 	unsigned int usb3_lpm_u1_permit:1;
 	unsigned int usb3_lpm_u2_permit:1;
drivers/usb/core/port.c +27 -0
index 38c1a4f4fdea..126da9408359 100644
@@ -17,6 +17,32 @@ static int usb_port_block_power_off;
 
 static const struct attribute_group *port_dev_group[];
 
+static ssize_t early_stop_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->early_stop ? "yes" : "no");
+}
+
+static ssize_t early_stop_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;
+
+	if (value)
+		port_dev->early_stop = 1;
+	else
+		port_dev->early_stop = 0;
+
+	return count;
+}
+static DEVICE_ATTR_RW(early_stop);
+
 static ssize_t disable_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {

@@ -236,6 +262,7 @@ static struct attribute *port_dev_attrs[] = {
 	&dev_attr_quirks.attr,
 	&dev_attr_over_current_count.attr,
 	&dev_attr_disable.attr,
+	&dev_attr_early_stop.attr,
 	NULL,
 };
Signature · 1 line
2.38.1.273.g43a17bfeac-goog
#1 lore raw
Quoted 11 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.

This patch provides an attribute early_stop to limit the numbers of retries for each port of a hub. If a port was marked with early_stop attribute, unsuccessful connection attempts will fail quickly. In addition, if an early_stop port has failed to initialize, it will ignore all future connection events until early_stop attribute is clear.

Signed-off-by Ray Chi <[email protected]>

This seems basically okay, but I would have put the tests right at the starts of the retry loops. This will make the logic much more obviously correct.

Quoted 39 lines
Documentation/ABI/testing/sysfs-bus-usb +0 -0
index 568103d3376e..545c2dd97ed0 100644
@@ -264,6 +264,17 @@ Description:
 		attached to the port will not be detected, initialized,
 		or enumerated.
drivers/usb/core/hub.c +23 -0
index bbab424b0d55..5510dbef3243 100644
@@ -3081,6 +3081,48 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
 	return status;
 }
 
+/*
+ * hub_port_stop_enumerate - stop USB enumeration or ignore port events
+ * @hub: target hub
+ * @port1: port num of the port
+ * @retries: port retries number of hub_port_init()
+ *
+ * Return:
+ *    true: ignore port actions/events or give up connection attempts.
+ *    false: keep original behavior.
+ *
+ * This function will be based on retries to check whether the port which is
+ * marked with early_stop attribute would stop enumeration or ignore events.
+ *
+ * Note:
+ * This function didn't change anything if early_stop is not set, and it will
+ * prevent all connection attempts when early_stop is set and the attempts of
+ * the port are more than 1.
+ */
+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->early_stop) {

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

Quoted 10 lines
drivers/usb/core/hub.c +10 -0
+		if (port_dev->ignore_event)
+			return true;
+
+		/*
+		 * We want unsuccessful attempts to fail quickly.
+		 * Since some devices may need one failure during
+		 * port initialization, we allow two tries but no
+		 * more.
+		 */
+		if (retries < 1)

Change "< 1" to "< 2". This is in better agreement with the comment, which will make it easier for readers to understand.

Quoted 13 lines
drivers/usb/core/hub.c +9 -0
+			return false;
+
+		port_dev->ignore_event = 1;
+	} else
+		port_dev->ignore_event = 0;
+
+	return port_dev->ignore_event;
+}
+
 /* Check if a port is power on */
 int usb_port_is_power_on(struct usb_hub *hub, unsigned int portstatus)
 {

drivers/usb/core/hub.c hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, · line 4903

Quoted 7 lines
drivers/usb/core/hub.c +4 -0
@@ -4855,6 +4897,11 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 					buf->bMaxPacketSize0;
 			kfree(buf);
 
+			if (r < 0 && hub_port_stop_enumerate(hub, port1, retries)) {
+				retval = r;
+				goto fail;
+			}

Then instead of putting this here, put:

			if (hub_port_stop_enumerate(hub, port1, retries)) {
				retval = -ENODEV;
				break;
			}

at line 4799.

drivers/usb/core/hub.c static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, · line 5438

Quoted 10 lines
drivers/usb/core/hub.c +3 -0
+
 			retval = hub_port_reset(hub, port1, udev, delay, false);
 			if (retval < 0)		/* error or disconnect */
 				goto fail;

@@ -5387,6 +5434,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;

And instead of putting this here, at line 5249 put:

		if (hub_port_stop_enumerate(hub, port1, i)) {
			status = -ENODEV;
			break;
		}

drivers/usb/core/hub.c static void port_event(struct usb_hub *hub, int port1) · line 5669

Quoted 11 lines
drivers/usb/core/hub.c +4 -0
+
 		/* 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 +5664,10 @@ static void port_event(struct usb_hub *hub, int port1)
 	if (!pm_runtime_active(&port_dev->dev))
 		return;
 
+	/* skip port actions if ignore_event is true*/
+	if (hub_port_stop_enumerate(hub, port1, 0))
+		return;

The relation between the comment and the code isn't obvious. Instead, do:

	/* Skip initialization and enumeration if ignore_event is true */
	if (port_dev->ignore_event)
		return;

(Notice the space character you left out at the end of the comment.)

drivers/usb/core/hub.c static int usb_reset_and_verify_device(struct usb_device *udev) · line 5993

Quoted 11 lines
drivers/usb/core/hub.c +4 -0
+
 	if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
 		connect_change = 1;
 

@@ -5934,6 +5988,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;

And instead of putting this here, at line 5930 put:

		if (hub_port_stop_enumerate(parent_hub, port1, i)) {
			ret = -ENODEV;
			break;
		}

Alan Stern