Re: [PATCH 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 4 potential issue(s) to consider:

New issues:
- [High] Calling devm_ioremap_resource() inside the .power_up() callback breaks system resume with -EBUSY.
- [High] Unvalidated num_lanes > 2 causes a NULL pointer dereference in the hardware configuration loop.
- [High] Missing clock disable on error path in mtk_pcie_en7581_power_up().

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

commit c4659641288856a22fb57ae4fe5c48d43375cff0
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 secondary PCIe
MAC memory mapping, SCU regmap parsing, and additional PERSTOUT reset logic.

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

[ ... ]

> @@ -955,12 +974,29 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
>  static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  {
>  	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	unsigned int num_lanes = max(1, pcie->num_lanes);
>  	struct device *dev = pcie->dev;
>  	struct resource_entry *entry;
>  	struct regmap *pbus_regmap;
>  	u32 val, args[2], size;
>  	resource_size_t addr;
> -	int err;
> +	int i, err;
> +
> +	if (num_lanes == 2) {
> +		struct platform_device *pdev = to_platform_device(dev);
> +		struct resource *regs;
> +
> +		regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sec-pcie-mac");
> +		if (!regs)
> +			return -EINVAL;
> +		pcie->sec_base = devm_ioremap_resource(dev, regs);

[Severity: High]
Since mtk_pcie_en7581_power_up() is assigned as the .power_up callback and
executes during both probe and resume_noirq(), won't calling
devm_ioremap_resource() here cause a conflict on system resume?

The devm-managed resources persist across suspend/resume cycles, so
devm_request_mem_region() will detect a conflict and cause
devm_ioremap_resource() to return -EBUSY. Could this mapping be moved
to the probe path, such as mtk_pcie_parse_port(), to avoid resume failures?

> +		if (IS_ERR(pcie->sec_base))
> +			return dev_err_probe(dev, PTR_ERR(pcie->sec_base), "failed to map secondary register base\n");

[ ... ]

> @@ -1024,34 +1060,60 @@ 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 should the return value
of pm_runtime_get_sync() be checked?

If the device power domain fails to transition to an active state,
pm_runtime_get_sync() will return a negative error code. Proceeding to access
registers on suspended hardware could trigger a synchronous external abort or
system crash.

> -	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);

[ ... ]

> +	/* Wait for refclk to stabilize */
>  	msleep(PCIE_T_PVPERL_MS);
>  
> +	/* Configure all the lines to the same EQ config */
> +	for (i = 0; i < num_lanes; i++) {
> +		void __iomem *base = pcie->base;
> +
> +		if (i == 1)
> +			base = pcie->sec_base;
> +
> +		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, base + PCIE_EQ_PRESET_01_REG);

[Severity: High]
If the Device Tree specifies a valid num-lanes value greater than 2 (e.g.,
4 or 8), will this loop dereference a NULL pointer?

Earlier in this function, pcie->sec_base is exclusively mapped when
num_lanes == 2. If num_lanes is greater than 2, pcie->sec_base remains NULL.
When i == 1, base becomes NULL, and writel_relaxed() will panic. Should
there be explicit bounds validation before writing to sec_base?

> +		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, base + PCIE_PIPE4_PIE8_REG);
> +	}
> +
> +	/* Deassert PERSTOUT for all relevant lines */
> +	err = reset_control_bulk_deassert(MAX_NUM_PERSTOUT_RESETS,
> +					  pcie->perstout_resets);
> +	if (err) {
> +		dev_err(dev, "failed to deassert PERSTOUTs\n");
> +		goto err_perstout_assert;
> +	}
> +
>  	return 0;
>  
> -err_clk_prepare_enable:
> +err_perstout_assert:
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);

[Severity: High]
If reset_control_bulk_deassert() fails, won't jumping to this label leak
the clock prepare/enable references?

The function successfully enables clocks earlier with
clk_bulk_prepare_enable(), but err_perstout_assert entirely omits calling
clk_bulk_disable_unprepare(), which will leave the hardware clocks
permanently running.

>  	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
>  				  pcie->phy_resets);

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