Re: [PATCH v2 1/2] net: stmmac: call of_node_put() and stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()

[Dan Carpenter <dan.carpenter@linaro.org>]

The subject is too long.

On Wed, Dec 18, 2024 at 12:22:29PM +0900, Joe Hattori wrote:
>  	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
>  							&pdev->dev, "ahb");
>  	if (IS_ERR(plat->stmmac_ahb_rst)) {
> -		ret = plat->stmmac_ahb_rst;
> -		goto error_hw_init;
> +		stmmac_remove_config_dt(pdev, plat);
> +		return ERR_CAST(plat->stmmac_ahb_rst);
>  	}
>  
>  	return plat;
> -
> -error_hw_init:
> -	clk_disable_unprepare(plat->pclk);
> -error_pclk_get:
> -	clk_disable_unprepare(plat->stmmac_clk);
> -
> -	return ret;

Ah...  This is a bug fix, but it's not fixed in the right way.

These labels at the end of the function are called an unwind ladder.
This is where people mostly expect the error handling to be done.  Don't
get rid of the unwind ladder, but instead add the calls to:

error_put_phy:
	of_node_put(plat->phy_node);
error_put_mdio:
	of_node_put(plat->mdio_node);

The original code had some code paths which called
stmmac_remove_config_dt().  Get rid of that.  Everything should use the
unwind ladder.  This business of mixing error handling styles is what led
to this bug.

Calling a function to cleanup "everything" doesn't work because we keep
adding more stuff so it starts out as "everything" today but tomorrow
it's a leak.

This can all be sent as one patch if you describe it in the right way.

    The error handling in stmmac_probe_config_dt() has some
    error paths which don't call of_node_put().  The problem is
    that some error paths call stmmac_remove_config_dt() to
    clean up but others use and unwind ladder.  These two types
    of error handling have not kept in sync and have been a
    recurring source of bugs.  Re-write the error handling in
    stmmac_probe_config_dt() to use an unwind ladder.

regards,
dan carpenter