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

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 <[email protected]>

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/[email protected]?part=1