Re: [net-next,PATCH 1/1] net: stmmac: Fix manage error case during stmmac_dvr_probe

[Jakub Kicinski]

Hi!

Please see the AI-generated review of your patch below.

If what the AI suggests (flipping the order) is correct -
we definitely need a comment on this, 'cause it's counter-intuitive.
---
net: stmmac: Fix manage error case during stmmac_dvr_probe

The probe error path starting at the error_mdio_register label in
__stmmac_dvr_probe() did not undo the runtime PM setup done just before
stmmac_mdio_register(). This patch adds a put and a disable of runtime PM
to that label.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b2b7d0242dd3c..37d06fc6dba47 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -8080,6 +8080,8 @@ static int __stmmac_dvr_probe(struct device *device,
>  error_pcs_setup:
>  	stmmac_mdio_unregister(ndev);
>  error_mdio_register:
> +	pm_runtime_put_sync(device);
> +	pm_runtime_disable(device);

[Severity: High]
Should the order of these two calls be swapped, as in stmmac_dvr_remove()?

As written, pm_runtime_put_sync() runs while runtime PM is still enabled
and the status is RPM_ACTIVE (set earlier in __stmmac_dvr_probe() by
pm_runtime_get_noresume() / pm_runtime_set_active() / pm_runtime_enable()).
There is no ->runtime_idle callback and no autosuspend in this driver:

drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:
	SET_RUNTIME_PM_OPS(stmmac_runtime_suspend, stmmac_runtime_resume, NULL)

so dropping the usage count to zero synchronously runs
stmmac_runtime_suspend() -> stmmac_bus_clks_config(priv, false):

	} else {
		clk_disable_unprepare(plat_dat->stmmac_clk);
		clk_disable_unprepare(plat_dat->pclk);
		if (plat_dat->clks_config)
			plat_dat->clks_config(plat_dat->bsp_priv, enabled);
	}

But pm_runtime_set_active() is only bookkeeping - runtime PM never took a
clock reference of its own. stmmac_clk and pclk were prepared/enabled once
in stmmac_probe_config_dt(), and devm_stmmac_probe_config_dt() registered
the devres action that disables them again:

drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:devm_stmmac_remove_config_dt() {
	clk_disable_unprepare(plat->stmmac_clk);
	clk_disable_unprepare(plat->pclk);
	...
}

Can this lead to a double clk_disable_unprepare() when probe unwinds?  The
new put gates the clocks, then the error propagates out and the glue driver
error path plus devres unwinding disable the same clocks again, for example
in dwmac-imx:

	ret = stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
	if (ret)
		imx_dwmac_clks_config(dwmac, false);

which would hit the clk core warnings in clk_core_disable()
("%s already disabled") and clk_core_unprepare() ("%s already unprepared"),
with a panic on panic_on_warn systems.  25 glue drivers reference
stmmac_pltfr_pm_ops, so the same callback applies broadly.

Is this reachable in practice?  stmmac_mdio_register() returns -ENODEV on
the "No PHY found" path, and stmmac_pcs_setup(), stmmac_phylink_setup(),
stmmac_register_devlink() and register_netdev() all fall through to this
same label.

A related question: since the clocks are now gated for the rest of the
teardown, is it still safe for the plat_dat->exit() callback invoked by
stmmac_dvr_probe() on failure to touch MAC registers?

Would the following match the existing convention in stmmac_dvr_remove(),
which disables first and then uses a put that cannot invoke
->runtime_suspend, leaving the clocks on for the platform/devres teardown?

	pm_runtime_disable(device);
	pm_runtime_put_noidle(device);

>  	stmmac_napi_del(ndev);
>  error_hw_init:
>  	destroy_workqueue(priv->wq);
-- 
pw-bot: cr