Re: [PATCH 39/60] drm/amdgpu_dm: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Memory leak and missing plane state initialization due to ignored return value of atomic_create_state
--

--- Patch [39]: [PATCH 39/60] drm/amdgpu_dm: Convert to atomic_create_state ---
commit ffb311ea66ae63df87af953af043654eafe9802e
Author: Maxime Ripard <mripard@kernel.org>

drm/amdgpu_dm: Convert to atomic_create_state

The plane reset implementation creates a custom state subclass, but only
initializes a pristine state without resetting any hardware. This commit
converts it to use the atomic_create_state callback.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 1b564cfe21202..64647f4a42908 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1515,22 +1515,21 @@ static const struct drm_plane_helper_funcs dm_primary_plane_helper_funcs = {
>  	.panic_flush = amdgpu_dm_plane_panic_flush,
>  };
>  
> -static void amdgpu_dm_plane_drm_plane_reset(struct drm_plane *plane)
> +static struct drm_plane_state *amdgpu_dm_plane_drm_plane_create_state(struct drm_plane *plane)
>  {
>  	struct dm_plane_state *amdgpu_state;
>  
>  	amdgpu_state = kzalloc_obj(*amdgpu_state);
>  	if (!amdgpu_state)
> -		return;
> -
> -	if (plane->state)
> -		plane->funcs->atomic_destroy_state(plane, plane->state);
> +		return ERR_PTR(-ENOMEM);
>  
> -	__drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
> +	__drm_atomic_helper_plane_state_init(&amdgpu_state->base, plane);

[ ... ]

> +	return &amdgpu_state->base;
>  }

[ ... ]

> @@ -1994,8 +1993,8 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  #endif
>  
>  	/* Create (reset) the plane state */
> -	if (plane->funcs->reset)
> -		plane->funcs->reset(plane);
> +	if (plane->funcs->atomic_create_state)
> +		plane->funcs->atomic_create_state(plane);
                ^^^^

[Severity: High]
Does this code leak the newly allocated plane state?

The old reset callback assigned the state internally to plane->state, 
but the new atomic_create_state callback returns the allocated state and 
relies on the caller to handle it.

Because the return value is ignored here, the state pointer appears to be
lost and plane->state remains NULL.

Later, when the driver tries to duplicate the plane state via
amdgpu_dm_plane_drm_plane_duplicate_state(), it might pass the NULL
plane->state to to_dm_plane_state() and dereference it while copying
driver-specific fields, which could cause a kernel panic.

Should we capture the return value and assign it to plane->state, or pass
it to the appropriate DRM helper function to properly initialize the state?

>  	return 0;
>  }

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