Re: [PATCH 1/1] usb: xhci-plat: keep IRQ masked while HCD is inaccessible across system sleep

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: xhci-plat: xhci_plat_resume_common() unmasks IRQ on error paths
- [High] usb: xhci-plat: xhci_plat_suspend_common() skips wakeup and blocks shared IRQ
--

commit 07ff10dd51b8e25e7045f365929b6001912f4351
Author: Jiazi Liu <liujiazi@amazon.com>

usb: xhci-plat: keep IRQ masked while HCD is inaccessible across system sleep

This commit masks hcd->irq in xhci_plat_suspend_common() and unmasks it
in xhci_plat_resume_common() when the device is not configured as a wakeup
source, preventing an unhandled interrupt storm while HCD_FLAG_HW_ACCESSIBLE
is cleared.

> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 6fd595f81a306..07c95f2ff32e5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -478,13 +478,22 @@ static int xhci_plat_suspend_common(struct device *dev)
>  	ret = xhci_priv_suspend_quirk(hcd);
>  	if (ret)
>  		return ret;
> +
> +	/*
> +	 * Mask the IRQ while the HCD is not accessible. The PM core unmasks
> +	 * it before xhci_resume() runs, and a pending event on the halted
> +	 * xHC would cause an interrupt storm.
> +	 */
> +	if (!device_may_wakeup(dev))
> +		disable_irq(hcd->irq);

[Severity: High]
Does skipping disable_irq(hcd->irq) when device_may_wakeup(dev) is true
leave wakeup-enabled controllers vulnerable to the same unhandled IRQ
storm?

In drivers/usb/host/xhci.c, xhci_suspend() clears HCD_FLAG_HW_ACCESSIBLE
unconditionally regardless of do_wakeup:

    spin_lock_irq(&xhci->lock);
    clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
    if (xhci->shared_hcd)
        clear_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);

While HCD_FLAG_HW_ACCESSIBLE is cleared, usb_hcd_irq() in
drivers/usb/core/hcd.c returns IRQ_NONE:

    if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
        rc = IRQ_NONE;

Glue drivers such as dwc3_host_init() in drivers/usb/dwc3/host.c enable
wakeup by default on the xhci-plat device via device_init_wakeup(). During
hibernation_snapshot(), xhci_plat_freeze() calls xhci_plat_suspend_common().
Because device_may_wakeup(dev) is true on wakeup-enabled devices even
during freeze/thaw, disable_irq(hcd->irq) is skipped here and the
dpm_resume_noirq() -> dpm_resume_early() interrupt storm can still occur
before xhci_plat_thaw() runs xhci_resume(). The same window exists during
normal system resume if a pending or wakeup event asserts the halted xHC's
level-triggered interrupt line before xhci_plat_resume() runs.

Additionally, xhci_plat_probe() registers hcd->irq with IRQF_SHARED:

    ret = usb_add_hcd(hcd, irq, IRQF_SHARED);

Calling disable_irq(hcd->irq) masks the entire shared interrupt line at
the irqchip across suspend and resume for any other device sharing that
line. Would it be better to silence the xHC interrupter in hardware (for
example, clearing CMD_EIE or IMAN_IE in xhci_suspend() while
HCD_FLAG_HW_ACCESSIBLE is cleared) instead of calling disable_irq()?

> +
>  	/*
>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
>  	 * to do wakeup during suspend.
>  	 */
>  	ret = xhci_suspend(xhci, device_may_wakeup(dev));
>  	if (ret)
> -		return ret;
> +		goto err_enable_irq;
>  
>  	if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) {
>  		clk_disable_unprepare(xhci->clk);
> @@ -492,6 +501,12 @@ static int xhci_plat_suspend_common(struct device *dev)
>  	}
>  
>  	return 0;
> +
> +err_enable_irq:
> +	if (!device_may_wakeup(dev))
> +		enable_irq(hcd->irq);
> +
> +	return ret;
>  }
>  
>  static int xhci_plat_suspend(struct device *dev)
> @@ -523,12 +538,12 @@ static int xhci_plat_resume_common(struct device *dev, bool power_lost)
>  	if (!device_may_wakeup(dev) && (xhci->quirks & XHCI_SUSPEND_RESUME_CLKS)) {
>  		ret = clk_prepare_enable(xhci->clk);
>  		if (ret)
> -			return ret;
> +			goto enable_irq;
>  
>  		ret = clk_prepare_enable(xhci->reg_clk);
>  		if (ret) {
>  			clk_disable_unprepare(xhci->clk);
> -			return ret;
> +			goto enable_irq;
>  		}
>  	}
>  
> @@ -544,6 +559,10 @@ static int xhci_plat_resume_common(struct device *dev, bool power_lost)
>  	if (ret)
>  		goto disable_clks;
>  
> +	/* The HCD is accessible again, pair with disable_irq() in suspend. */
> +	if (!device_may_wakeup(dev))
> +		enable_irq(hcd->irq);
> +
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> @@ -555,6 +574,9 @@ static int xhci_plat_resume_common(struct device *dev, bool power_lost)
>  		clk_disable_unprepare(xhci->clk);
>  		clk_disable_unprepare(xhci->reg_clk);
>  	}
> +enable_irq:
> +	if (!device_may_wakeup(dev))
> +		enable_irq(hcd->irq);
>  
>  	return ret;
>  }

[Severity: High]
What happens if xhci_plat_resume_common() fails and jumps to disable_clks
or enable_irq?

If clk_prepare_enable() or xhci_priv_resume_quirk() fails before
xhci_resume() is called, HCD_FLAG_HW_ACCESSIBLE is still 0 and the xHC is
still halted with any pending event unacknowledged. Calling
enable_irq(hcd->irq) on this error path immediately triggers the unhandled
IRQ storm in usb_hcd_irq().

If xhci_resume() fails after setting HCD_FLAG_HW_ACCESSIBLE in
drivers/usb/host/xhci.c:

    set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
    if (xhci->shared_hcd)
        set_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);

or if xhci_priv_post_resume_quirk() fails, HCD_FLAG_HW_ACCESSIBLE remains
set because xhci_resume() does not clear it on error. Jumping to
disable_clks disables xhci->clk and xhci->reg_clk (when
XHCI_SUSPEND_RESUME_CLKS is set) and then falls through to enable_irq. Any
pending xHC interrupt or shared interrupt on hcd->irq will then pass the
HCD_HW_ACCESSIBLE(hcd) check in usb_hcd_irq() and call xhci_irq() ->
readl(&xhci->op_regs->status) while the controller clocks are disabled.

Should hcd->irq remain disabled (and HCD_FLAG_HW_ACCESSIBLE be cleared) if
xhci_plat_resume_common() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020111035.25365-1-jiazi.liu1984@gmail.com?part=1