linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-20 10:36 [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection David Heinzelmann
@ 2019-09-20  8:55 ` Greg KH
  2019-09-20 13:17   ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2019-09-20  8:55 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: linux-usb

On Fri, Sep 20, 2019 at 12:36:28PM +0200, David Heinzelmann wrote:
> When a port connection-change occurs the hub driver tries to resuscitate an existing device.
> Activated from a firmware download a usb device can re-enumerate with new or changed device
> descriptors. Therefore it will be checked for changed device descriptors before the connection
> is resuscitated and the connection-change event is ignored.

Please wrap your lines at 72 columns :(

Anyway, what problem are you trying to solve here?  What is broken with
how things work today?  Are you trying to ignore a change that is
currently showing up as a change, or trying to do the opposite?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
@ 2019-09-20 10:36 David Heinzelmann
  2019-09-20  8:55 ` Greg KH
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-09-20 10:36 UTC (permalink / raw)
  To: linux-usb; +Cc: David Heinzelmann

When a port connection-change occurs the hub driver tries to resuscitate an existing device.
Activated from a firmware download a usb device can re-enumerate with new or changed device
descriptors. Therefore it will be checked for changed device descriptors before the connection
is resuscitated and the connection-change event is ignored.

Signed-off-by: David Heinzelmann <heinzelmann.david@gmail.com>
---
 drivers/usb/core/hub.c | 181 ++++++++++++++++++++++-------------------
 1 file changed, 96 insertions(+), 85 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 236313f41f4a..a7c0ada6eec4 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,6 +5252,7 @@ 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;
 
 	dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus,
@@ -5188,7 +5274,16 @@ 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 */
+			/* Device might have changed firmware (DFU or similar) */
+			descriptor = udev->descriptor;
+			if (usb_get_device_descriptor(udev, sizeof(udev->descriptor)) < 0) {
+				if (descriptors_changed(udev, &descriptor, udev->bos)) {
+					dev_dbg(&udev->dev, "device descriptor has changed\n");
+					udev->descriptor = descriptor;	/* for disconnect() calls */
+				} else {
+					status = 0; /* Nothing to do */
+				}
+			}
 #ifdef CONFIG_PM
 		} else if (udev->state == USB_STATE_SUSPENDED &&
 				udev->persist_enabled) {
@@ -5550,90 +5645,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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-20 13:17   ` David Heinzelmann
@ 2019-09-20 12:15     ` Greg KH
  2019-09-20 15:33       ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2019-09-20 12:15 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: linux-usb

On Fri, Sep 20, 2019 at 03:17:26PM +0200, David Heinzelmann wrote:
> Hi,
> 
> sorry for the wrong patch format.

No problem, that's normal.  But please do not top-post on linux mailing
lists.

> I am trying to detect a change. At the moment I think the change could be ignored if a
> port connection-change occurs and the port status has again the 'PORT_CONNECTION' bit set. 
> 
> I have a fx3 device which does a re-enumeration after a firmware download. This is working
> as expected and I am seeing a 'remove event' and a 'add event' monitoring via udevadm. But
> if I connect multiple devices at the same time via an usb hub I am sometimes not receiving
> a 'remove event' and 'add event' for a single device.

Sounds like a broken hub :)

> I think the problem could be that when a device disconnects and the port connection-change
> occurs and before the 'PORT_CONNECTION' bit is checked the device could already be
> reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not correct to
> resuscitate the exisiting device.

Does your patch actually fix the issue?  When a fx3 device downloads
firmware and re-enumerates, it should come back as a totally different
device, which will fail this check, right?  So I don't see how this
fixes the issues with your devices.

Unless all of the devices reset themselves at the same time and the hub
doesn't like that and can't notice that it happened?

If you use a different hub, does that work properly?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-20  8:55 ` Greg KH
@ 2019-09-20 13:17   ` David Heinzelmann
  2019-09-20 12:15     ` Greg KH
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-09-20 13:17 UTC (permalink / raw)
  To: Greg KH; +Cc: David Heinzelmann, linux-usb

Hi,

sorry for the wrong patch format.

I am trying to detect a change. At the moment I think the change could be ignored if a
port connection-change occurs and the port status has again the 'PORT_CONNECTION' bit set. 

I have a fx3 device which does a re-enumeration after a firmware download. This is working
as expected and I am seeing a 'remove event' and a 'add event' monitoring via udevadm. But
if I connect multiple devices at the same time via an usb hub I am sometimes not receiving
a 'remove event' and 'add event' for a single device.

I think the problem could be that when a device disconnects and the port connection-change
occurs and before the 'PORT_CONNECTION' bit is checked the device could already be
reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not correct to
resuscitate the exisiting device.

On Fri, Sep 20, 2019 at 10:55:56AM +0200, Greg KH wrote:
> On Fri, Sep 20, 2019 at 12:36:28PM +0200, David Heinzelmann wrote:
> > When a port connection-change occurs the hub driver tries to resuscitate an existing device.
> > Activated from a firmware download a usb device can re-enumerate with new or changed device
> > descriptors. Therefore it will be checked for changed device descriptors before the connection
> > is resuscitated and the connection-change event is ignored.
> 
> Please wrap your lines at 72 columns :(
> 
> Anyway, what problem are you trying to solve here?  What is broken with
> how things work today?  Are you trying to ignore a change that is
> currently showing up as a change, or trying to do the opposite?
> 
> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-20 12:15     ` Greg KH
@ 2019-09-20 15:33       ` David Heinzelmann
  2019-09-23 14:49         ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-09-20 15:33 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-usb

On Fri, Sep 20, 2019 at 02:15:38PM +0200, Greg KH wrote:
> On Fri, Sep 20, 2019 at 03:17:26PM +0200, David Heinzelmann wrote:
> > Hi,
> > 
> > sorry for the wrong patch format.
> 
> No problem, that's normal.  But please do not top-post on linux mailing
> lists.
> 
> > I am trying to detect a change. At the moment I think the change could be ignored if a
> > port connection-change occurs and the port status has again the 'PORT_CONNECTION' bit set. 
> > 
> > I have a fx3 device which does a re-enumeration after a firmware download. This is working
> > as expected and I am seeing a 'remove event' and a 'add event' monitoring via udevadm. But
> > if I connect multiple devices at the same time via an usb hub I am sometimes not receiving
> > a 'remove event' and 'add event' for a single device.
> 
> Sounds like a broken hub :)
> 

I tried different hubs but I forgot to mention that it is also possible to trigger the issue
without a hub if I reboot the devices via software at the same time. 

> > I think the problem could be that when a device disconnects and the port connection-change
> > occurs and before the 'PORT_CONNECTION' bit is checked the device could already be
> > reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not correct to
> > resuscitate the exisiting device.
> 
> Does your patch actually fix the issue?  When a fx3 device downloads
> firmware and re-enumerates, it should come back as a totally different
> device, which will fail this check, right?  So I don't see how this
> fixes the issues with your devices.
> 

With the patch I do not have the issue anymore. After re-enumerate the device comes back with the same
VID/PID but with a different device descriptor. Therefore the check will fail and hub_port_connect is
called which initiates a device disconnect and connect. Without this 'reconnect' lsusb still shows me 
the old device descriptor and I am not able to communicate with the device. 

> Unless all of the devices reset themselves at the same time and the hub
> doesn't like that and can't notice that it happened?
> 
> If you use a different hub, does that work properly?
> 

There is no difference if an other hub is used. It also happens without a hub when the devices are
rebooted via software. My thoughts on this is that when the device re-enumerates and the device
descriptor has changed a device disconnect and connect should be initiated instead of doing nothing?

If I understand it correctly the resuscitation is used for handling port enable-changes occured by EMI.
But when the device is doing a re-enumeration there should be no resuscitation.

David

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-20 15:33       ` David Heinzelmann
@ 2019-09-23 14:49         ` Alan Stern
  2019-09-24 10:01           ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-09-23 14:49 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: Greg KH, linux-usb

On Fri, 20 Sep 2019, David Heinzelmann wrote:

> On Fri, Sep 20, 2019 at 02:15:38PM +0200, Greg KH wrote:
> > On Fri, Sep 20, 2019 at 03:17:26PM +0200, David Heinzelmann wrote:
> > > Hi,
> > > 
> > > sorry for the wrong patch format.
> > 
> > No problem, that's normal.  But please do not top-post on linux mailing
> > lists.
> > 
> > > I am trying to detect a change. At the moment I think the change could be ignored if a
> > > port connection-change occurs and the port status has again the 'PORT_CONNECTION' bit set. 
> > > 
> > > I have a fx3 device which does a re-enumeration after a firmware download. This is working
> > > as expected and I am seeing a 'remove event' and a 'add event' monitoring via udevadm. But
> > > if I connect multiple devices at the same time via an usb hub I am sometimes not receiving
> > > a 'remove event' and 'add event' for a single device.
> > 
> > Sounds like a broken hub :)
> > 
> 
> I tried different hubs but I forgot to mention that it is also possible to trigger the issue
> without a hub if I reboot the devices via software at the same time. 
> 
> > > I think the problem could be that when a device disconnects and the port connection-change
> > > occurs and before the 'PORT_CONNECTION' bit is checked the device could already be
> > > reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not correct to
> > > resuscitate the exisiting device.
> > 
> > Does your patch actually fix the issue?  When a fx3 device downloads
> > firmware and re-enumerates, it should come back as a totally different
> > device, which will fail this check, right?  So I don't see how this
> > fixes the issues with your devices.
> > 
> 
> With the patch I do not have the issue anymore. After re-enumerate the device comes back with the same
> VID/PID but with a different device descriptor. Therefore the check will fail and hub_port_connect is
> called which initiates a device disconnect and connect. Without this 'reconnect' lsusb still shows me 
> the old device descriptor and I am not able to communicate with the device. 
> 
> > Unless all of the devices reset themselves at the same time and the hub
> > doesn't like that and can't notice that it happened?
> > 
> > If you use a different hub, does that work properly?
> > 
> 
> There is no difference if an other hub is used. It also happens without a hub when the devices are
> rebooted via software. My thoughts on this is that when the device re-enumerates and the device
> descriptor has changed a device disconnect and connect should be initiated instead of doing nothing?
> 
> If I understand it correctly the resuscitation is used for handling port enable-changes occured by EMI.
> But when the device is doing a re-enumeration there should be no resuscitation.

I really don't understand this.

Your patch involves the case where there was a connect-change event but 
the port is still enabled.  Now maybe I've forgotten about one of the 
pathways, but it seems like that combination should never occur.

Certainly it shouldn't occur in your case.  The device disconnects and 
then reconnects with a new set of descriptors.  The disconnect should 
cause the port to be disabled, and the port should remain disabled 
after the reconnect occurs.  So how can your new code run in the first 
place?

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-23 14:49         ` Alan Stern
@ 2019-09-24 10:01           ` David Heinzelmann
  2019-09-25 14:20             ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-09-24 10:01 UTC (permalink / raw)
  To: Alan Stern; +Cc: David Heinzelmann, Greg KH, linux-usb

On Mon, Sep 23, 2019 at 10:49:59AM -0400, Alan Stern wrote:
> On Fri, 20 Sep 2019, David Heinzelmann wrote:
> 
> > On Fri, Sep 20, 2019 at 02:15:38PM +0200, Greg KH wrote:
> > > On Fri, Sep 20, 2019 at 03:17:26PM +0200, David Heinzelmann wrote:
> > > > Hi,
> > > > 
> > > > sorry for the wrong patch format.
> > > 
> > > No problem, that's normal.  But please do not top-post on linux mailing
> > > lists.
> > > 
> > > > I am trying to detect a change. At the moment I think the change could be ignored if a
> > > > port connection-change occurs and the port status has again the 'PORT_CONNECTION' bit set. 
> > > > 
> > > > I have a fx3 device which does a re-enumeration after a firmware download. This is working
> > > > as expected and I am seeing a 'remove event' and a 'add event' monitoring via udevadm. But
> > > > if I connect multiple devices at the same time via an usb hub I am sometimes not receiving
> > > > a 'remove event' and 'add event' for a single device.
> > > 
> > > Sounds like a broken hub :)
> > > 
> > 
> > I tried different hubs but I forgot to mention that it is also possible to trigger the issue
> > without a hub if I reboot the devices via software at the same time. 
> > 
> > > > I think the problem could be that when a device disconnects and the port connection-change
> > > > occurs and before the 'PORT_CONNECTION' bit is checked the device could already be
> > > > reconnected and the 'PORT_CONNECTION' bit is set. Therefore I think it is not correct to
> > > > resuscitate the exisiting device.
> > > 
> > > Does your patch actually fix the issue?  When a fx3 device downloads
> > > firmware and re-enumerates, it should come back as a totally different
> > > device, which will fail this check, right?  So I don't see how this
> > > fixes the issues with your devices.
> > > 
> > 
> > With the patch I do not have the issue anymore. After re-enumerate the device comes back with the same
> > VID/PID but with a different device descriptor. Therefore the check will fail and hub_port_connect is
> > called which initiates a device disconnect and connect. Without this 'reconnect' lsusb still shows me 
> > the old device descriptor and I am not able to communicate with the device. 
> > 
> > > Unless all of the devices reset themselves at the same time and the hub
> > > doesn't like that and can't notice that it happened?
> > > 
> > > If you use a different hub, does that work properly?
> > > 
> > 
> > There is no difference if an other hub is used. It also happens without a hub when the devices are
> > rebooted via software. My thoughts on this is that when the device re-enumerates and the device
> > descriptor has changed a device disconnect and connect should be initiated instead of doing nothing?
> > 
> > If I understand it correctly the resuscitation is used for handling port enable-changes occured by EMI.
> > But when the device is doing a re-enumeration there should be no resuscitation.
> 
> I really don't understand this.
> 
> Your patch involves the case where there was a connect-change event but 
> the port is still enabled.  Now maybe I've forgotten about one of the 
> pathways, but it seems like that combination should never occur.
> 
> Certainly it shouldn't occur in your case.  The device disconnects and 
> then reconnects with a new set of descriptors.  The disconnect should 
> cause the port to be disabled, and the port should remain disabled 
> after the reconnect occurs.  So how can your new code run in the first 
> place?
> 
> Alan Stern
> 

Hi,

I have a log with two devices which are connected to a hub and the hub is plugged in. 

The device which is not working in this log:

Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203 change 0011
Sep 24 08:32:21 kernel: usb 2-6.1: new SuperSpeed Gen 1 USB device number 65 using xhci_hcd
Sep 24 08:32:21 kernel: usb 2-6.1: udev 65, busnum 2, minor = 192
Sep 24 08:32:21 kernel: usb 2-6.1: New USB device found, idVendor=1409, idProduct=3240, bcdDevice= 0.00
Sep 24 08:32:21 kernel: usb 2-6.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:21 kernel: usb 2-6.1: Product: USB 3.0 Camera
Sep 24 08:32:21 kernel: usb 2-6.1: Manufacturer: Camera Manufacturer

Now the firmware download happens and the device is re-enumerating and a disconnect/connect should occur.
But the only change which is seen is the following output:

Sep 24 08:32:23 kernel: usb 2-6-port1: link state change
Sep 24 08:32:23 kernel: usb 2-6-port1: status 0203, change 0041, 5.0 Gb/s

Now the resuscitation is happening but from my understanding this is not correct as in the reality there was a
reconnect from the device. So I tried to initiate a device reconnect if the device descriptor changed.

It also seems to me that the enumeration from the second device (usb 2-6-port1) is blocking 
the port change event and so the actual disconnect is missed.

Here is the complete log:

Sep 24 08:32:20 kernel: hub 1-0:1.0: state 7 ports 16 chg 0000 evt 0008
Sep 24 08:32:20 kernel: usb usb1-port3: status 0101, change 0001, 12 Mb/s
Sep 24 08:32:20 kernel: usb usb2: usb wakeup-resume
Sep 24 08:32:20 kernel: hub 2-0:1.0: hub_resume
Sep 24 08:32:20 kernel: usb usb2-port6: status 0203 change 0001
Sep 24 08:32:20 kernel: usb usb1-port3: debounce total 100ms stable 100ms status 0x101
Sep 24 08:32:20 kernel: hub 2-0:1.0: state 7 ports 10 chg 0040 evt 0000
Sep 24 08:32:20 kernel: usb usb2-port6: status 0203, change 0000, 5.0 Gb/s
Sep 24 08:32:20 kernel: usb 1-3: new high-speed USB device number 17 using xhci_hcd
Sep 24 08:32:20 kernel: usb 1-3: udev 17, busnum 1, minor = 16
Sep 24 08:32:20 kernel: usb 1-3: New USB device found, idVendor=2109, idProduct=2811, bcdDevice=91.00
Sep 24 08:32:20 kernel: usb 1-3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:20 kernel: usb 1-3: Product: USB2.0 Hub             
Sep 24 08:32:21 kernel: usb 1-3: Manufacturer: VIA Labs, Inc.         
Sep 24 08:32:21 kernel: hub 1-3:1.0: USB hub found
Sep 24 08:32:21 kernel: hub 1-3:1.0: 4 ports detected
Sep 24 08:32:21 kernel: hub 1-3:1.0: standalone hub
Sep 24 08:32:21 kernel: hub 1-3:1.0: individual port power switching
Sep 24 08:32:21 kernel: hub 1-3:1.0: individual port over-current protection
Sep 24 08:32:21 kernel: hub 1-3:1.0: Single TT
Sep 24 08:32:21 kernel: hub 1-3:1.0: TT requires at most 32 FS bit times (2664 ns)
Sep 24 08:32:21 kernel: hub 1-3:1.0: Port indicators are supported
Sep 24 08:32:21 kernel: hub 1-3:1.0: power on to power good time: 100ms
Sep 24 08:32:21 kernel: hub 1-3:1.0: local power source is good
Sep 24 08:32:21 kernel: hub 1-3:1.0: enabling power on all ports
Sep 24 08:32:21 kernel: hub 1-0:1.0: state 7 ports 16 chg 0000 evt 0008
Sep 24 08:32:21 kernel: usb usb1-port3: resume, status 0
Sep 24 08:32:21 kernel: hub 1-3:1.0: state 7 ports 4 chg 0000 evt 0000
Sep 24 08:32:21 kernel: hub 1-3:1.0: hub_suspend
Sep 24 08:32:21 kernel: usb 2-6: new SuperSpeed Gen 1 USB device number 64 using xhci_hcd
Sep 24 08:32:21 kernel: usb 1-3: usb auto-suspend, wakeup 1
Sep 24 08:32:21 kernel: usb 2-6: udev 64, busnum 2, minor = 191
Sep 24 08:32:21 kernel: usb 2-6: New USB device found, idVendor=2109, idProduct=8110, bcdDevice=91.05
Sep 24 08:32:21 kernel: usb 2-6: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:21 kernel: usb 2-6: Product: USB3.0 Hub             
Sep 24 08:32:21 kernel: usb 2-6: Manufacturer: VIA Labs, Inc.         
Sep 24 08:32:21 kernel: hub 2-6:1.0: USB hub found
Sep 24 08:32:21 kernel: hub 2-6:1.0: 4 ports detected
Sep 24 08:32:21 kernel: hub 2-6:1.0: standalone hub
Sep 24 08:32:21 kernel: hub 2-6:1.0: individual port power switching
Sep 24 08:32:21 kernel: hub 2-6:1.0: individual port over-current protection
Sep 24 08:32:21 kernel: hub 2-6:1.0: TT requires at most 8 FS bit times (666 ns)
Sep 24 08:32:21 kernel: hub 2-6:1.0: power on to power good time: 200ms
Sep 24 08:32:21 kernel: hub 2-6:1.0: local power source is good
Sep 24 08:32:21 kernel: hub 2-6:1.0: enabling power on all ports
Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203 change 0011
Sep 24 08:32:21 kernel: usb 2-6-port2: status 0203 change 0011
Sep 24 08:32:21 kernel: hub 2-6:1.0: state 7 ports 4 chg 0006 evt 0000
Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203, change 0000, 5.0 Gb/s
Sep 24 08:32:21 kernel: usb 2-6.1: new SuperSpeed Gen 1 USB device number 65 using xhci_hcd
Sep 24 08:32:21 kernel: usb 2-6.1: udev 65, busnum 2, minor = 192
Sep 24 08:32:21 kernel: usb 2-6.1: New USB device found, idVendor=1409, idProduct=3240, bcdDevice= 0.00
Sep 24 08:32:21 kernel: usb 2-6.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:21 kernel: usb 2-6.1: Product: USB 3.0 Camera
Sep 24 08:32:21 kernel: usb 2-6.1: Manufacturer: Camera Manufacturer
Sep 24 08:32:21 kernel: usb 2-6-port2: status 0203, change 0000, 5.0 Gb/s
Sep 24 08:32:21 kernel: usb 2-6.2: new SuperSpeed Gen 1 USB device number 66 using xhci_hcd
Sep 24 08:32:21 kernel: usb 2-6.2: udev 66, busnum 2, minor = 193
Sep 24 08:32:21 kernel: usb 2-6.2: New USB device found, idVendor=1409, idProduct=3590, bcdDevice= 0.00
Sep 24 08:32:21 kernel: usb 2-6.2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:21 kernel: usb 2-6.2: Product: USB 3.0 Camera
Sep 24 08:32:21 kernel: usb 2-6.2: Manufacturer: Camera Manufacturer
Sep 24 08:32:21 kernel: hub 2-6:1.0: state 7 ports 4 chg 0000 evt 0002
Sep 24 08:32:23 kernel: hub 2-6:1.0: state 7 ports 4 chg 0000 evt 0004
Sep 24 08:32:23 kernel: usb 2-6-port2: link state change
Sep 24 08:32:23 kernel: usb 2-6-port2: do warm reset
Sep 24 08:32:23 kernel: usb 2-6.2: Disable of device-initiated U1 failed.
Sep 24 08:32:23 kernel: usb 2-6.2: Disable of device-initiated U2 failed.
Sep 24 08:32:23 kernel: usb 2-6-port2: not reset yet, waiting 10ms
Sep 24 08:32:23 kernel: usb 2-6-port2: not reset yet, waiting 10ms
Sep 24 08:32:23 kernel: usb 2-6-port2: not reset yet, waiting 200ms
Sep 24 08:32:23 kernel: usb 2-6.2: reset SuperSpeed Gen 1 USB device number 66 using xhci_hcd
Sep 24 08:32:23 kernel: usb 2-6.2: config index 0, error 18
Sep 24 08:32:23 kernel: usb 2-6.2: device firmware changed
Sep 24 08:32:23 kernel: usb 2-6-port2: logical disconnect
Sep 24 08:32:23 kernel: hub 2-6:1.0: state 7 ports 4 chg 0004 evt 0004
Sep 24 08:32:23 kernel: usb 2-6-port2: status 0263, change 0000, 5.0 Gb/s
Sep 24 08:32:23 kernel: usb 2-6.2: USB disconnect, device number 66
Sep 24 08:32:23 kernel: usb 2-6.2: unregistering device
Sep 24 08:32:23 kernel: usb 2-6.2: new SuperSpeed Gen 1 USB device number 67 using xhci_hcd
Sep 24 08:32:23 kernel: usb 2-6.2: udev 67, busnum 2, minor = 194
Sep 24 08:32:23 kernel: usb 2-6.2: New USB device found, idVendor=1409, idProduct=3590, bcdDevice= 0.00
Sep 24 08:32:23 kernel: usb 2-6.2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:23 kernel: usb 2-6.2: Product: USB 3.0 Camera
Sep 24 08:32:23 kernel: usb 2-6.2: Manufacturer: Camera Manufacturer
Sep 24 08:32:23 kernel: hub 2-6:1.0: state 7 ports 4 chg 0000 evt 0002
Sep 24 08:32:23 kernel: usb 2-6-port1: link state change
Sep 24 08:32:23 kernel: usb 2-6-port1: status 0203, change 0041, 5.0 Gb/s
Sep 24 08:32:24 kernel: hub 2-6:1.0: state 7 ports 4 chg 0000 evt 0004
Sep 24 08:32:24 kernel: usb 2-6-port2: link state change
Sep 24 08:32:24 kernel: usb 2-6-port2: status 02a0, change 0041, 5.0 Gb/s
Sep 24 08:32:24 kernel: usb 2-6.2: USB disconnect, device number 67
Sep 24 08:32:24 kernel: usb 2-6.2: unregistering device
Sep 24 08:32:24 kernel: usb 2-6-port2: debounce total 200ms stable 100ms status 0x203
Sep 24 08:32:24 kernel: usb 2-6.2: new SuperSpeed Gen 1 USB device number 68 using xhci_hcd
Sep 24 08:32:24 kernel: usb 2-6.2: udev 68, busnum 2, minor = 195
Sep 24 08:32:24 kernel: usb 2-6.2: New USB device found, idVendor=1409, idProduct=3590, bcdDevice= 0.00
Sep 24 08:32:24 kernel: usb 2-6.2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
Sep 24 08:32:24 kernel: usb 2-6.2: Product: USB 3.0 Camera
Sep 24 08:32:24 kernel: usb 2-6.2: Manufacturer: Camera Manufacturer


David

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-24 10:01           ` David Heinzelmann
@ 2019-09-25 14:20             ` Alan Stern
  2019-09-30  7:26               ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-09-25 14:20 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: Greg KH, linux-usb

On Tue, 24 Sep 2019, David Heinzelmann wrote:

> > I really don't understand this.
> > 
> > Your patch involves the case where there was a connect-change event but 
> > the port is still enabled.  Now maybe I've forgotten about one of the 
> > pathways, but it seems like that combination should never occur.
> > 
> > Certainly it shouldn't occur in your case.  The device disconnects and 
> > then reconnects with a new set of descriptors.  The disconnect should 
> > cause the port to be disabled, and the port should remain disabled 
> > after the reconnect occurs.  So how can your new code run in the first 
> > place?
> > 
> > Alan Stern
> > 
> 
> Hi,
> 
> I have a log with two devices which are connected to a hub and the hub is plugged in. 
> 
> The device which is not working in this log:
> 
> Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203 change 0011
> Sep 24 08:32:21 kernel: usb 2-6.1: new SuperSpeed Gen 1 USB device number 65 using xhci_hcd

Ah, SuperSpeed.  You're using a USB-3 device.  That does make a
difference.

> Sep 24 08:32:21 kernel: usb 2-6.1: udev 65, busnum 2, minor = 192
> Sep 24 08:32:21 kernel: usb 2-6.1: New USB device found, idVendor=1409, idProduct=3240, bcdDevice= 0.00
> Sep 24 08:32:21 kernel: usb 2-6.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> Sep 24 08:32:21 kernel: usb 2-6.1: Product: USB 3.0 Camera
> Sep 24 08:32:21 kernel: usb 2-6.1: Manufacturer: Camera Manufacturer
> 
> Now the firmware download happens and the device is re-enumerating and a disconnect/connect should occur.
> But the only change which is seen is the following output:
> 
> Sep 24 08:32:23 kernel: usb 2-6-port1: link state change
> Sep 24 08:32:23 kernel: usb 2-6-port1: status 0203, change 0041, 5.0 Gb/s
> 
> Now the resuscitation is happening but from my understanding this is not correct as in the reality there was a
> reconnect from the device. So I tried to initiate a device reconnect if the device descriptor changed.
> 
> It also seems to me that the enumeration from the second device (usb 2-6-port1) is blocking 
> the port change event and so the actual disconnect is missed.

Now it all makes sense.  Yes, I agree that your patch is the
appropriate thing to do -- except that it contains at least one logic
error: It doesn't handle the return code from
usb_get_device_descriptor() properly.

Also, I think you should expand the immediately preceding comment.  
Explain that it is indeed possible for the port to be enabled at this
point, because USB-3 connections are initialized automatically by the
host controller hardware when the connection is detected.

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-25 14:20             ` Alan Stern
@ 2019-09-30  7:26               ` David Heinzelmann
  2019-09-30 14:25                 ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-09-30  7:26 UTC (permalink / raw)
  To: Alan Stern; +Cc: David Heinzelmann, Greg KH, linux-usb

On Wed, Sep 25, 2019 at 10:20:00AM -0400, Alan Stern wrote:
> On Tue, 24 Sep 2019, David Heinzelmann wrote:
> 
> > > I really don't understand this.
> > > 
> > > Your patch involves the case where there was a connect-change event but 
> > > the port is still enabled.  Now maybe I've forgotten about one of the 
> > > pathways, but it seems like that combination should never occur.
> > > 
> > > Certainly it shouldn't occur in your case.  The device disconnects and 
> > > then reconnects with a new set of descriptors.  The disconnect should 
> > > cause the port to be disabled, and the port should remain disabled 
> > > after the reconnect occurs.  So how can your new code run in the first 
> > > place?
> > > 
> > > Alan Stern
> > > 
> > 
> > Hi,
> > 
> > I have a log with two devices which are connected to a hub and the hub is plugged in. 
> > 
> > The device which is not working in this log:
> > 
> > Sep 24 08:32:21 kernel: usb 2-6-port1: status 0203 change 0011
> > Sep 24 08:32:21 kernel: usb 2-6.1: new SuperSpeed Gen 1 USB device number 65 using xhci_hcd
> 
> Ah, SuperSpeed.  You're using a USB-3 device.  That does make a
> difference.
> 
> > Sep 24 08:32:21 kernel: usb 2-6.1: udev 65, busnum 2, minor = 192
> > Sep 24 08:32:21 kernel: usb 2-6.1: New USB device found, idVendor=1409, idProduct=3240, bcdDevice= 0.00
> > Sep 24 08:32:21 kernel: usb 2-6.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> > Sep 24 08:32:21 kernel: usb 2-6.1: Product: USB 3.0 Camera
> > Sep 24 08:32:21 kernel: usb 2-6.1: Manufacturer: Camera Manufacturer
> > 
> > Now the firmware download happens and the device is re-enumerating and a disconnect/connect should occur.
> > But the only change which is seen is the following output:
> > 
> > Sep 24 08:32:23 kernel: usb 2-6-port1: link state change
> > Sep 24 08:32:23 kernel: usb 2-6-port1: status 0203, change 0041, 5.0 Gb/s
> > 
> > Now the resuscitation is happening but from my understanding this is not correct as in the reality there was a
> > reconnect from the device. So I tried to initiate a device reconnect if the device descriptor changed.
> > 
> > It also seems to me that the enumeration from the second device (usb 2-6-port1) is blocking 
> > the port change event and so the actual disconnect is missed.
> 
> Now it all makes sense.  Yes, I agree that your patch is the
> appropriate thing to do -- except that it contains at least one logic
> error: It doesn't handle the return code from
> usb_get_device_descriptor() properly.
> 
> Also, I think you should expand the immediately preceding comment.  
> Explain that it is indeed possible for the port to be enabled at this
> point, because USB-3 connections are initialized automatically by the
> host controller hardware when the connection is detected.
> 
> Alan Stern
> 

Hi,

I adjusted the patch. Any comments? If it's okay, I will re-sent the patch 
to the mailing list.

Here is the second version:


From dc78b8add72168215b8295e01ce3e2599b4998f7 Mon Sep 17 00:00:00 2001
From: David Heinzelmann <heinzelmann.david@gmail.com>
Date: Mon, 30 Sep 2019 07:11:31 +0200
Subject: [PATCH v2] Check for changed device descriptors when a connection-change
 occurs before validating the connection.

When a port connection-change occurs the hub driver tries to resuscitate an existing
device. Activated from a firmware download a usb device can re-enumerate with new or
changed device descriptors. Therefore it will be checked for changed device descriptors
before the connection is resuscitated and the connection-change event is ignored.

Signed-off-by: David Heinzelmann <heinzelmann.david@gmail.com>
---
 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..45d8bdc9eaae 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 host controller
+			 * 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-30  7:26               ` David Heinzelmann
@ 2019-09-30 14:25                 ` Alan Stern
  2019-10-04 13:23                   ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-09-30 14:25 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: Greg KH, linux-usb

On Mon, 30 Sep 2019, David Heinzelmann wrote:

> Hi,
> 
> I adjusted the patch. Any comments? If it's okay, I will re-sent the patch 
> to the mailing list.
> 
> Here is the second version:
> 
> 
> From dc78b8add72168215b8295e01ce3e2599b4998f7 Mon Sep 17 00:00:00 2001
> From: David Heinzelmann <heinzelmann.david@gmail.com>
> Date: Mon, 30 Sep 2019 07:11:31 +0200
> Subject: [PATCH v2] Check for changed device descriptors when a connection-change
>  occurs before validating the connection.
> 
> When a port connection-change occurs the hub driver tries to resuscitate an existing
> device. Activated from a firmware download a usb device can re-enumerate with new or
> changed device descriptors. Therefore it will be checked for changed device descriptors
> before the connection is resuscitated and the connection-change event is ignored.

The description text should should wrap before 80 characters. Also, the
description should explain the problem you are fixing -- this text does
not explain the problem.


> @@ -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 host controller
> +			 * hardware. Therefore check for changed
> +			 * device descriptors before resuscitating
> +			 * the device.
> +			 */

The accepted format for multi-line comments is:

	/*
	 * blah blah blah
	 * blah blah blah
	 */

Also, the commands can extend out farther, as long as they don't go 
past the 80-character limit.

> +			descriptor = udev->descriptor;
> +			retval = usb_get_device_descriptor(udev,
> +				sizeof(udev->descriptor));

Two tab stops, not one.

> +			if (retval < 0) {
> +				dev_dbg (&udev->dev, "can't read device "
> +					"descriptor %d\n", retval);
> +			} else {
> +				if (descriptors_changed(udev, &descriptor,
> +					udev->bos)) {

Indentation in continuation lines is two tab stops, not one.

> +						dev_dbg(&udev->dev, "device descriptor "
> +							"has changed\n");

Code in an inner block is supposed to be indented by one tab stop, not 
two.

Also, quoted strings are not subject to the 80-character limit.  They
are supposed to remain unbroken no matter how far they stretch.

> +
> +						/* 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) {

Otherwise this looks okay.

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-09-30 14:25                 ` Alan Stern
@ 2019-10-04 13:23                   ` David Heinzelmann
  2019-10-04 14:17                     ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-10-04 13:23 UTC (permalink / raw)
  To: Alan Stern; +Cc: David Heinzelmann, linux-usb

Hi,

thank you for the feedback! I hope this time everything is in order.
However, the correct error description is somewhat difficult for me.

Here is the third version:

From 58634d035508b621025da1d866179b59ed0ae37a Mon Sep 17 00:00:00 2001
From: David Heinzelmann <heinzelmann.david@gmail.com>
Date: Fri, 4 Oct 2019 12:28:36 +0200
Subject: [PATCH v3] usb: hub: Check device descriptor before resusciation

If a port connection-change occurs, the connection should not be
resusciated without a prior check if the port connection is enabled.

Signed-off-by: David Heinzelmann <heinzelmann.david@gmail.com>
---
 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



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-10-04 13:23                   ` David Heinzelmann
@ 2019-10-04 14:17                     ` Alan Stern
  2019-10-07  8:47                       ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-10-04 14:17 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: linux-usb

On Fri, 4 Oct 2019, David Heinzelmann wrote:

> Hi,
> 
> thank you for the feedback! I hope this time everything is in order.
> However, the correct error description is somewhat difficult for me.

You should follow the advice I gave you last time: Explain what the 
problem is and how the patch fixes it.

> Here is the third version:
> 
> From 58634d035508b621025da1d866179b59ed0ae37a Mon Sep 17 00:00:00 2001
> From: David Heinzelmann <heinzelmann.david@gmail.com>
> Date: Fri, 4 Oct 2019 12:28:36 +0200
> Subject: [PATCH v3] usb: hub: Check device descriptor before resusciation
> 
> If a port connection-change occurs, the connection should not be
> resusciated without a prior check if the port connection is enabled.
> Signed-off-by: David Heinzelmann <heinzelmann.david@gmail.com>

(Insert a blank line before the Signed-off-by:.)

See, this doesn't say what the problem is.  Someone reading your 
description won't know _why_ a check is needed.

The problem shows up when a device goes through a firmware update.  It
will disconnect from the USB bus and reconnect, and if it is attached
to an xHCI host controller then the controller hardware will
automatically initialize the connection and enable the port.  But
hub_port_connect_change() assumes that if the port is enabled then
nothing has changed; it doesn't check to see if the device's
descriptors have been updated.  As a result, the kernel's internal copy
of the descriptors ends up being incorrect and the device doesn't work
properly.

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.

Alan Stern


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-10-04 14:17                     ` Alan Stern
@ 2019-10-07  8:47                       ` David Heinzelmann
  2019-10-07 14:01                         ` Alan Stern
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-10-07  8:47 UTC (permalink / raw)
  To: Alan Stern; +Cc: David Heinzelmann, linux-usb

Hi,

I hope it all fits now.

David


From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
From: David Heinzelmann <heinzelmann.david@gmail.com>
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@gmail.com>
---
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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-10-07  8:47                       ` David Heinzelmann
@ 2019-10-07 14:01                         ` Alan Stern
  2019-10-07 15:35                           ` Greg KH
  0 siblings, 1 reply; 20+ messages in thread
From: Alan Stern @ 2019-10-07 14:01 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: linux-usb

On Mon, 7 Oct 2019, David Heinzelmann wrote:

> Hi,
> 
> I hope it all fits now.
> 
> David
> 
> 
> From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
> From: David Heinzelmann <heinzelmann.david@gmail.com>
> 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@gmail.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>


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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
  2019-10-07 14:01                         ` Alan Stern
@ 2019-10-07 15:35                           ` Greg KH
  2019-10-08  8:09                             ` [PATCH v4] usb: hub: Check device descriptor before resusciation David Heinzelmann
  2019-10-09  4:46                             ` [PATCH v5] " David Heinzelmann
  0 siblings, 2 replies; 20+ messages in thread
From: Greg KH @ 2019-10-07 15:35 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: Alan Stern, linux-usb

On Mon, Oct 07, 2019 at 10:01:47AM -0400, Alan Stern wrote:
> On Mon, 7 Oct 2019, David Heinzelmann wrote:
> 
> > Hi,
> > 
> > I hope it all fits now.
> > 
> > David
> > 
> > 
> > From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
> > From: David Heinzelmann <heinzelmann.david@gmail.com>
> > 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@gmail.com>
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

David, can you resend this in a format that I can apply it in?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v4] usb: hub: Check device descriptor before resusciation
  2019-10-07 15:35                           ` Greg KH
@ 2019-10-08  8:09                             ` David Heinzelmann
  2019-10-08 12:55                               ` Greg KH
  2019-10-09  4:46                             ` [PATCH v5] " David Heinzelmann
  1 sibling, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-10-08  8:09 UTC (permalink / raw)
  To: gregkh; +Cc: stern, linux-usb, David Heinzelmann

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@gmail.com>
---
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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
  2019-10-08  8:09                             ` [PATCH v4] usb: hub: Check device descriptor before resusciation David Heinzelmann
@ 2019-10-08 12:55                               ` Greg KH
  2019-10-08 16:10                                 ` David Heinzelmann
  0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2019-10-08 12:55 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: stern, linux-usb

On Tue, Oct 08, 2019 at 10:09:01AM +0200, David Heinzelmann wrote:
> 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@gmail.com>
> ---
> 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(-)

What happened to Alan's ack?

v5?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
  2019-10-08 16:10                                 ` David Heinzelmann
@ 2019-10-08 15:17                                   ` Greg KH
  0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2019-10-08 15:17 UTC (permalink / raw)
  To: David Heinzelmann; +Cc: stern, linux-usb

On Tue, Oct 08, 2019 at 06:10:22PM +0200, David Heinzelmann wrote:
> On Tue, Oct 08, 2019 at 02:55:46PM +0200, Greg KH wrote:
> > What happened to Alan's ack?
> > 
> 
> I'm not sure I'm allowed to add someone else's acked-by tag?  

If they provide it, and you don't change anything, then yes, you can.

> If so I will sent v5.

Please do.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4] usb: hub: Check device descriptor before resusciation
  2019-10-08 12:55                               ` Greg KH
@ 2019-10-08 16:10                                 ` David Heinzelmann
  2019-10-08 15:17                                   ` Greg KH
  0 siblings, 1 reply; 20+ messages in thread
From: David Heinzelmann @ 2019-10-08 16:10 UTC (permalink / raw)
  To: Greg KH; +Cc: David Heinzelmann, stern, linux-usb

On Tue, Oct 08, 2019 at 02:55:46PM +0200, Greg KH wrote:
> What happened to Alan's ack?
> 

I'm not sure I'm allowed to add someone else's acked-by tag?  
If so I will sent v5.

David

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v5] usb: hub: Check device descriptor before resusciation
  2019-10-07 15:35                           ` Greg KH
  2019-10-08  8:09                             ` [PATCH v4] usb: hub: Check device descriptor before resusciation David Heinzelmann
@ 2019-10-09  4:46                             ` David Heinzelmann
  1 sibling, 0 replies; 20+ messages in thread
From: David Heinzelmann @ 2019-10-09  4:46 UTC (permalink / raw)
  To: gregkh; +Cc: linux-usb, David Heinzelmann, Alan Stern

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@gmail.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
Changes in v5:
 - added Acked-by
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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-10-09  4:49 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 10:36 [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection David Heinzelmann
2019-09-20  8:55 ` Greg KH
2019-09-20 13:17   ` David Heinzelmann
2019-09-20 12:15     ` Greg KH
2019-09-20 15:33       ` David Heinzelmann
2019-09-23 14:49         ` Alan Stern
2019-09-24 10:01           ` David Heinzelmann
2019-09-25 14:20             ` Alan Stern
2019-09-30  7:26               ` David Heinzelmann
2019-09-30 14:25                 ` Alan Stern
2019-10-04 13:23                   ` David Heinzelmann
2019-10-04 14:17                     ` Alan Stern
2019-10-07  8:47                       ` David Heinzelmann
2019-10-07 14:01                         ` Alan Stern
2019-10-07 15:35                           ` Greg KH
2019-10-08  8:09                             ` [PATCH v4] usb: hub: Check device descriptor before resusciation David Heinzelmann
2019-10-08 12:55                               ` Greg KH
2019-10-08 16:10                                 ` David Heinzelmann
2019-10-08 15:17                                   ` Greg KH
2019-10-09  4:46                             ` [PATCH v5] " David Heinzelmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).