Re: [PATCH] PCI: qcom: Block accesses to downstream devices on link down

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The ECAM blocker enable bit (`ECAM_BLOCKER_EN`) is not explicitly cleared during host initialization, causing downstream devices to remain permanently blocked after a link recovery on platforms lacking a full hardware reset.
- [High] V2-specific ECAM blocker registers are unconditionally programmed on all IP revisions, causing system crashes or state corruption on older hardware.
- [High] Unsynchronized read-modify-write of the `PARF_SYS_CTRL` register between the link-down IRQ thread and the concurrent AER recovery workqueue leads to hardware state corruption.
--

--- Patch [1]: [PATCH] PCI: qcom: Block accesses to downstream devices on link down ---
commit 17695f1fbc53b125cd81eddfebc8a383d0abdbd8
Author: Qiang Yu <[email protected]>

PCI: qcom: Block accesses to downstream devices on link down

This patch introduces an ECAM blocker to drop accesses to the PCIe address
space when a link goes down. The blocker range is programmed during host
initialization, and the blocker is enabled in the link-down IRQ handler.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b58a607b713f5..7005d989f6ff2 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -1382,6 +1400,37 @@ static void qcom_pcie_configure_ports(struct qcom_pcie *pcie)
>  		dw_pcie_program_t_power_on(pcie->pci, port->l1ss_t_power_on);
>  }
>  
> +static void qcom_pcie_init_ecam_blocker(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	/* ECAM base must match the DBI base address */
> +	writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE);
> +	writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE_HI);
> +
> +	writel(0, pcie->parf + PARF_BLOCK_SLV_AXI_WR_BASE);
> +	writel(0, pcie->parf + PARF_BLOCK_SLV_AXI_WR_BASE_HI);

[Severity: High]
Does this code unconditionally write to V2-specific PARF offsets on all IP
revisions? On older IPs (like v1.0.0 and v2.3.2), these registers might not
exist, as their PARF_DBI_BASE_ADDR is at 0x168 instead. Could writing to these
unmapped or undocumented registers trigger a Synchronous External Abort
and cause a kernel panic during boot on older hardware?

[Severity: High]
Is the ECAM_BLOCKER_EN bit explicitly cleared during this host initialization?
If the controller lacks a full hardware reset on some platforms (like v2.3.2)
where it only cycles clocks and regulators, the bit might remain set from a
prior link-down event.

Could subsequent read-modify-write operations on PARF_SYS_CTRL, such as:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_post_init_2_3_2() {
    ...
	val = readl(pcie->parf + PARF_SYS_CTRL);
	val &= ~MAC_PHY_POWERDOWN_IN_P2_D_MUX_EN;
	writel(val, pcie->parf + PARF_SYS_CTRL);
    ...
}

silently preserve this bit and permanently block downstream devices after a
link recovery?

> +	writel(U32_MAX, pcie->parf + PARF_BLOCK_SLV_AXI_WR_LIMIT);
> +	writel(U32_MAX, pcie->parf + PARF_BLOCK_SLV_AXI_WR_LIMIT_HI);
> +
> +	writel(0, pcie->parf + PARF_BLOCK_SLV_AXI_RD_BASE);
> +	writel(0, pcie->parf + PARF_BLOCK_SLV_AXI_RD_BASE_HI);
> +	writel(U32_MAX, pcie->parf + PARF_BLOCK_SLV_AXI_RD_LIMIT);
> +	writel(U32_MAX, pcie->parf + PARF_BLOCK_SLV_AXI_RD_LIMIT_HI);
> +}
> +
> +static void qcom_pcie_enable_ecam_blocker(struct qcom_pcie *pcie)
> +{
> +	u32 sys_ctrl;
> +
> +	sys_ctrl = readl(pcie->parf + PARF_SYS_CTRL);
> +	sys_ctrl |= ECAM_BLOCKER_EN;
> +	writel(sys_ctrl, pcie->parf + PARF_SYS_CTRL);

[Severity: High]
Can this unprotected read-modify-write of PARF_SYS_CTRL race with a
concurrent AER recovery workqueue?

If a fatal AER error triggers a Root Port reset, the AER workqueue might
execute initialization functions like qcom_pcie_post_init_2_3_2() locklessly:

drivers/pci/controller/dwc/pcie-qcom.c:qcom_pcie_post_init_2_3_2() {
    ...
	val = readl(pcie->parf + PARF_SYS_CTRL);
	val &= ~MAC_PHY_POWERDOWN_IN_P2_D_MUX_EN;
	writel(val, pcie->parf + PARF_SYS_CTRL);
    ...
}

Since qcom_pcie_enable_ecam_blocker() is called from the link-down IRQ thread,
could this race silently overwrite vital initialization bits and corrupt
the hardware link state?

> +
> +	/* Flush the write so the blocker is enabled before this function returns */
> +	readl(pcie->parf + PARF_SYS_CTRL);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1