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

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Oct 10, 2023 at 12:31:43PM +0200, Hardik Gajjar wrote:
> On Mon, Oct 09, 2023 at 07:43:09PM +0200, Greg KH wrote:
> > On Mon, Oct 09, 2023 at 06:14:02PM +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.
> > 
> > So you have known-broken devices where you want a shorter error timeout?
> > But you don't list those devices in this patch adding the quirk
> > settings, shouldn't that be required, otherwise this looks like an
> > unused quirk.
> Yes, we have identified the device (hub) that is causing the issue. However,
> I would prefer not to force this timeout globally. Instead, I can dynamically
> control it by writing to the /sys/bus/usb/drivers/hub/module/parameter/quirks
> file from init.rc (Android) during the boot-up process.

Then add that device to the quirk list please so that everyone doesn't
have to figure this out on their own for that broken device.  That's why
we have a quirk list in the kernel.

> > >  static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > > -			     enum xhci_setup_dev setup)
> > > +			     enum xhci_setup_dev setup, int timeout)
> > 
> > What is the units of timeout here?
> The timeout is in jiffies. Should we consider adding 'jiffies' in the variable name
> for clarity? I searched the kernel source code but couldn't find a reference for the
> unit of timeout in jiffies.
> Nevertheless, I will add a comment in patch v3 for clarification.

You should not pass around jiffies in the kernel in an int, please pass
around a unit of time and then when you actually need to tell the kernel
to sleep, you can use the time to convert to whatever unit the delay
function needs.

> > > +/* short device address timeout */
> > > +#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT	BIT(16)
> > 
> > Don't you also need to have a Documentation/ update for the new
> > user/kernel api you are adding?
> > 
> When you recommend Documentation, I assume you suggest to add the
> detailed comment in quirks.h to clear intention of the change.

No, please document the info that you have in the changelog in someplace
where people will know what flag to use in the module option.  That's
already documented for the other flags somewhere, right?

thanks,

greg k-h