Re: [PATCH v2 4/4] PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninitialized stack variable `num_lanes` leads to unpredictable probe failures

Pre-existing issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored, which can lead to hardware access on suspended devices.
--

commit b24fe645228ac8c1e16f2ba7617e18b13b23173d
Author: Christian Marangi <ansuelsmth@gmail.com>

PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581

This commit adds 2-lanes mode support for the Airoha AN7581 SoC by
configuring the PCIe MUX in the SCU register. It introduces logic to
parse the secondary PCIe MAC region and handle necessary PERSTOUT resets
for the newly supported lane configuration.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index b0accd8285892..14893cff135a1 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

[ ... ]

> @@ -949,18 +965,38 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
>  			pcie->num_lanes = num_lanes;
>  	}
>  
> +	/* Map secondary PCIe for 2-lanes mode for EN7581 */
> +	if (num_lanes == 2 && device_is_compatible(dev, "airoha,en7581-pcie")) {

[Severity: High]
If the num-lanes property is missing from the device tree,
of_property_read_u32() returns an error without modifying the num_lanes output
parameter, leaving it uninitialized. If the stack memory happens to contain 2,
could this enter the block and fail the probe by returning -EINVAL since the
sec-pcie-mac resource would be missing in a 1-lane configuration?

Should this evaluate pcie->num_lanes instead?

> +		regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sec-pcie-mac");
> +		if (!regs)
> +			return -EINVAL;
> +		pcie->sec_base = devm_ioremap_resource(dev, regs);

[ ... ]

> @@ -1024,33 +1073,47 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_get_sync()?

If the runtime PM framework fails to power up or clock the device, accessing
hardware registers directly afterward might cause a synchronous external abort
and kernel panic on ARM platforms. The same pre-existing behavior is also
present in the legacy mtk_pcie_power_up() path.

> -	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> -	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> -	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> -	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> -	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> -
> -	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> -	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> -	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> -	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> -	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> -
>  	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);

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