Re: [PATCH net v2 2/2] net: bcmasp: fix double disable of clk

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: bcmasp: fix double disable of clk

Switch to devm_clk_get_optional() so we can manage the clock ourselves.
We dynamically control the clocks depending on the state of the interface
for power savings. The default state is clock disabled, so unbinding the
driver causes a double disable.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 2034a1593db7..dda89afb2917 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> @@ -1249,7 +1249,7 @@ static int bcmasp_probe(struct platform_device *pdev)
>  	if (priv->irq <= 0)
>  		return -EINVAL;
>
> -	priv->clk = devm_clk_get_optional_enabled(dev, "sw_asp");
> +	priv->clk = devm_clk_get_optional(dev, "sw_asp");
>  	if (IS_ERR(priv->clk))
>  		return dev_err_probe(dev, PTR_ERR(priv->clk),
>  				     "failed to request clock\n");
> @@ -1277,6 +1277,10 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	bcmasp_set_pdata(priv, pdata);
>
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to start clock\n");
> +
>  	/* Enable all clocks to ensure successful probing */
>  	bcmasp_core_clock_set(priv, ASP_CTRL_CLOCK_CTRL_ASP_ALL_DISABLE, 0);
>
> @@ -1288,8 +1292,10 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	ret = devm_request_irq(&pdev->dev, priv->irq, bcmasp_isr, 0,
>  			       pdev->name, priv);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to request ASP interrupt: %d", ret);
> +	if (ret) {
> +		dev_err(dev, "Failed to request ASP interrupt: %d", ret);
> +		goto err_clock_disable;
> +	}
>
>  	/* Register mdio child nodes */
>  	of_platform_populate(dev->of_node, bcmasp_mdio_of_match, NULL, dev);
> @@ -1301,13 +1307,17 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  	priv->mda_filters = devm_kcalloc(dev, priv->num_mda_filters,
>  					 sizeof(*priv->mda_filters), GFP_KERNEL);
> -	if (!priv->mda_filters)
> -		return -ENOMEM;
> +	if (!priv->mda_filters) {
> +		ret = -ENOMEM;
> +		goto err_clock_disable;
> +	}
>
>  	priv->net_filters = devm_kcalloc(dev, priv->num_net_filters,
>  					 sizeof(*priv->net_filters), GFP_KERNEL);
> -	if (!priv->net_filters)
> -		return -ENOMEM;
> +	if (!priv->net_filters) {
> +		ret = -ENOMEM;
> +		goto err_clock_disable;
> +	}
>
>  	bcmasp_core_init_filters(priv);
>
> @@ -1316,7 +1326,8 @@ static int bcmasp_probe(struct platform_device *pdev)
>  	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
>  	if (!ports_node) {
>  		dev_warn(dev, "No ports found\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_clock_disable;
>  	}
>
>  	i = 0;

[ ... ]

> @@ -1358,6 +1369,8 @@ static int bcmasp_probe(struct platform_device *pdev)
>
>  err_cleanup:
>  	bcmasp_remove_intfs(priv);
> +err_clock_disable:
> +	clk_disable_unprepare(priv->clk);
>                  ^^^^

Does this double-disable the clock on the register_netdev() error path?

Looking at bcmasp_probe(), the clock handling sequence is:

    Line 1280: clk_prepare_enable(priv->clk) enables the clock
    Line 1352: clk_disable_unprepare(priv->clk) disables it

Then register_netdev() is called in a loop starting around line 1358.
If register_netdev() fails, it jumps to err_cleanup which falls through
to err_clock_disable and calls clk_disable_unprepare() again.

Since the clock was already disabled at line 1352 before the network
device registration loop, calling clk_disable_unprepare() a second time
in the error path would unbalance the clock reference count.

Should the err_cleanup label return directly without falling through to
err_clock_disable, or should a separate error label be added for paths
after the clock is already disabled?
-- 
pw-bot: cr