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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch replaces `dw_pcie_find_capability()` with a cached `pci->pcie_cap`, but hardware clocks are not enabled during `dw_pcie_ep_init()` on `qcom-ep`, causing an unclocked MMIO access or caching an invalid 0 offset that later corrupts standard PCI configuration space.

Pre-existing issues:
- [High] Concurrent execution of global and PERST threaded IRQs causes fatal unclocked MMIO accesses and data races.
--

commit 4d4786c3c255dd8f971ef41e85f15f1fa9ead9d4
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: qcom-ep: Use cached PCIe capability offset

dw_pcie_ep_init() calls ep->ops->pre_init() (enables hardware) and then
dw_pcie_get_pcie_cap() to cache the offset. Therefore, the IRQ handlers
run after the cache is populated, so they can safely use pci->pcie_cap
directly.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 56184e6ca6e69..0471f96d3c781 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 caching work safely on qcom-ep? The commit message states that
dw_pcie_ep_init() calls pre_init() to enable hardware clocks. However, for
qcom-ep, clocks are only enabled later via qcom_pcie_enable_resources() in
the PERST deassert IRQ path.

This means clocks are disabled during dw_pcie_ep_init(), which will either
cause a synchronous external abort when reading the capability, or result in
pci->pcie_cap being populated with a cached offset of 0.

If pci->pcie_cap is 0, won't this readw() fetch from pci->dbi_base + 0x12,
which is part of BAR0, instead of the link status register? This could fetch
incorrect values and break interconnect bandwidth scaling.

[ ... ]

> @@ -492,13 +490,12 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  	dw_pcie_dbi_ro_wr_en(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);
>  	FIELD_MODIFY(PCI_EXP_LNKCAP_L0SEL, &val, 0x6);
>  	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, val);

[Severity: High]
Similarly, if pci->pcie_cap is cached as 0 because clocks were disabled
during initialization, won't this overwrite the base PCI configuration header?

Using an offset of 0 means offset + PCI_EXP_LNKCAP evaluates to 0x0c, which
would erroneously modify the cache line size, latency timer, header type,
and BIST registers instead of the link capabilities register.

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
PERST IRQ thread and the global IRQ thread that can cause a panic?

The qcom_pcie_ep_perst_irq_thread() and qcom_pcie_ep_global_irq_thread()
execute concurrently without mutual exclusion. If the host asserts PERST
while the global IRQ thread is processing an event, the PERST thread will
disable clocks:

qcom_pcie_ep_perst_irq_thread()
    qcom_pcie_perst_assert()
        qcom_pcie_disable_resources()

If the global IRQ thread is simultaneously performing MMIO access:

qcom_pcie_ep_global_irq_thread()
    status = readl_relaxed(pcie_ep->parf + PARF_INT_ALL_STATUS);

Could this MMIO access while clocks are asynchronously disabled cause an
immediate synchronous external abort?

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