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
