Re: [PATCH v3 10/16] PCI: dwc: qcom-ep: Use cached PCIe capability offset

[sashiko-bot@kernel.org]

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

New issues:
- [High] `pci->pcie_cap` is used uninitialized (as 0) in `qcom_pcie_ep_icc_update()` if the global IRQ triggers before `perst_irq` completes.

Pre-existing issues:
- [High] Lost interrupt events due to mutually exclusive `if-else if` chain dropping concurrently pending status bits.
- [High] Concurrent execution of global IRQ and PERST IRQ leads to unclocked MMIO accesses and system crash.
--

--- Patch [10]: [PATCH v3 10/16] PCI: dwc: qcom-ep: Use cached PCIe capability offset ---
commit 66f17f8dee9121b185f046f6247725965823798a
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: qcom-ep: Use cached PCIe capability offset
    
dw_pcie_ep_init() no longer caches the offset automatically. For qcom-ep,
hardware (clocks, PHY) is enabled only after PERST# deassert. Therefore,
we must call dw_pcie_get_pcie_cap() in qcom_pcie_perst_deassert() after
the resources are enabled, before any DBI access. This ensures the cached
offset is valid for subsequent uses in interrupt handlers and ICC updates.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 56184e6ca6e69..16b0891f95713 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -307,15 +307,13 @@ static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
>  {
>  	struct dw_pcie *pci = &pcie_ep->pci;
> -	u32 offset, status;
> -	int speed, width;
> -	int ret;
> +	int speed, width, ret;
> +	u32 status;
>  
>  	if (!pcie_ep->icc_mem)
>  		return;
>  
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +	status = readw(pci->dbi_base + pci->pcie_cap + PCI_EXP_LNKSTA);

[Severity: High]
Does this read from an uninitialized offset if the global IRQ triggers
before the PERST IRQ completes?

qcom_pcie_ep_enable_irq_resources() requests the global IRQ without
IRQ_NOAUTOEN, making it active immediately. If the PCIe link is already
established (e.g., by the bootloader or firmware), the IRQ can trigger
before pci_epc_start() enables the PERST IRQ and qcom_pcie_perst_deassert()
caches the offset.

If this happens, pci->pcie_cap would be 0, causing the function to read
from DBI offset 0x12 instead of PCI_EXP_LNKSTA. This would compute arbitrary
speed and width values to pass to icc_set_bw(), which could misconfigure the
system interconnect bandwidth.

>  
>  	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>  	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);

[ ... ]

> @@ -491,14 +489,15 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
>  
> +	dw_pcie_get_pcie_cap(pci);
> +
>  	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> -	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	offset = pci->pcie_cap;
>  	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);

[ ... ]


drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_ep_global_irq_thread() {
...
	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);

	if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) {
...
	} else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
...
}

[Severity: High]
This is a pre-existing issue, but does this if-else chain lose concurrent
interrupt events?

qcom_pcie_ep_global_irq_thread() clears all pending events simultaneously in
hardware. If the host sends multiple events in close succession (e.g., both
Bus Master Enable and Link Up are set in PARF_INT_ALL_STATUS), the else-if
logic will process the first matching event and discard the others.

Since the discarded events are already cleared from the register, they would
be permanently lost, potentially leaving the endpoint in an incorrect logical
state.


drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_ep_global_irq_thread() {
...
	u32 status = readl_relaxed(pcie_ep->parf + PARF_INT_ALL_STATUS);
...
}

drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_disable_resources() {
...
	clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
...
}

[Severity: High]
This is a pre-existing issue, but can the global IRQ thread race with
PERST deassertion/assertion and cause an unclocked MMIO access?

Both the global and PERST IRQs are requested as threaded IRQs without mutual
exclusion. If the host toggles PERST, qcom_pcie_ep_perst_irq_thread() calls
qcom_pcie_perst_assert(), which unconditionally disables the PCIe clocks via
clk_bulk_disable_unprepare().

If a simultaneous link state change triggers qcom_pcie_ep_global_irq_thread()
and it reads from PARF_INT_ALL_STATUS after the clocks are disabled, this
would cause a synchronous external abort (SError) and crash the system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720150619.24733-1-18255117159@163.com?part=10

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic