Re: [PATCH v3 14/16] PCI: dwc: spear13xx: Use cached PCIe capability offset

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

Pre-existing issues:
- [High] Mandatory API contract violation: ignored return value of `phy_init()`.
- [High] Unconditional `IRQ_HANDLED` in a shared IRQ handler.
- [High] Shared IRQ is requested before the hardware base address is initialized, leading to a potential NULL pointer dereference.
--

--- Patch [14]: [PATCH v3 14/16] PCI: dwc: spear13xx: Use cached PCIe capability offset ---
commit c41d49bf9e94ac238049768122148b7abb564665
Author: Hans Zhang <[email protected]>

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 isn't a bug introduced by this patch, but I noticed a potential
NULL pointer dereference involving app_base here.

In spear13xx_add_pcie_port(), devm_request_irq() is called with IRQF_SHARED
before dw_pcie_host_init() sets spear13xx_pcie->app_base:

spear13xx_add_pcie_port() {
    ...
    ret = devm_request_irq(dev, pp->irq, spear13xx_pcie_irq_handler,
                           IRQF_SHARED | IRQF_NO_THREAD,
                           "spear1340-pcie", spear13xx_pcie);
    ...
    ret = dw_pcie_host_init(pp);
    ...
}

If another device on the shared IRQ line triggers an interrupt before
spear13xx_pcie_host_init() is called, spear13xx_pcie_irq_handler() will run
and dereference the still-NULL app_base:

spear13xx_pcie_irq_handler() {
    ...
    struct pcie_app_reg __iomem *app_reg = spear13xx_pcie->app_base;
    ...
    status = readl(&app_reg->int_sts);
    ...
}

Could the IRQ request be moved until after the base address is initialized?

[ ... ]

I also noticed a few other pre-existing issues in this file.

[Severity: High]
This is a pre-existing issue, but the integer return value of
phy_init() is completely ignored in spear13xx_pcie_probe():

spear13xx_pcie_probe() {
    ...
    phy_init(spear13xx_pcie->phy);

    spear13xx_pcie->clk = devm_clk_get(dev, NULL);
    ...
}

If phy_init() fails, the driver blindly proceeds to configure clocks and
initialize the PCIe host controller. Accessing an uninitialized or
unpowered PHY can lead to hardware lockups or synchronous external aborts.
Should the return value of phy_init() be checked here?

[Severity: High]
This is a pre-existing issue, but the shared IRQ handler unconditionally
returns IRQ_HANDLED:

spear13xx_pcie_irq_handler() {
    ...
    status = readl(&app_reg->int_sts);

    if (status & MSI_CTRL_INT) {
        BUG_ON(!IS_ENABLED(CONFIG_PCI_MSI));
        dw_handle_msi_irq(pp);
    }

    writel(status, &app_reg->int_clr);

    return IRQ_HANDLED;
}

Returning IRQ_HANDLED unconditionally in a shared IRQ handler breaks the
kernel's spurious interrupt detection and can swallow interrupts. If the
shared IRQ line screams, the kernel will not disable the line because it
thinks this driver is successfully handling the interrupts, which can lead
to CPU stalls. Should this return IRQ_NONE when status is 0?

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic