Re: [PATCH] PCI: mediatek-gen3: Fix 64-bit type truncation in mtk_pcie_set_trans_table()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch replaces a 32-bit ffs() call with an undefined 64-bit function ffs64(), breaking the build. Furthermore, if trivially corrected to use the kernel's __ffs64(), it introduces a severe logic error due to 0-based indexing.

Pre-existing issues:
- [High] Silent failure when available PCIe translation tables are exhausted.
--

--- Patch [1]: [PATCH] PCI: mediatek-gen3: Fix 64-bit type truncation in mtk_pcie_set_trans_table() ---
commit 5abeef37aefeb0da627b6bdbff1cc75a0b1bd361
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: mediatek-gen3: Fix 64-bit type truncation in mtk_pcie_set_trans_table()

This commit updates address truncation calculations to use 64-bit helpers
like fls64() and BIT_ULL() to prevent truncation above 4GB.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

[ ... ]

> @@ -334,10 +334,10 @@ static int mtk_pcie_set_trans_table(struct mtk_gen3_pcie *pcie,
>  
>  	while (remaining && (*num < PCIE_MAX_TRANS_TABLES)) {
>  		/* Table size needs to be a power of 2 */
> -		table_size = BIT(fls(remaining) - 1);
> +		table_size = BIT_ULL(fls64(remaining) - 1);
>  
>  		if (cpu_addr > 0) {
> -			addr_align = BIT(ffs(cpu_addr) - 1);
> +			addr_align = BIT_ULL(ffs64(cpu_addr) - 1);

[Severity: High]
Does this introduce a build failure and a potential logic error?

The kernel does not provide an ffs64() macro, which seems like it will
cause an immediate build failure.

If this is trivially fixed by using the kernel's __ffs64(), wouldn't the 
0-based return value cause issues here? 

The original ffs() is 1-based, meaning ffs(cpu_addr) - 1 correctly calculates
the alignment shift.

With a 0-based __ffs64(cpu_addr) - 1, the shift could result in undefined
behavior by shifting by -1 if the first set bit is bit 0, or it could halve
the intended alignment requirement.

[ ... ]

>  		cpu_addr += table_size;
>  		pci_addr += table_size;
>  		remaining -= table_size;
>  		(*num)++;
>  	}
>  
>  	if (remaining)
>  		dev_warn(pcie->dev, "not enough translate table for addr: %#llx, limited to [%d]\n",
>  			 (unsigned long long)cpu_addr, PCIE_MAX_TRANS_TABLES);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does this function silently fail when
translation tables are exhausted?

If remaining is greater than 0 and *num reaches PCIE_MAX_TRANS_TABLES,
the loop exits and a warning is printed, but the function returns 0.

Could this leave the hardware in a partially unmapped state, leading to
subsequent synchronous aborts when the unmapped PCIe windows are accessed?

Should this return an error code like -ENOMEM instead of 0 on resource
exhaustion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728101413.4575-1-sreekuttan2156239@gmail.com?part=1