Hi,
I hope it all fits now.
David
>From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
From: David Heinzelmann <heinzelmann.david@xxxxxxxxx>
Date: Fri, 4 Oct 2019 12:28:36 +0200
Subject: [PATCH v4] usb: hub: Check device descriptor before resusciation
If a device connected to an xHCI host controller disconnects from the USB bus
and then reconnects, e.g. triggered by a firmware update, then the host
controller automatically activates the connection and the port is enabled. The
implementation of hub_port_connect_change() assumes that if the port is
enabled then nothing has changed. There is no check if the USB descriptors
have changed. As a result, the kernel's internal copy of the descriptors ends
up being incorrect and the device doesn't work properly anymore.
The solution to the problem is for hub_port_connect_change() always to
check whether the device's descriptors have changed before resuscitating
an enabled port.
Signed-off-by: David Heinzelmann <heinzelmann.david@xxxxxxxxx>
---
Changes in v4:
- changed commit description
Changes in v3:
- changed commit message and description
- fix code style
Changes in v2:
- fix logic error to handle return code from usb_get_device_descriptor()
properly
- fix line endings
---
drivers/usb/core/hub.c | 196 +++++++++++++++++++++++------------------
1 file changed, 111 insertions(+), 85 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 236313f41f4a..fdcfa85b5b12 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4930,6 +4930,91 @@ hub_power_remaining(struct usb_hub *hub)
return remaining;
}
+
+static int descriptors_changed(struct usb_device *udev,
+ struct usb_device_descriptor *old_device_descriptor,
+ struct usb_host_bos *old_bos)
+{
+ int changed = 0;
+ unsigned index;
+ unsigned serial_len = 0;
+ unsigned len;
+ unsigned old_length;
+ int length;
+ char *buf;
+
+ if (memcmp(&udev->descriptor, old_device_descriptor,
+ sizeof(*old_device_descriptor)) != 0)
+ return 1;
+
+ if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
+ return 1;
+ if (udev->bos) {
+ len = le16_to_cpu(udev->bos->desc->wTotalLength);
+ if (len != le16_to_cpu(old_bos->desc->wTotalLength))
+ return 1;
+ if (memcmp(udev->bos->desc, old_bos->desc, len))
+ return 1;
+ }
+
+ /* Since the idVendor, idProduct, and bcdDevice values in the
+ * device descriptor haven't changed, we will assume the
+ * Manufacturer and Product strings haven't changed either.
+ * But the SerialNumber string could be different (e.g., a
+ * different flash card of the same brand).
+ */
+ if (udev->serial)
+ serial_len = strlen(udev->serial) + 1;
+
+ len = serial_len;
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ len = max(len, old_length);
+ }
+
+ buf = kmalloc(len, GFP_NOIO);
+ if (!buf)
+ /* assume the worst */
+ return 1;
+
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
+ old_length);
+ if (length != old_length) {
+ dev_dbg(&udev->dev, "config index %d, error %d\n",
+ index, length);
+ changed = 1;
+ break;
+ }
+ if (memcmp(buf, udev->rawdescriptors[index], old_length)
+ != 0) {
+ dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
+ index,
+ ((struct usb_config_descriptor *) buf)->
+ bConfigurationValue);
+ changed = 1;
+ break;
+ }
+ }
+
+ if (!changed && serial_len) {
+ length = usb_string(udev, udev->descriptor.iSerialNumber,
+ buf, serial_len);
+ if (length + 1 != serial_len) {
+ dev_dbg(&udev->dev, "serial string error %d\n",
+ length);
+ changed = 1;
+ } else if (memcmp(buf, udev->serial, length) != 0) {
+ dev_dbg(&udev->dev, "serial string changed\n");
+ changed = 1;
+ }
+ }
+
+ kfree(buf);
+ return changed;
+}
+
static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
u16 portchange)
{
@@ -5167,7 +5252,9 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
{
struct usb_port *port_dev = hub->ports[port1 - 1];
struct usb_device *udev = port_dev->child;
+ struct usb_device_descriptor descriptor;
int status = -ENODEV;
+ int retval;
dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus,
portchange, portspeed(hub, portstatus));
@@ -5188,7 +5275,30 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
udev->state != USB_STATE_NOTATTACHED) {
if (portstatus & USB_PORT_STAT_ENABLE) {
- status = 0; /* Nothing to do */
+ /*
+ * USB-3 connections are initialized automatically by
+ * the hostcontroller hardware. Therefore check for
+ * changed device descriptors before resuscitating the
+ * device.
+ */
+ descriptor = udev->descriptor;
+ retval = usb_get_device_descriptor(udev,
+ sizeof(udev->descriptor));
+ if (retval < 0) {
+ dev_dbg(&udev->dev,
+ "can't read device descriptor %d\n",
+ retval);
+ } else {
+ if (descriptors_changed(udev, &descriptor,
+ udev->bos)) {
+ dev_dbg(&udev->dev,
+ "device descriptor has changed\n");
+ /* for disconnect() calls */
+ udev->descriptor = descriptor;
+ } else {
+ status = 0; /* Nothing to do */
+ }
+ }
#ifdef CONFIG_PM
} else if (udev->state == USB_STATE_SUSPENDED &&
udev->persist_enabled) {
@@ -5550,90 +5660,6 @@ void usb_hub_cleanup(void)
usb_deregister(&hub_driver);
} /* usb_hub_cleanup() */
-static int descriptors_changed(struct usb_device *udev,
- struct usb_device_descriptor *old_device_descriptor,
- struct usb_host_bos *old_bos)
-{
- int changed = 0;
- unsigned index;
- unsigned serial_len = 0;
- unsigned len;
- unsigned old_length;
- int length;
- char *buf;
-
- if (memcmp(&udev->descriptor, old_device_descriptor,
- sizeof(*old_device_descriptor)) != 0)
- return 1;
-
- if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
- return 1;
- if (udev->bos) {
- len = le16_to_cpu(udev->bos->desc->wTotalLength);
- if (len != le16_to_cpu(old_bos->desc->wTotalLength))
- return 1;
- if (memcmp(udev->bos->desc, old_bos->desc, len))
- return 1;
- }
-
- /* Since the idVendor, idProduct, and bcdDevice values in the
- * device descriptor haven't changed, we will assume the
- * Manufacturer and Product strings haven't changed either.
- * But the SerialNumber string could be different (e.g., a
- * different flash card of the same brand).
- */
- if (udev->serial)
- serial_len = strlen(udev->serial) + 1;
-
- len = serial_len;
- for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
- old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
- len = max(len, old_length);
- }
-
- buf = kmalloc(len, GFP_NOIO);
- if (!buf)
- /* assume the worst */
- return 1;
-
- for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
- old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
- length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
- old_length);
- if (length != old_length) {
- dev_dbg(&udev->dev, "config index %d, error %d\n",
- index, length);
- changed = 1;
- break;
- }
- if (memcmp(buf, udev->rawdescriptors[index], old_length)
- != 0) {
- dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
- index,
- ((struct usb_config_descriptor *) buf)->
- bConfigurationValue);
- changed = 1;
- break;
- }
- }
-
- if (!changed && serial_len) {
- length = usb_string(udev, udev->descriptor.iSerialNumber,
- buf, serial_len);
- if (length + 1 != serial_len) {
- dev_dbg(&udev->dev, "serial string error %d\n",
- length);
- changed = 1;
- } else if (memcmp(buf, udev->serial, length) != 0) {
- dev_dbg(&udev->dev, "serial string changed\n");
- changed = 1;
- }
- }
-
- kfree(buf);
- return changed;
-}
-
/**
* usb_reset_and_verify_device - perform a USB port reset to reinitialize a device
* @udev: device to reset (not in SUSPENDED or NOTATTACHED state)
--
2.20.1
