Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

I'm debugging a problem with the "xum1541" usb adapter (used to
interface with legacy cbm floppy drives), which doesn't work correctly
when plugged into some usb ports but works fine in others.

The symptom of failure is that the host command only works the first
time after the device has been plugged in.

This is what I found so far:

The device mostly uses bulk transfers for communication. After every
start, the host program issues a "set configuration" command (even
though the device only has a single configuration). On receiving the
"set configuration" message, the firmware of the xum1541 device does
an endpoint reset including a reset of the data toggles.

The problem is, that my host computer only seems to reset its data
toggles when the device is plugged into a usb port that as per syslog
uses the "ehci-pci" driver, while it does not in ports using the
"xhci_hcd" driver.

That's why the data toggles get out of sync when the device is plugged
into a port handled by the "xhci_hcd" driver and therefore stops
working.

For now I try to work around this issue by avoiding the "set
configuration" call altogether, but I'm still curious what the correct
behaviour is.

The notion of a "set configuration" call that doesn't really change
the configuration triggering a "lightweight reset" seems to be common,
but I'm not sure if there's consensus what the reset should include.

So I'm not sure which behaviour (to reset the data toggles or not) is
correct, but I think at least the linux kernel should behave
consistently regardless of the usb driver / port used?

Is resetting the data toggles even handled by the driver (or by the
hardware)? There are reports of what seems to be the same problem
(device not working after the first command but only when using an usb
3 port) on windows as well. I personally can't reproduce that, but
that's no big surprise because the current libusb on windows doesn't
actually send the "set configuration" message to the device. It might
do using a different libusb version and/or windows usb driver, though.

This is what lspci reports about the usb controllers on my machine:

> lspci | grep -i usb
00:14.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset
Family USB xHCI Host Controller (rev 04)
00:1a.0 USB controller: Intel Corporation 7 Series/C216 Chipset Family
USB Enhanced Host Controller #2 (rev 04)
00:1d.0 USB controller: Intel Corporation 7 Series/C216 Chipset Family
USB Enhanced Host Controller #1 (rev 04)

> lsusb | grep -i hub
Bus 003 Device 002: ID 8087:0024 Intel Corp. Integrated Rate Matching Hub
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 002: ID 8087:0024 Intel Corp. Integrated Rate Matching Hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

The device does not work correctly when connected to bus 2, but it
does when on bus 3.

The host is running arch linux with currently linux 5.8.1, but the
problem existed at least in the last few kernel versions. The host
program is using libusb 1.0.23.

Let me know if there's anything I can do to help debug this.

Thanks!

Martin

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Alan Stern]

On Fri, Aug 21, 2020 at 02:30:12PM +0200, Martin Thierer wrote:
> I'm debugging a problem with the "xum1541" usb adapter (used to
> interface with legacy cbm floppy drives), which doesn't work correctly
> when plugged into some usb ports but works fine in others.
> 
> The symptom of failure is that the host command only works the first
> time after the device has been plugged in.
> 
> This is what I found so far:
> 
> The device mostly uses bulk transfers for communication. After every
> start, the host program issues a "set configuration" command (even
> though the device only has a single configuration). On receiving the
> "set configuration" message, the firmware of the xum1541 device does
> an endpoint reset including a reset of the data toggles.
> 
> The problem is, that my host computer only seems to reset its data
> toggles when the device is plugged into a usb port that as per syslog
> uses the "ehci-pci" driver, while it does not in ports using the
> "xhci_hcd" driver.
> 
> That's why the data toggles get out of sync when the device is plugged
> into a port handled by the "xhci_hcd" driver and therefore stops
> working.
> 
> For now I try to work around this issue by avoiding the "set
> configuration" call altogether, but I'm still curious what the correct
> behaviour is.
> 
> The notion of a "set configuration" call that doesn't really change
> the configuration triggering a "lightweight reset" seems to be common,
> but I'm not sure if there's consensus what the reset should include.
> 
> So I'm not sure which behaviour (to reset the data toggles or not) is
> correct, but I think at least the linux kernel should behave
> consistently regardless of the usb driver / port used?

The USB 2.0 specification says (section 8.5.2):

	A bulk endpoint’s toggle sequence is initialized to DATA0 when 
	the endpoint experiences any configuration event (configuration 
	events are explained in Sections 9.1.1.5 and 9.4.5).

Section 9.1.1.5 says:

	Before a USB device’s function may be used, the device must be 
	configured. From the device’s perspective, configuration 
	involves correctly processing a SetConfiguration() request with 
	a non-zero configuration value. Configuring a device or changing 
	an alternate setting causes all of the status and configuration 
	values associated with endpoints in the affected interfaces to 
	be set to their default values. This includes setting the data 
	toggle of any endpoint using data toggles to the value DATA0.

Together these should explain the correct behavior.

> Is resetting the data toggles even handled by the driver (or by the
> hardware)?

The driver.

Alan Stern

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

I read the usb 2.0 specs and generally came to the same conclusion,
except I wasn't 100% sure because 9.1.1.5 talks about "configuring" in
the context of bringing a device into the "configured" state, which
one could argue isn't really the case if a "set configuration" message
is sent to a device that is already configured with the exact same
configuration.

> Together these should explain the correct behavior.

I'm not sure I understand what you're implying here. That the kernel's
behaviour is correct or not? (You're explicitly citing the usb *2.0*
specs, but that should also apply to a usb 2 device plugged into an
usb 3 port, right?)

Martin

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Alan Stern]

On Fri, Aug 21, 2020 at 06:34:55PM +0200, Martin Thierer wrote:
> I read the usb 2.0 specs and generally came to the same conclusion,
> except I wasn't 100% sure because 9.1.1.5 talks about "configuring" in
> the context of bringing a device into the "configured" state, which
> one could argue isn't really the case if a "set configuration" message
> is sent to a device that is already configured with the exact same
> configuration.

Nonsense.  The text explicitly says "configuration involves correctly 
processing a SetConfiguration() request with a non-zero configuration 
value."  There's no requirement about what state the device was in 
previously.

> > Together these should explain the correct behavior.
> 
> I'm not sure I understand what you're implying here. That the kernel's
> behaviour is correct or not?

That the EHCI behavior is correct and the xHCI behavior is wrong.

>  (You're explicitly citing the usb *2.0*
> specs, but that should also apply to a usb 2 device plugged into an
> usb 3 port, right?)

Yes; the USB 3 spec says that the behavior of USB-2 devices should be 
governed by the USB-2 spec, even when they are plugged into a hub or 
controller.

Alan Stern

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 21.8.2020 20.01, Alan Stern wrote:
> On Fri, Aug 21, 2020 at 06:34:55PM +0200, Martin Thierer wrote:
>> I read the usb 2.0 specs and generally came to the same conclusion,
>> except I wasn't 100% sure because 9.1.1.5 talks about "configuring" in
>> the context of bringing a device into the "configured" state, which
>> one could argue isn't really the case if a "set configuration" message
>> is sent to a device that is already configured with the exact same
>> configuration.
> 
> Nonsense.  The text explicitly says "configuration involves correctly 
> processing a SetConfiguration() request with a non-zero configuration 
> value."  There's no requirement about what state the device was in 
> previously.
> 
>>> Together these should explain the correct behavior.
>>
>> I'm not sure I understand what you're implying here. That the kernel's
>> behaviour is correct or not?
> 
> That the EHCI behavior is correct and the xHCI behavior is wrong.

True, xHCI doesn't reset the toggle in this case.
xHC only keeps track of added or dropped endpoints, it doesn't track
which configuration or interface is set.

If the SetConfiguration() request doesn't cause any endpoint add/drop change
then xhci driver won't do anything.
To reset the toggle we should mark the endpoints as both dropped and added,
and issue a configure endpoint xhci command. 

The same xhci codepath is called from other places as well, I need to check
a change like this won't cause any other issues

-Mathias
 

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

> True, xHCI doesn't reset the toggle in this case.
> xHC only keeps track of added or dropped endpoints, it doesn't track
> which configuration or interface is set.

I'm not sure if it's relevant, but I found that calling libusb's
set_interface() *does* seem to reset the data toggles also on xhci
ports, even if it does not actually change the interface.

Martin

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 24.8.2020 16.10, Martin Thierer wrote:
>> True, xHCI doesn't reset the toggle in this case.
>> xHC only keeps track of added or dropped endpoints, it doesn't track
>> which configuration or interface is set.
> 
> I'm not sure if it's relevant, but I found that calling libusb's
> set_interface() *does* seem to reset the data toggles also on xhci
> ports, even if it does not actually change the interface.
> 
> Martin

It does, I checked that usb_set_configuration() ends up dropping and re-adding 
the endpoint when it calls usb_hcd_alloc_bandwidth(), this should cause xhci
driver to reset the toggles.

Looks like libusb set_configuration could end up calling usb_reset_configuration() instead.
If there are no changes it's possible usb_hcd_alloc_bandwidth() never gets called, and
toggles never reset.

see drivers/usb/core/devio.c proc_setconfig()  

-Mathias

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 24.8.2020 16.48, Mathias Nyman wrote:
> 
> Looks like libusb set_configuration could end up calling usb_reset_configuration() instead.
> If there are no changes it's possible usb_hcd_alloc_bandwidth() never gets called, and
> toggles never reset.
> 
> see drivers/usb/core/devio.c proc_setconfig()  
> 

Can you try the code below? It should force dropping and adding the endpoints
for the intrface(s) of that configuration, and xhci should reset the toggles.

Completely untested, it compiles :)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..4a1439b29918 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1554,19 +1554,9 @@ int usb_reset_configuration(struct usb_device *dev)
 		return -ENOMEM;
 	}
 	/* Make sure we have enough bandwidth for each alternate setting 0 */
-	for (i = 0; i < config->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = config->interface[i];
-		struct usb_host_interface *alt;
+	 /* TEST, drop and re-add endpoints to clear toggle */
+	retval = usb_hcd_alloc_bandwidth(dev, config, NULL, NULL);
 
-		alt = usb_altnum_to_altsetting(intf, 0);
-		if (!alt)
-			alt = &intf->altsetting[0];
-		if (alt != intf->cur_altsetting)
-			retval = usb_hcd_alloc_bandwidth(dev, NULL,
-					intf->cur_altsetting, alt);
-		if (retval < 0)
-			break;
-	}
 	/* If not, reinstate the old alternate settings */
 	if (retval < 0) {
 reset_old_alts:

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

> Can you try the code below? It should force dropping and adding the endpoints
> for the intrface(s) of that configuration, and xhci should reset the toggles.
>
> Completely untested, it compiles :)

Sorry, no, that doesn't work:

xhci_hcd 0000:00:14.0: Trying to add endpoint 0x83 without dropping it.
BUG: kernel NULL pointer dereference, address: 0000000000000010
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 6 PID: 1192 Comm: python Tainted: P           OE
5.8.3-arch1-1-custom #2
Hardware name: Gigabyte Technology Co., Ltd. To be filled by
O.E.M./Z77-DS3H, BIOS F11a 11/13/2013
RIP: 0010:usb_altnum_to_altsetting+0x5/0x40
Code: 00 eb 09 48 83 c0 08 48 39 c8 74 12 4c 8b 00 49 8b 10 0f b6 52
02 39 f2 75 e9 4c 89 c0 c3 45 31 c0 4c 89 c0 c3 0f 1f 44 00 00 <8b> 4f
10 85 c9 74 26 48 8b 3f 31 c0 eb 07 83 c0 01 39 c8 74 18 48
RSP: 0018:ffffa0a0c0733e00 EFLAGS: 00010246
RAX: ffff97086414a000 RBX: ffff97086414a110 RCX: 0000000000000000
RDX: 0000000000000020 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffff97086e0ed000 R08: 0000000000000001 R09: ffffffffc0360290
R10: 0000000000000000 R11: 0000000000000001 R12: ffff97088c95c000
R13: ffff97086414a090 R14: 00000000ffffffea R15: 0000000000000000
FS:  00007ff728371740(0000) GS:ffff97088ed80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 00000003d9666001 CR4: 00000000001606e0
Call Trace:
 usb_reset_configuration+0x1da/0x240
 usbdev_ioctl+0x1276/0x1300
 ? do_sys_openat2+0xa6/0x170
 ? kmem_cache_free+0xa4/0x1d0
 ksys_ioctl+0x82/0xc0
 __x64_sys_ioctl+0x16/0x20
 do_syscall_64+0x44/0x70
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7ff72896ef6b
Code: 89 d8 49 8d 3c 1c 48 f7 d8 49 39 c4 72 b5 e8 1c ff ff ff 85 c0
78 ba 4c 89 e0 5b 5d 41 5c c3 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d
01 f0 ff ff 73 01 c3 48 8b 0d d5 ae 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007fff7efdce08 EFLAGS: 00000202 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000055bf1081bab0 RCX: 00007ff72896ef6b
RDX: 00007fff7efdce1c RSI: 0000000080045505 RDI: 0000000000000009
RBP: 000055bf10827040 R08: 00007fff7efdcd40 R09: 00007ff727e96470
R10: 00007fff7efdcd40 R11: 0000000000000202 R12: 0000000000000000
R13: 00007fff7efdcef0 R14: 00007fff7efdcf60 R15: 00007fff7efdceec
Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs
lockd grace sunrpc fscache fuse ip6table_filter ip6_tables xt_nat
iptable_nat nf_nat nf_log_ipv4 nf_log_common xt_set xt_multiport
xt_state xt_conntrac>
 snd_hda_codec ghash_clmulni_intel aesni_intel snd_hda_core
crypto_simd uas cryptd snd_hwdep iTCO_wdt glue_helper mei_hdcp
intel_pmc_bxt rapl usb_storage at24 drm_kms_helper iTCO_vendor_support
snd_pcm intel_cstate cec in>
CR2: 0000000000000010
---[ end trace 86a221d52d129373 ]---
RIP: 0010:usb_altnum_to_altsetting+0x5/0x40
Code: 00 eb 09 48 83 c0 08 48 39 c8 74 12 4c 8b 00 49 8b 10 0f b6 52
02 39 f2 75 e9 4c 89 c0 c3 45 31 c0 4c 89 c0 c3 0f 1f 44 00 00 <8b> 4f
10 85 c9 74 26 48 8b 3f 31 c0 eb 07 83 c0 01 39 c8 74 18 48
RSP: 0018:ffffa0a0c0733e00 EFLAGS: 00010246
RAX: ffff97086414a000 RBX: ffff97086414a110 RCX: 0000000000000000
RDX: 0000000000000020 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffff97086e0ed000 R08: 0000000000000001 R09: ffffffffc0360290
R10: 0000000000000000 R11: 0000000000000001 R12: ffff97088c95c000
R13: ffff97086414a090 R14: 00000000ffffffea R15: 0000000000000000
FS:  00007ff728371740(0000) GS:ffff97088ed80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 00000003d9666001 CR4: 00000000001606e0

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 25.8.2020 11.00, Martin Thierer wrote:
>> Can you try the code below? It should force dropping and adding the endpoints
>> for the intrface(s) of that configuration, and xhci should reset the toggles.
>>
>> Completely untested, it compiles :)
> 
> Sorry, no, that doesn't work:
> 
> xhci_hcd 0000:00:14.0: Trying to add endpoint 0x83 without dropping it.
> BUG: kernel NULL pointer dereference, address: 0000000000000010

Ah, I see.
Endpoints weren't dropped on host side as pointer to the endpoints were cleaned up before this.
And the code to recover from a failed call got messed up as we removed some stuff it depends on.

Here's a second version. 
I'm again not able to test this at all from my current location, so it might fail because
of some silly mistake, but it compiles..

This version keeps endpoint pointers intact until endpoints are dropped from hcd side, 
it also removes the recover path (might need to fix one later) 

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..e90e8781f872 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1537,9 +1537,21 @@ int usb_reset_configuration(struct usb_device *dev)
 	 * calls during probe() are fine
 	 */
 
+	/*
+	 * TEST2 flush and disable endpoints but leave the pointers intact until
+	 * usb_hcd_alloc_bandwidth() has dropped them from host controller side
+	 */
 	for (i = 1; i < 16; ++i) {
-		usb_disable_endpoint(dev, i, true);
-		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
+		if (dev->ep_out[i]) {
+			dev->ep_out[i]->enabled = 0;
+			usb_hcd_flush_endpoint(dev, dev->ep_out[i]);
+			usb_hcd_disable_endpoint(dev, dev->ep_out[i]);
+		}
+		if (dev->ep_in[i]) {
+			dev->ep_in[i]->enabled = 0;
+			usb_hcd_flush_endpoint(dev, dev->ep_in[i]);
+			usb_hcd_disable_endpoint(dev, dev->ep_in[i]);
+		}
 	}
 
 	config = dev->actconfig;
@@ -1554,33 +1566,21 @@ int usb_reset_configuration(struct usb_device *dev)
 		return -ENOMEM;
 	}
 	/* Make sure we have enough bandwidth for each alternate setting 0 */
-	for (i = 0; i < config->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = config->interface[i];
-		struct usb_host_interface *alt;
+	/* TEST drop and re-add endpoints to clear toggle */
+	retval = usb_hcd_alloc_bandwidth(dev, config, NULL, NULL);
 
-		alt = usb_altnum_to_altsetting(intf, 0);
-		if (!alt)
-			alt = &intf->altsetting[0];
-		if (alt != intf->cur_altsetting)
-			retval = usb_hcd_alloc_bandwidth(dev, NULL,
-					intf->cur_altsetting, alt);
-		if (retval < 0)
-			break;
+	/* clear the endpoint pointers, FIXME check if needed */
+	for (i = 1; i < 16; ++i) {
+		dev->ep_out[i] = NULL;
+		dev->ep_in[i] = NULL;
 	}
-	/* If not, reinstate the old alternate settings */
+
 	if (retval < 0) {
-reset_old_alts:
-		for (i--; i >= 0; i--) {
-			struct usb_interface *intf = config->interface[i];
-			struct usb_host_interface *alt;
-
-			alt = usb_altnum_to_altsetting(intf, 0);
-			if (!alt)
-				alt = &intf->altsetting[0];
-			if (alt != intf->cur_altsetting)
-				usb_hcd_alloc_bandwidth(dev, NULL,
-						alt, intf->cur_altsetting);
-		}
+		/*
+		 * Can't walk backwards and set the old alts back as we no longer
+		 * set one interface at a time, but full configuration in one go
+		 */
+
 		usb_enable_lpm(dev);
 		mutex_unlock(hcd->bandwidth_mutex);
 		return retval;
@@ -1589,8 +1589,11 @@ int usb_reset_configuration(struct usb_device *dev)
 			USB_REQ_SET_CONFIGURATION, 0,
 			config->desc.bConfigurationValue, 0,
 			NULL, 0, USB_CTRL_SET_TIMEOUT);
-	if (retval < 0)
-		goto reset_old_alts;
+	if (retval < 0) {
+		usb_enable_lpm(dev);
+		mutex_unlock(hcd->bandwidth_mutex);
+		return retval;
+	}
 	mutex_unlock(hcd->bandwidth_mutex);
 
 	/* re-init hc/hcd interface/endpoint state */

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Alan Stern]

On Tue, Aug 25, 2020 at 02:53:56PM +0300, Mathias Nyman wrote:
> On 25.8.2020 11.00, Martin Thierer wrote:
> >> Can you try the code below? It should force dropping and adding the endpoints
> >> for the intrface(s) of that configuration, and xhci should reset the toggles.
> >>
> >> Completely untested, it compiles :)
> > 
> > Sorry, no, that doesn't work:
> > 
> > xhci_hcd 0000:00:14.0: Trying to add endpoint 0x83 without dropping it.
> > BUG: kernel NULL pointer dereference, address: 0000000000000010
> 
> Ah, I see.
> Endpoints weren't dropped on host side as pointer to the endpoints were cleaned up before this.
> And the code to recover from a failed call got messed up as we removed some stuff it depends on.
> 
> Here's a second version. 
> I'm again not able to test this at all from my current location, so it might fail because
> of some silly mistake, but it compiles..
> 
> This version keeps endpoint pointers intact until endpoints are dropped from hcd side, 
> it also removes the recover path (might need to fix one later) 
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 6197938dcc2d..e90e8781f872 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1537,9 +1537,21 @@ int usb_reset_configuration(struct usb_device *dev)
>  	 * calls during probe() are fine
>  	 */
>  
> +	/*
> +	 * TEST2 flush and disable endpoints but leave the pointers intact until
> +	 * usb_hcd_alloc_bandwidth() has dropped them from host controller side
> +	 */
>  	for (i = 1; i < 16; ++i) {
> -		usb_disable_endpoint(dev, i, true);
> -		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
> +		if (dev->ep_out[i]) {
> +			dev->ep_out[i]->enabled = 0;
> +			usb_hcd_flush_endpoint(dev, dev->ep_out[i]);
> +			usb_hcd_disable_endpoint(dev, dev->ep_out[i]);
> +		}
> +		if (dev->ep_in[i]) {
> +			dev->ep_in[i]->enabled = 0;
> +			usb_hcd_flush_endpoint(dev, dev->ep_in[i]);
> +			usb_hcd_disable_endpoint(dev, dev->ep_in[i]);
> +		}
>  	}

There's got to be a better way to do this, something that doesn't 
involve so much code duplication.  For instance, maybe we could make 
this routine and usb_set_configuration() both call a new 
__usb_set_config(), with an extra flag telling the routine whether to 
change the interface devices and bindings.

Alan Stern

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 25.8.2020 18.10, Alan Stern wrote:
> On Tue, Aug 25, 2020 at 02:53:56PM +0300, Mathias Nyman wrote:
>> On 25.8.2020 11.00, Martin Thierer wrote:
>>>> Can you try the code below? It should force dropping and adding the endpoints
>>>> for the intrface(s) of that configuration, and xhci should reset the toggles.
>>>>
>>>> Completely untested, it compiles :)
>>>
>>> Sorry, no, that doesn't work:
>>>
>>> xhci_hcd 0000:00:14.0: Trying to add endpoint 0x83 without dropping it.
>>> BUG: kernel NULL pointer dereference, address: 0000000000000010
>>
>> Ah, I see.
>> Endpoints weren't dropped on host side as pointer to the endpoints were cleaned up before this.
>> And the code to recover from a failed call got messed up as we removed some stuff it depends on.
>>
>> Here's a second version. 
>> I'm again not able to test this at all from my current location, so it might fail because
>> of some silly mistake, but it compiles..
>>
>> This version keeps endpoint pointers intact until endpoints are dropped from hcd side, 
>> it also removes the recover path (might need to fix one later) 
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index 6197938dcc2d..e90e8781f872 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -1537,9 +1537,21 @@ int usb_reset_configuration(struct usb_device *dev)
>>  	 * calls during probe() are fine
>>  	 */
>>  
>> +	/*
>> +	 * TEST2 flush and disable endpoints but leave the pointers intact until
>> +	 * usb_hcd_alloc_bandwidth() has dropped them from host controller side
>> +	 */
>>  	for (i = 1; i < 16; ++i) {
>> -		usb_disable_endpoint(dev, i, true);
>> -		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
>> +		if (dev->ep_out[i]) {
>> +			dev->ep_out[i]->enabled = 0;
>> +			usb_hcd_flush_endpoint(dev, dev->ep_out[i]);
>> +			usb_hcd_disable_endpoint(dev, dev->ep_out[i]);
>> +		}
>> +		if (dev->ep_in[i]) {
>> +			dev->ep_in[i]->enabled = 0;
>> +			usb_hcd_flush_endpoint(dev, dev->ep_in[i]);
>> +			usb_hcd_disable_endpoint(dev, dev->ep_in[i]);
>> +		}
>>  	}
> 
> There's got to be a better way to do this, something that doesn't 
> involve so much code duplication.  For instance, maybe we could make 
> this routine and usb_set_configuration() both call a new 
> __usb_set_config(), with an extra flag telling the routine whether to 
> change the interface devices and bindings.

I agree that this needs cleaning up, this code was intended for testing.

It allows us to call usb_hcd_alloc_bandwidth() once with a configuration
and with the old endpoint pointers still intact, leading to one configure
endpoint command for xhci with the relevant drop and add endpoint flags set,
all in one go.

Looks like the last part usb_disable_device() does similar endpoint code
churning to flush, disable, drop, and remove endpoints. May we could start
by turning that code into some useful helper first?

-Mathias


maybe we could start

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Alan Stern]

On Wed, Aug 26, 2020 at 11:37:50AM +0300, Mathias Nyman wrote:
> On 25.8.2020 18.10, Alan Stern wrote:
> > There's got to be a better way to do this, something that doesn't 
> > involve so much code duplication.  For instance, maybe we could make 
> > this routine and usb_set_configuration() both call a new 
> > __usb_set_config(), with an extra flag telling the routine whether to 
> > change the interface devices and bindings.
> 
> I agree that this needs cleaning up, this code was intended for testing.
> 
> It allows us to call usb_hcd_alloc_bandwidth() once with a configuration
> and with the old endpoint pointers still intact, leading to one configure
> endpoint command for xhci with the relevant drop and add endpoint flags set,
> all in one go.
> 
> Looks like the last part usb_disable_device() does similar endpoint code
> churning to flush, disable, drop, and remove endpoints. May we could start
> by turning that code into some useful helper first?

usb_disable_device() is _supposed_ to be the useful helper!  :-)  But 
yes, it could be split into two pieces.

I still think it would be worthwhile to combine usb_set_configuration() 
and usb_reset_configuration() into one routine, since they have to do a 
lot of the same things.

Alan Stern

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

> Here's a second version.
> I'm again not able to test this at all from my current location, so it might fail because
> of some silly mistake, but it compiles..
>
> This version keeps endpoint pointers intact until endpoints are dropped from hcd side,
> it also removes the recover path (might need to fix one later)

Thanks, that seems to work! (Judging by the absence of my original
issue with the xum1541 adapter; I haven't checked what's actually
happening on the bus).

Martin

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 26.8.2020 10.40, Martin Thierer wrote:
>> Here's a second version.
>> I'm again not able to test this at all from my current location, so it might fail because
>> of some silly mistake, but it compiles..
>>
>> This version keeps endpoint pointers intact until endpoints are dropped from hcd side,
>> it also removes the recover path (might need to fix one later)
> 
> Thanks, that seems to work! (Judging by the absence of my original
> issue with the xum1541 adapter; I haven't checked what's actually
> happening on the bus).
> 
> Martin
> 

Great, thanks.

Now this test code needs to be cleaned up a turned into a real patch

-Mathias

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

>> Thanks, that seems to work! (Judging by the absence of my original
>> issue with the xum1541 adapter; I haven't checked what's actually
>> happening on the bus).
>>
>> Martin
>>
> 
> Great, thanks.
> 
> Now this test code needs to be cleaned up a turned into a real patch

Can you test one more round?
The code below is cleaned up but it also has a functional change.
This version issues separate commands for dropping and adding endpoints.
Previous code both did all on one command.

If it works I'll send it forward.

Thanks
-Mathias

---
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: [PATCH] usb: Fix out of sync data toggle if a configured device is
 reconfigured

Userspace drivers that use a SetConfiguration() request to "lightweight"
reset a already configured usb device might cause data toggles to get out
of sync between the device and host, and the device becomes unusable.

The xHCI host requires endpoints to be dropped and added back to reset the
toggle. USB core avoids these otherwise extra steps if the current active
configuration is the same as the new requested configuration.

A SetConfiguration() request will reset the device side data toggles.
Make sure usb core drops and adds back the endpoints in this case.

To avoid code duplication split the current usb_disable_device() function
and reuse the endpoint specific part.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/message.c | 89 +++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 49 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..46c0d5f633a7 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1205,6 +1205,35 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
 	}
 }
 
+/*
+ * usb_disable_device_endpoints -- Disable all endpoints for a device
+ * @dev: the device whose endpoints are being disabled
+ * @skip_ep0: 0 to disable endpoint 0, 1 to skip it.
+ */
+static void usb_disable_device_endpoints(struct usb_device *dev, int skip_ep0)
+{
+	struct usb_hcd *hcd = bus_to_hcd(dev->bus);
+	int i;
+
+	if (hcd->driver->check_bandwidth) {
+
+		/* First pass: Cancel URBs, leave endpoint pointers intact. */
+		for (i = skip_ep0; i < 16; ++i) {
+			usb_disable_endpoint(dev, i, false);
+			usb_disable_endpoint(dev, i + USB_DIR_IN, false);
+		}
+		/* Remove endpoints from the host controller internal state */
+		mutex_lock(hcd->bandwidth_mutex);
+		usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
+		mutex_unlock(hcd->bandwidth_mutex);
+	}
+	/* Second pass: remove endpoint pointers */
+	for (i = skip_ep0; i < 16; ++i) {
+		usb_disable_endpoint(dev, i, true);
+		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
+	}
+}
+
 /**
  * usb_disable_device - Disable all the endpoints for a USB device
  * @dev: the device whose endpoints are being disabled
@@ -1218,7 +1247,6 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
 void usb_disable_device(struct usb_device *dev, int skip_ep0)
 {
 	int i;
-	struct usb_hcd *hcd = bus_to_hcd(dev->bus);
 
 	/* getting rid of interfaces will disconnect
 	 * any drivers bound to them (a key side effect)
@@ -1264,22 +1292,8 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
 
 	dev_dbg(&dev->dev, "%s nuking %s URBs\n", __func__,
 		skip_ep0 ? "non-ep0" : "all");
-	if (hcd->driver->check_bandwidth) {
-		/* First pass: Cancel URBs, leave endpoint pointers intact. */
-		for (i = skip_ep0; i < 16; ++i) {
-			usb_disable_endpoint(dev, i, false);
-			usb_disable_endpoint(dev, i + USB_DIR_IN, false);
-		}
-		/* Remove endpoints from the host controller internal state */
-		mutex_lock(hcd->bandwidth_mutex);
-		usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
-		mutex_unlock(hcd->bandwidth_mutex);
-		/* Second pass: remove endpoint pointers */
-	}
-	for (i = skip_ep0; i < 16; ++i) {
-		usb_disable_endpoint(dev, i, true);
-		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
-	}
+
+	usb_disable_device_endpoints(dev, skip_ep0);
 }
 
 /**
@@ -1537,10 +1551,7 @@ int usb_reset_configuration(struct usb_device *dev)
 	 * calls during probe() are fine
 	 */
 
-	for (i = 1; i < 16; ++i) {
-		usb_disable_endpoint(dev, i, true);
-		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
-	}
+	usb_disable_device_endpoints(dev, 1); /* skip ep0*/
 
 	config = dev->actconfig;
 	retval = 0;
@@ -1553,34 +1564,10 @@ int usb_reset_configuration(struct usb_device *dev)
 		mutex_unlock(hcd->bandwidth_mutex);
 		return -ENOMEM;
 	}
-	/* Make sure we have enough bandwidth for each alternate setting 0 */
-	for (i = 0; i < config->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = config->interface[i];
-		struct usb_host_interface *alt;
 
-		alt = usb_altnum_to_altsetting(intf, 0);
-		if (!alt)
-			alt = &intf->altsetting[0];
-		if (alt != intf->cur_altsetting)
-			retval = usb_hcd_alloc_bandwidth(dev, NULL,
-					intf->cur_altsetting, alt);
-		if (retval < 0)
-			break;
-	}
-	/* If not, reinstate the old alternate settings */
+	/* xHCI adds all endpoints in usb_hcd_alloc_bandwidth */
+	retval = usb_hcd_alloc_bandwidth(dev, config, NULL, NULL);
 	if (retval < 0) {
-reset_old_alts:
-		for (i--; i >= 0; i--) {
-			struct usb_interface *intf = config->interface[i];
-			struct usb_host_interface *alt;
-
-			alt = usb_altnum_to_altsetting(intf, 0);
-			if (!alt)
-				alt = &intf->altsetting[0];
-			if (alt != intf->cur_altsetting)
-				usb_hcd_alloc_bandwidth(dev, NULL,
-						alt, intf->cur_altsetting);
-		}
 		usb_enable_lpm(dev);
 		mutex_unlock(hcd->bandwidth_mutex);
 		return retval;
@@ -1589,8 +1576,12 @@ int usb_reset_configuration(struct usb_device *dev)
 			USB_REQ_SET_CONFIGURATION, 0,
 			config->desc.bConfigurationValue, 0,
 			NULL, 0, USB_CTRL_SET_TIMEOUT);
-	if (retval < 0)
-		goto reset_old_alts;
+	if (retval < 0) {
+		retval = usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
+		usb_enable_lpm(dev);
+		mutex_unlock(hcd->bandwidth_mutex);
+		return retval;
+	}
 	mutex_unlock(hcd->bandwidth_mutex);
 
 	/* re-init hc/hcd interface/endpoint state */
-- 
2.17.1

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Alan Stern]

On Fri, Aug 28, 2020 at 04:10:47PM +0300, Mathias Nyman wrote:
> >> Thanks, that seems to work! (Judging by the absence of my original
> >> issue with the xum1541 adapter; I haven't checked what's actually
> >> happening on the bus).
> >>
> >> Martin
> >>
> > 
> > Great, thanks.
> > 
> > Now this test code needs to be cleaned up a turned into a real patch
> 
> Can you test one more round?
> The code below is cleaned up but it also has a functional change.
> This version issues separate commands for dropping and adding endpoints.
> Previous code both did all on one command.
> 
> If it works I'll send it forward.
> 
> Thanks
> -Mathias
> 
> ---
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> Subject: [PATCH] usb: Fix out of sync data toggle if a configured device is
>  reconfigured
> 
> Userspace drivers that use a SetConfiguration() request to "lightweight"
> reset a already configured usb device might cause data toggles to get out
> of sync between the device and host, and the device becomes unusable.
> 
> The xHCI host requires endpoints to be dropped and added back to reset the
> toggle. USB core avoids these otherwise extra steps if the current active
> configuration is the same as the new requested configuration.
> 
> A SetConfiguration() request will reset the device side data toggles.
> Make sure usb core drops and adds back the endpoints in this case.
> 
> To avoid code duplication split the current usb_disable_device() function
> and reuse the endpoint specific part.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

This seems reasonable.  But can you add something to the kerneldoc for
usb_reset_configuration() explaining that if the routine fails, the
device will probablly be in an unusable state (endpoints disabled,
interfaces only partially enabled)?

Alan Stern

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 28.8.2020 18.55, Alan Stern wrote:
> On Fri, Aug 28, 2020 at 04:10:47PM +0300, Mathias Nyman wrote:
>>>> Thanks, that seems to work! (Judging by the absence of my original
>>>> issue with the xum1541 adapter; I haven't checked what's actually
>>>> happening on the bus).
>>>>
>>>> Martin
>>>>
>>>
>>> Great, thanks.
>>>
>>> Now this test code needs to be cleaned up a turned into a real patch
>>
>> Can you test one more round?
>> The code below is cleaned up but it also has a functional change.
>> This version issues separate commands for dropping and adding endpoints.
>> Previous code both did all on one command.
>>
>> If it works I'll send it forward.
>>
>> Thanks
>> -Mathias
>>
>> ---
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Subject: [PATCH] usb: Fix out of sync data toggle if a configured device is
>>  reconfigured
>>
>> Userspace drivers that use a SetConfiguration() request to "lightweight"
>> reset a already configured usb device might cause data toggles to get out
>> of sync between the device and host, and the device becomes unusable.
>>
>> The xHCI host requires endpoints to be dropped and added back to reset the
>> toggle. USB core avoids these otherwise extra steps if the current active
>> configuration is the same as the new requested configuration.
>>
>> A SetConfiguration() request will reset the device side data toggles.
>> Make sure usb core drops and adds back the endpoints in this case.
>>
>> To avoid code duplication split the current usb_disable_device() function
>> and reuse the endpoint specific part.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> This seems reasonable.  But can you add something to the kerneldoc for
> usb_reset_configuration() explaining that if the routine fails, the
> device will probablly be in an unusable state (endpoints disabled,
> interfaces only partially enabled)?
> 
> Alan Stern
> 

Good point, will add

Thanks
Mathias

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

On Fri, Aug 28, 2020 at 3:07 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Can you test one more round?
> The code below is cleaned up but it also has a functional change.
> This version issues separate commands for dropping and adding endpoints.
> Previous code both did all on one command.
>
> If it works I'll send it forward.

Looks good - thanks! (Disclaimer: My original issue is gone; I haven't
checked if it breaks anything else, but didn't notice any problems,
either).

Martin

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman]

On 28.8.2020 21.04, Martin Thierer wrote:
> On Fri, Aug 28, 2020 at 3:07 PM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>> Can you test one more round?
>> The code below is cleaned up but it also has a functional change.
>> This version issues separate commands for dropping and adding endpoints.
>> Previous code both did all on one command.
>>
>> If it works I'll send it forward.
> 
> Looks good - thanks! (Disclaimer: My original issue is gone; I haven't
> checked if it breaks anything else, but didn't notice any problems,
> either).

Thanks for testing. 
Can I add a Tested-by: Martin Thierer <mthierer@gmail.com> tag to the patch?

-Mathias 

Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Martin Thierer]

On Mon, Aug 31, 2020 at 8:37 AM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> Thanks for testing.
> Can I add a Tested-by: Martin Thierer <mthierer@gmail.com> tag to the patch?

Sure. But as I wrote, that was a pretty marginal "test"... :)

Martin