Re: [PATCH v2 2/2] PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unbalanced `disable_irq()` on error paths permanently masks the INTx IRQ if the root port reset is retried.

Pre-existing issues:
- [Critical] Retrying `.reset_root_port()` after a failure causes clock and PHY reference underflows and a synchronous external abort.
- [High] The threaded `sys` IRQ handler can race with `.reset_root_port()` and access unclocked APB registers, causing a synchronous external abort.
--

commit cc5a74b643533e890391de6a7a4d14fe8bd7c3e8
Author: Shawn Lin <shawn.lin@rock-chips.com>

PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

This patch masks the INTx IRQ during root port reset to prevent the chained
handler from reading APB registers while clocks are gated. The IRQ is then
re-enabled after clocks are restored.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index ecf0d7e043b36..aee5ca8a6ca84 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -925,6 +924,16 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	u32 val;
>  	int ret;
>  
> +	/*
> +	 * Devices may keep their INTx line asserted across the reset. Mask
> +	 * the INTx IRQ so that the chained handler does not touch the
> +	 * unclocked APB bus, which would raise a synchronous external abort.
> +	 * The IRQ is re-enabled once the clocks are restored, and is
> +	 * deliberately left masked on the error paths where the controller
> +	 * remains unclocked.
> +	 */
> +	disable_irq(rockchip->intx_irq);
> +

[Severity: High]
If rockchip_pcie_rc_reset_root_port() fails and returns early, it
intentionally skips calling enable_irq() to keep the IRQ masked while the
controller is unclocked.

If the reset is later retried (e.g., via AER recovery or sysfs), will the
second call to disable_irq() increment the IRQ descriptor's nesting depth
counter to 2?

If a subsequent reset attempt succeeds and calls enable_irq() exactly once,
could this regression leave the INTx IRQ permanently masked with a depth of 1,
even though the clocks have been successfully restored?

>  	dw_pcie_stop_link(pci);
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);

[Severity: Critical]
This is a pre-existing issue, but retrying the reset after a failure appears
to be problematic here.

If a previous call to rockchip_pcie_rc_reset_root_port() failed after turning
off the APB clocks and returning early, the clocks and PHY remain
deinitialized.

On a retry, dw_pcie_stop_link() is called immediately while the APB clocks are
still gated. Since dw_pcie_stop_link() accesses the unclocked APB registers,
will this cause a fatal synchronous external abort?

Additionally, could the subsequent calls to clk_bulk_disable_unprepare() and
rockchip_pcie_phy_deinit() cause clock and PHY reference count underflows on
a retry?

[Severity: High]
This isn't a bug introduced by this patch, but does the threaded sys IRQ
handler have a similar clock gating race condition?

The threaded handler rockchip_pcie_rc_sys_irq_thread() reads the APB register
PCIE_CLIENT_INTR_STATUS_MISC without synchronizing against clock gating.

If dw_pcie_stop_link() triggers a link-down event, it could fire the sys IRQ
just before clk_bulk_disable_unprepare() disables the APB clocks. Can this
cause the concurrently running sys IRQ handler to access unclocked APB
registers and trigger a fatal synchronous external abort?

>  	rockchip_pcie_phy_deinit(rockchip);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1788484709-139123-1-git-send-email-shawn.lin@rock-chips.com?part=2