Re: [PATCH net-next 8/9] net: stmmac: qcom-ethqos: add per-platform NOC clock voting

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect power sequencing order in ethqos_clks_config() error path drops OPP performance state before disabling dependent clock.
- [Medium] Performance state (OPP vote) leaks if probe fails after qcom_ethqos_init_noc_clks().

Pre-existing issues:
- [High] Sleepable calls inside NOIRQ PM suspend phase (sleeping in atomic context).
--

commit 4d584908beed74d65a7bef91b593a47ddf6c4f85
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

net: stmmac: qcom-ethqos: add per-platform NOC clock voting
    
Some SoCs gate the EMAC's path to the System NOC behind dedicated clocks
that must be enabled before the DMA can reach memory. Add
ethqos_noc_clk_cfg and the corresponding fields in the driver-data and
runtime structs so each compatible can declare its own set with per-clock
rates.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 4f9bef662c579..a31541506a4c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -708,7 +758,12 @@ static int ethqos_clks_config(void *priv, bool enabled)
>  		qcom_ethqos_set_sgmii_loopback(ethqos, true);
>  		ethqos_set_func_clk_en(ethqos);
>  	} else {
> +		if (ethqos->num_noc_clks) {
> +			ret = clk_bulk_prepare_enable(ethqos->num_noc_clks,
> +						      ethqos->noc_clks);
> +			if (ret) {
> +				dev_err(&ethqos->pdev->dev,
> +					"NOC clocks enable failed: %d\n", ret);
> +				dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
> +				clk_disable_unprepare(ethqos->link_clk);
> +				return ret;
> +			}
> +		}

[Severity: High]
Does this error unwinding path invert the required hardware power sequence?

If clk_bulk_prepare_enable() fails, the code calls dev_pm_opp_set_rate() to
drop the OPP performance state before calling clk_disable_unprepare() on
the link_clk. 

Should the dependent link_clk be disabled first to prevent hardware lockups
or brownouts while it is still running?

>  		/* Enable functional clock to prevent DMA reset to timeout due
>  		 * to lacking PHY clock after the hardware block has been power
>  		 * cycled. The actual configuration will be adjusted once
> @@ -708,7 +758,12 @@ static int ethqos_clks_config(void *priv, bool enabled)
>  		qcom_ethqos_set_sgmii_loopback(ethqos, true);
>  		ethqos_set_func_clk_en(ethqos);
>  	} else {
> +		if (ethqos->num_noc_clks)
> +			clk_bulk_disable_unprepare(ethqos->num_noc_clks,
> +						   ethqos->noc_clks);
>  		clk_disable_unprepare(ethqos->link_clk);
> +		if (ethqos->num_noc_clks)
> +			dev_pm_opp_set_rate(&ethqos->pdev->dev, 0);
>  	}
>  
>  	return ret;

[Severity: High]
This is a pre-existing issue, but ethqos_clks_config() is called from
stmmac_pltfr_noirq_suspend() (via pm_runtime_force_suspend() and
stmmac_runtime_suspend()), which runs with local interrupts disabled.

The existing code already called clk_disable_unprepare() which acquires the
sleepable CCF prepare_lock mutex, risking a deadlock or kernel panic when
sleeping in atomic context.

This patch adds more sleepable function calls here, such as
clk_bulk_disable_unprepare() and dev_pm_opp_set_rate() (which acquires
the opp_table->lock mutex).

Could these sleepable hardware power operations be moved to a sleepable
suspend phase?

[ ... ]

> @@ -736,6 +791,46 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
>  	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
>  }
>  
> +/* Some SoCs gate NOC access behind dedicated clocks. Acquire them here
> + * so ethqos_clks_config() can enable/disable them at runtime. The OPP
> + * table is used to propagate the required VDD_CX performance state via
> + * dev_pm_opp_set_rate().
> + */
> +static int qcom_ethqos_init_noc_clks(struct qcom_ethqos *ethqos,
> +				     const struct ethqos_emac_driver_data *data)
> +{
> +	struct device *dev = &ethqos->pdev->dev;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!data->num_noc_clks)
> +		return 0;
> +
> +	for (i = 0; i < data->num_noc_clks; i++) {
> +		ethqos->noc_clks[i].id = data->noc_clk_cfg[i].id;
> +		ethqos->noc_clk_rates[i] = data->noc_clk_cfg[i].rate;
> +	}
> +	ethqos->num_noc_clks = data->num_noc_clks;
> +
> +	ret = devm_clk_bulk_get(dev, ethqos->num_noc_clks, ethqos->noc_clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get NOC clocks\n");
> +
> +	ret = devm_pm_opp_set_clkname(dev, data->noc_clk_cfg[0].id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set OPP clock name\n");
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add OPP table\n");
> +
> +	ret = dev_pm_opp_set_rate(dev, data->noc_clk_cfg[0].rate);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set initial NOC OPP rate\n");
> +
> +	return 0;
> +}
> +
>  static int qcom_ethqos_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -795,6 +890,12 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->has_emac_ge_3 = data->has_emac_ge_3;
>  	ethqos->needs_sgmii_loopback = data->needs_sgmii_loopback;
>  
> +	if (data->num_noc_clks) {
> +		ret = qcom_ethqos_init_noc_clks(ethqos, data);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
>  	if (IS_ERR(ethqos->link_clk))
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),

[Severity: Medium]
If devm_clk_get() fails (e.g., returning -EPROBE_DEFER), does this leak the
performance state (OPP) vote? 

qcom_ethqos_init_noc_clks() sets the initial rate via dev_pm_opp_set_rate(),
but there doesn't seem to be a devres cleanup action or manual unwind in the
error path of qcom_ethqos_probe() to drop the vote by calling
dev_pm_opp_set_rate(dev, 0).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035@oss.qualcomm.com?part=8