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

Greg KH
2023-10-11T15:23:34Z

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@de.adit-jv.com>
> > > ---
> > > 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