On Mon, Feb 03, 2025 at 11:58:24AM +0100, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Fixes: 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads")
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx>
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
>
