Re: [PATCH 2/2] PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe error path and driver unload fail to clean up INTx irq domain and chained handler, leading to a potential use-after-free and memory leak.
- [High] Disabling clocks during root port reset without masking IRQs can cause a synchronous external abort.
- [High] Double PHY de-initialization permanently bricks the PCI controller on subsequent resets.
- [High] DBI registers are accessed before the controller is set to RC mode during root port reset.
--

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

PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

This patch extracts the programming of Root Complex registers out of
the .init() callback into a separate rockchip_pcie_host_hw_init() function.
It then calls this new function from .reset_root_port() instead, ensuring
the INTx IRQ domain and chained handler are set up only once at probe time.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 50824c8b1d83e..ff9239403cf22 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -440,13 +459,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the INTx
irq domain and chained handler to be leaked on probe failure or driver
unload?

If rockchip_pcie_host_init() or a later step in probe fails, devres will free
the rockchip structure. However, it appears the irq domain is created in
rockchip_pcie_init_irq_domain() without devm management and is never removed.
Additionally, the chained handler is assigned with the rockchip structure as
data but is never cleared on the error path.

If the legacy IRQ fires after probe failure, could the chained handler
(rockchip_pcie_intx_handler) dereference the freed rockchip pointer and cause a
use-after-free panic? Also, since the driver lacks a .deinit callback, does
this permanently leak the domain on module removal?

[ ... ]
> @@ -920,11 +933,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	if (ret)
>  		goto deinit_phy;

[Severity: High]
This is a pre-existing issue, but does this jump lead to a double PHY
de-initialization that permanently bricks the PCI controller?

If the reset fails (e.g., dw_pcie_start_link() fails later in this function),
the code jumps to the deinit_phy label and calls rockchip_pcie_phy_deinit().
However, rockchip_pcie_rc_reset_root_port() already unconditionally calls
rockchip_pcie_phy_deinit() near the top of the function.

If the PCI core attempts another reset later, the unconditional deinit at the
top will execute again. Does this erroneously drop the PHY's power_count and
init_count from 0 to -1, causing subsequent rockchip_pcie_phy_init() calls to
silently fail to power on the PHY?

[Severity: High]
This is another pre-existing issue, but can disabling clocks during root port
reset cause a synchronous external abort (SError) if the chained IRQ is not
masked?

Earlier in rockchip_pcie_rc_reset_root_port(), the controller clocks are
disabled via clk_bulk_disable_unprepare() before resetting the hardware. If an
unhandled interrupt fires concurrently while the clocks are disabled, the CPU
will attempt to execute rockchip_pcie_intx_handler(), which directly reads
PCIE_CLIENT_INT_STATUS over the unclocked APB bus.

Should the chained IRQ be explicitly masked (or disable_irq() used) before
disabling the clocks to prevent accessing unclocked APB MMIO space?

> -	ret = pp->ops->init(pp);
> -	if (ret) {
> -		dev_err(dev, "Host init failed: %d\n", ret);
> -		goto deinit_clk;
> -	}
> +	rockchip_pcie_host_hw_init(pp);

[Severity: High]
This is a pre-existing issue, but does rockchip_pcie_host_hw_init() access DBI
registers before the controller is explicitly set to Root Complex mode?

During reset, rockchip_pcie_rc_reset_root_port() applies a hardware reset via
reset_control_assert(). It then calls rockchip_pcie_host_hw_init(), which writes
to DBI registers (like PCI_BASE_ADDRESS_0) and accesses PCIe capabilities.

Since the controller is instructed to act as a Root Complex
(PCIE_CLIENT_MODE_RC) only after rockchip_pcie_host_hw_init() returns, does
accessing the PCI configuration space prematurely violate hardware state
constraints and risk an SError on the AXI bus?

>  
>  	/* LTSSM enable control mode */
>  	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);

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