Re: [PATCH v2 02/11] usb: xhci: Unlock for command abort polling

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 09 Oct 2026 15:27:25 +0000, sashiko-bot@kernel.org wrote:
> 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?

Does it not? :)
Still an improvement over the status quo.

> Could this use a non-atomic polling macro like readl_poll_timeout()
> with an appropriate usleep_range to yield the CPU instead?

Maybe, but other callers still haven't been fixed to stop being atomic.

My plan was to omit xhci_handshake() in this case and instead increase
the timeout for subsequent completion wait to 5 seconds, but I realized
that this slows down abort processing on HCs that fail to generate the
event we are wating for.

Mathias, do you know what was the buggy HW mentioned in 3425aa03f484d?
Was it really a bug in HW and not SW, maybe it was Intel HW that you
still have access to? None of my HCs exhibit such behavior.

If it's important, I could split the completion wait into stages, poll
USBSTS in between and stop waiting if CRR stays clear for 2 seconds.

Still, I consider all of that outside the scope of *this* patch.

Regards,
Michal