Re: [PATCH] usb: hcd: xhci: Add set command timer delay API

[Hardik Gajjar]

On Fri, Aug 18, 2023 at 04:18:30PM +0300, Mathias Nyman wrote:
> On 18.8.2023 12.23, Hardik Gajjar wrote:
> > xHCI driver starts the response timer after sending each
> > command to the device. The default value of this timer is
> > 5 seconds (XHCI_CMD_DEFAULT_TIMEOUT = HZ*5). This seems
> > too high in time crtical use case.
> > 
> > This patch provides an API to change the default value of
> > the timer from the vendor USB driver.
> > 
> > The default value will be XHCI_CMD_DEFAULT_TIMEOUT (5 sec)
> > 
> > Use case:
> > According to the Smartphone integration certification
> > requirement in the automotive, the phone connected via USB
> > should complete enumeration and user space handshake
> > within 3 sec.
> 
> The above incorrectly makes it sound as if the command timeout
> timer causes the delay.
>
Thank you, Mathias, for your prompt response. I will enhance the message
to provide more specificity in the subsequent patch.
> > 
> > Reducing the response waiting time by setting the smaller
> > command timer delay helps to speed up overall re-enumeration
> > process of the USB device in case of device is not responding
> > properly in first enumeration iteration.
> 
> So is this a case where addressing a usb device behind xHC always
> fail on the first attempt, i.e. address device command in xhci
> never completes. Solution proposed here is to fail faster and
> retry?
> 
> Is the rootcause known why first enumeration fails?
> 
> Does setting old_scheme_first module parameter help?
> 
Yes, you are correct. The problem occurs when setting the address,
and in such cases, this patch helps to fail faster and retry.

Unfortunately, the root cause is unknown. This problem is mainly
observed with Android phones. Upon analyzing the USB analyzer logs,
it seems that the device is not responding to the SET_ADDRESS request.

I tried using "old_scheme_first=Y," but that did not help. Below are
the short logs of the first iteration enumeration failure.
> > 
> > Signed-off-by: Hardik Gajjar <hgajjar@xxxxxxxxxxxxxx>
> > ---
> >   drivers/usb/core/hcd.c       | 23 +++++++++++++++++++++++
> >   drivers/usb/host/xhci-ring.c | 10 +++++-----
> >   drivers/usb/host/xhci.c      | 15 +++++++++++++++
> >   drivers/usb/host/xhci.h      |  1 +
> >   include/linux/usb/hcd.h      |  2 ++
> >   5 files changed, 46 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 8300baedafd2..e392e90e918c 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -3157,6 +3157,29 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
> >   }
> >   EXPORT_SYMBOL_GPL(usb_hcd_setup_local_mem);
> > +/**
> > + * usb_hcd_set_cmd_timer_delay Set the delay of the command timer.
> > + * @hcd - pointer to the HCD representing the controller
> > + * @delay - Delay value to be used in command timer.
> > + *
> > + * wrapper function to call the set_cmd_timer_delay API of the host
> > + * diver.
> > + *
> > + * return 0 on success; otherwise -ENODEV means the feature not
> > + * supported by host driver.
> > + */
> > +
> > +int usb_hcd_set_cmd_timer_delay(struct usb_hcd *hcd, int delay)
> > +{
> > +	int ret = -ENODEV;
> > +
> > +	if (hcd->driver->set_cmd_timer_delay)
> > +		ret = hcd->driver->set_cmd_timer_delay(hcd, delay);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_set_cmd_timer_delay);
> > +
> 
> The xhci command timeout is more of a xhci internal thing, not sure it's a good
> idea to add this to hcd.
> 
> Would it make sense to add a timeout parameter to hcd->driver->address_device(hcd, udev)
> instead?
> 
> First priority should of course be finding out why the first enumeration fails,
> and solve that.
> 
> Thanks
> Mathias
Thanks for the suggestion to modify hcd->driver->address_device.
I will definitely implement it.However to confirm.

So, if I understand correctly, the idea is to avoid exposing any
API from the xhci driver, but instead, create an interface in hcd.c (such as sysfs or API)
and incorporate the delay in address_device as an additional parameter.

However, in that case, modifying xhci is still necessary as the timer is controlled from there.

Thanks,
Hardik