->drop_endpoint being called for disabled endpoints

[Felipe Balbi]

Cc: Linux USB Mailing List <linux-usb@vger.kernel.org>
Bcc: 
Subject: usbcore calling ->drop_endpoint() for disabled eps
Reply-To: ba...@ti.com

Hi all,

I keep seeing the following messages when transferring data to any USB3
mass storage I have (tried 3 different ones already):

[618002.014556] xhci_hcd 0000:03:00.0: xHCI xhci_drop_endpoint called with 
disabled ep ffff88012976cd40
[618002.014562] xhci_hcd 0000:03:00.0: xHCI xhci_drop_endpoint called with 
disabled ep ffff88012976cd80

It looks like usbcore is calling ->drop_endpoint() for endpoints which
are already disabled. I wonder if that's something legal to do or if we
want to protect calls to ->drop_endpoint() with if (ep->enabled) check.

I'll add a WARN_ONCE() later just to check who's to blame here, but it's
a pretty annoying message to see all the time. :-)

How about something like below ?

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 99b34a3..c2f8fc5 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1781,10 +1781,10 @@ int usb_hcd_alloc_bandwidth(struct usb_device *udev,
        if (!new_config && !cur_alt) {
                for (i = 1; i < 16; ++i) {
                        ep = udev->ep_out[i];
-                       if (ep)
+                       if (ep && ep->enabled)
                                hcd->driver->drop_endpoint(hcd, udev, ep);
                        ep = udev->ep_in[i];
-                       if (ep)
+                       if (ep && ep->enabled)
                                hcd->driver->drop_endpoint(hcd, udev, ep);
                }
                hcd->driver->check_bandwidth(hcd, udev);
@@ -1802,13 +1802,13 @@ int usb_hcd_alloc_bandwidth(struct usb_device *udev,
                 */
                for (i = 1; i < 16; ++i) {
                        ep = udev->ep_out[i];
-                       if (ep) {
+                       if (ep && ep->enabled) {
                                ret = hcd->driver->drop_endpoint(hcd, udev, ep);
                                if (ret < 0)
                                        goto reset;
                        }
                        ep = udev->ep_in[i];
-                       if (ep) {
+                       if (ep && ep->enabled) {
                                ret = hcd->driver->drop_endpoint(hcd, udev, ep);
                                if (ret < 0)
                                        goto reset;
@@ -1827,7 +1827,12 @@ int usb_hcd_alloc_bandwidth(struct usb_device *udev,
                                alt = first_alt;
 
                        for (j = 0; j < alt->desc.bNumEndpoints; j++) {
-                               ret = hcd->driver->add_endpoint(hcd, udev, 
&alt->endpoint[j]);
+                               ep = &alt->endpoint[i];
+
+                               if (!ep || !ep->enabled)
+                                       continue;
+
+                               ret = hcd->driver->add_endpoint(hcd, udev, ep);
                                if (ret < 0)
                                        goto reset;
                        }
@@ -1856,8 +1861,12 @@ int usb_hcd_alloc_bandwidth(struct usb_device *udev,
 
                /* Drop all the endpoints in the current alt setting */
                for (i = 0; i < cur_alt->desc.bNumEndpoints; i++) {
-                       ret = hcd->driver->drop_endpoint(hcd, udev,
-                                       &cur_alt->endpoint[i]);
+                       ep = &cur_alt->endpoint[i];
+
+                       if (!ep || !ep->enabled)
+                               continue;
+
+                       ret = hcd->driver->drop_endpoint(hcd, udev, ep);
                        if (ret < 0)
                                goto reset;
                }

-- 
balbi

signature.asc
Description: Digital signature