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

[Stefan Eichenberger]

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@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).

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