| Message ID | 20190920103628.5432-1-heinzelmann.david@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check for changed device descriptors when a connection-change occurs before validating the connection. | expand |
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
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
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
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
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
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
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
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)
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
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)
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
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)
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) >
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
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)
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(-)