Re: [PATCH 05/13] PCI: brcmstb: Declare and assign quirk OB_WIN_MAXSZ_128MB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The OB_WIN_MAXSZ_128MB quirk handler silently truncates outbound memory resources and completely ignores subsequent memory windows due to an unconditional early exit.
--

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

PCI: brcmstb: Declare and assign quirk OB_WIN_MAXSZ_128MB
    
Even though only BMIPs chips have this quirk, it is more uniform and
readable to define this specific quirk where each outbound window is must
be <= SZ_128M.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 71ddd8a760af9..401898d04d521 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c

[ ... ]

> @@ -1294,11 +1296,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  			return -EINVAL;
>  		}
>  
> -		if (is_bmips(pcie)) {
> +		if (BQUIRK(pcie, OB_WIN_MAXSZ_128MB)) {
>  			u64 start = res->start;
>  			unsigned int j, nwins = resource_size(res) / SZ_128M;
>  
> -			/* bmips PCIe outbound windows have a 128MB max size */
> +			/* PCIe outbound windows have a 128MB max size */
>  			if (nwins > BRCM_NUM_PCIE_OUT_WINS)
>  				nwins = BRCM_NUM_PCIE_OUT_WINS;

[Severity: High]
This is a pre-existing issue, but does this logic leave resources smaller than
128MB unmapped due to integer division dropping the remainder?

Also, instead of returning an error like the non-quirk path does when the
window count is exceeded, oversized resources seem to be silently capped here.

Could this lead to leaving some required memory regions unmapped?

>  			for (j = 0; j < nwins; j++, start += SZ_128M)

[Severity: High]
This is also a pre-existing issue inherited from the earlier is_bmips()
implementation, but looking at brcm_pcie_setup() just past this loop:

			for (j = 0; j < nwins; j++, start += SZ_128M)
				brcm_pcie_set_outbound_win(pcie, j, start,
							   start - entry->offset,
							   SZ_128M);
			break;

Does this unconditional break force an early exit from the resource iteration
loop over bridge->windows?

If platforms define multiple memory regions in their device tree (such as the
four 128MB PCIe memory regions in bcm7425.dtsi), wouldn't this break cause the
handler to map only the first resource and ignore the subsequent memory windows?

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