Re: [PATCH v2 14/16] PCI: dwc: spear13xx: 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] Uninitialized variable usage leading to PCI register corruption.

Pre-existing issues:
- [High] Race condition in shared IRQ registration causing NULL pointer dereference.
--

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

PCI: dwc: spear13xx: Use cached PCIe capability offset

Inside .init we must call dw_pcie_get_pcie_cap() to obtain the offset,
because the core has not yet cached it. The hardware is already enabled
by the driver's own initialization before this point.

> diff --git a/drivers/pci/controller/dwc/pcie-spear13xx.c b/drivers/pci/controller/dwc/pcie-spear13xx.c
> index 01794a9d3ad29..6e4c11b497d45 100644
> --- a/drivers/pci/controller/dwc/pcie-spear13xx.c
> +++ b/drivers/pci/controller/dwc/pcie-spear13xx.c
> @@ -122,7 +122,7 @@ static int spear13xx_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pci);
> -	u32 exp_cap_off = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 exp_cap_off = dw_pcie_get_pcie_cap(pci);
>  	u32 val;
>  
>  	spear13xx_pcie->app_base = pci->dbi_base + 0x2000;

[Severity: High]
This is a pre-existing issue, but is it possible for a shared interrupt
to fire before app_base is initialized here?

In spear13xx_add_pcie_port(), devm_request_irq() requests the shared IRQ
before dw_pcie_host_init() is called. If the shared IRQ fires early,
spear13xx_pcie_irq_handler() will execute and unconditionally read from
the uninitialized app_base:

drivers/pci/controller/dwc/pcie-spear13xx.c:spear13xx_pcie_irq_handler() {
    ...
    struct pcie_app_reg __iomem *app_reg = spear13xx_pcie->app_base;
    ...
    status = readl(&app_reg->int_sts);
    ...
}

Because app_base is still NULL, would this result in a read from physical
address 0x18 and lead to a kernel crash?


Additionally, an issue was found in another commit in this series:

commit 1d7762e21cba69c340ccc430485c442e06488028
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: qcom: Use cached PCIe capability offset

dw_pcie_host_init() caches the offset after .init, so .post_init callbacks
and later functions (.link_up, icc_opp_update) can use pci->pcie_cap
directly. For .init itself, we must call dw_pcie_get_pcie_cap() inside
qcom_pcie_host_init() to obtain the offset (hardware is already enabled).

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 11fc604898926..80783353d5398 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -360,7 +360,6 @@ static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
>  
>  static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  	u32 val;
>  
>  	dw_pcie_dbi_ro_wr_en(pci);
> @@ -370,9 +369,9 @@ static void qcom_pcie_set_slot_nccs(struct dw_pcie *pci)
>  	 * notifications for the Hot-Plug commands. So set the NCCS field to
>  	 * avoid waiting for the completions.
>  	 */
> -	val = readl(pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	val = readl(pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);
>  	val |= PCI_EXP_SLTCAP_NCCS;
> -	writel(val, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +	writel(val, pci->dbi_base + pci->pcie_cap + PCI_EXP_SLTCAP);

[Severity: High]
Since qcom_pcie_set_slot_nccs() is called during qcom_pcie_host_init()
via the pcie->cfg->ops->post_init(pcie) callback, is pci->pcie_cap
guaranteed to be initialized here?

The DWC core in dw_pcie_host_init() appears to cache pci->pcie_cap only
after the pp->ops->init callback returns. If pci->pcie_cap is still 0
when this executes, won't this write to offset 0x14 (PCI_BASE_ADDRESS_1)
and corrupt the configuration space instead of configuring the intended
slot capability?

[ ... ]
> @@ -935,7 +934,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 val;
>  
>  	/* Force PHY out of lowest power state */

[Severity: High]
Similarly, will pci->pcie_cap be 0 when qcom_pcie_post_init_2_3_3() is
called from the post_init callback during qcom_pcie_host_init()?

If so, this would result in a corrupted offset here as well.

[ ... ]
> @@ -1257,7 +1256,7 @@ static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>  static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u8 offset = pci->pcie_cap;
>  	u32 val;
>  	int i;

[Severity: High]
Does this suffer from the same uninitialized pci->pcie_cap issue as
qcom_pcie_post_init_2_3_3() when called from the post_init callback?

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