Re: [PATCH v3] usb: core: hub: Add quirks for reducing device address timeout

[Hardik Gajjar]

On Wed, Oct 11, 2023 at 11:02:27AM +0200, Greg KH wrote:
> On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote:
> > Currently, the timeout for the set address command is fixed at
> > 5 seconds in the xhci driver. This means the host waits up to 5
> > seconds to receive a response for the set_address command from
> > the device.
> > 
> > In the automotive context, most smartphone enumerations, including
> > screen projection, should ideally complete within 3 seconds.
> > Achieving this is impossible in scenarios where the set_address is
> > not successful and waits for a timeout.
> > 
> > The shortened address device timeout quirks provide the flexibility
> > to align with a 3-second time limit in the event of errors.
> > By swiftly triggering a failure response and swiftly initiating
> > retry procedures, these quirks ensure efficient and rapid recovery,
> > particularly in automotive contexts where rapid smartphone enumeration
> > and screen projection are vital.
> > 
> > The quirk will set the timeout to 500 ms from 5 seconds.
> > 
> > To use the quirk, please write "vendor_id:product_id:p" to
> > /sys/bus/usb/drivers/hub/module/parameter/quirks
> > 
> > For example,
> > echo "0x2c48:0x0132:p" > /sys/bus/usb/drivers/hub/module/parameter/quirks"
> > 
> > Signed-off-by: Hardik Gajjar <hgajjar@xxxxxxxxxxxxxx>
> > ---
> > changes since version 1:
> > 	- implement quirk instead of new API in xhci driver
> > 
> > changes since version 2:
> > 	- Add documentation for the new quirk.
> > 	- Define the timeout unit in milliseconds in variable names and function arguments.
> > 	- Change the xHCI command timeout from HZ (jiffies) to milliseconds.
> > 	- Add APTIV usb hub vendor and product ID in device quirk list
> > 	- Adding some other comments for clarity
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  3 +++
> >  drivers/usb/core/hub.c                        | 13 ++++++++--
> >  drivers/usb/core/quirks.c                     |  6 +++++
> >  drivers/usb/host/xhci-mem.c                   |  2 ++
> >  drivers/usb/host/xhci-ring.c                  | 11 ++++----
> >  drivers/usb/host/xhci.c                       | 25 +++++++++++++------
> >  drivers/usb/host/xhci.h                       |  6 +++--
> >  include/linux/usb/hcd.h                       |  5 ++--
> >  include/linux/usb/quirks.h                    |  3 +++
> >  9 files changed, 56 insertions(+), 18 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 0a1731a0f0ef..44732d179bce 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6817,6 +6817,9 @@
> >  					pause after every control message);
> >  				o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra
> >  					delay after resetting its port);
> > +				p = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT ( Timeout
> > +					of set_address command reduce from 5000 ms
> > +					to 500 ms
> 
> No trailing ")" character?  And no need for the extra space after the
> new "(" one, right?
>

Okay, update it. Interestingly, the 'scripts/checkpatch.pl' is not reporting such warnings

> also, this should say it is "reducing", not "reduce"?
> 
> >  			Example: quirks=0781:5580:bk,0a5c:5834:gij
> >  
> >  	usbhid.mousepoll=
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3c54b218301c..c0d727398cd1 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -54,6 +54,9 @@
> >  #define USB_TP_TRANSMISSION_DELAY_MAX	65535	/* ns */
> >  #define USB_PING_RESPONSE_TIME		400	/* ns */
> >  
> > +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS	5000 /* 5000ms */
> 
> This comes from the USB specification, right?  If so, can you add the
> USB spec location for it in the comment?
> 
> > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS	500  /* 500ms */
> 
> This is for "broken" devices, right?
> 
> > +
> >  /* Protect struct usb_device->state and ->children members
> >   * Note: Both are also protected by ->dev.sem, except that ->state can
> >   * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
> > @@ -4626,8 +4629,14 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit);
> >  static int hub_set_address(struct usb_device *udev, int devnum)
> >  {
> >  	int retval;
> > +	unsigned int timeout_ms = USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS;
> >  	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> >  
> > +	struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
> 
> Did you run https://urldefense.proofpoint.com/v2/url?u=http-3A__checkpatch.pl&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=6Am_63OPDGq3Jc3NvsSZFUkSZLQk4gIIiJUx4nh1LoSZtxv-gUlOsoQY-Ooe2uuX&s=gQ_CCUZ1pM8CXK5oamzWh365rtZOP3DiS3ne4Rcj__A&e= on your change?  It should say the extra blank
> line you added here isn't needed (if not, it shouldn't be added anyway,
> that's not good kernel coding style.)

Yes, I have executed 'scripts/checkpatch.pl' with the --strict option, but there
is no such warning. Am I missing something? The following is the output of
checkpatch.pl on my build machine:
------------------------------------------------------
scripts/checkpatch.pl --strict v3-0001-usb-core-hub-Add-quirks-for-reducing-device-addre.patch 
WARNING: function definition argument 'struct usb_hcd *' should also have an identifier name
#285: FILE: include/linux/usb/hcd.h:376:
+	int	(*address_device)(struct usb_hcd *, struct usb_device *udev,

total: 0 errors, 1 warnings, 0 checks, 193 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

v3-0001-usb-core-hub-Add-quirks-for-reducing-device-addre.patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.
-------------------------------------------------------

> 
> > +
> > +	if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT)
> > +		timeout_ms = USB_SHORT_ADDR_DEVICE_TIMEOUT_MS;
> > +
> >  	/*
> >  	 * The host controller will choose the device address,
> >  	 * instead of the core having chosen it earlier
> > @@ -4639,11 +4648,11 @@ static int hub_set_address(struct usb_device *udev, int devnum)
> >  	if (udev->state != USB_STATE_DEFAULT)
> >  		return -EINVAL;
> >  	if (hcd->driver->address_device)
> > -		retval = hcd->driver->address_device(hcd, udev);
> > +		retval = hcd->driver->address_device(hcd, udev, timeout_ms);
> >  	else
> >  		retval = usb_control_msg(udev, usb_sndaddr0pipe(),
> >  				USB_REQ_SET_ADDRESS, 0, devnum, 0,
> > -				NULL, 0, USB_CTRL_SET_TIMEOUT);
> > +				NULL, 0, timeout_ms);
> >  	if (retval == 0) {
> >  		update_devnum(udev, devnum);
> >  		/* Device now using proper address. */
> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index 15e9bd180a1d..a1137740b496 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp)
> >  			case 'o':
> >  				flags |= USB_QUIRK_HUB_SLOW_RESET;
> >  				break;
> > +			case 'p':
> > +				flags |= USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT;
> > +				break;
> >  			/* Ignore unrecognized flag characters */
> >  			}
> >  		}
> > @@ -542,6 +545,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> >  	/* INTEL VALUE SSD */
> >  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
> >  
> > +	/* APTIV AUTOMOTIVE HUB */
> > +	{ USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },
> > +
> >  	{ }  /* terminating entry must be last */
> >  };
> >  
> 
> I miss where you add the timeout delay in the other host controller
> drivers.  Why only xhci?  What about uhci/ohci/dwc3/etc.?
>

It is in hub.c file. Following is the code snippet of the patch.
    if (hcd->driver->address_device)
-           retval = hcd->driver->address_device(hcd, udev);
+           retval = hcd->driver->address_device(hcd, udev, timeout_ms);
    else
            retval = usb_control_msg(udev, usb_sndaddr0pipe(),
                            USB_REQ_SET_ADDRESS, 0, devnum, 0,
-                           NULL, 0, USB_CTRL_SET_TIMEOUT);
+                           NULL, 0, timeout_ms);

In other host drivers, the set address request is issued using the
usb_control_msg() API within the 'else' condition.

Other host drivers, except for xHCI, do not populate the 'address_device'
function in the hc_driver structure when defining the structure in the host
driver. 

For example, you can see the hc_driver structure of the Linux OHCI driver
at the following link, and you'll notice that there is no 'address_device' function.

https://github.com/torvalds/linux/blob/1c8b86a3799f7e5be903c3f49fcdaee29fd385b5/drivers/usb/host/ohci-hcd.c#L1183

> thanks,
> 
> greg k-h

Thanks,
Hardik