[PATCH 0/2] USB fixes: (Re)implement timeouts
A long time ago, the USB code was interrupt-driven and used top-level timeout handling. This has long been obsolete, and that code is just broken dead cruft. HC drivers instead hardcode timeouts today.
We need to be able to specify timeouts explicitly to handle cases like USB hard disks spinning up, without having ridiculously long timeouts across the board (which would cause endless waiting when things go wrong anywhere else). So, it's time to rip out the old broken nonsense and actually pass through timeouts to USB host controller drivers, so they can be implemented properly.
This series adds the necessary top-level scaffolding for control/bulk timeouts, and implements them in xHCI. I didn't bother with interrupt transfers, since I figure those probably never need long timeouts anyway.
The platform I deal with only has xHCI, so I'll leave implementing this for EHCI/OHCI to someone else if anyone cares :)
This series needs to be applied after [1], since the xHCI changes depend on changes made there.
[1] https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f6316e@marcan...
Signed-off-by: Hector Martin marcan@marcan.st --- Hector Martin (2): usb: Pass through timeout to drivers usb: xhci: Hook up timeouts
common/usb.c | 21 ++------------------- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 ++++-- drivers/usb/host/usb-uclass.c | 9 +++++---- drivers/usb/host/xhci-ring.c | 32 ++++++++++++++++++++------------ drivers/usb/host/xhci.c | 28 ++++++++++++++++------------ include/usb.h | 10 ++++++---- include/usb/xhci.h | 14 ++++++++++---- 10 files changed, 72 insertions(+), 63 deletions(-) --- base-commit: 3d5d748e4d66b98109669c05d0c473fe67795801 change-id: 20231029-usb-fixes-5-ca87bbedb40c
Best regards,
The old USB code was interrupt-driven and just polled at the top level. This has been obsolete since interrupts were removed, which means the timeout support has been completely broken.
Rip out the top-level polling and just pass through the timeout parameter to host controller drivers. Right now this is ignored in the individual drivers.
Signed-off-by: Hector Martin marcan@marcan.st --- common/usb.c | 21 ++------------------- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 ++++-- drivers/usb/host/usb-uclass.c | 9 +++++---- drivers/usb/host/xhci.c | 5 +++-- include/usb.h | 10 ++++++---- 8 files changed, 29 insertions(+), 37 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 836506dcd9e9..8d13c5899027 100644 --- a/common/usb.c +++ b/common/usb.c @@ -241,22 +241,10 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, request, requesttype, value, index, size); dev->status = USB_ST_NOT_PROC; /*not yet processed */
- err = submit_control_msg(dev, pipe, data, size, setup_packet); + err = submit_control_msg(dev, pipe, data, size, setup_packet, timeout); if (err < 0) return err; - if (timeout == 0) - return (int)size;
- /* - * Wait for status to update until timeout expires, USB driver - * interrupt handler may set the status when the USB operation has - * been completed. - */ - while (timeout--) { - if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC)) - break; - mdelay(1); - } if (dev->status) return -1;
@@ -275,13 +263,8 @@ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, if (len < 0) return -EINVAL; dev->status = USB_ST_NOT_PROC; /*not yet processed */ - if (submit_bulk_msg(dev, pipe, data, len) < 0) + if (submit_bulk_msg(dev, pipe, data, len, timeout) < 0) return -EIO; - while (timeout--) { - if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC)) - break; - mdelay(1); - } *actual_length = dev->act_len; if (dev->status == 0) return 0; diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 9839aa17492d..ad0a1b9d24b1 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1627,7 +1627,7 @@ int usb_lock_async(struct usb_device *dev, int lock) #if CONFIG_IS_ENABLED(DM_USB) static int ehci_submit_control_msg(struct udevice *dev, struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup) + struct devrequest *setup, int timeout) { debug("%s: dev='%s', udev=%p, udev->dev='%s', portnr=%d\n", __func__, dev->name, udev, udev->dev->name, udev->portnr); @@ -1636,7 +1636,8 @@ static int ehci_submit_control_msg(struct udevice *dev, struct usb_device *udev, }
static int ehci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev, - unsigned long pipe, void *buffer, int length) + unsigned long pipe, void *buffer, int length, + int timeout) { debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev); return _ehci_submit_bulk_msg(udev, pipe, buffer, length); diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index 3f4418198ccd..8dc1f6660077 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -2047,7 +2047,7 @@ int submit_control_msg(struct usb_device *dev, unsigned long pipe, #if CONFIG_IS_ENABLED(DM_USB) static int ohci_submit_control_msg(struct udevice *dev, struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup) + struct devrequest *setup, int timeout) { ohci_t *ohci = dev_get_priv(usb_get_bus(dev));
@@ -2056,7 +2056,8 @@ static int ohci_submit_control_msg(struct udevice *dev, struct usb_device *udev, }
static int ohci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev, - unsigned long pipe, void *buffer, int length) + unsigned long pipe, void *buffer, int length, + int timeout) { ohci_t *ohci = dev_get_priv(usb_get_bus(dev));
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c index 3ccbc16da379..0ac853dc558b 100644 --- a/drivers/usb/host/r8a66597-hcd.c +++ b/drivers/usb/host/r8a66597-hcd.c @@ -705,7 +705,8 @@ static int r8a66597_submit_rh_msg(struct udevice *udev, struct usb_device *dev, static int r8a66597_submit_control_msg(struct udevice *udev, struct usb_device *dev, unsigned long pipe, void *buffer, - int length, struct devrequest *setup) + int length, struct devrequest *setup, + int timeout) { struct r8a66597 *r8a66597 = dev_get_priv(udev); u16 r8a66597_address = setup->request == USB_REQ_SET_ADDRESS ? @@ -743,7 +744,7 @@ static int r8a66597_submit_control_msg(struct udevice *udev,
static int r8a66597_submit_bulk_msg(struct udevice *udev, struct usb_device *dev, unsigned long pipe, - void *buffer, int length) + void *buffer, int length, int timeout) { struct r8a66597 *r8a66597 = dev_get_priv(udev); int ret = 0; diff --git a/drivers/usb/host/usb-sandbox.c b/drivers/usb/host/usb-sandbox.c index 3d4f8d653b5d..8e5fdc44d642 100644 --- a/drivers/usb/host/usb-sandbox.c +++ b/drivers/usb/host/usb-sandbox.c @@ -47,7 +47,8 @@ static int sandbox_submit_control(struct udevice *bus, struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup) + struct devrequest *setup, + int timeout) { struct sandbox_usb_ctrl *ctrl = dev_get_priv(bus); struct udevice *emul; @@ -81,7 +82,8 @@ static int sandbox_submit_control(struct udevice *bus, }
static int sandbox_submit_bulk(struct udevice *bus, struct usb_device *udev, - unsigned long pipe, void *buffer, int length) + unsigned long pipe, void *buffer, int length, + int timeout) { struct udevice *emul; int ret; diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index a1cd0ad2d669..97e0ecad9408 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -58,7 +58,8 @@ int submit_int_msg(struct usb_device *udev, unsigned long pipe, void *buffer, }
int submit_control_msg(struct usb_device *udev, unsigned long pipe, - void *buffer, int length, struct devrequest *setup) + void *buffer, int length, struct devrequest *setup, + int timeout) { struct udevice *bus = udev->controller_dev; struct dm_usb_ops *ops = usb_get_ops(bus); @@ -68,7 +69,7 @@ int submit_control_msg(struct usb_device *udev, unsigned long pipe, if (!ops->control) return -ENOSYS;
- err = ops->control(bus, udev, pipe, buffer, length, setup); + err = ops->control(bus, udev, pipe, buffer, length, setup, timeout); if (setup->request == USB_REQ_SET_FEATURE && setup->requesttype == USB_RT_PORT && setup->value == cpu_to_le16(USB_PORT_FEAT_RESET) && @@ -81,7 +82,7 @@ int submit_control_msg(struct usb_device *udev, unsigned long pipe, }
int submit_bulk_msg(struct usb_device *udev, unsigned long pipe, void *buffer, - int length) + int length, int timeout) { struct udevice *bus = udev->controller_dev; struct dm_usb_ops *ops = usb_get_ops(bus); @@ -89,7 +90,7 @@ int submit_bulk_msg(struct usb_device *udev, unsigned long pipe, void *buffer, if (!ops->bulk) return -ENOSYS;
- return ops->bulk(bus, udev, pipe, buffer, length); + return ops->bulk(bus, udev, pipe, buffer, length, timeout); }
struct int_queue *create_int_queue(struct usb_device *udev, diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index d13cbff9b372..a33a2460f74d 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1262,7 +1262,7 @@ static int xhci_lowlevel_stop(struct xhci_ctrl *ctrl)
static int xhci_submit_control_msg(struct udevice *dev, struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup) + struct devrequest *setup, int timeout) { struct usb_device *uhop; struct udevice *hub; @@ -1294,7 +1294,8 @@ static int xhci_submit_control_msg(struct udevice *dev, struct usb_device *udev, }
static int xhci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev, - unsigned long pipe, void *buffer, int length) + unsigned long pipe, void *buffer, int length, + int timeout) { debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev); return _xhci_submit_bulk_msg(udev, pipe, buffer, length); diff --git a/include/usb.h b/include/usb.h index 09e3f0cb309c..431801d18a84 100644 --- a/include/usb.h +++ b/include/usb.h @@ -184,9 +184,10 @@ int usb_reset_root_port(struct usb_device *dev); #endif
int submit_bulk_msg(struct usb_device *dev, unsigned long pipe, - void *buffer, int transfer_len); + void *buffer, int transfer_len, int timeout); int submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer, - int transfer_len, struct devrequest *setup); + int transfer_len, struct devrequest *setup, + int timeout); int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, int transfer_len, int interval, bool nonblock);
@@ -709,14 +710,15 @@ struct dm_usb_ops { */ int (*control)(struct udevice *bus, struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup); + struct devrequest *setup, int timeout); /** * bulk() - Send a bulk message * * Parameters are as above. */ int (*bulk)(struct udevice *bus, struct usb_device *udev, - unsigned long pipe, void *buffer, int length); + unsigned long pipe, void *buffer, int length, + int timeout); /** * interrupt() - Send an interrupt message *
On 10/29/23 08:36, Hector Martin wrote:
The old USB code was interrupt-driven and just polled at the top level. This has been obsolete since interrupts were removed, which means the timeout support has been completely broken.
Rip out the top-level polling and just pass through the timeout parameter to host controller drivers. Right now this is ignored in the individual drivers.
Signed-off-by: Hector Martin marcan@marcan.st
common/usb.c | 21 ++------------------- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 ++++-- drivers/usb/host/usb-uclass.c | 9 +++++---- drivers/usb/host/xhci.c | 5 +++-- include/usb.h | 10 ++++++---- 8 files changed, 29 insertions(+), 37 deletions(-)
[...]
int (*bulk)(struct udevice *bus, struct usb_device *udev,
unsigned long pipe, void *buffer, int length);
unsigned long pipe, void *buffer, int length,int timeout);
- interrupt() - Send an interrupt message
Can timeout ever be negative value ?
On 10/29/23 08:36, Hector Martin wrote:
The old USB code was interrupt-driven and just polled at the top level. This has been obsolete since interrupts were removed, which means the timeout support has been completely broken.
Rip out the top-level polling and just pass through the timeout parameter to host controller drivers. Right now this is ignored in the individual drivers.
Signed-off-by: Hector Martin marcan@marcan.st
Except for the negative timeout value question, which needs to be sorted out:
Reviewed-by: Marek Vasut marex@denx.de
Now that the USB core passes through timeout info to the host controller, actually hook it up.
Signed-off-by: Hector Martin marcan@marcan.st --- drivers/usb/host/xhci-ring.c | 32 ++++++++++++++++++++------------ drivers/usb/host/xhci.c | 23 +++++++++++++---------- include/usb/xhci.h | 14 ++++++++++---- 3 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index dabe6cf86af2..14c0c60e8524 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -463,11 +463,16 @@ static int event_ready(struct xhci_ctrl *ctrl) * @param expected TRB type expected from Event TRB * Return: pointer to event trb */ -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected) +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, + int timeout) { trb_type type; unsigned long ts = get_timer(0);
+ /* Fallback in case someone passes in 0 */ + if (!timeout) + timeout = XHCI_TIMEOUT; + do { union xhci_trb *event = ctrl->event_ring->dequeue;
@@ -504,7 +509,7 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected) le32_to_cpu(event->generic.field[3]));
xhci_acknowledge_event(ctrl); - } while (get_timer(ts) < XHCI_TIMEOUT); + } while (get_timer(ts) < timeout);
if (expected == TRB_TRANSFER) return NULL; @@ -528,7 +533,7 @@ static void reset_ep(struct usb_device *udev, int ep_index)
printf("Resetting EP %d...\n", ep_index); xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_RESET_EP); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return;
@@ -539,7 +544,7 @@ static void reset_ep(struct usb_device *udev, int ep_index) addr = xhci_trb_virt_to_dma(ring->enq_seg, (void *)((uintptr_t)ring->enqueue | ring->cycle_state)); xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return;
@@ -569,7 +574,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_STOP_RING);
- event = xhci_wait_for_event(ctrl, TRB_NONE); + event = xhci_wait_for_event(ctrl, TRB_NONE, XHCI_SYS_TIMEOUT); if (!event) return;
@@ -582,7 +587,8 @@ static void abort_td(struct usb_device *udev, int ep_index) != COMP_STOP))); xhci_acknowledge_event(ctrl);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, + XHCI_SYS_TIMEOUT); if (!event) return; type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags)); @@ -601,7 +607,7 @@ static void abort_td(struct usb_device *udev, int ep_index) addr = xhci_trb_virt_to_dma(ring->enq_seg, (void *)((uintptr_t)ring->enqueue | ring->cycle_state)); xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return;
@@ -657,10 +663,11 @@ static void record_transfer_result(struct usb_device *udev, * @param pipe contains the DIR_IN or OUT , devnum * @param length length of the buffer * @param buffer buffer to be read/written based on the request + * @param timeout timeout in milliseconds * Return: returns 0 if successful else -1 on failure */ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, - int length, void *buffer) + int length, void *buffer, int timeout) { int num_trbs = 0; struct xhci_generic_trb *start_trb; @@ -825,7 +832,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, giveback_first_trb(udev, ep_index, start_cycle, start_trb);
again: - event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, timeout); if (!event) { debug("XHCI bulk transfer timed out, aborting...\n"); abort_td(udev, ep_index); @@ -862,11 +869,12 @@ again: * @param req request type * @param length length of the buffer * @param buffer buffer to be read/written based on the request + * @param timeout timeout in milliseconds * Return: returns 0 if successful else error code on failure */ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, struct devrequest *req, int length, - void *buffer) + void *buffer, int timeout) { int ret; int start_cycle; @@ -1027,7 +1035,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
giveback_first_trb(udev, ep_index, start_cycle, start_trb);
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, timeout); if (!event) goto abort; field = le32_to_cpu(event->trans_event.flags); @@ -1052,7 +1060,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, if (GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len)) == COMP_SHORT_TX) { /* Short data stage, clear up additional status stage event */ - event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, XHCI_SYS_TIMEOUT); if (!event) goto abort; BUG_ON(TRB_TO_SLOT_ID(field) != slot_id); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index a33a2460f74d..1c4afdf385f7 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -450,7 +450,7 @@ static int xhci_configure_endpoints(struct usb_device *udev, bool ctx_change) xhci_flush_cache((uintptr_t)in_ctx->bytes, in_ctx->size); xhci_queue_command(ctrl, in_ctx->dma, udev->slot_id, 0, ctx_change ? TRB_EVAL_CONTEXT : TRB_CONFIG_EP); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return -ETIMEDOUT;
@@ -649,7 +649,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr)
xhci_queue_command(ctrl, virt_dev->in_ctx->dma, slot_id, 0, TRB_ADDR_DEV); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return -ETIMEDOUT;
@@ -727,7 +727,7 @@ static int _xhci_alloc_device(struct usb_device *udev) }
xhci_queue_command(ctrl, 0, 0, 0, TRB_ENABLE_SLOT); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT); if (!event) return -ETIMEDOUT;
@@ -1125,7 +1125,7 @@ static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe, * (at most) one TD. A TD (comprised of sg list entries) can * take several service intervals to transmit. */ - return xhci_bulk_tx(udev, pipe, length, buffer); + return xhci_bulk_tx(udev, pipe, length, buffer, XHCI_INT_TIMEOUT); }
/** @@ -1135,17 +1135,18 @@ static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe, * @param pipe contains the DIR_IN or OUT , devnum * @param buffer buffer to be read/written based on the request * @param length length of the buffer + * @param timeout timeout in milliseconds * Return: returns 0 if successful else -1 on failure */ static int _xhci_submit_bulk_msg(struct usb_device *udev, unsigned long pipe, - void *buffer, int length) + void *buffer, int length, int timeout) { if (usb_pipetype(pipe) != PIPE_BULK) { printf("non-bulk pipe (type=%lu)", usb_pipetype(pipe)); return -EINVAL; }
- return xhci_bulk_tx(udev, pipe, length, buffer); + return xhci_bulk_tx(udev, pipe, length, buffer, timeout); }
/** @@ -1157,11 +1158,13 @@ static int _xhci_submit_bulk_msg(struct usb_device *udev, unsigned long pipe, * @param length length of the buffer * @param setup Request type * @param root_portnr Root port number that this device is on + * @param timeout timeout in milliseconds * Return: returns 0 if successful else -1 on failure */ static int _xhci_submit_control_msg(struct usb_device *udev, unsigned long pipe, void *buffer, int length, - struct devrequest *setup, int root_portnr) + struct devrequest *setup, int root_portnr, + int timeout) { struct xhci_ctrl *ctrl = xhci_get_ctrl(udev); int ret = 0; @@ -1187,7 +1190,7 @@ static int _xhci_submit_control_msg(struct usb_device *udev, unsigned long pipe, } }
- return xhci_ctrl_tx(udev, pipe, setup, length, buffer); + return xhci_ctrl_tx(udev, pipe, setup, length, buffer, timeout); }
static int xhci_lowlevel_init(struct xhci_ctrl *ctrl) @@ -1290,7 +1293,7 @@ static int xhci_submit_control_msg(struct udevice *dev, struct usb_device *udev, hop = hop->parent; */ return _xhci_submit_control_msg(udev, pipe, buffer, length, setup, - root_portnr); + root_portnr, timeout); }
static int xhci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev, @@ -1298,7 +1301,7 @@ static int xhci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev, int timeout) { debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev); - return _xhci_submit_bulk_msg(udev, pipe, buffer, length); + return _xhci_submit_bulk_msg(udev, pipe, buffer, length, timeout); }
static int xhci_submit_int_msg(struct udevice *dev, struct usb_device *udev, diff --git a/include/usb/xhci.h b/include/usb/xhci.h index 04d16a256bbd..02660803fdcf 100644 --- a/include/usb/xhci.h +++ b/include/usb/xhci.h @@ -27,7 +27,11 @@ #define MAX_EP_CTX_NUM 31 #define XHCI_ALIGNMENT 64 /* Generic timeout for XHCI events */ -#define XHCI_TIMEOUT 5000 +#define XHCI_TIMEOUT 1000 +/* Timeout for interrupt messages */ +#define XHCI_INT_TIMEOUT 1000 +/* Timeout for system commands */ +#define XHCI_SYS_TIMEOUT 200 /* Max number of USB devices for any host controller - limit in section 6.1 */ #define MAX_HC_SLOTS 256 /* Section 5.3.3 - MaxPorts */ @@ -1258,11 +1262,13 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, void xhci_queue_command(struct xhci_ctrl *ctrl, dma_addr_t addr, u32 slot_id, u32 ep_index, trb_type cmd); void xhci_acknowledge_event(struct xhci_ctrl *ctrl); -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected); +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, + int timeout); int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, - int length, void *buffer); + int length, void *buffer, int timeout); int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, - struct devrequest *req, int length, void *buffer); + struct devrequest *req, int length, void *buffer, + int timeout); int xhci_check_maxpacket(struct usb_device *udev); void xhci_flush_cache(uintptr_t addr, u32 type_len); void xhci_inval_cache(uintptr_t addr, u32 type_len);
On 10/29/23 08:36, Hector Martin wrote:
Now that the USB core passes through timeout info to the host controller, actually hook it up.
Signed-off-by: Hector Martin marcan@marcan.st
Except for the negative timeout value question, which needs to be sorted out:
Reviewed-by: Marek Vasut marex@denx.de
On 10/29/23 08:36, Hector Martin wrote:
A long time ago, the USB code was interrupt-driven and used top-level timeout handling. This has long been obsolete, and that code is just broken dead cruft. HC drivers instead hardcode timeouts today.
We need to be able to specify timeouts explicitly to handle cases like USB hard disks spinning up, without having ridiculously long timeouts across the board (which would cause endless waiting when things go wrong anywhere else). So, it's time to rip out the old broken nonsense and actually pass through timeouts to USB host controller drivers, so they can be implemented properly.
I like this.
[...]
On Sun, Oct 29, 2023 at 3:36 AM Hector Martin marcan@marcan.st wrote:
A long time ago, the USB code was interrupt-driven and used top-level timeout handling. This has long been obsolete, and that code is just broken dead cruft. HC drivers instead hardcode timeouts today.
We need to be able to specify timeouts explicitly to handle cases like USB hard disks spinning up, without having ridiculously long timeouts across the board (which would cause endless waiting when things go wrong anywhere else). So, it's time to rip out the old broken nonsense and actually pass through timeouts to USB host controller drivers, so they can be implemented properly.
This series adds the necessary top-level scaffolding for control/bulk timeouts, and implements them in xHCI. I didn't bother with interrupt transfers, since I figure those probably never need long timeouts anyway.
The platform I deal with only has xHCI, so I'll leave implementing this for EHCI/OHCI to someone else if anyone cares :)
This series needs to be applied after [1], since the xHCI changes depend on changes made there.
[1] https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f6316e@marcan...
Signed-off-by: Hector Martin marcan@marcan.st
Hector Martin (2): usb: Pass through timeout to drivers usb: xhci: Hook up timeouts
common/usb.c | 21 ++------------------- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 ++++-- drivers/usb/host/usb-uclass.c | 9 +++++---- drivers/usb/host/xhci-ring.c | 32 ++++++++++++++++++++------------ drivers/usb/host/xhci.c | 28 ++++++++++++++++------------ include/usb.h | 10 ++++++---- include/usb/xhci.h | 14 ++++++++++---- 10 files changed, 72 insertions(+), 63 deletions(-)
base-commit: 3d5d748e4d66b98109669c05d0c473fe67795801 change-id: 20231029-usb-fixes-5-ca87bbedb40c
Series LGTM.
Reviewed-by: Neal Gompa neal@gompa.dev
participants (3)
-
Hector Martin -
Marek Vasut -
Neal Gompa