Re: [PATCH 10/13] PCI: brcmstb: Declare and assign quirk PERST_PCIE_REV_CUTOFF

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free / NULL pointer dereference in error path when pci_host_probe() fails.
- [High] Use of uninitialized spinlock `pcie->bridge_lock`.
--

commit 8a2eb9e850a817eb0d5eb7cbe7b4bf1dbe6785e6
Author: Jim Quinlan <james.quinlan@broadcom.com>

PCI: brcmstb: Declare and assign quirk PERST_PCIE_REV_CUTOFF

This commit declares a new quirk for SoCs where the SW behavior for asserting
PERST# only works up to a specific PCIe HW version, and assigns it to the
bcm4908 configuration.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index dfd7035710918..047783e33ae42 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c

[ ... ]

> @@ -2168,7 +2171,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto fail;
>  
>  	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> -	if (pcie->cfg->soc_base == BCM4908 &&
> +	if (BQUIRK(pcie, PERST_PCIE_REV_CUTOFF) &&
>  	    pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
>  		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
>  		ret = -ENODEV;

[Severity: High]
This is a pre-existing issue, but looking at the error path later in
brcm_pcie_probe(), is it safe to call brcm_pcie_remove() when
pci_host_probe() fails?

If pci_host_probe(bridge) fails (for example, due to a memory allocation
failure inside pci_alloc_bus()), the bridge's bus structure might be freed
or remain NULL.

brcm_pcie_probe() {
    ...
    ret = pci_host_probe(bridge);
    if (ret) {
        brcm_pcie_remove(pdev);
        return ret;
    }
}

brcm_pcie_remove() {
    ...
    pci_stop_root_bus(bridge->bus);
}

Could this pci_stop_root_bus() call dereference a dangling or NULL
bridge->bus pointer, causing a crash instead of a graceful failure? Should
this path jump to the fail label instead to clean up local resources?

[Severity: High]
This is also a pre-existing issue, but does brcm_pcie_probe() use
pcie->bridge_lock before it is initialized?

Early in brcm_pcie_probe(), brcm_pcie_bridge_sw_init_set(pcie, 0) is called.
If the controller config has the HAS_ERR_REPORT flag set, this acquires
pcie->bridge_lock:

brcm_pcie_bridge_sw_init_set() {
    if (BFLAG(pcie, HAS_ERR_REPORT))
        spin_lock_irqsave(&pcie->bridge_lock, flags);
}

However, spin_lock_init(&pcie->bridge_lock) is not called until the very end
of brcm_pcie_probe(), right before it returns success.

Can this lead to an uninitialized spinlock usage on platforms with
HAS_ERR_REPORT enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911233541.1650895-1-james.quinlan@broadcom.com?part=10