| Message ID | 20250203105840.17539-1-eichest@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] usb: core: fix pipe creation for get_bMaxPacketSize0 | expand |
On Mon, Feb 03, 2025 at 11:58:24AM +0100, Stefan Eichenberger wrote: > From: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > When usb_control_msg is used in the get_bMaxPacketSize0 function, the > USB pipe does not include the endpoint device number. This can cause > failures when a usb hub port is reinitialized after encountering a bad > cable connection. As a result, the system logs the following error > messages: > usb usb2-port1: cannot reset (err = -32) > usb usb2-port1: Cannot enable. Maybe the USB cable is bad? > usb usb2-port1: attempt power cycle > usb 2-1: new high-speed USB device number 5 using ci_hdrc > usb 2-1: device descriptor read/8, error -71 > > The problem began after commit 85d07c556216 ("USB: core: Unite old > scheme and new scheme descriptor reads"). There > usb_get_device_descriptor was replaced with get_bMaxPacketSize0. Unlike > usb_get_device_descriptor, the get_bMaxPacketSize0 function uses the > macro usb_rcvaddr0pipe, which does not include the endpoint device > number. usb_get_device_descriptor, on the other hand, used the macro > usb_rcvctrlpipe, which includes the endpoint device number. > > By modifying the get_bMaxPacketSize0 function to use usb_rcvctrlpipe > instead of usb_rcvaddr0pipe, the issue can be resolved. This change will > ensure that the endpoint device number is included in the USB pipe, > preventing reinitialization failures. If the endpoint has not set the > device number yet, it will still work because the device number is 0 in > udev. > > Cc: stable@vger.kernel.org > Fixes: 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads") > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> > --- > Before commit 85d07c556216 ("USB: core: Unite old scheme and new scheme > descriptor reads") usb_rcvaddr0pipe was used in hub_port_init. With this > proposed change, usb_rcvctrlpipe will be used which includes devnum for > the pipe. I'm not sure if this might have some side effects. However, my > understanding is that devnum is set to the right value (might also be 0 > if not initialised) before get_bMaxPacketSize0 is called. Therefore, > this should work but please let me know if I'm wrong on this. I believe you are correct. This is a pretty glaring mistake; I'm surprised that it hasn't show up before now. Thanks for fixing it. Reviewed-by: Alan Stern <stern@rowland.harvard.edu> In fact, it looks like usb_sndaddr0pipe is used in only one place and it can similarly be replaced by usb_sndctrlpipe, if you want to make that change as well (although this usage is not actually a mistake). Alan Stern > --- > drivers/usb/core/hub.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index c3f839637cb5..59e38780f76d 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -4698,7 +4698,6 @@ void usb_ep0_reinit(struct usb_device *udev) > EXPORT_SYMBOL_GPL(usb_ep0_reinit); > > #define usb_sndaddr0pipe() (PIPE_CONTROL << 30) > -#define usb_rcvaddr0pipe() ((PIPE_CONTROL << 30) | USB_DIR_IN) > > static int hub_set_address(struct usb_device *udev, int devnum) > { > @@ -4804,7 +4803,7 @@ static int get_bMaxPacketSize0(struct usb_device *udev, > for (i = 0; i < GET_MAXPACKET0_TRIES; ++i) { > /* Start with invalid values in case the transfer fails */ > buf->bDescriptorType = buf->bMaxPacketSize0 = 0; > - rc = usb_control_msg(udev, usb_rcvaddr0pipe(), > + rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, > USB_DT_DEVICE << 8, 0, > buf, size, > -- > 2.45.2 >
On Mon, Feb 03, 2025 at 11:02:37AM -0500, Alan Stern wrote: > On Mon, Feb 03, 2025 at 11:58:24AM +0100, Stefan Eichenberger wrote: > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > > > When usb_control_msg is used in the get_bMaxPacketSize0 function, the > > USB pipe does not include the endpoint device number. This can cause > > failures when a usb hub port is reinitialized after encountering a bad > > cable connection. As a result, the system logs the following error > > messages: > > usb usb2-port1: cannot reset (err = -32) > > usb usb2-port1: Cannot enable. Maybe the USB cable is bad? > > usb usb2-port1: attempt power cycle > > usb 2-1: new high-speed USB device number 5 using ci_hdrc > > usb 2-1: device descriptor read/8, error -71 > > > > The problem began after commit 85d07c556216 ("USB: core: Unite old > > scheme and new scheme descriptor reads"). There > > usb_get_device_descriptor was replaced with get_bMaxPacketSize0. Unlike > > usb_get_device_descriptor, the get_bMaxPacketSize0 function uses the > > macro usb_rcvaddr0pipe, which does not include the endpoint device > > number. usb_get_device_descriptor, on the other hand, used the macro > > usb_rcvctrlpipe, which includes the endpoint device number. > > > > By modifying the get_bMaxPacketSize0 function to use usb_rcvctrlpipe > > instead of usb_rcvaddr0pipe, the issue can be resolved. This change will > > ensure that the endpoint device number is included in the USB pipe, > > preventing reinitialization failures. If the endpoint has not set the > > device number yet, it will still work because the device number is 0 in > > udev. > > > > Cc: stable@vger.kernel.org > > Fixes: 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads") > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > --- > > Before commit 85d07c556216 ("USB: core: Unite old scheme and new scheme > > descriptor reads") usb_rcvaddr0pipe was used in hub_port_init. With this > > proposed change, usb_rcvctrlpipe will be used which includes devnum for > > the pipe. I'm not sure if this might have some side effects. However, my > > understanding is that devnum is set to the right value (might also be 0 > > if not initialised) before get_bMaxPacketSize0 is called. Therefore, > > this should work but please let me know if I'm wrong on this. > > I believe you are correct. This is a pretty glaring mistake; I'm > surprised that it hasn't show up before now. Thanks for fixing it. > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > > In fact, it looks like usb_sndaddr0pipe is used in only one place and it > can similarly be replaced by usb_sndctrlpipe, if you want to make that > change as well (although this usage is not actually a mistake). Thanks a lot for the feedback. I was also thinking, if this macro is required. I will wait a few more days to see if no one else has any objection and if not I will send an additional patch to also replace usb_sndaddr0pipe with usb_sndctrlpipe. Regards, Stefan
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index c3f839637cb5..59e38780f76d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4698,7 +4698,6 @@ void usb_ep0_reinit(struct usb_device *udev) EXPORT_SYMBOL_GPL(usb_ep0_reinit); #define usb_sndaddr0pipe() (PIPE_CONTROL << 30) -#define usb_rcvaddr0pipe() ((PIPE_CONTROL << 30) | USB_DIR_IN) static int hub_set_address(struct usb_device *udev, int devnum) { @@ -4804,7 +4803,7 @@ static int get_bMaxPacketSize0(struct usb_device *udev, for (i = 0; i < GET_MAXPACKET0_TRIES; ++i) { /* Start with invalid values in case the transfer fails */ buf->bDescriptorType = buf->bMaxPacketSize0 = 0; - rc = usb_control_msg(udev, usb_rcvaddr0pipe(), + rc = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, USB_DT_DEVICE << 8, 0, buf, size,