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)))
+		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;
 }
-- 
2.54.0

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

 

On Mon, Jun 01, 2026 at 06:19:22AM +0000, Gustavo Kenji Mendonça Kaneko wrote:
> 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.

Given that the device is missing clocks either way, it is dead. What
is the benefit of returning the error code? It would be good to add
that to the commit message, as a justification for the
change. Otherwise this just seems like pointless churn.

   Andrew