Re: [PATCH v3 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:
- [Critical] Modifying the `SCU_PCIC` register via syscon regmap replaces unrelated bits (unintentionally asserting device resets) and races with the clock driver's raw memory accesses.
- [High] The dedicated PERSTOUT resets (perstout_resets) are never asserted during device teardown, leaving PCIe endpoints out of reset.

Pre-existing issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored, potentially leading to hardware access on a suspended device.
- [High] Early returns in `mtk_pcie_en7581_power_up()` leak the `phy_resets` deassert count, permanently breaking hardware initialization on probe retry.
--

commit 13851a15e592cd94ee574eb5716be89f2a065bba
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: PCI: mediatek-gen3: Add 2-lanes mode support for Airoha AN7581

The Airoha AN7581 SoC supports configuring the first PCIe0 lane to 2-lanes
mode by bonding it with the second PCIe lane. This change configures the
PCIe MUX in the SCU register and adds logic to assert and deassert the
PERSTOUT resets.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index b0accd8285892..ba28d95e5d44b 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 (pcie->num_lanes == 2 && device_is_compatible(dev, "airoha,en7581-pcie")) {
> +		regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sec-pcie-mac");
> +		if (!regs)
> +			return -EINVAL;
> +		pcie->sec_base = devm_ioremap_resource(dev, regs);
> +		if (IS_ERR(pcie->sec_base))
> +			return dev_err_probe(dev, PTR_ERR(pcie->sec_base), "failed to map secondary register base\n");
> +	}
> +
>  	return 0;
>  }
>  
>  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 regmap *pbus_regmap, *scu;
>  	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)
> +		return dev_err_probe(dev, -EINVAL, "unsupported num-lanes, maximum 2 lanes supported\n");
> +
> +	if (num_lanes == 2) {
> +		scu = syscon_regmap_lookup_by_phandle(dev->of_node, "airoha,scu");
> +		if (IS_ERR(scu))
> +			return dev_err_probe(dev, PTR_ERR(scu), "failed to map SCU regmap\n");
> +	}

[Severity: High]
This is a pre-existing issue, but do these early returns leak the
phy_resets deassert count?

If syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, or if num_lanes
is greater than 2, this function returns directly to mtk_pcie_setup().

mtk_pcie_setup() does an initial deassert before calling power_up:

mtk_pcie_setup() {
    reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
    ...
    err = pcie->soc->power_up(pcie);
    if (err)
        return err;
}

Since the error path in mtk_pcie_setup() returns without asserting the resets
again, does this leaked reset deassert count on shared resets prevent the
reset controller from physically asserting the line on subsequent probe
attempts, permanently breaking hardware initialization on probe retry?

>  
>  	/*
>  	 * The controller may have been left out of reset by the bootloader
> @@ -992,6 +1028,19 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	size = lower_32_bits(resource_size(entry->res));
>  	regmap_write(pbus_regmap, args[1], GENMASK(31, __fls(size)));
>  
> +	/* Assert PERSTOUT for all relevant lines */
> +	err = reset_control_bulk_assert(MAX_NUM_PERSTOUT_RESETS,
> +					pcie->perstout_resets);
> +	if (err) {
> +		dev_err(dev, "failed to assert PERSTOUTs\n");
> +		return err;
> +	}
> +
> +	/* Configure SCU MUX to disable PCIE1 for 2 lines mode */
> +	if (num_lanes == 2)
> +		regmap_update_bits(scu, SCU_PCIC, SCU_PCIC_PCIE_CTRL,
> +				   FIELD_PREP(SCU_PCIC_PCIE_CTRL, BIT(1)));

[Severity: Critical]
Does this regmap_update_bits() call on SCU_PCIC clear unrelated bits and
unintentionally assert device resets?

By using SCU_PCIC_PCIE_CTRL (GENMASK(7, 0)) as the mask to write BIT(1),
this forces bits 0 and 2-7 to 0. Since the clock/reset driver for this SoC
uses this same register to control device resets with inverted logic
(0 = assert, 1 = deassert), does this unintentionally assert the reset for
the PCIE0 controller itself and other peripherals?

Also, since the clock driver uses raw readl()/writel() without a shared
lock, does this introduce a data race that could corrupt the register state?

> +
>  	/*
>  	 * Unlike the other MediaTek Gen3 controllers, the Airoha EN7581
>  	 * requires PHY initialization and power-on before PHY reset deassert.
> @@ -1024,18 +1073,6 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of pm_runtime_get_sync() here?

If runtime PM fails to resume the device and returns a negative error code,
this unconditionally proceeds to enable clocks and access hardware registers.
Can accessing registers of a suspended device cause synchronous external
aborts or bus faults resulting in a kernel panic?

>  
> -	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);
>  	if (err) {
>  		dev_err(dev, "failed to prepare clock\n");
> @@ -1043,14 +1080,47 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	}
>  
>  	/*
> -	 * Airoha EN7581 performs PCIe reset via clk callbacks since it has a
> -	 * hw issue with PCIE_PE_RSTB signal. Add wait for the time needed to
> -	 * complete the PCIe reset.
> +	 * Old Airoha EN7581 clock driver performed PCIe reset via
> +	 * clk callbacks since it has a hw issue with PCIE_PE_RSTB signal.
> +	 * This is now handled by dedicated PERSTOUT resets with clk
> +	 * driver only enabling the refclk.
> +	 *
> +	 * Wait is still needed 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);
> +
> +		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);

[Severity: High]
Are the perstout_resets missing from the teardown path?

Since they are asserted and deasserted during power up, does the endpoint
remain fully powered on and out of reset while the host controller's PHYs
and clocks are disabled during mtk_pcie_power_down()?

mtk_pcie_power_down() {
    clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
    pm_runtime_put_sync(pcie->dev);
    ...
    phy_power_off(pcie->phy);
    phy_exit(pcie->phy);
    ...
}

This might violate the PCIe specification and leave endpoints active, which
could lead to spurious DMA, fatal bus errors, and system lockups when
unbinding the driver or suspending.

> +	if (err) {
> +		dev_err(dev, "failed to deassert PERSTOUTs\n");
> +		goto err_perstout_deassert;
> +	}
> +
>  	return 0;
>  

[ ... ]

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