The patch works great. Thanks!
On Wed, Jan 14, 2009 at 2:55 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 14 Jan 2009, Dan Streetman wrote:
>
>> The previous patch (as1197), to not reset the data toggle during driver
>> unbinding when an interface is already in altsetting 0, has introduced an
>> intermittent loss of packets due to data toggle mismatch. Specifically, the
>> ehci and ohci drivers do not use the device toggle field to actually track the
>> current toggle; they use hardware to track the device toggle. So avoiding the
>> call to usb_settoggle() in usb_endpoint_enable() isn't enough.
>
> Oho! You are right, although you didn't express the reason very
> clearly. usb_disable_endpoint's call to usb_hcd_disable_endpoint
> changes the hardware state in EHCI and OHCI controllers without
> changing the corresponding state in the device.
>
>> I think there is an easy way to fix this, and a hard way. The hard way (and
>> more correct way IMHO) is to make the data toggles HCD-internal, which is what
>> they really should be. The only interface that drivers or the USB core needs is
>> the ability to reset the data toggle after a set interface or a clear halt.
>> This would be a relatively large change, affecting all the HC drivers, the USB
>> core, and many of the USB device drivers.
>>
>> The easy way is what the patch below does. Currently, dev->toggle has 2
>> different meanings:
>> -For uhci, it is the actual toggle value
>> -For ehci and ohci, it is a flag to indicate if the toggle should be reset
>> This patch changes it to have 3 meanings:
>> -For uhci, it is the actual toggle value
>> -For ehci and ohci while there is a qh or ed present for an endpoint, it is a
>> flag to indicate if the toggle should be reset
>> -For ehci and ohci when the endpoint is disabled, it stores the last toggle
>> value, which is restored when the endpoint is re-enabled and a new qh or ed is
>> created for the endpoint. If the toggle really should be reset, it happens
>> normally during the call to usb_enable_endpoint().
>
> No, neither of your suggestions is right. The real problem is to
> distinguish between enabling/disabling endpoints just in the usbcore
> data structures or in both the core and the hardware. The reset_toggle
> argument added by as1197 was an attempt to do this, but it targeted
> only the enable-endpoint path. We need a corresponding change for the
> disable_endpoint path as well.
>
> Does this patch fix everything?
>
> Alan Stern
>
>
>
> Index: usb-2.6/drivers/usb/core/usb.h
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/usb.h
> +++ usb-2.6/drivers/usb/core/usb.h
> @@ -15,9 +15,10 @@ extern void usb_enable_endpoint(struct u
> struct usb_host_endpoint *ep, bool reset_toggle);
> extern void usb_enable_interface(struct usb_device *dev,
> struct usb_interface *intf, bool reset_toggles);
> -extern void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr);
> +extern void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr,
> + bool reset_hardware);
> extern void usb_disable_interface(struct usb_device *dev,
> - struct usb_interface *intf);
> + struct usb_interface *intf, bool reset_hardware);
> extern void usb_release_interface_cache(struct kref *ref);
> extern void usb_disable_device(struct usb_device *dev, int skip_ep0);
> extern int usb_deauthorize_device(struct usb_device *);
> Index: usb-2.6/drivers/usb/core/message.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/message.c
> +++ usb-2.6/drivers/usb/core/message.c
> @@ -1039,14 +1039,15 @@ static void remove_intf_ep_devs(struct u
> * @dev: the device whose endpoint is being disabled
> * @epaddr: the endpoint's address. Endpoint number for output,
> * endpoint number + USB_DIR_IN for input
> + * @reset_hardware: flag to erase any endpoint state stored in the
> + * controller hardware
> *
> - * Deallocates hcd/hardware state for this endpoint ... and nukes all
> - * pending urbs.
> - *
> - * If the HCD hasn't registered a disable() function, this sets the
> - * endpoint's maxpacket size to 0 to prevent further submissions.
> + * Disables the endpoint for URB submission and nukes all pending URBs.
> + * If @reset_hardware is set then also deallocates hcd/hardware state
> + * for the endpoint.
> */
> -void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr)
> +void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr,
> + bool reset_hardware)
> {
> unsigned int epnum = epaddr & USB_ENDPOINT_NUMBER_MASK;
> struct usb_host_endpoint *ep;
> @@ -1056,15 +1057,18 @@ void usb_disable_endpoint(struct usb_dev
>
> if (usb_endpoint_out(epaddr)) {
> ep = dev->ep_out[epnum];
> - dev->ep_out[epnum] = NULL;
> + if (reset_hardware)
> + dev->ep_out[epnum] = NULL;
> } else {
> ep = dev->ep_in[epnum];
> - dev->ep_in[epnum] = NULL;
> + if (reset_hardware)
> + dev->ep_in[epnum] = NULL;
> }
> if (ep) {
> ep->enabled = 0;
> usb_hcd_flush_endpoint(dev, ep);
> - usb_hcd_disable_endpoint(dev, ep);
> + if (reset_hardware)
> + usb_hcd_disable_endpoint(dev, ep);
> }
> }
>
> @@ -1072,17 +1076,21 @@ void usb_disable_endpoint(struct usb_dev
> * usb_disable_interface -- Disable all endpoints for an interface
> * @dev: the device whose interface is being disabled
> * @intf: pointer to the interface descriptor
> + * @reset_hardware: flag to erase any endpoint state stored in the
> + * controller hardware
> *
> * Disables all the endpoints for the interface's current altsetting.
> */
> -void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf)
> +void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
> + bool reset_hardware)
> {
> struct usb_host_interface *alt = intf->cur_altsetting;
> int i;
>
> for (i = 0; i < alt->desc.bNumEndpoints; ++i) {
> usb_disable_endpoint(dev,
> - alt->endpoint[i].desc.bEndpointAddress);
> + alt->endpoint[i].desc.bEndpointAddress,
> + reset_hardware);
> }
> }
>
> @@ -1103,8 +1111,8 @@ void usb_disable_device(struct usb_devic
> dev_dbg(&dev->dev, "%s nuking %s URBs\n", __func__,
> skip_ep0 ? "non-ep0" : "all");
> for (i = skip_ep0; i < 16; ++i) {
> - usb_disable_endpoint(dev, i);
> - usb_disable_endpoint(dev, i + USB_DIR_IN);
> + usb_disable_endpoint(dev, i, true);
> + usb_disable_endpoint(dev, i + USB_DIR_IN, true);
> }
> dev->toggle[0] = dev->toggle[1] = 0;
>
> @@ -1274,7 +1282,7 @@ int usb_set_interface(struct usb_device
> remove_intf_ep_devs(iface);
> usb_remove_sysfs_intf_files(iface);
> }
> - usb_disable_interface(dev, iface);
> + usb_disable_interface(dev, iface, true);
>
> iface->cur_altsetting = alt;
>
> @@ -1353,8 +1361,8 @@ int usb_reset_configuration(struct usb_d
> */
>
> for (i = 1; i < 16; ++i) {
> - usb_disable_endpoint(dev, i);
> - usb_disable_endpoint(dev, i + USB_DIR_IN);
> + usb_disable_endpoint(dev, i, true);
> + usb_disable_endpoint(dev, i + USB_DIR_IN, true);
> }
>
> config = dev->actconfig;
> Index: usb-2.6/drivers/usb/core/driver.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/driver.c
> +++ usb-2.6/drivers/usb/core/driver.c
> @@ -284,7 +284,7 @@ static int usb_unbind_interface(struct d
> * supports "soft" unbinding.
> */
> if (!driver->soft_unbind)
> - usb_disable_interface(udev, intf);
> + usb_disable_interface(udev, intf, false);
>
> driver->disconnect(intf);
> usb_cancel_queued_reset(intf);
> Index: usb-2.6/drivers/usb/core/hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hub.c
> +++ usb-2.6/drivers/usb/core/hub.c
> @@ -2382,8 +2382,8 @@ static int hub_port_debounce(struct usb_
>
> void usb_ep0_reinit(struct usb_device *udev)
> {
> - usb_disable_endpoint(udev, 0 + USB_DIR_IN);
> - usb_disable_endpoint(udev, 0 + USB_DIR_OUT);
> + usb_disable_endpoint(udev, 0 + USB_DIR_IN, true);
> + usb_disable_endpoint(udev, 0 + USB_DIR_OUT, true);
> usb_enable_endpoint(udev, &udev->ep0, true);
> }
> EXPORT_SYMBOL_GPL(usb_ep0_reinit);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
