Re: [PATCH v2] net: macb: check return value of clk_prepare_enable() in runtime resume

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>
Date: Mon, 01 Jun 2026 06:19:22 +0000

> macb_runtime_resume() calls clk_prepare_enable() five times but discards
> the return value each time. clk_prepare_enable() is marked __must_check,
> and if a clock fails to enable the driver would silently continue with
> potentially unclocked hardware.
> 
> The symmetric disable path, macb_clks_disable(), already uses
> clk_bulk_disable_unprepare(). Convert the enable path to the matching
> clk_bulk_prepare_enable(), which enables the clocks as a group and, on
> failure, unwinds the ones it already enabled before returning the error.
> 
> This was found by code review, not by an observed failure, and I do not
> have macb hardware to test on; it is a robustness fix that propagates the
> clk_prepare_enable() error instead of discarding it.
> 
> Signed-off-by: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>
> ---
> v2:
>  - Reword the commit message: drop the misleading "atomically" wording
>    (there is no locking involved) and describe this as a robustness fix
>    found by code review rather than a fix for an observed bug.
>  - Cc the networking maintainers that were missing from v1.
> 
>  drivers/net/ethernet/cadence/macb_main.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a12aa21244e8..85bd4ff0e4e6 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -6185,16 +6185,19 @@ static int __maybe_unused macb_runtime_resume(struct device *dev)
>  {
>  	struct net_device *netdev = dev_get_drvdata(dev);
>  	struct macb *bp = netdev_priv(netdev);
> +	struct clk_bulk_data clks[] = {
> +		{ .clk = bp->pclk },
> +		{ .clk = bp->hclk },
> +		{ .clk = bp->tx_clk },
> +		{ .clk = bp->rx_clk },
> +		{ .clk = bp->tsu_clk },
> +	};
>  
> -	if (!(device_may_wakeup(dev))) {
> -		clk_prepare_enable(bp->pclk);
> -		clk_prepare_enable(bp->hclk);
> -		clk_prepare_enable(bp->tx_clk);
> -		clk_prepare_enable(bp->rx_clk);
> -		clk_prepare_enable(bp->tsu_clk);
> -	} else if (!(bp->caps & MACB_CAPS_NEED_TSUCLK)) {
> -		clk_prepare_enable(bp->tsu_clk);
> -	}
> +	if (!(device_may_wakeup(dev)))

Oops, redundant parentheses.

> +		return clk_bulk_prepare_enable(ARRAY_SIZE(clks), clks);
> +
> +	if (!(bp->caps & MACB_CAPS_NEED_TSUCLK))
> +		return clk_prepare_enable(bp->tsu_clk);
>  
>  	return 0;
>  }

Apart from this:

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

Thanks,
Olek