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

On Mon Jul 27, 2026 at 4:13 AM CST, sashiko-bot wrote:
> 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 <[email protected]>
>
> 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).

False positive

>
>> -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.

False positive

>
> 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):

Good catch. If the CRTC is disabled after gm12u320_fb_update_work()
has consumed the pending framebuffer, old_fb will be NULL. The current
disable path then calls drm_framebuffer_put(old_fb) with a NULL pointer.

We can guard it the same way as gm12u320_fb_mark_dirty() [1]:

	if (old_fb)
		drm_framebuffer_put(old_fb);

Maybe we can fix this in a following patch.

[1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/tiny/gm12u320.c#L432

>
> 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.