Re: [PATCH v3 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver will crash in `gm12u320_stop_fb_update` due to a NULL pointer dereference when dropping a framebuffer reference. The asynchronous worker `gm12u320_fb_update_work` drops its reference to the framebuffer and sets `gm12u320->fb_update.fb = NULL` after copying the first frame. When the CRTC is later disabled (e.g., via device unplug or KMS disable), `gm12u320_stop_fb_update` reads this NULL pointer into `old_fb` and unconditionally calls `drm_framebuffer_put(old_fb)`. Because `drm_framebuffer_put` computes the address of `&fb->base` before passing it to `drm_mode_object_put()`, a NULL `fb` results in a non-NULL offset pointer, bypassing any potential NULL checks in the core and causing a fatal page fault when dereferenced.
- [High] The driver calls sleeping functions (`mutex_lock` and `cancel_delayed_work_sync`) from atomic context paths, violating DRM subsystem architectural guidelines. The patch converts the driver to use standard CRTC and plane atomic helpers (`gm12u320_crtc_helper_atomic_enable`, `gm12u320_crtc_helper_atomic_disable`, and `gm12u320_plane_helper_atomic_update`). These callbacks are explicitly classified by DRM guidelines as atomic context where sleeping is forbidden, yet they call functions that block.
--

--- Patch [5]: [PATCH v3 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 0dd628ecb0a7395ec2d8faaa3a751862640afcb7
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers
    
Convert gm12u320 to direct primary plane, CRTC and encoder setup.
    
Keep shadow-plane helper state, framebuffer access helpers and
no-scaling plane-state check from simple-KMS path.

> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 4ad074337af07..754617722a0db 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c

[ ... ]

> @@ -555,43 +560,106 @@ static int gm12u320_conn_init(struct gm12u320_device *gm12u320)
>  }
>  
>  /* ------------------------------------------------------------------ */
> -/* gm12u320 (simple) display pipe				      */
> +/* gm12u320 display pipe					      */
>  
> -static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe,
> -				 struct drm_crtc_state *crtc_state,
> -				 struct drm_plane_state *plane_state)
> +static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					       struct drm_atomic_commit *commit)
>  {
>  	struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT };
> -	struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> -	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
> +	struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, &gm12u320->plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(pstate);
>  
>  	gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT;
> -	gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], &rect);
> +	gm12u320_fb_mark_dirty(pstate->fb, &shadow_plane_state->data[0], &rect);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does calling
gm12u320_fb_mark_dirty() here violate DRM subsystem architectural guidelines by
sleeping in an atomic context?

gm12u320_crtc_helper_atomic_enable() runs in an atomic context during
non-blocking commits where sleeping is forbidden. However,
gm12u320_fb_mark_dirty() unconditionally acquires a mutex:

	mutex_lock(&gm12u320->fb_update.lock);

Could this lead to kernel warnings like 'scheduling while atomic' or deadlocks
during display modesets? (The same applies to
gm12u320_plane_helper_atomic_update below).

> -static void gm12u320_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void gm12u320_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +						struct drm_atomic_commit *commit)
>  {
> -	struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev);
> +	struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev);
>  
>  	gm12u320_stop_fb_update(gm12u320);
>  }

[Severity: High]
This is also a pre-existing issue, but does gm12u320_stop_fb_update() sleep in
an atomic context, and could it cause a NULL pointer dereference?

First, like the enable path above, gm12u320_crtc_helper_atomic_disable() runs
in an atomic context where sleeping is forbidden. But gm12u320_stop_fb_update()
calls cancel_delayed_work_sync() and acquires a mutex.

Second, can old_fb be NULL when passed to drm_framebuffer_put() inside
gm12u320_stop_fb_update()?

The asynchronous worker gm12u320_fb_update_work() drops its reference to the
framebuffer and sets gm12u320->fb_update.fb = NULL after copying the first
frame. If the CRTC is later disabled (e.g., via device unplug or KMS disable),
gm12u320_stop_fb_update() reads this NULL pointer into old_fb and passes it to
drm_framebuffer_put(old_fb):

static void gm12u320_stop_fb_update(struct gm12u320_device *gm12u320)
{
	struct drm_framebuffer *old_fb;
	...
	old_fb = gm12u320->fb_update.fb;
	...
	drm_framebuffer_put(old_fb);
}

Because drm_framebuffer_put() computes the address of &fb->base before passing
it to drm_mode_object_put(), passing a NULL fb results in a non-NULL offset
pointer. This bypasses any NULL checks in the core and causes a fatal page
fault when dereferenced.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-drm-simple-kms-removal-v3-0-cd5dc89858c6@oss.qualcomm.com?part=5