* [PATCH v2 01/11] usb: xhci: return an error if the host is not halted
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:24 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 02/11] usb: xhci: Unlock for command abort polling Mathias Nyman
` (9 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Henry Tseng, Mathias Nyman
From: Henry Tseng <henrytseng@qnap.com>
xhci_reset() returns 0 when the host is not halted, without ever writing
CMD_RESET. Every other path that fails to reset the host returns an
error, so callers that check the return value are told the reset
succeeded on the one path where it did not happen.
Return -EBUSY when the reset is aborted because the host is not halted.
Signed-off-by: Henry Tseng <henrytseng@qnap.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a9e47e178c28..2af6a7b91e55 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -195,7 +195,7 @@ int xhci_reset(struct xhci_hcd *xhci, u64 timeout_us)
if ((state & STS_HALT) == 0) {
xhci_warn(xhci, "Host controller not halted, aborting reset.\n");
- return 0;
+ return -EBUSY;
}
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Reset the HC");
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 02/11] usb: xhci: Unlock for command abort polling
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
2026-10-09 15:16 ` [PATCH v2 01/11] usb: xhci: return an error if the host is not halted Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:27 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 03/11] usb: xhci: fix typos in comments Mathias Nyman
` (8 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, Pedro Fonseca, Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
xhci_abort_cmd_ring() requests abort, waits for the CRR bit to clear,
drops xhci->lock and waits for the Command Ring Stopped event.
The CRR wait timeout is 5 seconds as suggested by xHCI 4.6.1.2, which
means that if the xHC fails to complete the operation at all, we poll
with the lock held and IRQs disabled for several seconds. If any other
CPU tries to acquire the lock, it will spin likewise. IRQs get delays,
drivers log errors, tasks freeze, it's a mess.
So drop the lock earlier, before waiting for the CRR bit. It should be
safe - the sole caller sets cmd_ring_state to CMD_RING_STATE_ABORTED
before calling us, which will prevent others from ringing the command
doorbell and interfering with the abort. Queuing new commands during
this time poses no danger, and if the command we try to abort actually
completes concurrently, existing code already needs to deal with this.
And in my testing it does - it's trivial to trigger this on ASM1042,
where Address Device can't be aborted, but it completes as soon as the
offending device is unplugged, including during abort attempt.
Note that the lock still covers reinit_completion(), so it won't race
with complete() being called by the event handler. And works are not
reentrant, so another timeout can't expire while the lock is dropped.
We will configure timeout anew when restarting the ring.
One other difference is that now we also drop the lock if abort fails.
This too should be harmless. Commands queued during this time will be
released like any other pending commands. If the aborted command does
complete before we regain the lock, it's a waste, but not regression.
Reported-by: Pedro Fonseca <pedro@fonseca.com.pt>
Link: https://lore.kernel.org/linux-usb/16f65081-5a3c-4c30-9811-9017796a3373@fonseca.com.pt/
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ec278a9f9540..82dd93c2afdd 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -494,7 +494,7 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
struct xhci_segment *new_seg = xhci->cmd_ring->deq_seg;
union xhci_trb *new_deq = xhci->cmd_ring->dequeue;
u64 crcr;
- int ret;
+ int ret, completed;
xhci_dbg(xhci, "Abort command ring\n");
@@ -521,25 +521,27 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
* In the future we should distinguish between -ENODEV and -ETIMEDOUT
* and try to recover a -ETIMEDOUT with a host controller reset.
*/
+ spin_unlock_irqrestore(&xhci->lock, flags);
ret = xhci_handshake(&xhci->op_regs->cmd_ring,
CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
- if (ret < 0) {
- xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
- xhci_halt(xhci);
- xhci_hc_died(xhci);
- return ret;
- }
/*
* Writing the CMD_RING_ABORT bit should cause a cmd completion event,
* however on some host hw the CMD_RING_RUNNING bit is correctly cleared
* but the completion event in never sent. Wait 2 secs (arbitrary
* number) to handle those cases after negation of CMD_RING_RUNNING.
*/
- spin_unlock_irqrestore(&xhci->lock, flags);
- ret = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
+ if (ret >= 0)
+ completed = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
msecs_to_jiffies(2000));
spin_lock_irqsave(&xhci->lock, flags);
- if (!ret) {
+
+ if (ret < 0) {
+ xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
+ xhci_halt(xhci);
+ xhci_hc_died(xhci);
+ return ret;
+ }
+ if (!completed) {
xhci_dbg(xhci, "No stop event for abort, ring start fail?\n");
xhci_cleanup_command_queue(xhci);
} else {
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 02/11] usb: xhci: Unlock for command abort polling
2026-10-09 15:16 ` [PATCH v2 02/11] usb: xhci: Unlock for command abort polling Mathias Nyman
@ 2026-10-09 15:27 ` sashiko-bot
0 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-10-09 15:27 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: xhci: 5-second udelay() busy-wait in sleepable command timeout path
--
commit 8ece4a222044715c34357d2e1240af7af9feef1a
Author: Michal Pecio <michal.pecio@gmail.com>
usb: xhci: Unlock for command abort polling
This commit drops the xhci->lock earlier during xhci_abort_cmd_ring() to
prevent stalling other CPUs while polling for the command ring to stop.
It subsequently waits for the command ring stop completion event.
Link: https://lore.kernel.org/linux-usb/16f65081-5a3c-4c30-9811-9017796a3373@fonseca.com.pt/
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[ ... ]
> @@ -521,25 +521,27 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
> * In the future we should distinguish between -ENODEV and -ETIMEDOUT
> * and try to recover a -ETIMEDOUT with a host controller reset.
> */
> + spin_unlock_irqrestore(&xhci->lock, flags);
> ret = xhci_handshake(&xhci->op_regs->cmd_ring,
> CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
[Severity: Medium]
The patch drops xhci->lock here to avoid stalling other CPUs, but
xhci_handshake() unconditionally uses readl_poll_timeout_atomic():
int xhci_handshake(void __iomem *ptr, u32 mask, u32 done, u64 timeout_us)
{
...
ret = readl_poll_timeout_atomic(ptr, result,
(result & mask) == done ||
result == U32_MAX,
1, timeout_us);
Since the context allows sleeping (the lock is dropped, and we call
wait_for_completion_timeout() right after), does this code burn 100% of a
CPU core in a busy-wait for up to 5 seconds if the hardware fails?
Could this use a non-atomic polling macro like readl_poll_timeout() with an
appropriate usleep_range to yield the CPU instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=2
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 03/11] usb: xhci: fix typos in comments
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
2026-10-09 15:16 ` [PATCH v2 01/11] usb: xhci: return an error if the host is not halted Mathias Nyman
2026-10-09 15:16 ` [PATCH v2 02/11] usb: xhci: Unlock for command abort polling Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:18 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 04/11] xhci: check device notification type before forwarding wake event Mathias Nyman
` (7 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Zain Aboobacker, Mathias Nyman
From: Zain Aboobacker <zainaboobacker33@gmail.com>
Fix various spelling mistakes in comments found by codespell.
Assisted-by: Claude:claude-opus-5-5 codespell
Signed-off-by: Zain Aboobacker <zainaboobacker33@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 14 +++++++-------
drivers/usb/host/xhci.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 82dd93c2afdd..cbce9f8f07fa 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -243,7 +243,7 @@ static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32
* fixed in the 0.96 specification errata, but we have to assume that all 0.95
* xHCI hardware can't handle the chain bit being cleared on a link TRB.
*
- * On 0.95 and some 0.96 HCs the chain bit is set once at segment initalization
+ * On 0.95 and some 0.96 HCs the chain bit is set once at segment initialization
* and never changed here. On all others, modify it as requested by the caller.
*/
if (!xhci_link_chain_quirk(xhci, ring->type)) {
@@ -663,7 +663,7 @@ struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
* Get the hw dequeue pointer xHC stopped on, either directly from the
* endpoint context, or if streams are in use from the stream context.
* The returned hw_dequeue contains the lowest four bits with cycle state
- * and possbile stream context type.
+ * and possible stream context type.
*/
static u64 xhci_get_hw_deq(struct xhci_hcd *xhci, struct xhci_virt_device *vdev,
unsigned int ep_index, unsigned int stream_id)
@@ -991,7 +991,7 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
int err;
/*
- * Avoid resetting endpoint if link is inactive or device disonnected.
+ * Avoid resetting endpoint if link is inactive or device disconnected.
* Can cause host hang.
* Device will be reset to recover an inactive link, so don't do anything
*/
@@ -1386,7 +1386,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
* held for the URBs to finish during device disconnect, blocking host remove.
*
* Call with xhci->lock held.
- * lock is relased and re-acquired while giving back urb.
+ * lock is released and re-acquired while giving back urb.
*/
void xhci_hc_died(struct xhci_hcd *xhci)
{
@@ -1971,7 +1971,7 @@ static void handle_device_notification(struct xhci_hcd *xhci,
}
/*
- * Quirk hanlder for errata seen on Cavium ThunderX2 processor XHCI
+ * Quirk handler for errata seen on Cavium ThunderX2 processor XHCI
* Controller.
* As per ThunderX2errata-129 USB 2 device may come up as USB 1
* If a connection to a USB 1 device is followed by another connection
@@ -3089,7 +3089,7 @@ static void xhci_clear_interrupt_pending(struct xhci_interrupter *ir)
/*
* Handle all OS-owned events on an interrupter event ring. It may drop
- * and reaquire xhci->lock between event processing.
+ * and reacquire xhci->lock between event processing.
*/
static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
bool skip_events)
@@ -3803,7 +3803,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
/*
* If next available TRB is the Link TRB in the ring segment then
* enqueue a No Op TRB, this can prevent the Setup and Data Stage
- * TRB to be breaked by the Link TRB.
+ * TRB to be broken by the Link TRB.
*/
if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue + 1)) {
field = TRB_TYPE(TRB_TR_NOOP) | ep_ring->cycle_state;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2af6a7b91e55..9564dde8bb34 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -406,7 +406,7 @@ static void compliance_mode_recovery(struct timer_list *t)
* The quirk creates a timer that polls every 2 seconds the link state of
* each host controller's port and recovers it by issuing a Warm reset
* if Compliance mode is detected, otherwise the port will become "dead" (no
- * device connections or disconnections will be detected anymore). Becasue no
+ * device connections or disconnections will be detected anymore). Because no
* status event is generated when entering compliance mode (per xhci spec),
* this quirk is needed on systems that have the failing hardware installed.
*/
@@ -796,7 +796,7 @@ static void xhci_save_registers(struct xhci_hcd *xhci)
xhci->s3.config_reg = readl(&xhci->op_regs->config_reg);
/* save both primary and all secondary interrupters */
- /* fixme, shold we lock to prevent race with remove secondary interrupter? */
+ /* fixme, should we lock to prevent race with remove secondary interrupter? */
for (i = 0; i < xhci->max_interrupters; i++) {
ir = xhci->interrupters[i];
if (!ir)
@@ -2046,7 +2046,7 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
/*
* Configuration and alternate setting changes must be done in
- * process context, not interrupt context (or so documenation
+ * process context, not interrupt context (or so documentation
* for usb_set_interface() and usb_set_configuration() claim).
*/
if (xhci_endpoint_init(xhci, virt_dev, udev, ep, GFP_NOIO) < 0) {
@@ -3302,7 +3302,7 @@ static void xhci_endpoint_disable(struct usb_hcd *hcd,
* state. For software that wishes to reset the data toggle or sequence number
* of an endpoint that isn't in the halted state this function will issue a
* configure endpoint command with the Drop and Add bits set for the target
- * endpoint. Refer to the additional note in xhci spcification section 4.6.8.
+ * endpoint. Refer to the additional note in xhci specification section 4.6.8.
*
* vdev may be lost due to xHC restore error and re-initialization during S3/S4
* resume. A new vdev will be allocated later by xhci_discover_or_reset_device()
@@ -4296,7 +4296,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
pm_runtime_get_noresume(hcd->self.controller);
/* Is this a LS or FS device under a HS hub? */
- /* Hub or peripherial? */
+ /* Hub or peripheral? */
return 1;
disable_slot:
@@ -4512,7 +4512,7 @@ static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
/*
* Transfer the port index into real index in the HW port status
- * registers. Caculate offset between the port's PORTSC register
+ * registers. Calculate offset between the port's PORTSC register
* and port status base. Divide the number of per port register
* to get the real index. The raw port number bases 1.
*/
@@ -5394,7 +5394,7 @@ static void xhci_hcd_init_usb3_data(struct xhci_hcd *xhci, struct usb_hcd *hcd)
/*
* Early xHCI 1.1 spec did not mention USB 3.1 capable hosts
* should return 0x31 for sbrn, or that the minor revision
- * is a two digit BCD containig minor and sub-minor numbers.
+ * is a two digit BCD containing minor and sub-minor numbers.
* This was later clarified in xHCI 1.2.
*
* Some USB 3.1 capable hosts therefore have sbrn 0x30, and
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 04/11] xhci: check device notification type before forwarding wake event
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (2 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 03/11] usb: xhci: fix typos in comments Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:23 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 05/11] xhci: dbc: lock the minor IDR on registration failure Mathias Nyman
` (6 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Hongyu Xie, Mathias Nyman
From: Hongyu Xie <xiehongyu1@kylinos.cn>
The xHCI driver programs the Device Notification Control register to
only enable the Function Wake device notification (N1), so any Device
Notification Event TRB received is expected to be a function wake
notification. handle_device_notification() does however not check the
Notification Type field of the event (xHCI 1.2 section 6.4.2.7, DW0
bits 7:4), and forwards every device notification event as a function
wake.
A host controller that delivers an unexpected notification type (e.g.
due to broken firmware or emulation) would trigger a spurious wake
notification on the parent hub.
Parse the notification type and drop events other than Function Wake
with a warning, mirroring the slot ID validation in the same function.
DEV_NOTE_FWAKE is the DNCTRL register bit for notification type 1
(N1), while the event TRB carries the notification type value itself,
so add a separate DEV_NOTE_TYPE_FWAKE constant for the comparison.
[mn:] reduce warning to a debug message
Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 9 +++++++++
drivers/usb/host/xhci.h | 5 +++++
2 files changed, 14 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index cbce9f8f07fa..7f480db2983e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1954,6 +1954,7 @@ static void handle_device_notification(struct xhci_hcd *xhci,
union xhci_trb *event)
{
u32 slot_id;
+ u32 type;
struct usb_device *udev;
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->generic.field[3]));
@@ -1963,6 +1964,14 @@ static void handle_device_notification(struct xhci_hcd *xhci,
return;
}
+ /* xHCI 1.2 6.4.2.7: Notification Type is DW0 bits 7:4 */
+ type = TRB_TO_DEV_NOTE_TYPE(le32_to_cpu(event->generic.field[0]));
+ if (type != DEV_NOTE_TYPE_FWAKE) {
+ xhci_dbg(xhci, "Unsupported device notification type %u for slot ID %u\n",
+ type, slot_id);
+ return;
+ }
+
xhci_dbg(xhci, "Device Wake Notification event for slot ID %u\n",
slot_id);
udev = xhci->devs[slot_id]->udev;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c7bfa7f028d3..ec4bfeb4887c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -187,6 +187,8 @@ struct xhci_op_regs {
* SW does need to pay attention to function wake notifications.
*/
#define DEV_NOTE_FWAKE BIT(1)
+/* Notification Type value carried by a Device Notification Event TRB (6.4.2.7) */
+#define DEV_NOTE_TYPE_FWAKE 1
/* CRCR - Command Ring Control Register - cmd_ring bitmasks */
/* bit 0 - Cycle bit indicates the ownership of the command ring */
@@ -996,6 +998,9 @@ enum xhci_ep_reset_type {
#define TRB_TO_PACKET_TYPE(p) ((p) & 0x1f)
#define TRB_TO_ROOTHUB_PORT(p) (((p) & (0xff << 24)) >> 24)
+/* Device Notification Event TRB fields, 6.4.2.7 */
+#define TRB_TO_DEV_NOTE_TYPE(p) (((p) & (0xf << 4)) >> 4)
+
enum xhci_setup_dev {
SETUP_CONTEXT_ONLY,
SETUP_CONTEXT_ADDRESS,
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 04/11] xhci: check device notification type before forwarding wake event
2026-10-09 15:16 ` [PATCH v2 04/11] xhci: check device notification type before forwarding wake event Mathias Nyman
@ 2026-10-09 15:23 ` sashiko-bot
0 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-10-09 15:23 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb
> From: Hongyu Xie <xiehongyu1@kylinos.cn>
>
> The xHCI driver programs the Device Notification Control register to
> only enable the Function Wake device notification (N1), so any Device
> Notification Event TRB received is expected to be a function wake
> notification. handle_device_notification() does however not check the
> Notification Type field of the event (xHCI 1.2 section 6.4.2.7, DW0
> bits 7:4), and forwards every device notification event as a function
> wake.
>
> A host controller that delivers an unexpected notification type (e.g.
> due to broken firmware or emulation) would trigger a spurious wake
> notification on the parent hub.
>
> Parse the notification type and drop events other than Function Wake
> with a warning, mirroring the slot ID validation in the same function.
>
> DEV_NOTE_FWAKE is the DNCTRL register bit for notification type 1
> (N1), while the event TRB carries the notification type value itself,
> so add a separate DEV_NOTE_TYPE_FWAKE constant for the comparison.
>
> [mn:] reduce warning to a debug message
>
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=4
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 05/11] xhci: dbc: lock the minor IDR on registration failure
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (3 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 04/11] xhci: check device notification type before forwarding wake event Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:25 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 06/11] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun Mathias Nyman
` (5 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Sang-Hoon Choi, Changyul Lee, Mathias Nyman
From: Sang-Hoon Choi <csh0052@gmail.com>
dbc_tty_minors is protected by dbc_tty_minors_lock when entries are
allocated and during normal device removal. The registration error path
removes an entry without taking that lock. Different DbC instances have
separate event work items, so this removal can race with an IDR update
for another instance.
Take the same mutex around the error-path removal.
Fixes: e1ec140f273e ("xhci: dbgtty: use IDR to support several dbc instances.")
Reported-by: Changyul Lee <lcy8047@gmail.com>
Assisted-by: LLM
Signed-off-by: Sang-Hoon Choi <csh0052@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-dbgtty.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index 3d51e8d82659..2249cc16800c 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
err_free_fifo:
kfifo_free(&port->port.xmit_fifo);
err_exit_port:
+ mutex_lock(&dbc_tty_minors_lock);
idr_remove(&dbc_tty_minors, port->minor);
+ mutex_unlock(&dbc_tty_minors_lock);
err_idr:
xhci_dbc_tty_exit_port(port);
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 06/11] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (4 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 05/11] xhci: dbc: lock the minor IDR on registration failure Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:25 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 07/11] usb: xhci: Don't set the skip flag on non-isoc endpoints Mathias Nyman
` (4 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
In this case we know that the xHC has released ownership of all missed
TDs, we only don't know which were missed and which were queued later.
URBs are queued atomically, so we can safely give back all TDs of the
currently executing URB. Unlike the previous policy, this does actually
ensure that the class driver will learn about the error and won't see
all of its URBs still in progress when all TDs are missed on xHCI 1.0.
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7f480db2983e..8b915a1d5b25 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2649,6 +2649,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
unsigned int slot_id;
int ep_index;
struct xhci_td *td = NULL;
+ struct urb *missed_urb = NULL;
dma_addr_t ep_trb_dma;
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
@@ -2868,26 +2869,31 @@ static int handle_tx_event(struct xhci_hcd *xhci,
return 0;
/*
- * TD was missed, skip it. Core already initialized frame->status
- * to -EXDEV and frame->actual_length to 0, nothing more to do.
+ * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
+ * pointer is zero again. All missed TDs can be given back, but we
+ * don't know which were missed and which were queued after the xrun
+ * occurred. We can safely give back the first pending URB.
*/
- xhci_dequeue_td(xhci, td, ep_ring, 0);
+ if (ring_xrun_event) {
+ if (!missed_urb)
+ missed_urb = td->urb;
- if (!list_empty(&ep_ring->td_list)) {
- if (ring_xrun_event) {
- /*
- * If we are here, we are on xHCI 1.0 host with no
- * idea how many TDs were missed or where the xrun
- * occurred. New TDs may have been added after the
- * xrun, so skip only one TD to be safe.
- */
- xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
+ if (td->urb != missed_urb) {
+ xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
slot_id, ep_index);
return 0;
}
- continue;
}
+ /*
+ * TD was missed, skip it. Core already initialized frame->status
+ * to -EXDEV and frame->actual_length to 0, nothing more to do.
+ */
+ xhci_dequeue_td(xhci, td, ep_ring, 0);
+
+ if (!list_empty(&ep_ring->td_list))
+ continue;
+
xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
slot_id, ep_index);
ep->skip = false;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 07/11] usb: xhci: Don't set the skip flag on non-isoc endpoints
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (5 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 06/11] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:23 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 08/11] usb: xhci: Shorten the TD skipping loop Mathias Nyman
` (3 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
These events are unique to isochronous endpoints, ignore them otherwise.
Update debug messages to reflect new policies. We could also log invalid
events as errors, but it seems nobody has ever had problems with that,
so don't bother.
This allows dropping the isoc check when skipping TDs.
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8b915a1d5b25..2dd11732bb87 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2778,16 +2778,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
* Set skip flag of the ep_ring; Complete the missed tds as
* short transfer when process the ep_ring next time.
*/
- ep->skip = true;
+ if (ep_ring->type == TYPE_ISOC)
+ ep->skip = true;
xhci_dbg(xhci,
- "Miss service interval error for slot %u ep %u, set skip flag%s\n",
- slot_id, ep_index, ep_trb_dma ? ", skip now" : "");
+ "Missed Service Error for slot %u ep %u, skip %d, try now %d\n",
+ slot_id, ep_index, ep->skip, !!ep_trb_dma);
break;
case COMP_NO_PING_RESPONSE_ERROR:
- ep->skip = true;
+ if (ep_ring->type == TYPE_ISOC)
+ ep->skip = true;
xhci_dbg(xhci,
- "No Ping response error for slot %u ep %u, Skip one Isoc TD\n",
- slot_id, ep_index);
+ "No Ping response error for slot %u ep %u, skip %d\n",
+ slot_id, ep_index, ep->skip);
return 0;
case COMP_INCOMPATIBLE_DEVICE_ERROR:
@@ -2863,7 +2865,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
/* Is this TRB not part of the currently executing TD? */
if (!trb_in_td(td, ep_trb_dma)) {
- if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+ if (ep->skip) {
/* this event is unlikely to match any TD, don't skip them all */
if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 08/11] usb: xhci: Shorten the TD skipping loop
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (6 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 07/11] usb: xhci: Don't set the skip flag on non-isoc endpoints Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:22 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 09/11] usb: xhci: Rework and improve the TD matching and skipping logic Mathias Nyman
` (2 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
Half of this loop is code which only executes once to deal with cases
where no TD matches the event and then it returns. This code needs not
to be in any kind of loop, so get it out.
Optimize conditionals remaining in the loop body.
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 68 +++++++++++++++++-------------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2dd11732bb87..7597ef8105c6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2862,10 +2862,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
td = list_first_entry(&ep_ring->td_list, struct xhci_td,
td_list);
- /* Is this TRB not part of the currently executing TD? */
- if (!trb_in_td(td, ep_trb_dma)) {
+ if (ep->skip) {
- if (ep->skip) {
+ if (!trb_in_td(td, ep_trb_dma)) {
/* this event is unlikely to match any TD, don't skip them all */
if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
return 0;
@@ -2903,38 +2902,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
goto check_endpoint_halted;
}
- /* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
- if (ring_xrun_event)
- return 0;
-
- /*
- * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
- * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
- * pointer still at the previous TRB of the current TD. The previous TRB
- * maybe a Link TD or the last TRB of the previous TD. The command
- * completion handle will take care the rest.
- */
- if (trb_comp_code == COMP_STOPPED ||
- trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
- return 0;
- }
-
- /*
- * Some hosts give a spurious success event after a short
- * transfer or error on last TRB. Ignore it.
- */
- if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
- xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
- &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
- ep_ring->old_trb_comp_code = 0;
- return 0;
- }
-
- /* HC is busted, give up! */
- goto debug_finding_td;
- }
-
- if (ep->skip) {
xhci_dbg(xhci,
"Found td. Clear skip flag for slot %u ep %u.\n",
slot_id, ep_index);
@@ -2949,6 +2916,37 @@ static int handle_tx_event(struct xhci_hcd *xhci,
*/
} while (ep->skip);
+ /* Handle events not referencing the current TD */
+ if (!trb_in_td(td, ep_trb_dma)) {
+ /* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
+ if (ring_xrun_event)
+ return 0;
+
+ /*
+ * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
+ * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
+ * pointer still at the previous TRB of the current TD. The previous TRB
+ * maybe a Link TD or the last TRB of the previous TD. The command
+ * completion handle will take care the rest.
+ */
+ if (trb_comp_code == COMP_STOPPED || trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
+ return 0;
+
+ /*
+ * Some hosts give a spurious success event after a short
+ * transfer or error on last TRB. Ignore it.
+ */
+ if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
+ xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
+ &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
+ ep_ring->old_trb_comp_code = 0;
+ return 0;
+ }
+
+ /* HC is busted, give up! */
+ goto debug_finding_td;
+ }
+
ep_ring->old_trb_comp_code = trb_comp_code;
/* Get out if a TD was queued at enqueue after the xrun occurred */
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 09/11] usb: xhci: Rework and improve the TD matching and skipping logic
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (7 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 08/11] usb: xhci: Shorten the TD skipping loop Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:34 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 10/11] usb: xhci: Fix bounce buffer overflow Mathias Nyman
2026-10-09 15:16 ` [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister Mathias Nyman
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
Matching events with TDs and giving back missed TDs is carried out
by a complicated loop. Replace it with a simpler linear logic:
0. Having verified that 'td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.
Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match
or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future"
TD so we can log this in the Scary Error Message to aid debugging.
While altering the error message, drop a pointless goto.
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 137 ++++++++++++++++-------------------
1 file changed, 61 insertions(+), 76 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7597ef8105c6..243b1fd2b2f6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -127,11 +127,16 @@ static bool link_trb_toggles_cycle(union xhci_trb *trb)
return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
}
-static bool last_td_in_urb(struct xhci_td *td)
+static int num_tds_not_done(struct urb *urb)
{
- struct urb_priv *urb_priv = td->urb->hcpriv;
+ struct urb_priv *urb_priv = urb->hcpriv;
- return urb_priv->num_tds_done == urb_priv->num_tds;
+ return urb_priv->num_tds - urb_priv->num_tds_done;
+}
+
+static bool last_td_in_urb(struct xhci_td *td)
+{
+ return !num_tds_not_done(td->urb);
}
static bool unhandled_event_trb(struct xhci_ring *ring)
@@ -2624,14 +2629,19 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
}
}
-static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, dma_addr_t dma)
+static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, int *missed_tds, dma_addr_t dma)
{
struct xhci_td *td;
- if (dma)
+ if (dma) {
list_for_each_entry(td, &ep_ring->td_list, td_list)
if (trb_in_td(td, dma))
return td;
+ else
+ (*missed_tds)++;
+ }
+
+ *missed_tds = 0;
return NULL;
}
@@ -2648,8 +2658,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
struct xhci_ring *ep_ring;
unsigned int slot_id;
int ep_index;
- struct xhci_td *td = NULL;
- struct urb *missed_urb = NULL;
+ struct xhci_td *td;
+ int missed_tds = 0;
dma_addr_t ep_trb_dma;
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
@@ -2832,13 +2842,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
xhci_dequeue_td(xhci, td, ep_ring, td->status);
}
- /*
- * We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by
- * xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out.
- */
- if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !find_td_by_dma(ep_ring, ep_trb_dma))
- return 0;
-
if (list_empty(&ep_ring->td_list)) {
/*
* Don't print wanings if ring is empty due to a stopped endpoint generating an
@@ -2858,66 +2861,50 @@ static int handle_tx_event(struct xhci_hcd *xhci,
goto check_endpoint_halted;
}
- do {
- td = list_first_entry(&ep_ring->td_list, struct xhci_td,
- td_list);
-
- if (ep->skip) {
-
- if (!trb_in_td(td, ep_trb_dma)) {
- /* this event is unlikely to match any TD, don't skip them all */
- if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
- return 0;
-
- /*
- * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
- * pointer is zero again. All missed TDs can be given back, but we
- * don't know which were missed and which were queued after the xrun
- * occurred. We can safely give back the first pending URB.
- */
- if (ring_xrun_event) {
- if (!missed_urb)
- missed_urb = td->urb;
-
- if (td->urb != missed_urb) {
- xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
- slot_id, ep_index);
- return 0;
- }
- }
-
- /*
- * TD was missed, skip it. Core already initialized frame->status
- * to -EXDEV and frame->actual_length to 0, nothing more to do.
- */
- xhci_dequeue_td(xhci, td, ep_ring, 0);
+ td = find_td_by_dma(ep_ring, &missed_tds, ep_trb_dma);
- if (!list_empty(&ep_ring->td_list))
- continue;
+ if (ep->skip) {
+ if (!td) {
+ /*
+ * xHCI 1.0 allowed MSE events to have zero TRB pointers. Some old chips
+ * also generate bogus non-zero pointers. We know, don't bother warning.
+ * Missed TDs will be given back by the next event with a valid pointer.
+ */
+ if (trb_comp_code == COMP_MISSED_SERVICE_ERROR &&
+ xhci->hci_version <= 0x100)
+ return 0;
+ /*
+ * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB pointer
+ * is zero again. All missed TDs can be given back, but we don't know which
+ * were missed and which were queued after the xrun occurred. We can safely
+ * give back the first pending URB to let the class driver know.
+ */
+ if (ring_xrun_event)
+ missed_tds = num_tds_not_done(list_first_entry(&ep_ring->td_list,
+ struct xhci_td, td_list)->urb);
+ /* In other cases missed_tds is zero */
+ }
- xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
- slot_id, ep_index);
- ep->skip = false;
- td = NULL;
- goto check_endpoint_halted;
- }
+ /*
+ * Give back missed TDs. Core already initialized their frame->status to -EXDEV
+ * and frame->actual_length to 0, nothing more to do.
+ */
+ for (int i = 0; i < missed_tds; i++)
+ xhci_dequeue_td(xhci,
+ list_first_entry(&ep_ring->td_list, struct xhci_td, td_list),
+ ep_ring, 0);
- xhci_dbg(xhci,
- "Found td. Clear skip flag for slot %u ep %u.\n",
- slot_id, ep_index);
+ /* the list may become empty on ring_xrun_event */
+ if (td || list_empty(&ep_ring->td_list))
ep->skip = false;
- }
- /*
- * If ep->skip is set, it means there are missed tds on the
- * endpoint ring need to take care of.
- * Process them as short transfer until reach the td pointed by
- * the event.
- */
- } while (ep->skip);
+ xhci_dbg(xhci, "Skipped %d TDs on slot %u ep %u comp_code %u, TD found %d, skip flag %d\n",
+ missed_tds, slot_id, ep_index, trb_comp_code, !!td, ep->skip);
+ missed_tds = 0;
+ }
/* Handle events not referencing the current TD */
- if (!trb_in_td(td, ep_trb_dma)) {
+ if (!td || missed_tds) {
/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
if (ring_xrun_event)
return 0;
@@ -2944,7 +2931,13 @@ static int handle_tx_event(struct xhci_hcd *xhci,
}
/* HC is busted, give up! */
- goto debug_finding_td;
+ td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list);
+ xhci_err(xhci, "Event dma %pad for ep %d comp_code %u not part of TD at %016llx - %016llx, missed %d\n",
+ &ep_trb_dma, ep_index, trb_comp_code,
+ (u64)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
+ (u64)xhci_trb_virt_to_dma(td->end_seg, td->end_trb),
+ missed_tds);
+ return -ESHUTDOWN;
}
ep_ring->old_trb_comp_code = trb_comp_code;
@@ -2982,14 +2975,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
return 0;
-debug_finding_td:
- xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
- &ep_trb_dma, ep_index, trb_comp_code,
- (unsigned long long)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
- (unsigned long long)xhci_trb_virt_to_dma(td->end_seg, td->end_trb));
-
- return -ESHUTDOWN;
-
err_out:
xhci_err(xhci, "@%016llx %08x %08x %08x %08x\n",
(unsigned long long) xhci_trb_virt_to_dma(
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 09/11] usb: xhci: Rework and improve the TD matching and skipping logic
2026-10-09 15:16 ` [PATCH v2 09/11] usb: xhci: Rework and improve the TD matching and skipping logic Mathias Nyman
@ 2026-10-09 15:34 ` sashiko-bot
0 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-10-09 15:34 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: handle_tx_event() halts >1.0 hosts on bogus MSE pointers
--
commit cc3554d902e447131e844c7826abc9b1b4f75f82
Author: Michal Pecio <michal.pecio@gmail.com>
usb: xhci: Rework and improve the TD matching and skipping logic
Matching events with TDs and giving back missed TDs is carried out
by a complicated loop. Replace it with a simpler linear logic:
0. Having verified that 'td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7597ef8105c65..243b1fd2b2f68 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[ ... ]
> @@ -2858,66 +2861,50 @@ static int handle_tx_event(struct xhci_hcd *xhci,
[ ... ]
> + td = find_td_by_dma(ep_ring, &missed_tds, ep_trb_dma);
>
> - xhci_dbg(xhci,
> - "Found td. Clear skip flag for slot %u ep %u.\n",
> - slot_id, ep_index);
> + if (ep->skip) {
> + if (!td) {
> + /*
> + * xHCI 1.0 allowed MSE events to have zero TRB pointers. Some old chips
> + * also generate bogus non-zero pointers. We know, don't bother warning.
> + * Missed TDs will be given back by the next event with a valid pointer.
> + */
> + if (trb_comp_code == COMP_MISSED_SERVICE_ERROR &&
> + xhci->hci_version <= 0x100)
> + return 0;
[Severity: High]
Does this code inadvertently halt hosts > 1.0 on bogus MSE pointers?
By restricting the leniency for bogus Missed Service Error (MSE) pointers
strictly to xHCI 1.0, controllers > 1.0 that generate a bogus pointer for MSE
will fall through and hit the "HC is busted" block below.
[ ... ]
> /* Handle events not referencing the current TD */
> - if (!trb_in_td(td, ep_trb_dma)) {
> + if (!td || missed_tds) {
> /* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
> if (ring_xrun_event)
> return 0;
[ ... ]
> /* HC is busted, give up! */
> - goto debug_finding_td;
> + td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list);
> + xhci_err(xhci, "Event dma %pad for ep %d comp_code %u not part of TD at %016llx - %016llx, missed %d\n",
> + &ep_trb_dma, ep_index, trb_comp_code,
> + (u64)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
> + (u64)xhci_trb_virt_to_dma(td->end_seg, td->end_trb),
> + missed_tds);
> + return -ESHUTDOWN;
> }
[Severity: High]
Will falling through to this block cause the host controller to fatally shut
down (-ESHUTDOWN) when a bogus MSE pointer is received on >= 1.1 hardware?
Isochronous transfers frequently experience MSEs under load, and some USB
firmware is known to issue bogus TRB pointers. Before this patch, the driver
unconditionally and gracefully ignored MSE events with unrecognized pointers,
letting the next event sort it out.
The commit message mentions a goal to log a scary error message for future
TDs when the skip flag is clear, but does not justify intentionally panicking
>= 1.1 controllers for this quirk.
Could the commit message clarify if punishing >= 1.1 chips with a fatal host
shutdown for a non-fatal, recoverable event was intended?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=9
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 10/11] usb: xhci: Fix bounce buffer overflow
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (8 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 09/11] usb: xhci: Rework and improve the TD matching and skipping logic Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:28 ` sashiko-bot
2026-10-09 15:16 ` [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister Mathias Nyman
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Michal Pecio, co+fd80bc5967eb22c3, stable,
Mathias Nyman
From: Michal Pecio <michal.pecio@gmail.com>
High-speed devices with out of spec 1024 byte bulk endpoints exist and
are allowed by USB core, but xhci-hcd always sets packet size to 512.
The exact nature of these devices isn't documented, commit fb5ee84ea72c
("USB: Accept bulk endpoints with 1024-byte maxpacket") only states
that they "don't work with xHCI host controllers", whatever it means.
But somebody (or a malicious device) can try, and then the driver will
allocate a 512 byte bounce buffer for this endpoint and may write up to
1024 bytes into it if particular scatter-gather URBs are used, because
xhci_align_td() obtains packet size from the descriptor. Fix this.
As a side effect, TRBs will be aligned to the packet size chosen by the
driver on all endpoints of all speeds. Alignment serves the xHC, not
device, so this is fine. Only out of spec devices are affected anyway.
Reported-by: co+fd80bc5967eb22c3@bugs.sh
Link: https://lore.kernel.org/linux-usb/D4tcSGerkYkIV1DmaUo1t8TaR5qQElDLkidn@bugs.sh/
Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 243b1fd2b2f6..c23434001e9c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3546,15 +3546,13 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
- u32 *trb_buff_len, struct xhci_segment *seg)
+ u32 *trb_buff_len, struct xhci_segment *seg, u32 max_pkt)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned int unalign;
- unsigned int max_pkt;
u32 new_buff_len;
size_t len;
- max_pkt = xhci_usb_endpoint_maxp(urb->dev, urb->ep);
unalign = (enqd_len + *trb_buff_len) % max_pkt;
/* we got lucky, last normal TRB data on segment is packet aligned */
@@ -3699,9 +3697,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
if (enqd_len + trb_buff_len < full_len) {
field |= TRB_CHAIN;
if (trb_is_link(ring->enqueue + 1)) {
- if (xhci_align_td(xhci, urb, enqd_len,
- &trb_buff_len,
- ring->enq_seg)) {
+ if (xhci_align_td(xhci, urb, enqd_len, &trb_buff_len,
+ ring->enq_seg, ring->bounce_buf_len)) {
send_addr = ring->enq_seg->bounce_dma;
/* TD bounced at least, and last on this seg */
td->bounce_seg = ring->enq_seg;
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 10/11] usb: xhci: Fix bounce buffer overflow
2026-10-09 15:16 ` [PATCH v2 10/11] usb: xhci: Fix bounce buffer overflow Mathias Nyman
@ 2026-10-09 15:28 ` sashiko-bot
0 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-10-09 15:28 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb
> From: Michal Pecio <michal.pecio@gmail.com>
>
> High-speed devices with out of spec 1024 byte bulk endpoints exist and
> are allowed by USB core, but xhci-hcd always sets packet size to 512.
> The exact nature of these devices isn't documented, commit fb5ee84ea72c
> ("USB: Accept bulk endpoints with 1024-byte maxpacket") only states
> that they "don't work with xHCI host controllers", whatever it means.
>
> But somebody (or a malicious device) can try, and then the driver will
> allocate a 512 byte bounce buffer for this endpoint and may write up to
> 1024 bytes into it if particular scatter-gather URBs are used, because
> xhci_align_td() obtains packet size from the descriptor. Fix this.
>
> As a side effect, TRBs will be aligned to the packet size chosen by the
> driver on all endpoints of all speeds. Alignment serves the xHC, not
> device, so this is fine. Only out of spec devices are affected anyway.
>
> Reported-by: co+fd80bc5967eb22c3@bugs.sh
> Link: https://lore.kernel.org/linux-usb/D4tcSGerkYkIV1DmaUo1t8TaR5qQElDLkidn@bugs.sh/
> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=10
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister
2026-10-09 15:16 [PATCH v2 00/11] xhci features and fixes for usb-next Mathias Nyman
` (9 preceding siblings ...)
2026-10-09 15:16 ` [PATCH v2 10/11] usb: xhci: Fix bounce buffer overflow Mathias Nyman
@ 2026-10-09 15:16 ` Mathias Nyman
2026-10-09 15:30 ` sashiko-bot
10 siblings, 1 reply; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 15:16 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Mathias Nyman, Lianqin Hu, stable
Offloaded USB audio devices using the xhci-sideband API store a pointer to
the xhci virtual device (vdev) in the sideband structure when registering.
This pointer typically remains valid throughout the lifetime of the USB
device. If a configured offloaded device requires a reset, the USB core
usually unbinds or notifies the audio driver beforehand, ensuring that the
sideband is unregistered before the vdev is freed.
An exception occurs when the USB core resets a device to recover from a
failed resume, but a subsequent 'address device' request also fails. To
recover in this specific scenario, the xHCI driver disables and re-enables
the slot, which frees and re-allocates the vdev.
xhci_sideband_unregister() later dereferences the stale, previously freed
vdev pointer during disconnect, triggering a kernel oops:
Unable to handle kernel paging request at virtual address dead000000000122
Call trace:
xhci_get_ep_ctx+0x0/0x38
xhci_sideband_unregister+0x68/0xf0
uaudio_disconnect+0x70/0x144
usb_audio_disconnect+0x7c/0x268
usb_unbind_interface+0x13c/0x340
device_release_driver_internal+0x1c4/0x2bc
usb_disable_device+0x84/0x190
usb_disconnect+0xe8/0x338
hub_event+0xbd8/0x19ac
Fix this by preventing the reallocation of the vdev in this specific error
path if the device is registered for sideband use.
Just propagate the error directly to the USB core. It should either retry
enumeration or detect the disconnected device, and handle it accordingly.
Debugging this issue to the dangling vdev pointer, reporting it, testing,
and initial patch with different solution by Lianqin Hu
Reported-by: Lianqin Hu <hulianqin@vivo.com>
Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
Tested-by: Lianqin Hu <hulianqin@vivo.com>
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9564dde8bb34..30ac1bc4559b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4435,10 +4435,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
mutex_unlock(&xhci->mutex);
- ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
- if (!ret) {
- if (xhci_alloc_dev(hcd, udev) == 1)
- xhci_setup_addressable_virt_dev(xhci, udev);
+
+ if (!virt_dev->sideband) {
+ ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
+ if (!ret) {
+ if (xhci_alloc_dev(hcd, udev) == 1)
+ xhci_setup_addressable_virt_dev(xhci, udev);
+ }
}
kfree(command->completion);
kfree(command);
--
2.43.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister
2026-10-09 15:16 ` [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister Mathias Nyman
@ 2026-10-09 15:30 ` sashiko-bot
2026-10-09 22:47 ` Mathias Nyman
0 siblings, 1 reply; 24+ messages in thread
From: sashiko-bot @ 2026-10-09 15:30 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xhci: TOCTOU race and Use-After-Free in sideband registration
--
commit 7085c7c59881c747b14aeec07262203621aaa8b5
Author: Mathias Nyman <mathias.nyman@linux.intel.com>
xhci: Prevent invalid vdev dereference during sideband unregister
This commit prevents the reallocation of the xhci virtual device in the setup
device error path if a sideband is registered. It aims to fix a kernel oops
caused by dereferencing a stale vdev pointer during USB audio disconnect.
Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4435,10 +4435,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>
> mutex_unlock(&xhci->mutex);
> - ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> - if (!ret) {
> - if (xhci_alloc_dev(hcd, udev) == 1)
> - xhci_setup_addressable_virt_dev(xhci, udev);
> +
> + if (!virt_dev->sideband) {
> + ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
[Severity: High]
Can this lockless check introduce a Time-Of-Check to Time-Of-Use (TOCTOU)
race condition leading to a use-after-free?
If an asynchronous caller like snd_usb_rediscover_devices() calls
xhci_sideband_register() concurrently, it acquires xhci->lock, reads
xhci->devs[slot_id], and attaches the sideband.
If this lockless check evaluates to true just before the sideband is
attached, we proceed to xhci_disable_and_free_slot() without the lock:
xhci_setup_device()
if (!virt_dev->sideband) {
xhci_disable_and_free_slot()
xhci_free_virt_device()
xhci->devs[slot_id] = NULL;
kfree(dev);
Because xhci_free_virt_device() does not hold xhci->lock, it can free the
device immediately after xhci_sideband_register() attaches the sideband,
leaving the sideband with a dangling pointer to a freed vdev.
> + if (!ret) {
> + if (xhci_alloc_dev(hcd, udev) == 1)
> + xhci_setup_addressable_virt_dev(xhci, udev);
> + }
> }
> kfree(command->completion);
> kfree(command);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261009151624.618967-1-mathias.nyman@linux.intel.com?part=11
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister
2026-10-09 15:30 ` sashiko-bot
@ 2026-10-09 22:47 ` Mathias Nyman
0 siblings, 0 replies; 24+ messages in thread
From: Mathias Nyman @ 2026-10-09 22:47 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-usb
On 10/9/26 18:30, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] xhci: TOCTOU race and Use-After-Free in sideband registration
> --
>
> commit 7085c7c59881c747b14aeec07262203621aaa8b5
> Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> xhci: Prevent invalid vdev dereference during sideband unregister
>
> This commit prevents the reallocation of the xhci virtual device in the setup
> device error path if a sideband is registered. It aims to fix a kernel oops
> caused by dereferencing a stale vdev pointer during USB audio disconnect.
>
> Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4435,10 +4435,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>> dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>>
>> mutex_unlock(&xhci->mutex);
>> - ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
>> - if (!ret) {
>> - if (xhci_alloc_dev(hcd, udev) == 1)
>> - xhci_setup_addressable_virt_dev(xhci, udev);
>> +
>> + if (!virt_dev->sideband) {
>> + ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
>
> [Severity: High]
> Can this lockless check introduce a Time-Of-Check to Time-Of-Use (TOCTOU)
> race condition leading to a use-after-free?
>
> If an asynchronous caller like snd_usb_rediscover_devices() calls
> xhci_sideband_register() concurrently, it acquires xhci->lock, reads
> xhci->devs[slot_id], and attaches the sideband.
>
> If this lockless check evaluates to true just before the sideband is
> attached, we proceed to xhci_disable_and_free_slot() without the lock:
>
> xhci_setup_device()
> if (!virt_dev->sideband) {
> xhci_disable_and_free_slot()
> xhci_free_virt_device()
> xhci->devs[slot_id] = NULL;
> kfree(dev);
>
> Because xhci_free_virt_device() does not hold xhci->lock, it can free the
> device immediately after xhci_sideband_register() attaches the sideband,
> leaving the sideband with a dangling pointer to a freed vdev.
Not an issue, or extremely unlikely.
This xhci_setup_device() codepath is called during usb device (re-)enumeration.
In this case problematic case is after a failed usb port resume calling reset-resume.
This means that the suspended audio class interface driver would have to register
audio sideband before the audio class interface itself resumed, and do this
while the parent port is mid resume.
Mathias
^ permalink raw reply [flat|nested] 24+ messages in thread