->drop_endpoint being called for disabled endpoints

[Felipe Balbi <balbi@xxxxxx>]

Cc: Linux USB Mailing List <linux-usb@xxxxxxxxxxxxxxx>
Bcc: 
Subject: usbcore calling ->drop_endpoint() for disabled eps
Reply-To: balbi@xxxxxx

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

Attachment: signature.asc
Description: Digital signature