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

[Greg KH]

On Wed, Oct 11, 2023 at 02:05:35PM +0200, Hardik Gajjar wrote:
> 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

checkpatch doesn't parse documentation like this, so don't expect it to
catch stuff here.

thanks,

greg k-h