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

[sashiko-bot@kernel.org]

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