Re: [PATCH 02/13] PCI: brcmstb: Use flags u32 instead of bools

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly introduced BFLAG macro lacks outer parentheses around the bitwise AND expression, introducing a dangerous operator precedence pitfall.

Pre-existing issues:
- [High] Uninitialized spinlock `bridge_lock` is acquired during early probe before being initialized.
--

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

    PCI: brcmstb: Use flags u32 instead of bools

    This commit replaces separate boolean fields for SoC configurations with a
    single u32 bitvector. It introduces the BFLAG macro to provide a uniform
    way to assign and check these flags.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 89aaa19d508a7..47862569937eb 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -286,16 +286,24 @@ struct inbound_win {
>   */
>  #define CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN		BIT(0)
>  
> +/* FLAGS */
> +#define BFLAG(pcie, flag)			((pcie)->cfg->flags & CFG_FLG_ ## flag)

[Severity: Medium]
Will this macro evaluate correctly if used in a logical NOT expression like
!BFLAG(pcie, HAS_PHY)?

Without outer parentheses around the bitwise AND expression, an expression
like !BFLAG(...) expands to !(pcie)->cfg->flags & flag. This applies the
logical NOT before the bitwise AND, which could lead to incorrect evaluation.

Could outer parentheses be added to prevent operator precedence issues?

[ ... ]

> @@ -356,14 +364,14 @@ static int brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
>  	unsigned long flags;
>  	int ret;
>  
> -	if (pcie->cfg->has_err_report)
> +	if (BFLAG(pcie, HAS_ERR_REPORT))
>  		spin_lock_irqsave(&pcie->bridge_lock, flags);

[Severity: High]
This is a pre-existing issue, but does this acquire an uninitialized spinlock
during early probe?

During device probe, brcm_pcie_probe() calls brcm_pcie_bridge_sw_init_set()
early in the sequence:

brcm_pcie_probe() {
    ...
    ret = brcm_pcie_bridge_sw_init_set(pcie, 0);
    if (ret)
        return dev_err_probe(&pdev->dev, ret,
                             "could not de-assert bridge reset\n");
    ...
}

If the SoC configuration has the HAS_ERR_REPORT flag set, this attempts to
acquire pcie->bridge_lock.

However, the spinlock isn't initialized until much later in probe:

[ ... ]

> @@ -2179,7 +2186,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (pcie->cfg->has_err_report) {
> +	if (BFLAG(pcie, HAS_ERR_REPORT)) {
>  		spin_lock_init(&pcie->bridge_lock);
>  		brcm_register_die_notifiers(pcie);
>  	}

As shown here at the end of brcm_pcie_probe(), spin_lock_init() happens
long after the first call to brcm_pcie_bridge_sw_init_set().

Could this cause lockdep splats or kernel crashes when PREEMPT_RT or lock
debugging is enabled?

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