[PATCH] usb_set_interface: correct toggle reset

this is a patch to prevent usb_set_interface() from erroneously resetting
the toggles for all endpoints instead of only the affected ones from the
requested interface/altsetting. I've also added some missing parentheses
to related macros in usb.h as I prefered not to take special care for
nasty side-effects ;-)

Patch below was created against 2.4.18-pre9 (with some lines of offset it
applies to 2.5.4-pre5 as well).

Tested in multi-interface configuration to provide evidence it:
* correctly identifies the affected endpoints and resets the toggles
* doesn't touch endpoints from other interfaces
* provides correct handling of shared EP0
* solves an issue I had with 2.4.18-pre9 where setting one interface
  occasionally caused transfers on other interface to hang due to lost
  toggle synchronisation

Despite being a pure bugfix, well localized and (IMHO) pretty obviously
correct wrt. USB-spec, I'd like to suggest including this in early
2.4.19-pre. Just in case some existing driver would somehow workaround
the currently wrong behavior and might break with this fix. And it's
not very urgent right now, as we are probably close to 2.4.18-rc1.

Regards,
Martin

diff --git a/drivers/usb/usb.c b/drivers/usb/usb.c
index d6d446e84d86f0cfd4dade0110af8b66dfe9ac01..075e4aa46d7eb1c147b8e60258f9c6810e0cde47 100644 (file)
--- a/drivers/usb/usb.c
+++ b/drivers/usb/usb.c
@@ -2322,7 +2322,8 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
 int usb_set_interface(struct usb_device *dev, int interface, int alternate)
 {
        struct usb_interface *iface;
-       int ret;
+       struct usb_interface_descriptor *iface_as;
+       int i, ret;
 
        iface = usb_ifnum_to_if(dev, interface);
        if (!iface) {
@@ -2344,8 +2345,30 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
                return ret;
 
        iface->act_altsetting = alternate;
-       dev->toggle[0] = 0;     /* 9.1.1.5 says to do this */
-       dev->toggle[1] = 0;
+
+       /* 9.1.1.5: reset toggles for all endpoints affected by this iface-as
+        *
+        * Note:
+        * Despite EP0 is always present in all interfaces/AS, the list of
+        * endpoints from the descriptor does not contain EP0. Due to its
+        * omnipresence one might expect EP0 being considered "affected" by
+        * any SetInterface request and hence assume toggles need to be reset.
+        * However, EP0 toggles are re-synced for every individual transfer
+        * during the SETUP stage - hence EP0 toggles are "don't care" here.
+        */
+
+       iface_as = &iface->altsetting[alternate];
+       for (i = 0; i < iface_as->bNumEndpoints; i++) {
+               u8      ep = iface_as->endpoint[i].bEndpointAddress;
+
+               usb_settoggle(dev, ep&USB_ENDPOINT_NUMBER_MASK, usb_endpoint_out(ep), 0);
+       }
+
+       /* usb_set_maxpacket() sets the maxpacket size for all EP in all
+        * interfaces but it shouldn't do any harm here: we have changed
+        * the AS for the requested interface only, hence for unaffected
+        * interfaces it's just re-application of still-valid values.
+        */
        usb_set_maxpacket(dev);
        return 0;
 }

diff --git a/include/linux/usb.h b/include/linux/usb.h
index b577fce7d0ee125e24a6a4daea51ef1d6d400549..82aeb9a646b1c66b531741b4aa523acfc640f6a5 100644 (file)
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1156,12 +1156,12 @@ const struct usb_device_id *usb_match_id(struct usb_device *dev,
 #define PIPE_DEVEP_MASK                0x0007ff00
 
 /* The D0/D1 toggle bits */
-#define usb_gettoggle(dev, ep, out) (((dev)->toggle[out] >> ep) & 1)
-#define        usb_dotoggle(dev, ep, out)  ((dev)->toggle[out] ^= (1 << ep))
-#define usb_settoggle(dev, ep, out, bit) ((dev)->toggle[out] = ((dev)->toggle[out] & ~(1 << ep)) | ((bit) << ep))
+#define usb_gettoggle(dev, ep, out) (((dev)->toggle[out] >> (ep)) & 1)
+#define        usb_dotoggle(dev, ep, out)  ((dev)->toggle[out] ^= (1 << (ep)))
+#define usb_settoggle(dev, ep, out, bit) ((dev)->toggle[out] = ((dev)->toggle[out] & ~(1 << (ep))) | ((bit) << (ep)))
 
 /* Endpoint halt control/status */
-#define usb_endpoint_out(ep_dir)       (((ep_dir >> 7) & 1) ^ 1)
+#define usb_endpoint_out(ep_dir)       ((((ep_dir) >> 7) & 1) ^ 1)
 #define usb_endpoint_halt(dev, ep, out) ((dev)->halted[out] |= (1 << (ep)))
 #define usb_endpoint_running(dev, ep, out) ((dev)->halted[out] &= ~(1 << (ep)))
 #define usb_endpoint_halted(dev, ep, out) ((dev)->halted[out] & (1 << (ep)))