Re: [PATCH 01/60] drm/simple-kms: Add create_plane_state hook

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional call to the optional `destroy_plane_state` callback causes a NULL pointer dereference if the driver does not implement it.
- [Medium] The fallback priority between `create_plane_state` and `reset_plane` contradicts the intended design, prioritizing the legacy hook.
--

--- Patch [1]: [PATCH 01/60] drm/simple-kms: Add create_plane_state hook ---
commit 2eb693cbd25d991d0fc7bf28e45683c97579b97a
Author: Maxime Ripard <mripard@kernel.org>

drm/simple-kms: Add create_plane_state hook

This commit introduces a create_plane_state hook to the simple-kms helper to
allow drivers to allocate and initialize pristine state without side effects.
It updates drm_simple_kms_plane_reset() to use this new hook while falling
back to the legacy reset_plane hook if it is not provided.

> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 8e1d07b9f1e30..efb643ab64487 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
[ ... ]
> @@ -286,10 +286,29 @@ static void drm_simple_kms_plane_reset(struct drm_plane *plane)
>  	struct drm_simple_display_pipe *pipe;
>  
>  	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> -	if (!pipe->funcs || !pipe->funcs->reset_plane)
> +	if (!pipe->funcs)
>  		return drm_atomic_helper_plane_reset(plane);
>  
> -	return pipe->funcs->reset_plane(pipe);
> +	if (pipe->funcs->reset_plane)
> +		return pipe->funcs->reset_plane(pipe);
> +
> +	if (pipe->funcs->create_plane_state) {

[Severity: Medium]
Does this evaluation order match the intended design? The commit message 
states the code will use create_plane_state to allocate a fresh state, 
falling back to reset_plane. But here, if a driver provides both during a 
migration, it returns early and create_plane_state will not be executed.
Should create_plane_state be prioritized before checking reset_plane?

> +		struct drm_plane_state *state;
> +
> +		if (plane->state) {
> +			pipe->funcs->destroy_plane_state(pipe, plane->state);

[Severity: High]
Can this cause a NULL pointer dereference? If a driver implements 
create_plane_state but relies on the default destruction logic (meaning 
pipe->funcs->destroy_plane_state is NULL), this unconditional call will 
panic when plane->state is already set, such as during suspend/resume or 
consecutive resets. Other functions in this file check for NULL and safely 
fall back to drm_atomic_helper_plane_destroy_state().

> +			plane->state = NULL;
> +		}
> +
> +		state = pipe->funcs->create_plane_state(pipe);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-drm-no-more-plane-reset-v1-0-302d986fe5f0@kernel.org?part=1