Re: [PATCH v1] usb: core: fix pipe creation for get_bMaxPacketSize0

[Alan Stern]

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
>