[PATCH] drm/sun4i: frontend: fix clock leak on reset failure in runtime resume

[PATCH] drm/sun4i: frontend: fix clock leak on reset failure in runtime resume

[Jiawen Liu]

sun4i_frontend_runtime_resume() can fail after enabling the bus, mod,
and ram clocks if reset_control_reset() returns an error. The function
previously returned without disabling those clocks, leaking them and
leaving the hardware in an inconsistent state.

Check the return value of each clk_prepare_enable() call and unwind the
clocks that were successfully enabled on any subsequent failure,
including reset failure.

Signed-off-by: jiawen <1298662399@qq.com>
---
diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
--- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
@@ -647,14 +647,22 @@
 
 	clk_set_rate(frontend->mod_clk, 300000000);
 
-	clk_prepare_enable(frontend->bus_clk);
-	clk_prepare_enable(frontend->mod_clk);
-	clk_prepare_enable(frontend->ram_clk);
+	ret = clk_prepare_enable(frontend->bus_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(frontend->mod_clk);
+	if (ret)
+		goto err_disable_bus_clk;
+
+	ret = clk_prepare_enable(frontend->ram_clk);
+	if (ret)
+		goto err_disable_mod_clk;
 
 	ret = reset_control_reset(frontend->reset);
 	if (ret) {
 		dev_err(dev, "Couldn't reset our device\n");
-		return ret;
+		goto err_disable_ram_clk;
 	}
 
 	regmap_update_bits(frontend->regs, SUN4I_FRONTEND_EN_REG,
@@ -664,6 +672,14 @@
 	sun4i_frontend_scaler_init(frontend);
 
 	return 0;
+
+err_disable_ram_clk:
+	clk_disable_unprepare(frontend->ram_clk);
+err_disable_mod_clk:
+	clk_disable_unprepare(frontend->mod_clk);
+err_disable_bus_clk:
+	clk_disable_unprepare(frontend->bus_clk);
+	return ret;
 }
 
 static int sun4i_frontend_runtime_suspend(struct device *dev)

Re: [PATCH] drm/sun4i: frontend: fix clock leak on reset failure in runtime resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled error from sun4i_frontend_init leads to kernel panic on hardware register access.
- [High] Plane atomic update callback invokes sleeping functions via pm_runtime_get_sync.
--

--- Patch [1]: [PATCH] drm/sun4i: frontend: fix clock leak on reset failure in runtime resume ---
commit 545a5eac90415ee6d6efb1f3f808e86b0dd094da
Author: Jiawen Liu <1298662399@qq.com>

drm/sun4i: frontend: fix clock leak on reset failure in runtime resume

sun4i_frontend_runtime_resume() can fail after enabling the bus, mod,
and ram clocks if reset_control_reset() returns an error. The function
previously returned without disabling those clocks, leaking them and
leaving the hardware in an inconsistent state.

> diff --git a/drivers/gpu/drm/sun4i/sun4i_frontend.c b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> index 5e9c4b97c84c3..db3fad9d8617a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_frontend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_frontend.c
> @@ -647,14 +647,22 @@ static int sun4i_frontend_runtime_resume(struct device *dev)
>  
>  	clk_set_rate(frontend->mod_clk, 300000000);
>  
> -	clk_prepare_enable(frontend->bus_clk);
> -	clk_prepare_enable(frontend->mod_clk);
> -	clk_prepare_enable(frontend->ram_clk);
> +	ret = clk_prepare_enable(frontend->bus_clk);

[Severity: High]
This is a pre-existing issue, but does calling clk_prepare_enable() here
violate atomic context rules?

During display updates, sun4i_backend_layer_atomic_update() executes in an
atomic context where sleeping is forbidden. It calls sun4i_frontend_init()
which synchronously calls pm_runtime_get_sync(). If the device is suspended,
this executes sun4i_frontend_runtime_resume() and reaches these sleeping
clk_prepare_enable() functions, which can cause scheduling while atomic
warnings.

> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(frontend->mod_clk);

[ ... ]

> @@ -664,6 +672,14 @@ static int sun4i_frontend_runtime_resume(struct device *dev)
>  	sun4i_frontend_scaler_init(frontend);
>  
>  	return 0;
> +
> +err_disable_ram_clk:
> +	clk_disable_unprepare(frontend->ram_clk);
> +err_disable_mod_clk:
> +	clk_disable_unprepare(frontend->mod_clk);
> +err_disable_bus_clk:
> +	clk_disable_unprepare(frontend->bus_clk);
> +	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does returning this error with
the bus clock disabled now risk a kernel panic?

In drivers/gpu/drm/sun4i/sun4i_layer.c:sun4i_backend_layer_atomic_update(),
the return value of sun4i_frontend_init() is ignored:

	sun4i_frontend_init(frontend);
	sun4i_frontend_update_coord(frontend, layer);

If sun4i_frontend_runtime_resume() fails (e.g., reset failure) and this
unwinding path successfully disables frontend->bus_clk, the frontend is left
without its bus clock. When sun4i_frontend_update_coord() and other functions
subsequently attempt to write to hardware registers, accessing them while the
bus clock is disabled can trigger a synchronous external abort on ARM.

>  }
>  
>  static int sun4i_frontend_runtime_suspend(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_3AEE86932EE0F9AF7D6549FD705139A4AF06@qq.com?part=1