LinuxLists.cc - [PATCH v2] usb: core: hub: Add quirks for reducing device address timeout

[permalink] [raw]

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

2023-10-10 10:32:19

[permalink] [raw]

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

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.
>
> > 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 <[email protected]>
> > ---
> > changes since version 1:
> > - implement quirk instead of new API in xhci driver
> > ---
> > drivers/usb/core/hub.c | 15 +++++++++++++--
> > drivers/usb/core/quirks.c | 3 +++
> > drivers/usb/host/xhci-mem.c | 1 +
> > drivers/usb/host/xhci-ring.c | 3 ++-
> > drivers/usb/host/xhci.c | 9 +++++----
> > drivers/usb/host/xhci.h | 1 +
> > include/linux/usb/hcd.h | 3 ++-
> > include/linux/usb/quirks.h | 3 +++
> > 8 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3c54b218301c..975449b03426 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 (HZ * 5) /* 5000ms */
> > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT 125 /* ~500ms */
> > +
> > /* 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,16 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit);
> > static int hub_set_address(struct usb_device *udev, int devnum)
> > {
> > int retval;
> > + int timeout = USB_DEFAULT_ADDR_DEVICE_TIMEOUT;
> > struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> >
> > + struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
> > +
> > + if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT)
> > + timeout = USB_SHORT_ADDR_DEVICE_TIMEOUT;
> > +
> > + dev_dbg(&udev->dev, "address_device timeout %d\n", timeout);
>
> Is this debugging code still needed?
will remove in patch v3
>
> > /*
> > * The host controller will choose the device address,
> > * instead of the core having chosen it earlier
> > @@ -4639,11 +4650,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);
> > else
> > retval = usb_control_msg(udev, usb_sndaddr0pipe(),
> > USB_REQ_SET_ADDRESS, 0, devnum, 0,
> > - NULL, 0, USB_CTRL_SET_TIMEOUT);
> > + NULL, 0, jiffies_to_msecs(timeout));
> > 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..01ed26bd41f0 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 */
> > }
> > }
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index 8714ab5bf04d..492433fdac77 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -1729,6 +1729,7 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> > }
> >
> > command->status = 0;
> > + command->timeout = 0;
> > INIT_LIST_HEAD(&command->cmd_list);
> > return command;
> > }
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 1dde53f6eb31..0bd19a1efdec 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -4301,7 +4301,8 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
> > /* if there are no other commands queued we start the timeout timer */
> > if (list_empty(&xhci->cmd_list)) {
> > xhci->current_cmd = cmd;
> > - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> > + xhci_mod_cmd_timer(xhci, (cmd->timeout) ? cmd->timeout :
> > + XHCI_CMD_DEFAULT_TIMEOUT);
> > }
> >
> > list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index e1b1b64a0723..1d088ceb2b74 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -4002,7 +4002,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> > * SetAddress request to the device.
> > */
> > 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.
>
> > {
> > const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
> > unsigned long flags;
> > @@ -4059,6 +4059,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > }
> >
> > command->in_ctx = virt_dev->in_ctx;
> > + command->timeout = timeout;
> >
> > slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
> > ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
> > @@ -4185,14 +4186,14 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > return ret;
> > }
> >
> > -static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> > +static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev, int timeout)
> > {
> > - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
> > + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout);
> > }
> >
> > static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
> > {
> > - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
> > + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY, 0);
>
> 0 is no timeout at all? Or max timeout? Where is this documented?
>
> And why is this only added to the xhci driver and not all other host
> controllers?
Because 'address_device' is only assigned in the xHCI driver, other host
drivers (OHCI, EHCI) issue 'set_address' requests directly from 'hub.c'
and utilize the timeout

I have already updated that API in this patch to utilize timeout.
>
> > }
> >
> > /*
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 7e282b4522c0..ebdca8dd01c2 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -818,6 +818,7 @@ struct xhci_command {
> > struct completion *completion;
> > union xhci_trb *command_trb;
> > struct list_head cmd_list;
> > + int timeout;
>
> What is the units 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.
>
> > };
> >
> > /* drop context bitmasks */
> > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> > index 61d4f0b793dc..b0fda87ad3a2 100644
> > --- a/include/linux/usb/hcd.h
> > +++ b/include/linux/usb/hcd.h
> > @@ -373,7 +373,8 @@ struct hc_driver {
> > */
> > void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
> > /* Returns the hardware-chosen device address */
> > - int (*address_device)(struct usb_hcd *, struct usb_device *udev);
> > + int (*address_device)(struct usb_hcd *, struct usb_device *udev,
> > + int timeout);
>
> Again, units please.
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.
>
> > /* prepares the hardware to send commands to the device */
> > int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
> > /* Notifies the HCD after a hub descriptor is fetched.
> > diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> > index eeb7c2157c72..0cb464e3eaf4 100644
> > --- a/include/linux/usb/quirks.h
> > +++ b/include/linux/usb/quirks.h
> > @@ -72,4 +72,7 @@
> > /* device has endpoints that should be ignored */
> > #define USB_QUIRK_ENDPOINT_IGNORE BIT(15)
> >
> > +/* 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.
I will update it in patch v3.
> thanks,
>
> greg k-h

2023-10-10 10:50:17

[permalink] [raw]

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

On Mon, Oct 09, 2023 at 03:16:25PM -0400, Alan Stern 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.
>
> What will you do about scenarios where the Set-Address completes very
> quickly but the following Get-Device-Descriptor times out after 5
> seconds? Or any of the other transfers involved in device
> initialization and enumeration?

This issue occurs when the device first enumerates as full speed and then
as a high-speed device.

It appears that the set_address request is issued as soon as the device is
detected as full speed. However, during the progress of the set_address
request, the device changes its state from full speed to high speed, causing
the set_address request to become stuck until it times out.

Stress testing of USB devices indicates that the problem is specific to the
set_address request. Other requests, such as device descriptor requests,
consistently fail immediately with a protocol error in almost all cases.

>
> > 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 <[email protected]>
> > ---
> > changes since version 1:
> > - implement quirk instead of new API in xhci driver
> > ---
> > drivers/usb/core/hub.c | 15 +++++++++++++--
> > drivers/usb/core/quirks.c | 3 +++
> > drivers/usb/host/xhci-mem.c | 1 +
> > drivers/usb/host/xhci-ring.c | 3 ++-
> > drivers/usb/host/xhci.c | 9 +++++----
> > drivers/usb/host/xhci.h | 1 +
> > include/linux/usb/hcd.h | 3 ++-
> > include/linux/usb/quirks.h | 3 +++
> > 8 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3c54b218301c..975449b03426 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 (HZ * 5) /* 5000ms */
> > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT 125 /* ~500ms */
>
> That number, 125, is meaningless. It's in units of jiffies, which vary
> from one system to another. If you want the timeout to be about 500 ms,
> you should write it as (HZ / 2).

Good Point, Thanks, I will update in patch v3

>
> Alan Stern

2023-10-10 11:25:23

[permalink] [raw]

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

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

2023-10-10 13:53:05

by Mathias Nyman

[permalink] [raw]

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

On 9.10.2023 19.14, 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 <[email protected]>
> ---
> changes since version 1:
> - implement quirk instead of new API in xhci driver
> ---
> drivers/usb/core/hub.c | 15 +++++++++++++--
> drivers/usb/core/quirks.c | 3 +++
> drivers/usb/host/xhci-mem.c | 1 +
> drivers/usb/host/xhci-ring.c | 3 ++-
> drivers/usb/host/xhci.c | 9 +++++----
> drivers/usb/host/xhci.h | 1 +
> include/linux/usb/hcd.h | 3 ++-
> include/linux/usb/quirks.h | 3 +++
> 8 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3c54b218301c..975449b03426 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 (HZ * 5) /* 5000ms */
> +#define USB_SHORT_ADDR_DEVICE_TIMEOUT 125 /* ~500ms */

maybe use milliseconds directly
define USB_DEFAULT_ADDR_DEVICE_TIMEOUT 5000 /* ms */
...

> +
> /* 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,16 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit);
> static int hub_set_address(struct usb_device *udev, int devnum)
> {
> int retval;
> + int timeout = USB_DEFAULT_ADDR_DEVICE_TIMEOUT;
> struct usb_hcd *hcd = bus_to_hcd(udev->bus);
>
> + struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
> +
> + if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT)
> + timeout = USB_SHORT_ADDR_DEVICE_TIMEOUT;
> +
> + dev_dbg(&udev->dev, "address_device timeout %d\n", timeout);
> +
> /*
> * The host controller will choose the device address,
> * instead of the core having chosen it earlier
> @@ -4639,11 +4650,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);
> else
> retval = usb_control_msg(udev, usb_sndaddr0pipe(),
> USB_REQ_SET_ADDRESS, 0, devnum, 0,
> - NULL, 0, USB_CTRL_SET_TIMEOUT);
> + NULL, 0, jiffies_to_msecs(timeout));
> 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..01ed26bd41f0 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 */
> }
> }
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 8714ab5bf04d..492433fdac77 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1729,6 +1729,7 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> }
>
> command->status = 0;
> + command->timeout = 0;

This could be command->timeout = XHCI_CMD_DEFAULT_TIMEOUT_MS;
For address device commands we then override it later in xhci_setup_device()

The default xhci command timeout could be changed to 5000ms instead of 5 * HZ,
so in xhci.h:

/* Default xhci command timeout in milliseconds */
XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

> INIT_LIST_HEAD(&command->cmd_list);
> return command;
> }
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1dde53f6eb31..0bd19a1efdec 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -4301,7 +4301,8 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
> /* if there are no other commands queued we start the timeout timer */
> if (list_empty(&xhci->cmd_list)) {
> xhci->current_cmd = cmd;
> - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> + xhci_mod_cmd_timer(xhci, (cmd->timeout) ? cmd->timeout :
> + XHCI_CMD_DEFAULT_TIMEOUT);

xhci_mod_cmd_timer(xhci, cmd->timeout);

We would then also need to convert ms to jiffies in xhci_mod_cmd_timer():

return mod_delayed_work(system_wq, &xhci->cmd_timer, msecs_to_jiffies(delay));

> }
>
> list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..1d088ceb2b74 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4002,7 +4002,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> * SetAddress request to the device.
> */
> 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)

Hmm, I know usb_control_msg() uses int for timeout, but unsigned int would seem more
appropriate.

Same for xhci_address_device() and xhci_enable_device()

> {
> const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
> unsigned long flags;
> @@ -4059,6 +4059,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> }
>
> command->in_ctx = virt_dev->in_ctx;
> + command->timeout = timeout;
>
> slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
> ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
> @@ -4185,14 +4186,14 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> return ret;
> }
>
> -static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev, int timeout)
> {
> - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout);
> }
>
> static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
> {
> - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY, 0);

return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY, XHCI_CMD_DEFAULT_TIMEOUT_MS);

> }
>
> /*
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 7e282b4522c0..ebdca8dd01c2 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -818,6 +818,7 @@ struct xhci_command {
> struct completion *completion;
> union xhci_trb *command_trb;
> struct list_head cmd_list;
> + int timeout;

unsigned int?

Thanks
Mathias

2023-10-11 08:51:14

[permalink] [raw]

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

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 <[email protected]>
---
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
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 */
+#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */
+
/* 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);
+
+ 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 */
};

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4a286136d1a8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
}

command->status = 0;
+ /* set default timeout to 5000 ms */
+ command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..8f36c2914938 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
readl(&xhci->dba->doorbell[0]);
}

-static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
+static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
{
- return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
+ return mod_delayed_work(system_wq, &xhci->cmd_timer,
+ msecs_to_jiffies(xhci->current_cmd->timeout_ms));
}

static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
xhci_ring_cmd_db(xhci);
}
}
@@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_first_entry(&cmd->cmd_list,
struct xhci_command, cmd_list);
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
}
@@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
/* if there are no other commands queued we start the timeout timer */
if (list_empty(&xhci->cmd_list)) {
xhci->current_cmd = cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
}

list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..cd24f9222b1b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3997,12 +3997,20 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
return 0;
}

-/*
- * Issue an Address Device command and optionally send a corresponding
- * SetAddress request to the device.
+/**
+ * This function issues an Address Device command to assign a unique USB bus
+ * address. Optionally, it sends a SetAddress request.
+ *
+ * @param hcd USB host controller data structure.
+ * @param udev USB device structure representing the connected device.
+ * @param setup Enum specifying setup mode: address only or with context.
+ * @param timeout_ms Max wait time (ms) for the command operation to complete.
+ *
+ * @return Integer status code: 0 on success, negative on error.
+ *
*/
static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
- enum xhci_setup_dev setup)
+ enum xhci_setup_dev setup, unsigned int timeout_ms)
{
const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
unsigned long flags;
@@ -4059,6 +4067,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
}

command->in_ctx = virt_dev->in_ctx;
+ command->timeout_ms = timeout_ms;

slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
@@ -4185,14 +4194,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
+ unsigned int timeout_ms)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
}

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
+ XHCI_CMD_DEFAULT_TIMEOUT_MS);
}

/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..1ac3caca6b6f 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -818,6 +818,8 @@ struct xhci_command {
struct completion *completion;
union xhci_trb *command_trb;
struct list_head cmd_list;
+ /* xHCI command response timeout in milliseconds */
+ unsigned int timeout_ms;
};

/* drop context bitmasks */
@@ -1576,8 +1578,8 @@ struct xhci_td {
unsigned int num_trbs;
};

-/* xHCI command default timeout value */
-#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
+/* xHCI command default timeout value in milliseconds */
+#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

/* command descriptor */
struct xhci_cd {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 61d4f0b793dc..d0e19ac3ba6c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -372,8 +372,9 @@ struct hc_driver {
* or bandwidth constraints.
*/
void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
- /* Returns the hardware-chosen device address */
- int (*address_device)(struct usb_hcd *, struct usb_device *udev);
+ /* Set the hardware-chosen device address */
+ int (*address_device)(struct usb_hcd *, struct usb_device *udev,
+ unsigned int timeout_ms);
/* prepares the hardware to send commands to the device */
int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
/* Notifies the HCD after a hub descriptor is fetched.
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..0cb464e3eaf4 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
/* device has endpoints that should be ignored */
#define USB_QUIRK_ENDPOINT_IGNORE BIT(15)

+/* short device address timeout */
+#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)
+
#endif /* __LINUX_USB_QUIRKS_H */
--
2.17.1

2023-10-11 09:03:48

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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?

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 checkpatch.pl 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.)

> +
> + 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.?

thanks,

greg k-h

2023-10-11 10:52:40

by kernel test robot

[permalink] [raw]

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

Hi Hardik,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc5 next-20231011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Hardik-Gajjar/usb-core-hub-Add-quirks-for-reducing-device-address-timeout/20231011-165246
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20231011085011.89198-1-hgajjar%40de.adit-jv.com
patch subject: [PATCH v3] usb: core: hub: Add quirks for reducing device address timeout
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231011/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

drivers/usb/host/xhci.c:1318: warning: Function parameter or member 'desc' not described in 'xhci_get_endpoint_index'
>> drivers/usb/host/xhci.c:4001: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* This function issues an Address Device command to assign a unique USB bus

vim +4001 drivers/usb/host/xhci.c

3999
4000 /**
> 4001 * This function issues an Address Device command to assign a unique USB bus
4002 * address. Optionally, it sends a SetAddress request.
4003 *
4004 * @param hcd USB host controller data structure.
4005 * @param udev USB device structure representing the connected device.
4006 * @param setup Enum specifying setup mode: address only or with context.
4007 * @param timeout_ms Max wait time (ms) for the command operation to complete.
4008 *
4009 * @return Integer status code: 0 on success, negative on error.
4010 *
4011 */
4012 static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
4013 enum xhci_setup_dev setup, unsigned int timeout_ms)
4014 {
4015 const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
4016 unsigned long flags;
4017 struct xhci_virt_device *virt_dev;
4018 int ret = 0;
4019 struct xhci_hcd *xhci = hcd_to_xhci(hcd);
4020 struct xhci_slot_ctx *slot_ctx;
4021 struct xhci_input_control_ctx *ctrl_ctx;
4022 u64 temp_64;
4023 struct xhci_command *command = NULL;
4024
4025 mutex_lock(&xhci->mutex);
4026
4027 if (xhci->xhc_state) { /* dying, removing or halted */
4028 ret = -ESHUTDOWN;
4029 goto out;
4030 }
4031
4032 if (!udev->slot_id) {
4033 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4034 "Bad Slot ID %d", udev->slot_id);
4035 ret = -EINVAL;
4036 goto out;
4037 }
4038
4039 virt_dev = xhci->devs[udev->slot_id];
4040
4041 if (WARN_ON(!virt_dev)) {
4042 /*
4043 * In plug/unplug torture test with an NEC controller,
4044 * a zero-dereference was observed once due to virt_dev = 0.
4045 * Print useful debug rather than crash if it is observed again!
4046 */
4047 xhci_warn(xhci, "Virt dev invalid for slot_id 0x%x!\n",
4048 udev->slot_id);
4049 ret = -EINVAL;
4050 goto out;
4051 }
4052 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
4053 trace_xhci_setup_device_slot(slot_ctx);
4054
4055 if (setup == SETUP_CONTEXT_ONLY) {
4056 if (GET_SLOT_STATE(le32_to_cpu(slot_ctx->dev_state)) ==
4057 SLOT_STATE_DEFAULT) {
4058 xhci_dbg(xhci, "Slot already in default state\n");
4059 goto out;
4060 }
4061 }
4062
4063 command = xhci_alloc_command(xhci, true, GFP_KERNEL);
4064 if (!command) {
4065 ret = -ENOMEM;
4066 goto out;
4067 }
4068
4069 command->in_ctx = virt_dev->in_ctx;
4070 command->timeout_ms = timeout_ms;
4071
4072 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
4073 ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
4074 if (!ctrl_ctx) {
4075 xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
4076 __func__);
4077 ret = -EINVAL;
4078 goto out;
4079 }
4080 /*
4081 * If this is the first Set Address since device plug-in or
4082 * virt_device realloaction after a resume with an xHCI power loss,
4083 * then set up the slot context.
4084 */
4085 if (!slot_ctx->dev_info)
4086 xhci_setup_addressable_virt_dev(xhci, udev);
4087 /* Otherwise, update the control endpoint ring enqueue pointer. */
4088 else
4089 xhci_copy_ep0_dequeue_into_input_ctx(xhci, udev);
4090 ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG | EP0_FLAG);
4091 ctrl_ctx->drop_flags = 0;
4092
4093 trace_xhci_address_ctx(xhci, virt_dev->in_ctx,
4094 le32_to_cpu(slot_ctx->dev_info) >> 27);
4095
4096 trace_xhci_address_ctrl_ctx(ctrl_ctx);
4097 spin_lock_irqsave(&xhci->lock, flags);
4098 trace_xhci_setup_device(virt_dev);
4099 ret = xhci_queue_address_device(xhci, command, virt_dev->in_ctx->dma,
4100 udev->slot_id, setup);
4101 if (ret) {
4102 spin_unlock_irqrestore(&xhci->lock, flags);
4103 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4104 "FIXME: allocate a command ring segment");
4105 goto out;
4106 }
4107 xhci_ring_cmd_db(xhci);
4108 spin_unlock_irqrestore(&xhci->lock, flags);
4109
4110 /* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */
4111 wait_for_completion(command->completion);
4112
4113 /* FIXME: From section 4.3.4: "Software shall be responsible for timing
4114 * the SetAddress() "recovery interval" required by USB and aborting the
4115 * command on a timeout.
4116 */
4117 switch (command->status) {
4118 case COMP_COMMAND_ABORTED:
4119 case COMP_COMMAND_RING_STOPPED:
4120 xhci_warn(xhci, "Timeout while waiting for setup device command\n");
4121 ret = -ETIME;
4122 break;
4123 case COMP_CONTEXT_STATE_ERROR:
4124 case COMP_SLOT_NOT_ENABLED_ERROR:
4125 xhci_err(xhci, "Setup ERROR: setup %s command for slot %d.\n",
4126 act, udev->slot_id);
4127 ret = -EINVAL;
4128 break;
4129 case COMP_USB_TRANSACTION_ERROR:
4130 dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
4131
4132 mutex_unlock(&xhci->mutex);
4133 ret = xhci_disable_slot(xhci, udev->slot_id);
4134 xhci_free_virt_device(xhci, udev->slot_id);
4135 if (!ret)
4136 xhci_alloc_dev(hcd, udev);
4137 kfree(command->completion);
4138 kfree(command);
4139 return -EPROTO;
4140 case COMP_INCOMPATIBLE_DEVICE_ERROR:
4141 dev_warn(&udev->dev,
4142 "ERROR: Incompatible device for setup %s command\n", act);
4143 ret = -ENODEV;
4144 break;
4145 case COMP_SUCCESS:
4146 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4147 "Successful setup %s command", act);
4148 break;
4149 default:
4150 xhci_err(xhci,
4151 "ERROR: unexpected setup %s command completion code 0x%x.\n",
4152 act, command->status);
4153 trace_xhci_address_ctx(xhci, virt_dev->out_ctx, 1);
4154 ret = -EINVAL;
4155 break;
4156 }
4157 if (ret)
4158 goto out;
4159 temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
4160 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4161 "Op regs DCBAA ptr = %#016llx", temp_64);
4162 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4163 "Slot ID %d dcbaa entry @%p = %#016llx",
4164 udev->slot_id,
4165 &xhci->dcbaa->dev_context_ptrs[udev->slot_id],
4166 (unsigned long long)
4167 le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]));
4168 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4169 "Output Context DMA address = %#08llx",
4170 (unsigned long long)virt_dev->out_ctx->dma);
4171 trace_xhci_address_ctx(xhci, virt_dev->in_ctx,
4172 le32_to_cpu(slot_ctx->dev_info) >> 27);
4173 /*
4174 * USB core uses address 1 for the roothubs, so we add one to the
4175 * address given back to us by the HC.
4176 */
4177 trace_xhci_address_ctx(xhci, virt_dev->out_ctx,
4178 le32_to_cpu(slot_ctx->dev_info) >> 27);
4179 /* Zero the input context control for later use */
4180 ctrl_ctx->add_flags = 0;
4181 ctrl_ctx->drop_flags = 0;
4182 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
4183 udev->devaddr = (u8)(le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK);
4184
4185 xhci_dbg_trace(xhci, trace_xhci_dbg_address,
4186 "Internal device address = %d",
4187 le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK);
4188 out:
4189 mutex_unlock(&xhci->mutex);
4190 if (command) {
4191 kfree(command->completion);
4192 kfree(command);
4193 }
4194 return ret;
4195 }
4196

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-10-11 12:06:05

[permalink] [raw]

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

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 <[email protected]>
> > ---
> > 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

2023-10-11 14:16:15

[permalink] [raw]

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

On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote:

> --- 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 */
> +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */

You don't have to repeat the numbers in the comments. You can just
write /* ms */ -- just like in the comments immediately above.

> 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(),
> --- 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 },

This table is sorted by Vendor ID, then Product ID, then Class ID, as
stated in the comment at the beginning of the definition. Your new
entry is in the wrong position.

Alan Stern

2023-10-11 15:23:36

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > > 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

2023-10-11 16:46:09

[permalink] [raw]

Subject: [PATCH v4] usb: core: hub: Add quirks for reducing device address timeout

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 <[email protected]>
---
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

Changes since version 3:
- Add some comments for clarity.
- Minor indentation and sequence change.
---
.../admin-guide/kernel-parameters.txt | 3 +++
drivers/usb/core/hub.c | 21 ++++++++++++++++--
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 | 22 ++++++++++++++-----
drivers/usb/host/xhci.h | 9 ++++++--
include/linux/usb/hcd.h | 5 +++--
include/linux/usb/quirks.h | 3 +++
9 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
+ 5000 ms to 500 ms)
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..83d1af0a3953 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -54,6 +54,18 @@
#define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
#define USB_PING_RESPONSE_TIME 400 /* ns */

+/*
+ * address device command timeout 5000 ms is recommended in
+ * USB 2.0 spec, section 9.2.6.1
+ */
+#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
+
+/*
+ * address device command timeout will be 500 ms when
+ * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
+ */
+#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
+
/* 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,7 +4638,12 @@ 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);
+
+ 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,
@@ -4639,11 +4656,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..863e7fe24157 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 */
}
}
@@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {

{ USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },

+ /* APTIV AUTOMOTIVE HUB */
+ { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },
+
/* DJI CineSSD */
{ USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4a286136d1a8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
}

command->status = 0;
+ /* set default timeout to 5000 ms */
+ command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..8f36c2914938 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
readl(&xhci->dba->doorbell[0]);
}

-static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
+static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
{
- return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
+ return mod_delayed_work(system_wq, &xhci->cmd_timer,
+ msecs_to_jiffies(xhci->current_cmd->timeout_ms));
}

static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
xhci_ring_cmd_db(xhci);
}
}
@@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_first_entry(&cmd->cmd_list,
struct xhci_command, cmd_list);
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
}
@@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
/* if there are no other commands queued we start the timeout timer */
if (list_empty(&xhci->cmd_list)) {
xhci->current_cmd = cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
}

list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..85ea4e17d2a0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3998,11 +3998,18 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
}

/*
- * Issue an Address Device command and optionally send a corresponding
- * SetAddress request to the device.
+ * This function issues an Address Device command to assign a unique USB bus
+ * address. Optionally, it sends a SetAddress request.
+ *
+ * @param hcd USB host controller data structure.
+ * @param udev USB device structure representing the connected device.
+ * @param setup Enum specifying setup mode: address only or with context.
+ * @param timeout_ms Max wait time (ms) for the command operation to complete.
+ *
+ * @return Integer status code: 0 on success, negative on error.
*/
static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
- enum xhci_setup_dev setup)
+ enum xhci_setup_dev setup, unsigned int timeout_ms)
{
const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
unsigned long flags;
@@ -4059,6 +4066,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
}

command->in_ctx = virt_dev->in_ctx;
+ command->timeout_ms = timeout_ms;

slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
@@ -4185,14 +4193,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
+ unsigned int timeout_ms)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
}

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
+ XHCI_CMD_DEFAULT_TIMEOUT_MS);
}

/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..ec5c663246e5 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -818,6 +818,8 @@ struct xhci_command {
struct completion *completion;
union xhci_trb *command_trb;
struct list_head cmd_list;
+ /* xHCI command response timeout in milliseconds */
+ unsigned int timeout_ms;
};

/* drop context bitmasks */
@@ -1576,8 +1578,11 @@ struct xhci_td {
unsigned int num_trbs;
};

-/* xHCI command default timeout value */
-#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
+/*
+ * xHCI command default timeout value in milliseconds.
+ * USB 2.0 spec, section 9.2.6.1
+ */
+#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

/* command descriptor */
struct xhci_cd {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 61d4f0b793dc..d0e19ac3ba6c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -372,8 +372,9 @@ struct hc_driver {
* or bandwidth constraints.
*/
void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
- /* Returns the hardware-chosen device address */
- int (*address_device)(struct usb_hcd *, struct usb_device *udev);
+ /* Set the hardware-chosen device address */
+ int (*address_device)(struct usb_hcd *, struct usb_device *udev,
+ unsigned int timeout_ms);
/* prepares the hardware to send commands to the device */
int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
/* Notifies the HCD after a hub descriptor is fetched.
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..0cb464e3eaf4 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
/* device has endpoints that should be ignored */
#define USB_QUIRK_ENDPOINT_IGNORE BIT(15)

+/* short device address timeout */
+#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)
+
#endif /* __LINUX_USB_QUIRKS_H */
--
2.17.1

2023-10-16 17:59:13

[permalink] [raw]

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

On Wed, Oct 11, 2023 at 06:45:25PM +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.

"should" according to whom? That goes against the USB specification, so
why not take it up with them?

> Achieving this is impossible in scenarios where the set_address is
> not successful and waits for a timeout.

Agreed, broken hardware is a pain, but if your device is allowed to take
longer, it can, and will, so you have to support that.

> 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.

Screen projection is a requirement that you should not be relying on USB
for as USB has a different set of required timeouts, right? This sounds
like a bad hardware design, if not an impossible one.

> 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 <[email protected]>
> ---
> 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
>
> Changes since version 3:
> - Add some comments for clarity.
> - Minor indentation and sequence change.
> ---
> .../admin-guide/kernel-parameters.txt | 3 +++
> drivers/usb/core/hub.c | 21 ++++++++++++++++--
> 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 | 22 ++++++++++++++-----
> drivers/usb/host/xhci.h | 9 ++++++--
> include/linux/usb/hcd.h | 5 +++--
> include/linux/usb/quirks.h | 3 +++
> 9 files changed, 65 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
> + 5000 ms to 500 ms)
> 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..83d1af0a3953 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -54,6 +54,18 @@
> #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> #define USB_PING_RESPONSE_TIME 400 /* ns */
>
> +/*
> + * address device command timeout 5000 ms is recommended in
> + * USB 2.0 spec, section 9.2.6.1

The 2.0 spec is superseeded by the USB 3.1 specification (or is it 3.2
now?) Please use the latest specification as 2.0 is very very old by
now.

> + */
> +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
> +
> +/*
> + * address device command timeout will be 500 ms when
> + * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
> + */
> +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
> +
> /* 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,7 +4638,12 @@ 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);
> +
> + 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,
> @@ -4639,11 +4656,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..863e7fe24157 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 */
> }
> }
> @@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>
> { USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },
>
> + /* APTIV AUTOMOTIVE HUB */
> + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },

So the real issue that you have here is a broken built-in USB hub that
does not error out quick enough, right? Why not fix the firmware in
that hub as you know it's broken? Why is it the operating system's job
to work around non-compliant devices?

Ok, that last question was redundant, of course it's our job to work
around broken devices, but this feels different. You are trying to say
"hey, I know this device is broken, so error out quick so we can just
ignore it", right? If so, why not just never allow that device to
enumerate at all? You don't have to accept it as a valid device to the
system (just don't authorize it), and then no device will ever connect
to it so what is the delay issue?

> +
> /* DJI CineSSD */
> { USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 8714ab5bf04d..4a286136d1a8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> }
>
> command->status = 0;
> + /* set default timeout to 5000 ms */
> + command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
> INIT_LIST_HEAD(&command->cmd_list);
> return command;
> }
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1dde53f6eb31..8f36c2914938 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
> readl(&xhci->dba->doorbell[0]);
> }
>
> -static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
> +static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
> {
> - return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
> + return mod_delayed_work(system_wq, &xhci->cmd_timer,
> + msecs_to_jiffies(xhci->current_cmd->timeout_ms));
> }
>
> static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
> @@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
> if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
> !(xhci->xhc_state & XHCI_STATE_DYING)) {
> xhci->current_cmd = cur_cmd;
> - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> + xhci_mod_cmd_timer(xhci);
> xhci_ring_cmd_db(xhci);
> }
> }
> @@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> if (!list_is_singular(&xhci->cmd_list)) {
> xhci->current_cmd = list_first_entry(&cmd->cmd_list,
> struct xhci_command, cmd_list);
> - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> + xhci_mod_cmd_timer(xhci);
> } else if (xhci->current_cmd == cmd) {
> xhci->current_cmd = NULL;
> }
> @@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
> /* if there are no other commands queued we start the timeout timer */
> if (list_empty(&xhci->cmd_list)) {
> xhci->current_cmd = cmd;
> - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> + xhci_mod_cmd_timer(xhci);
> }
>
> list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..85ea4e17d2a0 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3998,11 +3998,18 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> }
>
> /*
> - * Issue an Address Device command and optionally send a corresponding
> - * SetAddress request to the device.
> + * This function issues an Address Device command to assign a unique USB bus
> + * address. Optionally, it sends a SetAddress request.
> + *
> + * @param hcd USB host controller data structure.
> + * @param udev USB device structure representing the connected device.
> + * @param setup Enum specifying setup mode: address only or with context.
> + * @param timeout_ms Max wait time (ms) for the command operation to complete.

"param" is not how kernel doc formatting works at all, sorry. If you
are going to document this that way, please use the correct style
otherwise our tools will choke.

> + *
> + * @return Integer status code: 0 on success, negative on error.
> */
> static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> - enum xhci_setup_dev setup)
> + enum xhci_setup_dev setup, unsigned int timeout_ms)
> {
> const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
> unsigned long flags;
> @@ -4059,6 +4066,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> }
>
> command->in_ctx = virt_dev->in_ctx;
> + command->timeout_ms = timeout_ms;
>
> slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
> ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
> @@ -4185,14 +4193,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> return ret;
> }
>
> -static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
> + unsigned int timeout_ms)
> {
> - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
> }
>
> static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
> {
> - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
> + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
> + XHCI_CMD_DEFAULT_TIMEOUT_MS);
> }
>
> /*
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 7e282b4522c0..ec5c663246e5 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -818,6 +818,8 @@ struct xhci_command {
> struct completion *completion;
> union xhci_trb *command_trb;
> struct list_head cmd_list;
> + /* xHCI command response timeout in milliseconds */
> + unsigned int timeout_ms;
> };
>
> /* drop context bitmasks */
> @@ -1576,8 +1578,11 @@ struct xhci_td {
> unsigned int num_trbs;
> };
>
> -/* xHCI command default timeout value */
> -#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
> +/*
> + * xHCI command default timeout value in milliseconds.
> + * USB 2.0 spec, section 9.2.6.1

xHCI came about in the 3.0 specification, it was not around in the 2.0
one, right?

> + */
> +#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000
>
> /* command descriptor */
> struct xhci_cd {
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 61d4f0b793dc..d0e19ac3ba6c 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -372,8 +372,9 @@ struct hc_driver {
> * or bandwidth constraints.
> */
> void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
> - /* Returns the hardware-chosen device address */
> - int (*address_device)(struct usb_hcd *, struct usb_device *udev);
> + /* Set the hardware-chosen device address */
> + int (*address_device)(struct usb_hcd *, struct usb_device *udev,
> + unsigned int timeout_ms);

Did this function callback just change operation? If not, why change
the comment? Or has the comment always been wrong?

> /* prepares the hardware to send commands to the device */
> int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
> /* Notifies the HCD after a hub descriptor is fetched.
> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index eeb7c2157c72..0cb464e3eaf4 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -72,4 +72,7 @@
> /* device has endpoints that should be ignored */
> #define USB_QUIRK_ENDPOINT_IGNORE BIT(15)
>
> +/* short device address timeout */
> +#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)

As you really just want to fail this device, why not just make a "This
is a broken device, never talk to it" type of quirk instead?

thanks,

greg k-h

2023-10-17 16:11:03

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 07:58:36PM +0200, Greg KH wrote:
> On Wed, Oct 11, 2023 at 06:45:25PM +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.
>
> "should" according to whom? That goes against the USB specification, so
> why not take it up with them?
We're implementing this change to meet Google's Android Auto certification
requirements, even though these requirements aren't publicly available.

The issue arises when we connect an Android phone to a Linux host using a special
automotive USB hub. The host's xHCI doesn't receive a response from the hub after
sending a 'set_address' request, resulting in a timeout. This problem is specific
to one phone model.

It's worth noting that we can't argue that this requirement violates USB standards
because the 3-second time limit applies only when connecting an Android mobile phone.
I assume that Android phones also undergo Google certification, which likely includes
a condition for successful enumeration within a specified time frame. However, I can't confirm this.

More logs and detailed in patch V1:
https://lore.kernel.org/linux-usb/[email protected]/T/#m452ec9dad94e8181fdb050cd29483dd89437f7c1
>
> > Achieving this is impossible in scenarios where the set_address is
> > not successful and waits for a timeout.
>
> Agreed, broken hardware is a pain, but if your device is allowed to take
> longer, it can, and will, so you have to support that.
>
The problem is not caused by the device taking an extended amount of time to
process the 'set_address' request. Instead, the issue lies in the absence of
any activity on the upstream bus until a timeout occurs.

This situation arises when the host has already transmitted the 'set_address' command to the hub,
assuming that the device operates at full speed. However, the device connected
to the hub undergoes a state change from full speed to high-speed during this process.

> > 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.
>
> Screen projection is a requirement that you should not be relying on USB
> for as USB has a different set of required timeouts, right? This sounds
> like a bad hardware design, if not an impossible one.
>

Screen projection for us means displaying the connected phone on the screen and
launching Carplay and Android Auto for the user. This works perfectly in nearly all
cases, except in scenarios like this one where a combination of a special hub and
a specific phone model is causing the issue

> > 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 <[email protected]>
> > ---
> > 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
> >
> > Changes since version 3:
> > - Add some comments for clarity.
> > - Minor indentation and sequence change.
> > ---
> > .../admin-guide/kernel-parameters.txt | 3 +++
> > drivers/usb/core/hub.c | 21 ++++++++++++++++--
> > 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 | 22 ++++++++++++++-----
> > drivers/usb/host/xhci.h | 9 ++++++--
> > include/linux/usb/hcd.h | 5 +++--
> > include/linux/usb/quirks.h | 3 +++
> > 9 files changed, 65 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
> > + 5000 ms to 500 ms)
> > 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..83d1af0a3953 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -54,6 +54,18 @@
> > #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> > #define USB_PING_RESPONSE_TIME 400 /* ns */
> >
> > +/*
> > + * address device command timeout 5000 ms is recommended in
> > + * USB 2.0 spec, section 9.2.6.1
>
> The 2.0 spec is superseeded by the USB 3.1 specification (or is it 3.2
> now?) Please use the latest specification as 2.0 is very very old by
> now.
will update to USB3 specs.
>
> > + */
> > +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
> > +
> > +/*
> > + * address device command timeout will be 500 ms when
> > + * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
> > + */
> > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
> > +
> > /* 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,7 +4638,12 @@ 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);
> > +
> > + 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,
> > @@ -4639,11 +4656,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..863e7fe24157 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 */
> > }
> > }
> > @@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> >
> > { USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },
> >
> > + /* APTIV AUTOMOTIVE HUB */
> > + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },
>
> So the real issue that you have here is a broken built-in USB hub that
> does not error out quick enough, right? Why not fix the firmware in
> that hub as you know it's broken? Why is it the operating system's job
> to work around non-compliant devices?
>
> Ok, that last question was redundant, of course it's our job to work
> around broken devices, but this feels different. You are trying to say
> "hey, I know this device is broken, so error out quick so we can just
> ignore it", right? If so, why not just never allow that device to
> enumerate at all? You don't have to accept it as a valid device to the
> system (just don't authorize it), and then no device will ever connect
> to it so what is the delay issue?
>

We require the device (HUB) as it is a vital component of our infotainment system.
The issue arises when the host has already issued the 'set_address' command to the hub,
assuming the device is operating at full speed. However, in such cases, when the device
connected to the hub changes its state from full speed to high-speed, the 'set_address'
request becomes blocked, waiting for the full 5-second timeout. This patch reduces the
timeout from 5 seconds to 500 milliseconds, allowing for quicker failure and re-enumeration
of the device as high-speed

>
> > +
> > /* DJI CineSSD */
> > { USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },
> >
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index 8714ab5bf04d..4a286136d1a8 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
> > }
> >
> > command->status = 0;
> > + /* set default timeout to 5000 ms */
> > + command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
> > INIT_LIST_HEAD(&command->cmd_list);
> > return command;
> > }
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 1dde53f6eb31..8f36c2914938 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
> > readl(&xhci->dba->doorbell[0]);
> > }
> >
> > -static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
> > +static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
> > {
> > - return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
> > + return mod_delayed_work(system_wq, &xhci->cmd_timer,
> > + msecs_to_jiffies(xhci->current_cmd->timeout_ms));
> > }
> >
> > static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
> > @@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
> > if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
> > !(xhci->xhc_state & XHCI_STATE_DYING)) {
> > xhci->current_cmd = cur_cmd;
> > - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> > + xhci_mod_cmd_timer(xhci);
> > xhci_ring_cmd_db(xhci);
> > }
> > }
> > @@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> > if (!list_is_singular(&xhci->cmd_list)) {
> > xhci->current_cmd = list_first_entry(&cmd->cmd_list,
> > struct xhci_command, cmd_list);
> > - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> > + xhci_mod_cmd_timer(xhci);
> > } else if (xhci->current_cmd == cmd) {
> > xhci->current_cmd = NULL;
> > }
> > @@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
> > /* if there are no other commands queued we start the timeout timer */
> > if (list_empty(&xhci->cmd_list)) {
> > xhci->current_cmd = cmd;
> > - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
> > + xhci_mod_cmd_timer(xhci);
> > }
> >
> > list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index e1b1b64a0723..85ea4e17d2a0 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -3998,11 +3998,18 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> > }
> >
> > /*
> > - * Issue an Address Device command and optionally send a corresponding
> > - * SetAddress request to the device.
> > + * This function issues an Address Device command to assign a unique USB bus
> > + * address. Optionally, it sends a SetAddress request.
> > + *
> > + * @param hcd USB host controller data structure.
> > + * @param udev USB device structure representing the connected device.
> > + * @param setup Enum specifying setup mode: address only or with context.
> > + * @param timeout_ms Max wait time (ms) for the command operation to complete.
>
> "param" is not how kernel doc formatting works at all, sorry. If you
> are going to document this that way, please use the correct style
> otherwise our tools will choke.
>

will check this.

> > + *
> > + * @return Integer status code: 0 on success, negative on error.
> > */
> > static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > - enum xhci_setup_dev setup)
> > + enum xhci_setup_dev setup, unsigned int timeout_ms)
> > {
> > const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
> > unsigned long flags;
> > @@ -4059,6 +4066,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > }
> >
> > command->in_ctx = virt_dev->in_ctx;
> > + command->timeout_ms = timeout_ms;
> >
> > slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
> > ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
> > @@ -4185,14 +4193,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> > return ret;
> > }
> >
> > -static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> > +static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
> > + unsigned int timeout_ms)
> > {
> > - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
> > + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
> > }
> >
> > static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
> > {
> > - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
> > + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
> > + XHCI_CMD_DEFAULT_TIMEOUT_MS);
> > }
> >
> > /*
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 7e282b4522c0..ec5c663246e5 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -818,6 +818,8 @@ struct xhci_command {
> > struct completion *completion;
> > union xhci_trb *command_trb;
> > struct list_head cmd_list;
> > + /* xHCI command response timeout in milliseconds */
> > + unsigned int timeout_ms;
> > };
> >
> > /* drop context bitmasks */
> > @@ -1576,8 +1578,11 @@ struct xhci_td {
> > unsigned int num_trbs;
> > };
> >
> > -/* xHCI command default timeout value */
> > -#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
> > +/*
> > + * xHCI command default timeout value in milliseconds.
> > + * USB 2.0 spec, section 9.2.6.1
>
> xHCI came about in the 3.0 specification, it was not around in the 2.0
> one, right?

will update to usb3 specs

>
> > + */
> > +#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000
> >
> > /* command descriptor */
> > struct xhci_cd {
> > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> > index 61d4f0b793dc..d0e19ac3ba6c 100644
> > --- a/include/linux/usb/hcd.h
> > +++ b/include/linux/usb/hcd.h
> > @@ -372,8 +372,9 @@ struct hc_driver {
> > * or bandwidth constraints.
> > */
> > void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
> > - /* Returns the hardware-chosen device address */
> > - int (*address_device)(struct usb_hcd *, struct usb_device *udev);
> > + /* Set the hardware-chosen device address */
> > + int (*address_device)(struct usb_hcd *, struct usb_device *udev,
> > + unsigned int timeout_ms);
>
> Did this function callback just change operation? If not, why change
> the comment? Or has the comment always been wrong?

It was written like "Returns the Hardware device addres" that sounds
wrong. It only return success or failure.

>
> > /* prepares the hardware to send commands to the device */
> > int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
> > /* Notifies the HCD after a hub descriptor is fetched.
> > diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> > index eeb7c2157c72..0cb464e3eaf4 100644
> > --- a/include/linux/usb/quirks.h
> > +++ b/include/linux/usb/quirks.h
> > @@ -72,4 +72,7 @@
> > /* device has endpoints that should be ignored */
> > #define USB_QUIRK_ENDPOINT_IGNORE BIT(15)
> >
> > +/* short device address timeout */
> > +#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)
>
> As you really just want to fail this device, why not just make a "This
> is a broken device, never talk to it" type of quirk instead?
>
> thanks,
>
> greg k-h

We need the device(hub) because It is working as expected except this one scenario
The quirk is for fail fast(in-case of error) and retry next attempt quickly(perhapse as
high speed device enumeration).

As mentioned earlier this is happning when end device connected to Hub
change its state while the set_address request already received to hub
from host.

Thanks,
Hardik

2023-10-17 16:54:03

[permalink] [raw]

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

On Tue, Oct 17, 2023 at 06:10:21PM +0200, Hardik Gajjar wrote:
> On Mon, Oct 16, 2023 at 07:58:36PM +0200, Greg KH wrote:
> > On Wed, Oct 11, 2023 at 06:45:25PM +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.
> >
> > "should" according to whom? That goes against the USB specification, so
> > why not take it up with them?
> We're implementing this change to meet Google's Android Auto certification
> requirements, even though these requirements aren't publicly available.

Then perhaps Google needs to talk about this somewhere if they are
forcing us to take such an out-of-spec requirement?

> The issue arises when we connect an Android phone to a Linux host using a special
> automotive USB hub. The host's xHCI doesn't receive a response from the hub after
> sending a 'set_address' request, resulting in a timeout. This problem is specific
> to one phone model.
>
> It's worth noting that we can't argue that this requirement violates USB standards
> because the 3-second time limit applies only when connecting an Android mobile phone.
> I assume that Android phones also undergo Google certification, which likely includes
> a condition for successful enumeration within a specified time frame. However, I can't confirm this.

Android's CTS/VTS testing is pretty public, but huge, so you might want
to dig into that and see if they really test for this or not.

> More logs and detailed in patch V1:
> https://lore.kernel.org/linux-usb/[email protected]/T/#m452ec9dad94e8181fdb050cd29483dd89437f7c1
> >
> > > Achieving this is impossible in scenarios where the set_address is
> > > not successful and waits for a timeout.
> >
> > Agreed, broken hardware is a pain, but if your device is allowed to take
> > longer, it can, and will, so you have to support that.
> >
> The problem is not caused by the device taking an extended amount of time to
> process the 'set_address' request. Instead, the issue lies in the absence of
> any activity on the upstream bus until a timeout occurs.

So, a broken device. Why are you then adding the hub to the quirk list
and not the broken device? We are used to adding broken devices to
qurik lists all the time, this shouldn't be new.

> This situation arises when the host has already transmitted the 'set_address' command to the hub,
> assuming that the device operates at full speed. However, the device connected
> to the hub undergoes a state change from full speed to high-speed during this process.

During which process? While the set-address happens? That feels like a
hub bug then.

> > > 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.
> >
> > Screen projection is a requirement that you should not be relying on USB
> > for as USB has a different set of required timeouts, right? This sounds
> > like a bad hardware design, if not an impossible one.
> >
>
> Screen projection for us means displaying the connected phone on the screen and
> launching Carplay and Android Auto for the user. This works perfectly in nearly all
> cases, except in scenarios like this one where a combination of a special hub and
> a specific phone model is causing the issue

So which is broken, the hub or phone?

> > So the real issue that you have here is a broken built-in USB hub that
> > does not error out quick enough, right? Why not fix the firmware in
> > that hub as you know it's broken? Why is it the operating system's job
> > to work around non-compliant devices?
> >
> > Ok, that last question was redundant, of course it's our job to work
> > around broken devices, but this feels different. You are trying to say
> > "hey, I know this device is broken, so error out quick so we can just
> > ignore it", right? If so, why not just never allow that device to
> > enumerate at all? You don't have to accept it as a valid device to the
> > system (just don't authorize it), and then no device will ever connect
> > to it so what is the delay issue?
> >
>
> We require the device (HUB) as it is a vital component of our infotainment system.

Great, then you can fix the firmware in it!

> The issue arises when the host has already issued the 'set_address' command to the hub,
> assuming the device is operating at full speed. However, in such cases, when the device
> connected to the hub changes its state from full speed to high-speed, the 'set_address'
> request becomes blocked, waiting for the full 5-second timeout. This patch reduces the
> timeout from 5 seconds to 500 milliseconds, allowing for quicker failure and re-enumeration
> of the device as high-speed

Changing speed is under hub control, not device control, right? Are you
sure the firmware is correct in that hub? Has the hub passed all of the
USB-IF testing requirements?

thanks,

greg k-h

2023-10-17 19:01:02

[permalink] [raw]

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

On Tue, Oct 17, 2023 at 06:53:44PM +0200, Greg KH wrote:
> On Tue, Oct 17, 2023 at 06:10:21PM +0200, Hardik Gajjar wrote:
> > More logs and detailed in patch V1:
> > https://lore.kernel.org/linux-usb/[email protected]/T/#m452ec9dad94e8181fdb050cd29483dd89437f7c1
> > >
> > > > Achieving this is impossible in scenarios where the set_address is
> > > > not successful and waits for a timeout.
> > >
> > > Agreed, broken hardware is a pain, but if your device is allowed to take
> > > longer, it can, and will, so you have to support that.
> > >
> > The problem is not caused by the device taking an extended amount of time to
> > process the 'set_address' request. Instead, the issue lies in the absence of
> > any activity on the upstream bus until a timeout occurs.
>
> So, a broken device. Why are you then adding the hub to the quirk list
> and not the broken device? We are used to adding broken devices to
> qurik lists all the time, this shouldn't be new.

Adding a quirk for the device isn't feasible, because the problem occurs
before the device has been initialized and enumerated. The kernel
doesn't know anything about the device at this point; only that it has
just connected.

> > This situation arises when the host has already transmitted the 'set_address' command to the hub,
> > assuming that the device operates at full speed. However, the device connected
> > to the hub undergoes a state change from full speed to high-speed during this process.
>
> During which process? While the set-address happens? That feels like a
> hub bug then.
>
> > > > 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.
> > >
> > > Screen projection is a requirement that you should not be relying on USB
> > > for as USB has a different set of required timeouts, right? This sounds
> > > like a bad hardware design, if not an impossible one.
> > >
> >
> > Screen projection for us means displaying the connected phone on the screen and
> > launching Carplay and Android Auto for the user. This works perfectly in nearly all
> > cases, except in scenarios like this one where a combination of a special hub and
> > a specific phone model is causing the issue
>
> So which is broken, the hub or phone?

It sounds like both of them are broken to some extent, although we can't
tell for sure without seeing what's actually happening on the USB bus
(i.e., bus analyzer output):

The phone seems to take too long to activate its high-speed
terminations and deactivate the full-speed terminations.

The hub doesn't seem to realize that the phone has disconnected
its full-speed connection and switched to high-speed.

But without real data, these are just best guesses.

> > The issue arises when the host has already issued the 'set_address' command to the hub,
> > assuming the device is operating at full speed. However, in such cases, when the device
> > connected to the hub changes its state from full speed to high-speed, the 'set_address'
> > request becomes blocked, waiting for the full 5-second timeout. This patch reduces the
> > timeout from 5 seconds to 500 milliseconds, allowing for quicker failure and re-enumeration
> > of the device as high-speed
>
> Changing speed is under hub control, not device control, right? Are you

Changing speed is under device control. But of course, the hub has to
detect the change and react to it properly.

Alan Stern

> sure the firmware is correct in that hub? Has the hub passed all of the
> USB-IF testing requirements?
>
> thanks,
>
> greg k-h

2023-10-21 10:16:35

[permalink] [raw]

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

On Tue, Oct 17, 2023 at 02:59:54PM -0400, Alan Stern wrote:
> On Tue, Oct 17, 2023 at 06:53:44PM +0200, Greg KH wrote:
> > On Tue, Oct 17, 2023 at 06:10:21PM +0200, Hardik Gajjar wrote:
> > > More logs and detailed in patch V1:
> > > https://lore.kernel.org/linux-usb/[email protected]/T/#m452ec9dad94e8181fdb050cd29483dd89437f7c1
> > > >
> > > > > Achieving this is impossible in scenarios where the set_address is
> > > > > not successful and waits for a timeout.
> > > >
> > > > Agreed, broken hardware is a pain, but if your device is allowed to take
> > > > longer, it can, and will, so you have to support that.
> > > >
> > > The problem is not caused by the device taking an extended amount of time to
> > > process the 'set_address' request. Instead, the issue lies in the absence of
> > > any activity on the upstream bus until a timeout occurs.
> >
> > So, a broken device. Why are you then adding the hub to the quirk list
> > and not the broken device? We are used to adding broken devices to
> > qurik lists all the time, this shouldn't be new.
>
> Adding a quirk for the device isn't feasible, because the problem occurs
> before the device has been initialized and enumerated. The kernel
> doesn't know anything about the device at this point; only that it has
> just connected.

Ah, ick, you are right, but we do know the "broken hub" id, so that
makes a bit more sense. Should this be a hub-only type quirk?

> > > This situation arises when the host has already transmitted the 'set_address' command to the hub,
> > > assuming that the device operates at full speed. However, the device connected
> > > to the hub undergoes a state change from full speed to high-speed during this process.
> >
> > During which process? While the set-address happens? That feels like a
> > hub bug then.
> >
> > > > > 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.
> > > >
> > > > Screen projection is a requirement that you should not be relying on USB
> > > > for as USB has a different set of required timeouts, right? This sounds
> > > > like a bad hardware design, if not an impossible one.
> > > >
> > >
> > > Screen projection for us means displaying the connected phone on the screen and
> > > launching Carplay and Android Auto for the user. This works perfectly in nearly all
> > > cases, except in scenarios like this one where a combination of a special hub and
> > > a specific phone model is causing the issue
> >
> > So which is broken, the hub or phone?
>
> It sounds like both of them are broken to some extent, although we can't
> tell for sure without seeing what's actually happening on the USB bus
> (i.e., bus analyzer output):
>
> The phone seems to take too long to activate its high-speed
> terminations and deactivate the full-speed terminations.
>
> The hub doesn't seem to realize that the phone has disconnected
> its full-speed connection and switched to high-speed.
>
> But without real data, these are just best guesses.

Agreed, Hardik, can you look at some bus traces to figure out where the
root problem here is?

thanks,

greg k-h

2023-10-23 16:14:24

[permalink] [raw]

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

On Sat, Oct 21, 2023 at 12:15:35PM +0200, Greg KH wrote:
> On Tue, Oct 17, 2023 at 02:59:54PM -0400, Alan Stern wrote:
> > On Tue, Oct 17, 2023 at 06:53:44PM +0200, Greg KH wrote:
> > > On Tue, Oct 17, 2023 at 06:10:21PM +0200, Hardik Gajjar wrote:
> > > > More logs and detailed in patch V1:
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dusb_20230818092353.124658-2D1-2Dhgajjar-40de.adit-2Djv.com_T_-23m452ec9dad94e8181fdb050cd29483dd89437f7c1&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=P0HXZTx6ta7v5M4y2Y7WZkPrY-dpKkxBq8tAzuX8cI9aj9tE2NuVvJjLl3Uvojpw&s=N_HwnQeZb_gHMmgz53uTGDUZVi28EXb1l9Pg6PdbvVI&e=
> > > > >
> > > > > > Achieving this is impossible in scenarios where the set_address is
> > > > > > not successful and waits for a timeout.
> > > > >
> > > > > Agreed, broken hardware is a pain, but if your device is allowed to take
> > > > > longer, it can, and will, so you have to support that.
> > > > >
> > > > The problem is not caused by the device taking an extended amount of time to
> > > > process the 'set_address' request. Instead, the issue lies in the absence of
> > > > any activity on the upstream bus until a timeout occurs.
> > >
> > > So, a broken device. Why are you then adding the hub to the quirk list
> > > and not the broken device? We are used to adding broken devices to
> > > qurik lists all the time, this shouldn't be new.
> >
> > Adding a quirk for the device isn't feasible, because the problem occurs
> > before the device has been initialized and enumerated. The kernel
> > doesn't know anything about the device at this point; only that it has
> > just connected.
>
> Ah, ick, you are right, but we do know the "broken hub" id, so that
> makes a bit more sense. Should this be a hub-only type quirk?

In addition to the earlier comment, it appears that the issue is most likely
related to the hub. While we have identified one specific phone that triggers
this problem, we cannot determine how many other devices might encounter a
similar issue, where they enumerate as full speed initially and then switch
to high speed. To address this, we are proposing to use a 500 ms timeout for
all devices connected via the hub. This change aims to prevent potential
timeout-related problems with various devices
>
> > > > This situation arises when the host has already transmitted the 'set_address' command to the hub,
> > > > assuming that the device operates at full speed. However, the device connected
> > > > to the hub undergoes a state change from full speed to high-speed during this process.
> > >
> > > During which process? While the set-address happens? That feels like a
> > > hub bug then.
> > >
> > > > > > 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.
> > > > >
> > > > > Screen projection is a requirement that you should not be relying on USB
> > > > > for as USB has a different set of required timeouts, right? This sounds
> > > > > like a bad hardware design, if not an impossible one.
> > > > >
> > > >
> > > > Screen projection for us means displaying the connected phone on the screen and
> > > > launching Carplay and Android Auto for the user. This works perfectly in nearly all
> > > > cases, except in scenarios like this one where a combination of a special hub and
> > > > a specific phone model is causing the issue
> > >
> > > So which is broken, the hub or phone?
> >
> > It sounds like both of them are broken to some extent, although we can't
> > tell for sure without seeing what's actually happening on the USB bus
> > (i.e., bus analyzer output):
> >
> > The phone seems to take too long to activate its high-speed
> > terminations and deactivate the full-speed terminations.
> >
> > The hub doesn't seem to realize that the phone has disconnected
> > its full-speed connection and switched to high-speed.0
> >
> > But without real data, these are just best guesses.
>
> Agreed, Hardik, can you look at some bus traces to figure out where the
> root problem here is?

It does appear that the issue is related to the hub, and the ideal solution would involve
modifying the hub's firmware. However, implementing such a firmware fix is currently not
a straightforward task. As a result, we have implemented this quirk-based solution to
mitigate the issue to some extent

Following is the LeCroy analyzer logs:

1. logs between Hub and phone with broken hub.

In packet 58, there is a Full-speed J (suspend) event that lasted for 5.347 seconds.
It's suspected that the hub was suspended due to incorrect chirp parsing.
This anomaly in chirp parsing may be a contributing factor to the issue we're facing.

Packet(0)
_______| Dir SE0?(?????)-BAD Idle(0.000 ns) Time Stamp(44 . 964 254 882)
_______|_______________________________________________________________________Ch0
10 Packets(1-10) Dir Chirp J(27.233 sec) Chirp J Chirp K
_______| Time( 27.905 sec) Time Stamp(0 . 000 001 050)
_______|_______________________________________________________________________Ch0
Packet(11) Dir(?) Full Speed J (Suspend)(151.614 ms) Idle(50.000 ns)
_______| Time Stamp(27 . 905 210 900)
_______|_______________________________________________________________________Ch0
Packet(12) Dir Chirp J( 10.923 ms) Idle( 11.866 us)
_______| Time Stamp(28 . 056 825 150)
_______|_______________________________________________________________________Ch0
Packet(13) Dir(?) Full Speed J( 2.167 us) Idle( 11.401 us)
_______| Time Stamp(28 . 067 759 916)
_______|_______________________________________________________________________Ch0
Packet(14) Dir(?) Full Speed J( 2.167 us) Idle( 11.399 us)
_______| Time Stamp(28 . 067 773 484)
_______|_______________________________________________________________________Ch0
Packet(15) Dir(?) Full Speed J(122.082 us) Idle( 2.834 us)
_______| Time Stamp(28 . 067 787 050)
_______|_______________________________________________________________________Ch0
Packet(16) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 067 911 966)
_______|_______________________________________________________________________Ch0
Packet(17) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 068 911 916)
_______|_______________________________________________________________________Ch0
Packet(18) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(28 . 069 911 866)
_______|_______________________________________________________________________Ch0
Packet(19) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 070 911 850)
_______|_______________________________________________________________________Ch0
Packet(20) Dir(?) Full Speed J(997.132 us) Idle( 2.850 us)
_______| Time Stamp(28 . 071 911 800)
_______|_______________________________________________________________________Ch0
Packet(21) Dir(?) Full Speed J(997.068 us) Idle( 2.850 us)
_______| Time Stamp(28 . 072 911 782)
_______|_______________________________________________________________________Ch0
Packet(22) Dir(?) Full Speed J(997.200 us) Idle( 2.832 us)
_______| Time Stamp(28 . 073 911 700)
_______|_______________________________________________________________________Ch0
Packet(23) Dir(?) Full Speed J(997.118 us) Idle( 2.832 us)
_______| Time Stamp(28 . 074 911 732)
_______|_______________________________________________________________________Ch0
Packet(24) Dir(?) Full Speed J(997.134 us) Idle( 2.834 us)
_______| Time Stamp(28 . 075 911 682)
_______|_______________________________________________________________________Ch0
Packet(25) Dir(?) Full Speed J(997.132 us) Idle( 2.834 us)
_______| Time Stamp(28 . 076 911 650)
_______|_______________________________________________________________________Ch0
Packet(26) Dir(?) Full Speed J(997.134 us) Idle( 2.832 us)
_______| Time Stamp(28 . 077 911 616)
_______|_______________________________________________________________________Ch0
Packet(27) Dir(?) Full Speed J(997.134 us) Idle( 2.834 us)
_______| Time Stamp(28 . 078 911 582)
_______|_______________________________________________________________________Ch0
Packet(28) Dir(?) Full Speed J(997.116 us) Idle( 2.834 us)
_______| Time Stamp(28 . 079 911 550)
_______|_______________________________________________________________________Ch0
Packet(29) Dir(?) Full Speed J(997.150 us) Idle( 2.832 us)
_______| Time Stamp(28 . 080 911 500)
_______|_______________________________________________________________________Ch0
Packet(30) Dir(?) Full Speed J(997.118 us) Idle( 2.832 us)
_______| Time Stamp(28 . 081 911 482)
_______|_______________________________________________________________________Ch0
Packet(31) Dir(?) Full Speed J(997.150 us) Idle( 2.834 us)
_______| Time Stamp(28 . 082 911 432)
_______|_______________________________________________________________________Ch0
Packet(32) Dir(?) Full Speed J(997.116 us) Idle( 2.918 us)
_______| Time Stamp(28 . 083 911 416)
_______|_______________________________________________________________________Ch0
Packet(33) Dir(?) Full Speed J(997.032 us) Idle( 2.918 us)
_______| Time Stamp(28 . 084 911 450)
_______|_______________________________________________________________________Ch0
Packet(34) Dir(?) Full Speed J(997.066 us) Idle( 2.834 us)
_______| Time Stamp(28 . 085 911 400)
_______|_______________________________________________________________________Ch0
Packet(35) Dir(?) Full Speed J(997.082 us) Idle( 2.850 us)
_______| Time Stamp(28 . 086 911 300)
_______|_______________________________________________________________________Ch0
Packet(36) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 087 911 232)
_______|_______________________________________________________________________Ch0
Packet(37) Dir(?) Full Speed J(997.182 us) Idle( 2.850 us)
_______| Time Stamp(28 . 088 911 200)
_______|_______________________________________________________________________Ch0
Packet(38) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 089 911 232)
_______|_______________________________________________________________________Ch0
Packet(39) Dir(?) Full Speed J(997.100 us) Idle( 2.832 us)
_______| Time Stamp(28 . 090 911 200)
_______|_______________________________________________________________________Ch0
Packet(40) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(28 . 091 911 132)
_______|_______________________________________________________________________Ch0
Packet(41) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(28 . 092 911 116)
_______|_______________________________________________________________________Ch0
Packet(42) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 093 911 082)
_______|_______________________________________________________________________Ch0
Packet(43) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(28 . 094 911 050)
_______|_______________________________________________________________________Ch0
Packet(44) Dir(?) Full Speed J(997.100 us) Idle( 2.834 us)
_______| Time Stamp(28 . 095 911 016)
_______|_______________________________________________________________________Ch0
Packet(45) Dir(?) Full Speed J(997.150 us) Idle( 2.850 us)
_______| Time Stamp(28 . 096 910 950)
_______|_______________________________________________________________________Ch0
Packet(46) Dir(?) Full Speed J(997.100 us) Idle( 2.832 us)
_______| Time Stamp(28 . 097 910 950)
_______|_______________________________________________________________________Ch0
Packet(47) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(28 . 098 910 882)
_______|_______________________________________________________________________Ch0
Packet(48) Dir(?) Full Speed J(997.116 us) Idle( 2.834 us)
_______| Time Stamp(28 . 099 910 866)
_______|_______________________________________________________________________Ch0
Packet(49) Dir(?) Full Speed J(997.116 us) Idle( 2.834 us)
_______| Time Stamp(28 . 100 910 816)
_______|_______________________________________________________________________Ch0
Packet(50) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 101 910 766)
_______|_______________________________________________________________________Ch0
Packet(51) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(28 . 102 910 716)
_______|_______________________________________________________________________Ch0
Packet(52) Dir(?) Full Speed J(997.184 us) Idle( 2.850 us)
_______| Time Stamp(28 . 103 910 682)
_______|_______________________________________________________________________Ch0
Packet(53) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(28 . 104 910 716)
_______|_______________________________________________________________________Ch0
Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 105 910 682)
_______|_______________________________________________________________________Ch0
Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 106 910 632)
_______|_______________________________________________________________________Ch0
Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
_______| Time Stamp(28 . 107 910 600)
_______|_______________________________________________________________________Ch0
Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
_______| Time Stamp(28 . 108 532 832)
_______|_______________________________________________________________________Ch0
Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
_______| Time Stamp(28 . 247 657 600)
_______|_______________________________________________________________________Ch0
173 Packets(59-231) Dir(?) Chirp K( 2.000 ms) Chirp J Chirp K
_______| Time( 10.918 ms) Time Stamp(33 . 594 605 882)
_______|_______________________________________________________________________Ch0
Packet(232) Dir H(S) SPLIT(0x1E) SC(1) Hub Addr(1) Port(3) S(0) U(0)
_______| ET(Ctrl) CRC5(0x01) Pkt Len(9) Duration(150.000 ns) Idle(200.000 ns)
_______| Time Stamp(33 . 605 524 366)
_______|_______________________________________________________________________Ch0

2. Following is the logs between Host and Hub:

In transfer 12, the Full-Speed (FS) SET Address request commenced at
8.995 seconds, following an SSPLIT request. Unfortunately, it eventually
failed in Transaction 2866864 at 14.784 seconds with a Timeout error,
which is nearly a 5-second duration.

It appears that the timeout occurred due to a suspension in the upstream
logs.

Transfer(12) F(H) Control(SET) ADDR(0) ENDP(0) bRequest(SET_ADDRESS)
_______| wValue(New address 4) wIndex(0x0000) wLength(0) STALL(0x08)
_______| Time Stamp(8 . 955 781 832)
_______|_______________________________________________________________________
Split Trans(0) F(H) SETUP(0xB4) ADDR(0) ENDP(0) T(0) D(H->D) Tp(S) R(D)
_______| bRequest(0x05) wValue(0x0004) wIndex(0x0000) wLength(0) STALL(0x78)
_______| Time Stamp(8 . 955 781 832)
_______|_______________________________________________________________________
Transaction(307) H(S) SSplit(Ctrl) Hub Addr(1) Port(2) Speed(Full) SETUP(0xB4)
_______| ADDR(0) ENDP(0) T(0) D(H->D) Tp(S) R(D) bRequest(0x05) wValue(0x0004)
_______| wIndex(0x0000) wLength(0) ACK(0x4B) Time Stamp(8 . 955 781 832)
_______|_______________________________________________________________________Ch0
Packet(72291) Dir H(S) SPLIT(0x1E) SC(0) Hub Addr(1) Port(2) S(0) E(0)
_______| ET(Ctrl) CRC5(0x05) Pkt Len(9) Duration(150.000 ns) Idle(200.000 ns)
_______| Time Stamp(8 . 955 781 832)
_______|_______________________________________________________________________Ch0
Packet(72292) Dir H(S) SETUP(0xB4) ADDR(0) ENDP(0) CRC5(0x08) Pkt Len(8)
_______| Duration(133.330 ns) Idle(200.660 ns) Time Stamp(8 . 955 782 182)
_______|_______________________________________________________________________Ch0
Packet(72293) Dir H(S) DATA0(0xC3) Data(8 bytes) CRC16(0xD70E)
_______| Pkt Len(16) Duration(266.660 ns) Idle(299.330 ns)
_______| Time Stamp(8 . 955 782 516)
_______|_______________________________________________________________________Ch0
Packet(72294) Dir H(S) ACK(0x4B) Pkt Len(6) Duration(100.000 ns)
_______| Time(984.000 ns) Time Stamp(8 . 955 783 082)
_______|_______________________________________________________________________
Transaction(308) H(S) CSplit(Ctrl) Hub Addr(1) Port(2) Speed(Full) SETUP(0xB4)
_______| ADDR(0) ENDP(0) STALL(0x78) Time Stamp(8 . 955 784 066)
_______|_______________________________________________________________________Ch0
Packet(72295) Dir H(S) SPLIT(0x1E) SC(1) Hub Addr(1) Port(2) S(0) U(0)
_______| ET(Ctrl) CRC5(0x1E) Pkt Len(9) Duration(150.000 ns) Idle(200.000 ns)
_______| Time Stamp(8 . 955 784 066)
_______|_______________________________________________________________________Ch0
Packet(72296) Dir H(S) SETUP(0xB4) ADDR(0) ENDP(0) CRC5(0x08) Pkt Len(8)
_______| Duration(133.330 ns) Idle(300.660 ns) Time Stamp(8 . 955 784 416)
_______|_______________________________________________________________________Ch0
Packet(72297) Dir H(S) STALL(0x78) Pkt Len(6) Duration(100.000 ns)
_______| Time( 5.829 sec) Time Stamp(8 . 955 784 850)
_______|_______________________________________________________________________
Transaction(2866864) H(S) SETUP(0xB4) ADDR(0) ENDP(0) Cplt(NO) T(0) D(H->D)
_______| Tp(S) R(D) bRequest(0x05) wValue(0x0000) wIndex(0x0000) wLength(0)
_______| !! Propagated Error !!(Turnaround/Timeout Error)
_______| Time Stamp(14 . 784 876 232)

3. Following is the logs between Hub and phone using normal hub

As you can see, the suspend timeout is only ~300 ms (packet 65) compare to 5 sec in
broken hub logs in point 1.

_______| Time Stamp(5 . 247 278 566)
_______|_______________________________________________________________________Ch0
Packet(54) Dir(?) Full Speed J(997.082 us) Idle( 2.850 us)
_______| Time Stamp(5 . 248 278 550)
_______|_______________________________________________________________________Ch0
Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.832 us)
_______| Time Stamp(5 . 249 278 482)
_______|_______________________________________________________________________Ch0
Packet(56) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(5 . 250 278 432)
_______|_______________________________________________________________________Ch0
Packet(57) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(5 . 251 278 416)
_______|_______________________________________________________________________Ch0
Packet(58) Dir(?) Full Speed J(997.132 us) Idle( 2.834 us)
_______| Time Stamp(5 . 252 278 400)
_______|_______________________________________________________________________Ch0
Packet(59) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(5 . 253 278 366)
_______|_______________________________________________________________________Ch0
Packet(60) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(5 . 254 278 332)
_______|_______________________________________________________________________Ch0
Packet(61) Dir(?) Full Speed J(997.116 us) Idle( 2.834 us)
_______| Time Stamp(5 . 255 278 316)
_______|_______________________________________________________________________Ch0
Packet(62) Dir(?) Full Speed J(997.134 us) Idle( 2.850 us)
_______| Time Stamp(5 . 256 278 266)
_______|_______________________________________________________________________Ch0
Packet(63) Dir(?) Full Speed J(997.116 us) Idle( 2.850 us)
_______| Time Stamp(5 . 257 278 250)
_______|_______________________________________________________________________Ch0
Packet(64) Dir(?) Full Speed J(466.516 us) Idle(130.706 ms)
_______| Time Stamp(5 . 258 278 216)
_______|_______________________________________________________________________Ch0
Packet(65) Dir(?) Full Speed J (Suspend)(311.252 ms) Idle( 5.384 us)
_______| Time Stamp(5 . 389 451 066)
_______|_______________________________________________________________________Ch0
256 Packets(66-321) Dir(?) Chirp K( 2.000 ms) Chirp J Chirp K
_______| Time( 15.979 ms) Time Stamp(5 . 700 708 316)
_______|_______________________________________________________________________Ch0
Packet(330) Dir H(S) SETUP(0xB4) ADDR(1) ENDP(0) CRC5(0x17) Pkt Len(8)
_______| Duration(133.330 ns) Idle(198.660 ns) Time Stamp(5 . 716 687 650)
_______|_______________________________________________________________________Ch0

I uploaded the detailed logs on gdrive

https://drive.google.com/file/d/1sbitUOIQTZ4XwbrcB0gAUucGq4ReSroW/view?usp=share_link

>
> thanks,
>
> greg k-h

2023-10-24 15:44:36

[permalink] [raw]

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

On Mon, Oct 23, 2023 at 06:13:48PM +0200, Hardik Gajjar wrote:
> On Sat, Oct 21, 2023 at 12:15:35PM +0200, Greg KH wrote:
> > On Tue, Oct 17, 2023 at 02:59:54PM -0400, Alan Stern wrote:
> > > On Tue, Oct 17, 2023 at 06:53:44PM +0200, Greg KH wrote:
> > > > On Tue, Oct 17, 2023 at 06:10:21PM +0200, Hardik Gajjar wrote:
> > > > > More logs and detailed in patch V1:
> > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dusb_20230818092353.124658-2D1-2Dhgajjar-40de.adit-2Djv.com_T_-23m452ec9dad94e8181fdb050cd29483dd89437f7c1&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=P0HXZTx6ta7v5M4y2Y7WZkPrY-dpKkxBq8tAzuX8cI9aj9tE2NuVvJjLl3Uvojpw&s=N_HwnQeZb_gHMmgz53uTGDUZVi28EXb1l9Pg6PdbvVI&e=
> > > > > >
> > > > > > > Achieving this is impossible in scenarios where the set_address is
> > > > > > > not successful and waits for a timeout.
> > > > > >
> > > > > > Agreed, broken hardware is a pain, but if your device is allowed to take
> > > > > > longer, it can, and will, so you have to support that.
> > > > > >
> > > > > The problem is not caused by the device taking an extended amount of time to
> > > > > process the 'set_address' request. Instead, the issue lies in the absence of
> > > > > any activity on the upstream bus until a timeout occurs.
> > > >
> > > > So, a broken device. Why are you then adding the hub to the quirk list
> > > > and not the broken device? We are used to adding broken devices to
> > > > qurik lists all the time, this shouldn't be new.
> > >
> > > Adding a quirk for the device isn't feasible, because the problem occurs
> > > before the device has been initialized and enumerated. The kernel
> > > doesn't know anything about the device at this point; only that it has
> > > just connected.
> >
> > Ah, ick, you are right, but we do know the "broken hub" id, so that
> > makes a bit more sense. Should this be a hub-only type quirk?
>
> In addition to the earlier comment, it appears that the issue is most likely
> related to the hub. While we have identified one specific phone that triggers
> this problem, we cannot determine how many other devices might encounter a
> similar issue, where they enumerate as full speed initially and then switch
> to high speed. To address this, we are proposing to use a 500 ms timeout for
> all devices connected via the hub. This change aims to prevent potential
> timeout-related problems with various devices

So it sounds like the best approach is to make this a hub-specific
quirk.

> It does appear that the issue is related to the hub, and the ideal solution would involve
> modifying the hub's firmware. However, implementing such a firmware fix is currently not
> a straightforward task. As a result, we have implemented this quirk-based solution to
> mitigate the issue to some extent
>
> Following is the LeCroy analyzer logs:
>
> 1. logs between Hub and phone with broken hub.
>
> In packet 58, there is a Full-speed J (suspend) event that lasted for 5.347 seconds.
> It's suspected that the hub was suspended due to incorrect chirp parsing.
> This anomaly in chirp parsing may be a contributing factor to the issue we're facing.

Yes, that's probably true. It's another indication that the hub is
somehow at fault.

Alan Stern

2023-10-25 14:12:32

[permalink] [raw]

Subject: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
which modifies the timeout value for the 'set_address' command. The
standard timeout for this command is 5000 ms, as recommended in the USB
3.2 specification (section 9.2.6.1).

However, certain scenarios, such as connecting devices through an APTIV hub,
can lead to timeout errors when the device enumerates as full speed initially
and later switches to high speed during chirp negotiation.

In such cases, USB analyzer logs reveal that the bus suspends for 5 seconds
due to incorrect chirp parsing and resumes only after two consecutive
timeout errors trigger a hub driver reset.

Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 105 910 682)
_______|_________________________________________________________________Ch0
Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 106 910 632)
_______|_________________________________________________________________Ch0
Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
_______| Time Stamp(28 . 107 910 600)
_______|_________________________________________________________________Ch0
Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
_______| Time Stamp(28 . 108 532 832)
_______|_________________________________________________________________Ch0
Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
_______| Time Stamp(28 . 247 657 600)
_______|_________________________________________________________________Ch0

This 5-second delay in device enumeration is undesirable, particularly in
automotive applications where quick enumeration is crucial
(ideally within 3 seconds).

The newly introduced quirks provide the flexibility to align with a
3-second time limit, as required in specific contexts like automotive
applications.

By reducing the 'set_address' command timeout to 500 ms, the
system can respond more swiftly to errors, initiate rapid recovery, and
ensure efficient device enumeration. This change is vital for scenarios
where rapid smartphone enumeration and screen projection are essential.
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 <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 3 +++
drivers/usb/core/hub.c | 21 +++++++++++++++++--
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 | 21 +++++++++++++------
drivers/usb/host/xhci.h | 9 ++++++--
include/linux/usb/hcd.h | 5 +++--
include/linux/usb/quirks.h | 3 +++
9 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
+ 5000 ms to 500 ms)
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..3594eeb892ac 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -54,6 +54,18 @@
#define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
#define USB_PING_RESPONSE_TIME 400 /* ns */

+/*
+ * address device command timeout 5000 ms is recommended in
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
+
+/*
+ * address device command timeout will be 500 ms when
+ * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
+ */
+#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
+
/* 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,7 +4638,12 @@ 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);
+
+ 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,
@@ -4639,11 +4656,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..863e7fe24157 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 */
}
}
@@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {

{ USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },

+ /* APTIV AUTOMOTIVE HUB */
+ { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },
+
/* DJI CineSSD */
{ USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4a286136d1a8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
}

command->status = 0;
+ /* set default timeout to 5000 ms */
+ command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..8f36c2914938 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
readl(&xhci->dba->doorbell[0]);
}

-static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
+static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
{
- return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
+ return mod_delayed_work(system_wq, &xhci->cmd_timer,
+ msecs_to_jiffies(xhci->current_cmd->timeout_ms));
}

static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
xhci_ring_cmd_db(xhci);
}
}
@@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_first_entry(&cmd->cmd_list,
struct xhci_command, cmd_list);
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
}
@@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
/* if there are no other commands queued we start the timeout timer */
if (list_empty(&xhci->cmd_list)) {
xhci->current_cmd = cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
}

list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..0c610a853aef 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
}

/*
- * Issue an Address Device command and optionally send a corresponding
- * SetAddress request to the device.
+ * xhci_setup_device - issues an Address Device command to assign a unique
+ * USB bus address.
+ * @hcd USB host controller data structure.
+ * @udev USB dev structure representing the connected device.
+ * @setup Enum specifying setup mode: address only or with context.
+ * @timeout_ms Max wait time (ms) for the command operation to complete.
+ *
+ * Return: 0 if successful; otherwise, negative error code.
*/
static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
- enum xhci_setup_dev setup)
+ enum xhci_setup_dev setup, unsigned int timeout_ms)
{
const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
unsigned long flags;
@@ -4059,6 +4065,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
}

command->in_ctx = virt_dev->in_ctx;
+ command->timeout_ms = timeout_ms;

slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
@@ -4185,14 +4192,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
+ unsigned int timeout_ms)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
}

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
+ XHCI_CMD_DEFAULT_TIMEOUT_MS);
}

/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..c0ff6b399769 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -818,6 +818,8 @@ struct xhci_command {
struct completion *completion;
union xhci_trb *command_trb;
struct list_head cmd_list;
+ /* xHCI command response timeout in milliseconds */
+ unsigned int timeout_ms;
};

/* drop context bitmasks */
@@ -1576,8 +1578,11 @@ struct xhci_td {
unsigned int num_trbs;
};

-/* xHCI command default timeout value */
-#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
+/*
+ * xHCI command default timeout value in milliseconds.
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

/* command descriptor */
struct xhci_cd {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 61d4f0b793dc..d0e19ac3ba6c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -372,8 +372,9 @@ struct hc_driver {
* or bandwidth constraints.
*/
void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
- /* Returns the hardware-chosen device address */
- int (*address_device)(struct usb_hcd *, struct usb_device *udev);
+ /* Set the hardware-chosen device address */
+ int (*address_device)(struct usb_hcd *, struct usb_device *udev,
+ unsigned int timeout_ms);
/* prepares the hardware to send commands to the device */
int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
/* Notifies the HCD after a hub descriptor is fetched.
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..0cb464e3eaf4 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
/* device has endpoints that should be ignored */
#define USB_QUIRK_ENDPOINT_IGNORE BIT(15)

+/* short device address timeout */
+#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)
+
#endif /* __LINUX_USB_QUIRKS_H */
--
2.17.1

2023-10-25 14:14:02

[permalink] [raw]

Subject: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
which modifies the timeout value for the 'set_address' command. The
standard timeout for this command is 5000 ms, as recommended in the USB
3.2 specification (section 9.2.6.1).

However, certain scenarios, such as connecting devices through an APTIV hub,
can lead to timeout errors when the device enumerates as full speed initially
and later switches to high speed during chirp negotiation.

In such cases, USB analyzer logs reveal that the bus suspends for 5 seconds
due to incorrect chirp parsing and resumes only after two consecutive
timeout errors trigger a hub driver reset.

Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 105 910 682)
_______|_________________________________________________________________Ch0
Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 106 910 632)
_______|_________________________________________________________________Ch0
Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
_______| Time Stamp(28 . 107 910 600)
_______|_________________________________________________________________Ch0
Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
_______| Time Stamp(28 . 108 532 832)
_______|_________________________________________________________________Ch0
Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
_______| Time Stamp(28 . 247 657 600)
_______|_________________________________________________________________Ch0

This 5-second delay in device enumeration is undesirable, particularly in
automotive applications where quick enumeration is crucial
(ideally within 3 seconds).

The newly introduced quirks provide the flexibility to align with a
3-second time limit, as required in specific contexts like automotive
applications.

By reducing the 'set_address' command timeout to 500 ms, the
system can respond more swiftly to errors, initiate rapid recovery, and
ensure efficient device enumeration. This change is vital for scenarios
where rapid smartphone enumeration and screen projection are essential.
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 <[email protected]>
---
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

Changes since version 3:
- Add some comments for clarity.
- Minor indentation and sequence change.

Changes since version 4:
- Changing the USB specification reference to version 3.2.
- Enhancing the commit message to provide more details about the technical issue.
- Improving the structure of function comments.
---
.../admin-guide/kernel-parameters.txt | 3 +++
drivers/usb/core/hub.c | 21 +++++++++++++++++--
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 | 21 +++++++++++++------
drivers/usb/host/xhci.h | 9 ++++++--
include/linux/usb/hcd.h | 5 +++--
include/linux/usb/quirks.h | 3 +++
9 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
+ 5000 ms to 500 ms)
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..3594eeb892ac 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -54,6 +54,18 @@
#define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
#define USB_PING_RESPONSE_TIME 400 /* ns */

+/*
+ * address device command timeout 5000 ms is recommended in
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
+
+/*
+ * address device command timeout will be 500 ms when
+ * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
+ */
+#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
+
/* 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,7 +4638,12 @@ 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);
+
+ 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,
@@ -4639,11 +4656,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..863e7fe24157 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 */
}
}
@@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {

{ USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },

+ /* APTIV AUTOMOTIVE HUB */
+ { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT },
+
/* DJI CineSSD */
{ USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4a286136d1a8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
}

command->status = 0;
+ /* set default timeout to 5000 ms */
+ command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..8f36c2914938 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
readl(&xhci->dba->doorbell[0]);
}

-static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
+static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
{
- return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
+ return mod_delayed_work(system_wq, &xhci->cmd_timer,
+ msecs_to_jiffies(xhci->current_cmd->timeout_ms));
}

static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
xhci_ring_cmd_db(xhci);
}
}
@@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_first_entry(&cmd->cmd_list,
struct xhci_command, cmd_list);
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
}
@@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
/* if there are no other commands queued we start the timeout timer */
if (list_empty(&xhci->cmd_list)) {
xhci->current_cmd = cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
}

list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..0c610a853aef 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
}

/*
- * Issue an Address Device command and optionally send a corresponding
- * SetAddress request to the device.
+ * xhci_setup_device - issues an Address Device command to assign a unique
+ * USB bus address.
+ * @hcd USB host controller data structure.
+ * @udev USB dev structure representing the connected device.
+ * @setup Enum specifying setup mode: address only or with context.
+ * @timeout_ms Max wait time (ms) for the command operation to complete.
+ *
+ * Return: 0 if successful; otherwise, negative error code.
*/
static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
- enum xhci_setup_dev setup)
+ enum xhci_setup_dev setup, unsigned int timeout_ms)
{
const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
unsigned long flags;
@@ -4059,6 +4065,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
}

command->in_ctx = virt_dev->in_ctx;
+ command->timeout_ms = timeout_ms;

slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
@@ -4185,14 +4192,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
+ unsigned int timeout_ms)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
}

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
+ XHCI_CMD_DEFAULT_TIMEOUT_MS);
}

/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..c0ff6b399769 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -818,6 +818,8 @@ struct xhci_command {
struct completion *completion;
union xhci_trb *command_trb;
struct list_head cmd_list;
+ /* xHCI command response timeout in milliseconds */
+ unsigned int timeout_ms;
};

/* drop context bitmasks */
@@ -1576,8 +1578,11 @@ struct xhci_td {
unsigned int num_trbs;
};

-/* xHCI command default timeout value */
-#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
+/*
+ * xHCI command default timeout value in milliseconds.
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

/* command descriptor */
struct xhci_cd {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 61d4f0b793dc..d0e19ac3ba6c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -372,8 +372,9 @@ struct hc_driver {
* or bandwidth constraints.
*/
void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
- /* Returns the hardware-chosen device address */
- int (*address_device)(struct usb_hcd *, struct usb_device *udev);
+ /* Set the hardware-chosen device address */
+ int (*address_device)(struct usb_hcd *, struct usb_device *udev,
+ unsigned int timeout_ms);
/* prepares the hardware to send commands to the device */
int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
/* Notifies the HCD after a hub descriptor is fetched.
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..0cb464e3eaf4 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
/* device has endpoints that should be ignored */
#define USB_QUIRK_ENDPOINT_IGNORE BIT(15)

+/* short device address timeout */
+#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16)
+
#endif /* __LINUX_USB_QUIRKS_H */
--
2.17.1

2023-10-25 14:45:05

[permalink] [raw]

Subject: Re: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

On Wed, Oct 25, 2023 at 04:13:16PM +0200, Hardik Gajjar wrote:
> This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
> which modifies the timeout value for the 'set_address' command. The
> standard timeout for this command is 5000 ms, as recommended in the USB
> 3.2 specification (section 9.2.6.1).
>
> However, certain scenarios, such as connecting devices through an APTIV hub,
> can lead to timeout errors when the device enumerates as full speed initially
> and later switches to high speed during chirp negotiation.
>
> In such cases, USB analyzer logs reveal that the bus suspends for 5 seconds
> due to incorrect chirp parsing and resumes only after two consecutive
> timeout errors trigger a hub driver reset.
>
> Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 105 910 682)
> _______|_________________________________________________________________Ch0
> Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 106 910 632)
> _______|_________________________________________________________________Ch0
> Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
> _______| Time Stamp(28 . 107 910 600)
> _______|_________________________________________________________________Ch0
> Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
> _______| Time Stamp(28 . 108 532 832)
> _______|_________________________________________________________________Ch0
> Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
> _______| Time Stamp(28 . 247 657 600)
> _______|_________________________________________________________________Ch0
>
> This 5-second delay in device enumeration is undesirable, particularly in
> automotive applications where quick enumeration is crucial
> (ideally within 3 seconds).
>
> The newly introduced quirks provide the flexibility to align with a
> 3-second time limit, as required in specific contexts like automotive
> applications.
>
> By reducing the 'set_address' command timeout to 500 ms, the
> system can respond more swiftly to errors, initiate rapid recovery, and
> ensure efficient device enumeration. This change is vital for scenarios
> where rapid smartphone enumeration and screen projection are essential.
> 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 <[email protected]>
> ---

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from
> + 5000 ms to 500 ms)

As a matter of grammatical style, it would be better to rephrase this as:

Reduce timeout of set_address command from 5000 ms to 500 ms

Apart from that one little nit, for the usbcore portions of the patch:

Reviewed-by: Alan Stern <[email protected]>

Alan Stern

2023-10-25 16:01:16

[permalink] [raw]

Subject: Re: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

Hello!

On 10/25/23 5:13 PM, Hardik Gajjar wrote:

Sorry to be PITA but... (-:

> This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
> which modifies the timeout value for the 'set_address' command. The

This is called a request, not a command by the spec. And the USB spec
names the requests in all uppercase, e.g. SET_ADDRESS...

> standard timeout for this command is 5000 ms, as recommended in the USB
> 3.2 specification (section 9.2.6.1).

This section in the USB specs 1.1/2.0/3.0 talks about _all_ requests.
I don't have USB 3.2 but It believe it has the same wording.

[...]

> Signed-off-by: Hardik Gajjar <[email protected]>
[...]

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..3c03f23bd5d5 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 reducing from

Please, "The SET_ADDRESS request" instead...

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3c54b218301c..3594eeb892ac 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -54,6 +54,18 @@
> #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> #define USB_PING_RESPONSE_TIME 400 /* ns */
>
> +/*
> + * address device command timeout 5000 ms is recommended in

What address device command? Are you sure you're not mixing
the USB and xHCI terminologies?

> + * USB 3.2 spec, section 9.2.6.1
> + */
> +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* ms */
> +
> +/*
> + * address device command timeout will be 500 ms when

Same here. This is a generic USB file, not the xHCI driver...

> + * USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT enable.
> + */
> +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* ms */
> +
> /* 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. */
[...]
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..0c610a853aef 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> }
>
> /*

You seem to be converting the existing comment to a kernel-doc one
but you miss changing from /* /** at the start and adding colons after
the param names below...

> - * Issue an Address Device command and optionally send a corresponding
> - * SetAddress request to the device.
> + * xhci_setup_device - issues an Address Device command to assign a unique
> + * USB bus address.
> + * @hcd USB host controller data structure.
> + * @udev USB dev structure representing the connected device.
> + * @setup Enum specifying setup mode: address only or with context.
> + * @timeout_ms Max wait time (ms) for the command operation to complete.
> + *
> + * Return: 0 if successful; otherwise, negative error code.
> */
> static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> - enum xhci_setup_dev setup)
> + enum xhci_setup_dev setup, unsigned int timeout_ms)
[...]

MBR, Sergey

2023-10-25 16:16:31

[permalink] [raw]

Subject: Re: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

On 10/25/23 7:00 PM, Sergey Shtylyov wrote:

[...]
> Sorry to be PITA but... (-:

I just had to speak up after Alan's ACK. :-)

>> This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
>> which modifies the timeout value for the 'set_address' command. The
>
> This is called a request, not a command by the spec. And the USB spec
> names the requests in all uppercase, e.g. SET_ADDRESS...
>
>> standard timeout for this command is 5000 ms, as recommended in the USB
>> 3.2 specification (section 9.2.6.1).
>
> This section in the USB specs 1.1/2.0/3.0 talks about _all_ requests.
> I don't have USB 3.2 but It believe it has the same wording.
>
> [...]
>
>> Signed-off-by: Hardik Gajjar <[email protected]>
[...]

>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index e1b1b64a0723..0c610a853aef 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>> }
>>
>> /*
>
> You seem to be converting the existing comment to a kernel-doc one
> but you miss changing from /* /** at the start and adding colons after

From /* to /**, I meant to type...

> the param names below...

This comment update also looks like a meterial for a separate patch...

>> - * Issue an Address Device command and optionally send a corresponding
>> - * SetAddress request to the device.
>> + * xhci_setup_device - issues an Address Device command to assign a unique
>> + * USB bus address.
>> + * @hcd USB host controller data structure.
>> + * @udev USB dev structure representing the connected device.
>> + * @setup Enum specifying setup mode: address only or with context.
>> + * @timeout_ms Max wait time (ms) for the command operation to complete.
>> + *
>> + * Return: 0 if successful; otherwise, negative error code.
>> */
>> static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>> - enum xhci_setup_dev setup)
>> + enum xhci_setup_dev setup, unsigned int timeout_ms)
> [...]

MBR, Sergey

2023-10-25 16:18:01

[permalink] [raw]

Subject: Re: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

2023-10-25 16:40:34

[permalink] [raw]

Subject: Re: [PATCH v5] usb: Reduce 'set_address' command timeout with a new quirk

On Wed, Oct 25, 2023 at 07:16:05PM +0300, Sergey Shtylyov wrote:
> On 10/25/23 7:00 PM, Sergey Shtylyov wrote:
>
> [...]
> > Sorry to be PITA but... (-:
>
> I just had to speak up after Alan's ACK. :-)
No problem, Thanks for the feedback. I agreed with many of them
>
> >> This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT,
> >> which modifies the timeout value for the 'set_address' command. The
> >
> > This is called a request, not a command by the spec. And the USB spec
> > names the requests in all uppercase, e.g. SET_ADDRESS...
> >
> >> standard timeout for this command is 5000 ms, as recommended in the USB
> >> 3.2 specification (section 9.2.6.1).
> >
> > This section in the USB specs 1.1/2.0/3.0 talks about _all_ requests.
> > I don't have USB 3.2 but It believe it has the same wording.
> >

The patch modifies both xhci and hub.c, and the keywords 'command' come from the xhci driver.
I will change 'command' to '(all)request' and newly added 'address_device' to 'SET_ADDRESS' in hub.c.
That sounds better for hub.c

> > [...]
> >
> >> Signed-off-by: Hardik Gajjar <[email protected]>
> [...]
>
> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> >> index e1b1b64a0723..0c610a853aef 100644
> >> --- a/drivers/usb/host/xhci.c
> >> +++ b/drivers/usb/host/xhci.c
> >> @@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> >> }
> >>
> >> /*
> >
> > You seem to be converting the existing comment to a kernel-doc one
> > but you miss changing from /* /** at the start and adding colons after
>
> From /* to /**, I meant to type...
>
> > the param names below...
>
> This comment update also looks like a meterial for a separate patch...
>

I think this is acceptable in the patch since it modifies the function arguments
as well. I improved the existing comments while adding information about the new
argument. However will update /* to /**

> >> - * Issue an Address Device command and optionally send a corresponding
> >> - * SetAddress request to the device.
> >> + * xhci_setup_device - issues an Address Device command to assign a unique
> >> + * USB bus address.
> >> + * @hcd USB host controller data structure.
> >> + * @udev USB dev structure representing the connected device.
> >> + * @setup Enum specifying setup mode: address only or with context.
> >> + * @timeout_ms Max wait time (ms) for the command operation to complete.
> >> + *
> >> + * Return: 0 if successful; otherwise, negative error code.
> >> */
> >> static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> >> - enum xhci_setup_dev setup)
> >> + enum xhci_setup_dev setup, unsigned int timeout_ms)
> > [...]
>
> MBR, Sergey

Thanks,
Hardik

2023-10-26 10:16:41

[permalink] [raw]

Subject: [PATCH v6] usb: Reduce the 'SET_ADDRESS' request timeout with a new quirk

This patch introduces a new USB quirk,
USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT, which modifies the timeout value
for the 'SET_ADDRESS' request. The standard timeout for USB request/command
is 5000 ms, as recommended in the USB 3.2 specification (section 9.2.6.1).

However, certain scenarios, such as connecting devices through an APTIV
hub, can lead to timeout errors when the device enumerates as full speed
initially and later switches to high speed during chirp negotiation.

In such cases, USB analyzer logs reveal that the bus suspends for
5 seconds due to incorrect chirp parsing and resumes only after two
consecutive timeout errors trigger a hub driver reset.

Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
_______| Time Stamp(28 . 105 910 682)
_______|_____________________________________________________________Ch0
Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
_______| Time Stamp(28 . 106 910 632)
_______|_____________________________________________________________Ch0
Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
_______| Time Stamp(28 . 107 910 600)
_______|_____________________________________________________________Ch0
Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
_______| Time Stamp(28 . 108 532 832)
_______|_____________________________________________________________Ch0
Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
_______| Time Stamp(28 . 247 657 600)
_______|_____________________________________________________________Ch0

This 5-second delay in device enumeration is undesirable, particularly
in automotive applications where quick enumeration is crucial
(ideally within 3 seconds).

The newly introduced quirks provide the flexibility to align with a
3-second time limit, as required in specific contexts like automotive
applications.

By reducing the 'SET_ADDRESS' request timeout to 500 ms, the
system can respond more swiftly to errors, initiate rapid recovery, and
ensure efficient device enumeration. This change is vital for scenarios
where rapid smartphone enumeration and screen projection are essential.

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/parameters/quirks"

Signed-off-by: Hardik Gajjar <[email protected]>
---
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

Changes since version 3:
- Add some comments for clarity.
- Minor indentation and sequence change.

Changes since version 4:
- Changing the USB specification reference to version 3.2.
- Enhancing the commit message to provide more details about the technical issue.
- Improving the structure of function comments.

Changes since version 5:
- Changed the terminology in USB core driver files from 'command' to 'request'
as it is more commonly used.
It's important to note that USB specifications indicate these terms are interchangeable.
For example, USB spec 3.2, section 9.2.6.1, uses the term 'command' in its text
"USB sets an upper limit of 5 seconds for any command to be processed. "
- Change set_address to SET_ADDRESS.
---
.../admin-guide/kernel-parameters.txt | 3 +++
drivers/usb/core/hub.c | 22 ++++++++++++++++--
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 | 23 +++++++++++++------
drivers/usb/host/xhci.h | 9 ++++++--
include/linux/usb/hcd.h | 5 ++--
include/linux/usb/quirks.h | 3 +++
9 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..4aa3723d2eaf 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_SET_ADDRESS_REQ_TIMEOUT (Reduce
+ timeout of the SET_ADDRESS request from
+ 5000 ms to 500 ms)
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..98db92af2cce 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -54,6 +54,19 @@
#define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
#define USB_PING_RESPONSE_TIME 400 /* ns */

+/*
+ * USB 3.2 spec, section 9.2.6.1
+ * USB sets an upper limit of 5000 ms for any command/request
+ * to be processed.
+ */
+#define USB_DEFAULT_REQUEST_TIMEOUT_MS 5000 /* ms */
+
+/*
+ * The SET_ADDRESS request timeout will be 500 ms when
+ * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.
+ */
+#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */
+
/* 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,7 +4639,12 @@ 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_REQUEST_TIMEOUT_MS;
struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+ struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
+
+ if (hub->hdev->quirks & USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT)
+ timeout_ms = USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS;

/*
* The host controller will choose the device address,
@@ -4639,11 +4657,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..815e71f8ec59 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_SET_ADDRESS_REQ_TIMEOUT;
+ break;
/* Ignore unrecognized flag characters */
}
}
@@ -527,6 +530,9 @@ static const struct usb_device_id usb_quirk_list[] = {

{ USB_DEVICE(0x2386, 0x350e), .driver_info = USB_QUIRK_NO_LPM },

+ /* APTIV AUTOMOTIVE HUB */
+ { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT },
+
/* DJI CineSSD */
{ USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4a286136d1a8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
}

command->status = 0;
+ /* set default timeout to 5000 ms */
+ command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS;
INIT_LIST_HEAD(&command->cmd_list);
return command;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1dde53f6eb31..8f36c2914938 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci)
readl(&xhci->dba->doorbell[0]);
}

-static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay)
+static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci)
{
- return mod_delayed_work(system_wq, &xhci->cmd_timer, delay);
+ return mod_delayed_work(system_wq, &xhci->cmd_timer,
+ msecs_to_jiffies(xhci->current_cmd->timeout_ms));
}

static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
!(xhci->xhc_state & XHCI_STATE_DYING)) {
xhci->current_cmd = cur_cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
xhci_ring_cmd_db(xhci);
}
}
@@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
if (!list_is_singular(&xhci->cmd_list)) {
xhci->current_cmd = list_first_entry(&cmd->cmd_list,
struct xhci_command, cmd_list);
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
} else if (xhci->current_cmd == cmd) {
xhci->current_cmd = NULL;
}
@@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
/* if there are no other commands queued we start the timeout timer */
if (list_empty(&xhci->cmd_list)) {
xhci->current_cmd = cmd;
- xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT);
+ xhci_mod_cmd_timer(xhci);
}

list_add_tail(&cmd->cmd_list, &xhci->cmd_list);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..d856c4717ca9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3997,12 +3997,18 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
return 0;
}

-/*
- * Issue an Address Device command and optionally send a corresponding
- * SetAddress request to the device.
+/**
+ * xhci_setup_device - issues an Address Device command to assign a unique
+ * USB bus address.
+ * @hcd: USB host controller data structure.
+ * @udev: USB dev structure representing the connected device.
+ * @setup: Enum specifying setup mode: address only or with context.
+ * @timeout_ms: Max wait time (ms) for the command operation to complete.
+ *
+ * Return: 0 if successful; otherwise, negative error code.
*/
static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
- enum xhci_setup_dev setup)
+ enum xhci_setup_dev setup, unsigned int timeout_ms)
{
const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
unsigned long flags;
@@ -4059,6 +4065,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
}

command->in_ctx = virt_dev->in_ctx;
+ command->timeout_ms = timeout_ms;

slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx);
@@ -4185,14 +4192,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}

-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev,
+ unsigned int timeout_ms)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms);
}

static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
- return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY);
+ return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY,
+ XHCI_CMD_DEFAULT_TIMEOUT_MS);
}

/*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..c0ff6b399769 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -818,6 +818,8 @@ struct xhci_command {
struct completion *completion;
union xhci_trb *command_trb;
struct list_head cmd_list;
+ /* xHCI command response timeout in milliseconds */
+ unsigned int timeout_ms;
};

/* drop context bitmasks */
@@ -1576,8 +1578,11 @@ struct xhci_td {
unsigned int num_trbs;
};

-/* xHCI command default timeout value */
-#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ)
+/*
+ * xHCI command default timeout value in milliseconds.
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000

/* command descriptor */
struct xhci_cd {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 61d4f0b793dc..d0e19ac3ba6c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -372,8 +372,9 @@ struct hc_driver {
* or bandwidth constraints.
*/
void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
- /* Returns the hardware-chosen device address */
- int (*address_device)(struct usb_hcd *, struct usb_device *udev);
+ /* Set the hardware-chosen device address */
+ int (*address_device)(struct usb_hcd *, struct usb_device *udev,
+ unsigned int timeout_ms);
/* prepares the hardware to send commands to the device */
int (*enable_device)(struct usb_hcd *, struct usb_device *udev);
/* Notifies the HCD after a hub descriptor is fetched.
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..59409c1fc3de 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
/* device has endpoints that should be ignored */
#define USB_QUIRK_ENDPOINT_IGNORE BIT(15)

+/* short SET_ADDRESS request timeout */
+#define USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT BIT(16)
+
#endif /* __LINUX_USB_QUIRKS_H */
--
2.17.1

2023-10-26 13:09:31

by Mathias Nyman

[permalink] [raw]

Subject: Re: [PATCH v6] usb: Reduce the 'SET_ADDRESS' request timeout with a new quirk

On 26.10.2023 13.15, Hardik Gajjar wrote:
> This patch introduces a new USB quirk,
> USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT, which modifies the timeout value
> for the 'SET_ADDRESS' request. The standard timeout for USB request/command
> is 5000 ms, as recommended in the USB 3.2 specification (section 9.2.6.1).
>
> However, certain scenarios, such as connecting devices through an APTIV
> hub, can lead to timeout errors when the device enumerates as full speed
> initially and later switches to high speed during chirp negotiation.
>
> In such cases, USB analyzer logs reveal that the bus suspends for
> 5 seconds due to incorrect chirp parsing and resumes only after two
> consecutive timeout errors trigger a hub driver reset.
>
> Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 105 910 682)
> _______|_____________________________________________________________Ch0
> Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 106 910 632)
> _______|_____________________________________________________________Ch0
> Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
> _______| Time Stamp(28 . 107 910 600)
> _______|_____________________________________________________________Ch0
> Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
> _______| Time Stamp(28 . 108 532 832)
> _______|_____________________________________________________________Ch0
> Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
> _______| Time Stamp(28 . 247 657 600)
> _______|_____________________________________________________________Ch0
>
> This 5-second delay in device enumeration is undesirable, particularly
> in automotive applications where quick enumeration is crucial
> (ideally within 3 seconds).
>
> The newly introduced quirks provide the flexibility to align with a
> 3-second time limit, as required in specific contexts like automotive
> applications.
>
> By reducing the 'SET_ADDRESS' request timeout to 500 ms, the
> system can respond more swiftly to errors, initiate rapid recovery, and
> ensure efficient device enumeration. This change is vital for scenarios
> where rapid smartphone enumeration and screen projection are essential.
>
> 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/parameters/quirks"
>
> Signed-off-by: Hardik Gajjar <[email protected]>
> ---

For the xhci parts:

Reviewed-by: Mathias Nyman <[email protected]>

2023-10-26 16:01:30

[permalink] [raw]

Subject: Re: [PATCH v6] usb: Reduce the 'SET_ADDRESS' request timeout with a new quirk

Hello!

Please don't post the patches as a reply to the other thread, start a new
thread with a new patch version (I thought others would tell you that but nobody
has so far).
And how about changing the wording of the subject to s/th like below?

usb: new quirk to reduce the SET_ADDRESS request timeout

On 10/26/23 1:15 PM, Hardik Gajjar wrote:

> This patch introduces a new USB quirk,
> USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT, which modifies the timeout value
> for the 'SET_ADDRESS' request. The standard timeout for USB request/command

The upper case is enough of the emphasis, I don't think the apostrophes
are needed arounnd SET_ADDRESS...

[...]

> Signed-off-by: Hardik Gajjar <[email protected]>
> ---
[...]
> Changes since version 5:
> - Changed the terminology in USB core driver files from 'command' to 'request'
> as it is more commonly used.
> It's important to note that USB specifications indicate these terms are interchangeable.

Didn't know that... tried to find the proof in the USB specs but haven't
managed to do it...

> For example, USB spec 3.2, section 9.2.6.1, uses the term 'command' in its text
> "USB sets an upper limit of 5 seconds for any command to be processed. "

Hm, indeed; and this wording is even inherited from USB 1.1...

[...]

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3c54b218301c..98db92af2cce 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -54,6 +54,19 @@
> #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> #define USB_PING_RESPONSE_TIME 400 /* ns */
>
> +/*
> + * USB 3.2 spec, section 9.2.6.1
> + * USB sets an upper limit of 5000 ms for any command/request
> + * to be processed.
> + */
> +#define USB_DEFAULT_REQUEST_TIMEOUT_MS 5000 /* ms */
> +
> +/*
> + * The SET_ADDRESS request timeout will be 500 ms when
> + * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.
> + */
> +#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */

I don'ts see the _MS-like suffixes in the other timeout #define's there...

[...]
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..d856c4717ca9 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3997,12 +3997,18 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> return 0;
> }
>
> -/*
> - * Issue an Address Device command and optionally send a corresponding
> - * SetAddress request to the device.
> +/**
> + * xhci_setup_device - issues an Address Device command to assign a unique
> + * USB bus address.
> + * @hcd: USB host controller data structure.
> + * @udev: USB dev structure representing the connected device.
> + * @setup: Enum specifying setup mode: address only or with context.
> + * @timeout_ms: Max wait time (ms) for the command operation to complete.
> + *
> + * Return: 0 if successful; otherwise, negative error code.

I still think the above change should be a separate follow-up (or even
a preceding) patch...

[...]

MBR, Sergey

2023-10-26 18:34:47

[permalink] [raw]

Subject: Re: [PATCH v6] usb: Reduce the 'SET_ADDRESS' request timeout with a new quirk

On Thu, Oct 26, 2023 at 12:15:51PM +0200, Hardik Gajjar wrote:
> This patch introduces a new USB quirk,
> USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT, which modifies the timeout value
> for the 'SET_ADDRESS' request. The standard timeout for USB request/command
> is 5000 ms, as recommended in the USB 3.2 specification (section 9.2.6.1).
>
> However, certain scenarios, such as connecting devices through an APTIV
> hub, can lead to timeout errors when the device enumerates as full speed
> initially and later switches to high speed during chirp negotiation.
>
> In such cases, USB analyzer logs reveal that the bus suspends for
> 5 seconds due to incorrect chirp parsing and resumes only after two
> consecutive timeout errors trigger a hub driver reset.
>
> Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 105 910 682)
> _______|_____________________________________________________________Ch0
> Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
> _______| Time Stamp(28 . 106 910 632)
> _______|_____________________________________________________________Ch0
> Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
> _______| Time Stamp(28 . 107 910 600)
> _______|_____________________________________________________________Ch0
> Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
> _______| Time Stamp(28 . 108 532 832)
> _______|_____________________________________________________________Ch0
> Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
> _______| Time Stamp(28 . 247 657 600)
> _______|_____________________________________________________________Ch0
>
> This 5-second delay in device enumeration is undesirable, particularly
> in automotive applications where quick enumeration is crucial
> (ideally within 3 seconds).
>
> The newly introduced quirks provide the flexibility to align with a
> 3-second time limit, as required in specific contexts like automotive
> applications.
>
> By reducing the 'SET_ADDRESS' request timeout to 500 ms, the
> system can respond more swiftly to errors, initiate rapid recovery, and
> ensure efficient device enumeration. This change is vital for scenarios
> where rapid smartphone enumeration and screen projection are essential.
>
> 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/parameters/quirks"
>
> Signed-off-by: Hardik Gajjar <[email protected]>
> ---

For the usbcore parts: A couple of very minor things can be improved.
Once those improvements have been made, you can add:

Reviewed-by: Alan Stern <[email protected]>

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..4aa3723d2eaf 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_SET_ADDRESS_REQ_TIMEOUT (Reduce
> + timeout of the SET_ADDRESS request from
> + 5000 ms to 500 ms)

To avoid going over the 80-column limit, move "(Reduce" to the next
line and reflow the text in parentheses.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3c54b218301c..98db92af2cce 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -54,6 +54,19 @@
> #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> #define USB_PING_RESPONSE_TIME 400 /* ns */
>
> +/*
> + * USB 3.2 spec, section 9.2.6.1
> + * USB sets an upper limit of 5000 ms for any command/request
> + * to be processed.
> + */
> +#define USB_DEFAULT_REQUEST_TIMEOUT_MS 5000 /* ms */

You don't need to define this macro at all. Just use
USB_CTRL_SET_TIMEOUT (which is already defined to be 5000) as the
existing code already does.

> +/*
> + * The SET_ADDRESS request timeout will be 500 ms when
> + * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.
> + */
> +#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */

As remarked earlier, we don't need to have the "_MS" suffix on either
the macro name or the "timeout_ms" variable name. Removing the suffix
will be more consistent with the USB_TP_TRANSMISSION_DELAY_MAX and
USB_PING_RESPONSE_TIME names you see above.

> +
> +/*
> + * The SET_ADDRESS request timeout will be 500 ms when
> + * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.

Change the second line to:

* the USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT quirk flag is set.

> + */
> +#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */

Overall, I agree with Sergey that this would be cleaner if you split it
up into two patches. The first should change the comment for the
set_address() callback function and the implementation in xhci-hcd. The
second should add the quirk flag and make the corresponding changes to
the USB core.

Alan Stern

2023-10-27 09:58:13

[permalink] [raw]

Subject: Re: [PATCH v6] usb: Reduce the 'SET_ADDRESS' request timeout with a new quirk

On Thu, Oct 26, 2023 at 02:34:22PM -0400, Alan Stern wrote:
> On Thu, Oct 26, 2023 at 12:15:51PM +0200, Hardik Gajjar wrote:
> > This patch introduces a new USB quirk,
> > USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT, which modifies the timeout value
> > for the 'SET_ADDRESS' request. The standard timeout for USB request/command
> > is 5000 ms, as recommended in the USB 3.2 specification (section 9.2.6.1).
> >
> > However, certain scenarios, such as connecting devices through an APTIV
> > hub, can lead to timeout errors when the device enumerates as full speed
> > initially and later switches to high speed during chirp negotiation.
> >
> > In such cases, USB analyzer logs reveal that the bus suspends for
> > 5 seconds due to incorrect chirp parsing and resumes only after two
> > consecutive timeout errors trigger a hub driver reset.
> >
> > Packet(54) Dir(?) Full Speed J(997.100 us) Idle( 2.850 us)
> > _______| Time Stamp(28 . 105 910 682)
> > _______|_____________________________________________________________Ch0
> > Packet(55) Dir(?) Full Speed J(997.118 us) Idle( 2.850 us)
> > _______| Time Stamp(28 . 106 910 632)
> > _______|_____________________________________________________________Ch0
> > Packet(56) Dir(?) Full Speed J(399.650 us) Idle(222.582 us)
> > _______| Time Stamp(28 . 107 910 600)
> > _______|_____________________________________________________________Ch0
> > Packet(57) Dir Chirp J( 23.955 ms) Idle(115.169 ms)
> > _______| Time Stamp(28 . 108 532 832)
> > _______|_____________________________________________________________Ch0
> > Packet(58) Dir(?) Full Speed J (Suspend)( 5.347 sec) Idle( 5.366 us)
> > _______| Time Stamp(28 . 247 657 600)
> > _______|_____________________________________________________________Ch0
> >
> > This 5-second delay in device enumeration is undesirable, particularly
> > in automotive applications where quick enumeration is crucial
> > (ideally within 3 seconds).
> >
> > The newly introduced quirks provide the flexibility to align with a
> > 3-second time limit, as required in specific contexts like automotive
> > applications.
> >
> > By reducing the 'SET_ADDRESS' request timeout to 500 ms, the
> > system can respond more swiftly to errors, initiate rapid recovery, and
> > ensure efficient device enumeration. This change is vital for scenarios
> > where rapid smartphone enumeration and screen projection are essential.
> >
> > 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/parameters/quirks"
> >
> > Signed-off-by: Hardik Gajjar <[email protected]>
> > ---
>
> For the usbcore parts: A couple of very minor things can be improved.
> Once those improvements have been made, you can add:
>
> Reviewed-by: Alan Stern <[email protected]>
>
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 0a1731a0f0ef..4aa3723d2eaf 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_SET_ADDRESS_REQ_TIMEOUT (Reduce
> > + timeout of the SET_ADDRESS request from
> > + 5000 ms to 500 ms)
>
> To avoid going over the 80-column limit, move "(Reduce" to the next
> line and reflow the text in parentheses.
>
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3c54b218301c..98db92af2cce 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -54,6 +54,19 @@
> > #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */
> > #define USB_PING_RESPONSE_TIME 400 /* ns */
> >
> > +/*
> > + * USB 3.2 spec, section 9.2.6.1
> > + * USB sets an upper limit of 5000 ms for any command/request
> > + * to be processed.
> > + */
> > +#define USB_DEFAULT_REQUEST_TIMEOUT_MS 5000 /* ms */
>
> You don't need to define this macro at all. Just use
> USB_CTRL_SET_TIMEOUT (which is already defined to be 5000) as the
> existing code already does.
>
> > +/*
> > + * The SET_ADDRESS request timeout will be 500 ms when
> > + * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.
> > + */
> > +#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */
>
> As remarked earlier, we don't need to have the "_MS" suffix on either
> the macro name or the "timeout_ms" variable name. Removing the suffix
> will be more consistent with the USB_TP_TRANSMISSION_DELAY_MAX and
> USB_PING_RESPONSE_TIME names you see above.

I understand the suffix _MS in macro but the suffix is variable name was
added after the comment about timeout unit from Greg in patchV2.

https://marc.info/?l=linux-usb&m=169687322126192&w=2

Other comments will be resolved in next patch.
>
> > +
> > +/*
> > + * The SET_ADDRESS request timeout will be 500 ms when
> > + * USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT enable.
>
> Change the second line to:
>
> * the USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT quirk flag is set.
>
> > + */
> > +#define USB_SHORT_SET_ADDRESS_REQ_TIMEOUT_MS 500 /* ms */
>
> Overall, I agree with Sergey that this would be cleaner if you split it
> up into two patches. The first should change the comment for the
> set_address() callback function and the implementation in xhci-hcd. The
> second should add the quirk flag and make the corresponding changes to
> the USB core.
>
> Alan Stern

Thanks,
Hardik

2023-10-27 14:46:06