| Message ID | 20231027152029.104363-1-hgajjar@de.adit-jv.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,1/2] usb: xhci: Add timeout argument in address_device USB HCD callback | expand |
On 27.10.2023 18.20, Hardik Gajjar wrote: > - The HCD address_device callback now accepts a user-defined timeout value > in milliseconds, providing better control over command execution times. > - The default timeout value for the address_device command has been set > to 5000 ms, aligning with the USB 3.2 specification. However, this > timeout can be adjusted as needed. > - The xhci_setup_device function has been updated to accept the timeout > value, allowing it to specify the maximum wait time for the command > operation to complete. > - The hub driver has also been updated to accommodate the newly added > timeout parameter during the SET_ADDRESS request. > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> For the xhci parts Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
On Fri, Oct 27, 2023 at 11:49:13AM -0400, Alan Stern wrote: > On Fri, Oct 27, 2023 at 05:20:29PM +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 <hgajjar@de.adit-jv.com> > > --- > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> @Greg KH, Friendly reminder. Thanks, Hardik
On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote: > On 27.10.2023 18.20, Hardik Gajjar wrote: > > - The HCD address_device callback now accepts a user-defined timeout value > > in milliseconds, providing better control over command execution times. > > - The default timeout value for the address_device command has been set > > to 5000 ms, aligning with the USB 3.2 specification. However, this > > timeout can be adjusted as needed. > > - The xhci_setup_device function has been updated to accept the timeout > > value, allowing it to specify the maximum wait time for the command > > operation to complete. > > - The hub driver has also been updated to accommodate the newly added > > timeout parameter during the SET_ADDRESS request. > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > For the xhci parts > > Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > @Greg KH, Friendly reminder. Thanks, Hardik
On Fri, Nov 03, 2023 at 04:18:22PM +0100, Hardik Gajjar wrote: > On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote: > > On 27.10.2023 18.20, Hardik Gajjar wrote: > > > - The HCD address_device callback now accepts a user-defined timeout value > > > in milliseconds, providing better control over command execution times. > > > - The default timeout value for the address_device command has been set > > > to 5000 ms, aligning with the USB 3.2 specification. However, this > > > timeout can be adjusted as needed. > > > - The xhci_setup_device function has been updated to accept the timeout > > > value, allowing it to specify the maximum wait time for the command > > > operation to complete. > > > - The hub driver has also been updated to accommodate the newly added > > > timeout parameter during the SET_ADDRESS request. > > > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > > > For the xhci parts > > > > Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > > > > > @Greg KH, Friendly reminder. It is the m iddle of the merge window, my branches are closed for obvious reasons until after -rc1 is out. Please relax and wait for a week or so _after_ -rc1 is out before worrying about anything. thanks, greg k-h
On Fri, Nov 03, 2023 at 04:26:37PM +0100, Greg KH wrote: > On Fri, Nov 03, 2023 at 04:18:22PM +0100, Hardik Gajjar wrote: > > On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote: > > > On 27.10.2023 18.20, Hardik Gajjar wrote: > > > > - The HCD address_device callback now accepts a user-defined timeout value > > > > in milliseconds, providing better control over command execution times. > > > > - The default timeout value for the address_device command has been set > > > > to 5000 ms, aligning with the USB 3.2 specification. However, this > > > > timeout can be adjusted as needed. > > > > - The xhci_setup_device function has been updated to accept the timeout > > > > value, allowing it to specify the maximum wait time for the command > > > > operation to complete. > > > > - The hub driver has also been updated to accommodate the newly added > > > > timeout parameter during the SET_ADDRESS request. > > > > > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > > > > > For the xhci parts > > > > > > Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > > > > > > > > > @Greg KH, Friendly reminder. > > It is the m iddle of the merge window, my branches are closed for > obvious reasons until after -rc1 is out. Please relax and wait for a > week or so _after_ -rc1 is out before worrying about anything. In the meantime, to make things go faster, please help review patches from others on the mailing list so that when the merge window does open back up, my queue will be much smaller and lighter and yours will be closer to the top. thanks, greg k-h
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 3c54b218301c..376147af287f 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4639,7 +4639,7 @@ 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, USB_CTRL_SET_TIMEOUT); else retval = usb_control_msg(udev, usb_sndaddr0pipe(), USB_REQ_SET_ADDRESS, 0, devnum, 0, diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 8714ab5bf04d..4d6df03d255e 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; 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..7c972d5b475b 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); } /* diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 7e282b4522c0..43193af562f4 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 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.
- The HCD address_device callback now accepts a user-defined timeout value in milliseconds, providing better control over command execution times. - The default timeout value for the address_device command has been set to 5000 ms, aligning with the USB 3.2 specification. However, this timeout can be adjusted as needed. - The xhci_setup_device function has been updated to accept the timeout value, allowing it to specify the maximum wait time for the command operation to complete. - The hub driver has also been updated to accommodate the newly added timeout parameter during the SET_ADDRESS request. Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> --- Changes from version 1 to 6: - The changes were part of the large patch until v6 Link to v6:https://lore.kernel.org/linux-usb/20231026101551.36551-1-hgajjar@de.adit-jv.com/ This new patch was created by splitting patch v6 into two separate patches. --- drivers/usb/core/hub.c | 2 +- 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 +++-- 6 files changed, 35 insertions(+), 17 deletions(-)