On Tue, Sep 05, 2023 at 05:31:12PM +0300, Mathias Nyman wrote:
> On 4.9.2023 12.57, Hardik Gajjar wrote:
> >
> > In our setup, we have a arm64 target with a DWC3 USB3 IP core,
> > connected through a USB hub (specifically designed for automotive use)
> > to the Android phone. Interestingly,when replicating this setup with
> > the x86 Ubuntu machine, we encountered a discrepancy.
> >
> > The primary difference between the two setups lies in the utilization
> > of the DWC3 USB IP core.
>
> Because of the SPLIT transactions seen below it looks like phone is being
> enumerated at Full or Low speed behind the automotive High-speed HUB.
>
> >
> > Working Case:
> >
> > 0,HS,202,0:17.672.512,1.333 us,8 B,I,00,00,SETUP txn (SPLIT),80 06 00 01 00 00 40 00
> > 1,HS,203,0:17.672.512,83 ns,4 B,,00,00, SPLIT packet,78 0D 03 70
> > 1,HS,204,0:17.672.513,66 ns,3 B,,00,00, SETUP packet,2D 00 10
> > 1,HS,205,0:17.672.513,200 ns,11 B,,00,00, DATA0 packet,C3 80 06 00 01 00 00 40 00 DD 94
> > 1,HS,206,0:17.672.514,33 ns,1 B,,00,00, ACK packet,D2
> > 0,HS,207,0:17.672.535,5.625.983 ms,,I,00,00,[256 CSPLIT-SETUP-NYET],
> >
>
> The working case shows a GET_DESCRIPTOR DEVICE request (80 06 00 01).
> Was the SET ADDRESS request (00 05) seen before or after this?
>
> > Non-Working Case:
> >
> > 0,HS,164,0:10.982.303,3.116 us,8 B,I,00,00,SETUP txn (SPLIT-STALL),00 05 04 00 00 00 00 00
> > 1,HS,165,0:10.982.303,83 ns,4 B,,00,00, SPLIT packet,78 01 02 A0
> > 1,HS,166,0:10.982.303,66 ns,3 B,,00,00, SETUP packet,2D 00 10
> > 1,HS,167,0:10.982.304,200 ns,11 B,,00,00, DATA0 packet,C3 00 05 04 00 00 00 00 00 EB 70
> > 1,HS,168,0:10.982.304,50 ns,1 B,,00,00, ACK packet,D2
> > 1,HS,169,0:10.982.305,866 ns,,,00,00, CSPLIT-SETUP-STALL,
> > 2,HS,170,0:10.982.305,83 ns,4 B,,00,00, SPLIT packet,78 81 02 78
> > 2,HS,171,0:10.982.306,66 ns,3 B,,00,00, SETUP packet,2D 00 10
> > 2,HS,172,0:10.982.306,50 ns,1 B,,00,00, STALL packet,1E
>
> In the failing case the SET ADDRESS (00 05) request STALLs.
> Was there a GET_DESCRIPOR request before this?
>
> This could still be a case where the order of SET ADDRES and GET DESCRIPTOR
> requests matter.
>
> See if you can force new scheme enumeration, it should make hub driver issue
> a GET_DESCRIPTOR request before SET ADDRESS.
>
> Probably testable with some quick hack that forces use_new_scheme() to return
> "true" in hub.c
In failing case, there is no GET_DESCRIPTOR DEVICE request.
The result is even worse when enabling the 'new_scheme.'
Instead of a timeout, it directly crashes and restarts the USB subsystem.
In any case, we cannot use the 'new_scheme' because it exposes additional transactions
in USB enumeration. This leads to additional delays in the enumeration of all devices,
potentially exceeding the suggested time limit
>
> >
> > Upon close inspection of the USB analyzer data for both cases,
> > it becomes evident that a STALL packet consistently appears in
> > the non-working scenario.
> >
> > This stands in contrast to the working case, where the channel
> > remains open, allowing for the successful execution of the following
> > high-speed enumeration commands.
> >
> > In the event of a stall, we require an interruption from the host to
> > resume communication.
> > Given that there is a fixed 5-second command delay, this interruption
> > can only occur after this timeout period.
> >
> > Therefore, I would like to propose a solution: providing an additional
> > API to control this command timeout or as you suggested, adding a timeout
> > parameter to struct xhci_command, and use that when calling xhci_mod_cmd_timer()
> > without the need to address any potential bugs in the xHCI driver.
>
> Lets first see if changing enumeration scheme helps.
>
> If not, and the enumeration issue is only seen for devices behind this specific
> automotive hub, then a quirk could be set for it that adjusts the timeout.
>
> Also let me know if you find xhci driver bugs, those need to be fixed anyway.
>
I couldn't identify any issues with the xHCI driver either. However, I'd like
to propose the implementation of a more generic mechanism to control the 5-second
(HZ*5) timeout. This would offer better flexibility and help reduce enumeration
time in case of bus errors.
As of now, following your suggestion, I will add a parameter in the set_address
API and use the Quirk in hub.c to pass either the default or customized timeout
to the xHCI driver. However, in that case also we may need minor modifications in
xhci driver.
soon will submit Patch-v2
Thanks,
Hardik
> Thanks
> -Mathias
@@ -3157,6 +3157,29 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
}
EXPORT_SYMBOL_GPL(usb_hcd_setup_local_mem);
+/**
+ * usb_hcd_set_cmd_timer_delay Set the delay of the command timer.
+ * @hcd - pointer to the HCD representing the controller
+ * @delay - Delay value to be used in command timer.
+ *
+ * wrapper function to call the set_cmd_timer_delay API of the host
+ * diver.
+ *
+ * return 0 on success; otherwise -ENODEV means the feature not
+ * supported by host driver.
+ */
+
+int usb_hcd_set_cmd_timer_delay(struct usb_hcd *hcd, int delay)
+{
+ int ret = -ENODEV;
+
+ if (hcd->driver->set_cmd_timer_delay)
+ ret = hcd->driver->set_cmd_timer_delay(hcd, delay);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_set_cmd_timer_delay);
+
/*-------------------------------------------------------------------------*/
#if IS_ENABLED(CONFIG_USB_MON)
@@ -366,9 +366,9 @@ 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, xhci->cmd_timer_delay);
}
static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci)
@@ -412,7 +412,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 +1786,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 +4301,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);
@@ -449,6 +449,8 @@ static int xhci_init(struct usb_hcd *hcd)
compliance_mode_recovery_timer_init(xhci);
}
+ xhci->cmd_timer_delay = XHCI_CMD_DEFAULT_TIMEOUT;
+
return retval;
}
@@ -5267,6 +5269,18 @@ static void xhci_clear_tt_buffer_complete(struct usb_hcd *hcd,
spin_unlock_irqrestore(&xhci->lock, flags);
}
+static int xhci_set_cmd_delay(struct usb_hcd *hcd, int cmd_delay_ms)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+ if (cmd_delay_ms < 150 || cmd_delay_ms > 10000)
+ return -EINVAL;
+
+ xhci->cmd_timer_delay = cmd_delay_ms;
+
+ return 0;
+}
+
static const struct hc_driver xhci_hc_driver = {
.description = "xhci-hcd",
.product_desc = "xHCI Host Controller",
@@ -5332,6 +5346,7 @@ static const struct hc_driver xhci_hc_driver = {
.disable_usb3_lpm_timeout = xhci_disable_usb3_lpm_timeout,
.find_raw_port_number = xhci_find_raw_port_number,
.clear_tt_buffer_complete = xhci_clear_tt_buffer_complete,
+ .set_cmd_timer_delay = xhci_set_cmd_delay,
};
void xhci_init_driver(struct hc_driver *drv,
@@ -1808,6 +1808,7 @@ struct xhci_hcd {
struct list_head cmd_list;
unsigned int cmd_ring_reserved_trbs;
struct delayed_work cmd_timer;
+ unsigned long cmd_timer_delay;
struct completion cmd_ring_stop_completion;
struct xhci_command *current_cmd;
@@ -405,6 +405,7 @@ struct hc_driver {
#define EHSET_TEST_SINGLE_STEP_SET_FEATURE 0x06
int (*submit_single_step_set_feature)(struct usb_hcd *,
struct urb *, int);
+ int (*set_cmd_timer_delay)(struct usb_hcd *hcd, int delay);
};
static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
@@ -466,6 +467,7 @@ extern void usb_remove_hcd(struct usb_hcd *hcd);
extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
dma_addr_t dma, size_t size);
+int usb_hcd_set_cmd_timer_delay(struct usb_hcd *hcd, int delay);
struct platform_device;
extern void usb_hcd_platform_shutdown(struct platform_device *dev);