| Message ID | 20200831114649.24183-1-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Fix out of sync data toggle if a configured device is reconfigured | expand |
Hi, Mathias Nyman <mathias.nyman@linux.intel.com> writes: > Userspace drivers that use a SetConfiguration() request to "lightweight" > reset a already configured usb device might cause data toggles to get out ^ an > 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. it looks like the refactoring is unrelated to the bug fix, perhaps? But then again, it would mean adding more duplication just for the sake of keeping bug fixes as pure bug fixes. No strong opinion. > Cc: stable <stable@vger.kernel.org> missing fixes? > Tested-by: Martin Thierer <mthierer@gmail.com> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/core/message.c | 92 ++++++++++++++++++-------------------- > 1 file changed, 43 insertions(+), 49 deletions(-) > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 6197938dcc2d..a1f67efc261f 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) { > + maybe remove this blank line? > + /* 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); > + } maybe a blank line here? > + /* 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); > + } maybe a blank line here? > + /* 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 > @@ -1522,6 +1536,9 @@ EXPORT_SYMBOL_GPL(usb_set_interface); > * The caller must own the device lock. > * > * Return: Zero on success, else a negative error code. > + * > + * If this routine fails the device will probably be in an unusable state > + * with endpoints disabled, and interfaces only partially enabled. should you force U3 in that case?
On 31.8.2020 15.02, Felipe Balbi wrote: > > Hi, > > Mathias Nyman <mathias.nyman@linux.intel.com> writes: >> Userspace drivers that use a SetConfiguration() request to "lightweight" >> reset a already configured usb device might cause data toggles to get out > ^ > an true > >> 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. > > it looks like the refactoring is unrelated to the bug fix, perhaps? But > then again, it would mean adding more duplication just for the sake of > keeping bug fixes as pure bug fixes. No strong opinion. > I like it like this :) Avoids code duplication, and is simple enough for stable releases compared to complete usb_set_configuration() and usb_reset_configuration(), which was an option. >> Cc: stable <stable@vger.kernel.org> > > missing fixes? Not really, this has just never really worked, nothing broke it. Closest would be one of the patches that add usb3 specific code to usb_reset_configuration() in 2.6 kernel. > >> Tested-by: Martin Thierer <mthierer@gmail.com> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> --- >> drivers/usb/core/message.c | 92 ++++++++++++++++++-------------------- >> 1 file changed, 43 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c >> index 6197938dcc2d..a1f67efc261f 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) { >> + > > maybe remove this blank line? yes, this one was unintentional. > >> + /* 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); >> + } > > maybe a blank line here? Here I didn't want to alter the original style, like the next one. > >> + /* 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); >> + } > > maybe a blank line here? > >> + /* 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 >> @@ -1522,6 +1536,9 @@ EXPORT_SYMBOL_GPL(usb_set_interface); >> * The caller must own the device lock. >> * >> * Return: Zero on success, else a negative error code. >> + * >> + * If this routine fails the device will probably be in an unusable state >> + * with endpoints disabled, and interfaces only partially enabled. > > should you force U3 in that case? > It didn't use to in the failing case, nor does usb_set_configuration() on failure. I assumed the caller would handle a failure in their preferred way. Probably reset the device, or perhaps as you suggest set it to U3 with a usb_autosuspend_device()? -Mathias
On Mon, Aug 31, 2020 at 02:46:49PM +0300, Mathias Nyman wrote: > 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. You should mention usb_reset_configuration() here. After all, that's where most of the changes in this patch occur. > > 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. > > Cc: stable <stable@vger.kernel.org> > Tested-by: Martin Thierer <mthierer@gmail.com> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > @@ -1589,8 +1579,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; That's not right; we want to return the original error code. Not 0, which usb_hcd_alloc_bandwidth() will probably give us. Just remove the "retval =" from the call above. Alan Stern
On 31.8.2020 18.15, Alan Stern wrote: > On Mon, Aug 31, 2020 at 02:46:49PM +0300, Mathias Nyman wrote: >> 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. > > You should mention usb_reset_configuration() here. After all, that's > where most of the changes in this patch occur. > >> >> 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. >> >> Cc: stable <stable@vger.kernel.org> >> Tested-by: Martin Thierer <mthierer@gmail.com> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> --- > >> @@ -1589,8 +1579,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; > > That's not right; we want to return the original error code. Not 0, > which usb_hcd_alloc_bandwidth() will probably give us. Just remove > the "retval =" from the call above. > Thanks, somehow I didn't notice that. -Mathias
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 6197938dcc2d..a1f67efc261f 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); } /** @@ -1522,6 +1536,9 @@ EXPORT_SYMBOL_GPL(usb_set_interface); * The caller must own the device lock. * * Return: Zero on success, else a negative error code. + * + * If this routine fails the device will probably be in an unusable state + * with endpoints disabled, and interfaces only partially enabled. */ int usb_reset_configuration(struct usb_device *dev) { @@ -1537,10 +1554,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 +1567,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 +1579,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 */