Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Race condition and potential NULL pointer dereference due to direct access to `plane->state` and missing visibility checks in atomic callbacks. Pre-existing issues: - [High] Sleeping in atomic context within CRTC and plane atomic callbacks. -- commit a10b3d5781e3b7143f432c86643ef47e8ddc3a9a 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..b476793d0401e 100644 > --- a/drivers/gpu/drm/tiny/gm12u320.c > +++ b/drivers/gpu/drm/tiny/gm12u320.c [ ... ] > +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 gm12u320_device *gm12u320 = to_gm12u320(crtc->dev); > + struct drm_plane_state *plane_state = crtc->primary->state; [Severity: High] Will this cause a NULL pointer dereference if a subsequent commit disables the plane? Because non-blocking commits execute asynchronously, crtc->primary->state reflects the state of the most recently queued commit, rather than the commit currently executing. If a fast subsequent commit disables the plane, plane_state->fb could be NULL. Should this use drm_atomic_get_new_plane_state(commit, crtc->primary) to get the state tied to this specific commit? > struct drm_shadow_plane_state *shadow_plane_state = > to_drm_shadow_plane_state(plane_state); > > gm12u320->fb_update.draw_status_timeout = FIRST_FRAME_TIMEOUT; > gm12u320_fb_mark_dirty(plane_state->fb, &shadow_plane_state->data[0], > &rect); > } > > -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 isn't a bug introduced by this patch, but does gm12u320_stop_fb_update() sleep in atomic context? gm12u320_stop_fb_update() calls cancel_delayed_work_sync() and mutex_lock(). Since CRTC atomic disable callbacks can run in atomic context during non-blocking commits, sleeping here is forbidden and could cause kernel warnings or deadlocks. > } > > -static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe, > - struct drm_plane_state *old_state) > +static void gm12u320_plane_helper_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit > *commit) > { > - struct drm_plane_state *state = pipe->plane.state; > + struct drm_plane_state *old_state = > drm_atomic_get_old_plane_state(commit, plane); > + struct drm_plane_state *state = plane->state; [Severity: High] Could accessing plane->state directly here lead to a crash? Similar to the CRTC enable function, reading plane->state directly instead of using drm_atomic_get_new_plane_state(commit, plane) accesses the future state. If a subsequent commit disables the plane, state->fb will be NULL. In this scenario, drm_atomic_helper_damage_merged() returns true for plane disablements, leading to gm12u320_fb_mark_dirty() being called with a NULL fb, which will oops when trying to dereference fb->dev. This wasn't possible before because the drm_simple_kms_helper explicitly checked if (!plane->state->visible) before calling the update hook, but that protection is dropped in this conversion. > + struct drm_shadow_plane_state *shadow_plane_state = > to_drm_shadow_plane_state(state); > struct drm_rect rect; > > - if (drm_atomic_helper_damage_merged(old_state, state, &rect)) > + if (drm_atomic_helper_damage_merged(old_state, state, &rect)) > gm12u320_fb_mark_dirty(state->fb, &shadow_plane_state->data[0], > &rect); [Severity: High] This is a pre-existing issue, but does gm12u320_fb_mark_dirty() sleep in atomic context? It calls mutex_lock(&gm12u320->fb_update.lock), which is unsafe to call from a plane update callback during non-blocking commits, as these run in atomic context. > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053...@oss.qualcomm.com?part=6
