On Thu Jul 16, 2026 at 5:25 PM CST, sashiko-bot wrote: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) > to consider: > > New issues: > - [High] Directly accessing `priv->plane.state` in the CRTC atomic enable > callback introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition > with subsequent non-blocking commits. > - [High] Missing `.cleanup_fb` callback in `tve200_plane_helper_funcs` leads > to memory and resource leaks of pinned GEM objects and DMA-buf mappings. > > Pre-existing issues: > - [High] The CRTC atomic enable and disable callbacks call functions that can > sleep (`clk_prepare_enable`, `usleep_range`), violating DRM atomic context > guidelines. > -- > > --- Patch [7]: [PATCH v2 7/8] drm/tve200: replace struct > drm_simple_display_pipe with regular atomic helpers --- > commit 30e007b0736d27ee72ba4a5f39e203c3808e470e > Author: Ze Huang <[email protected]> > drm/tve200: replace struct drm_simple_display_pipe with regular atomic helpers > > Convert TVE200 to explicit plane, CRTC and encoder objects. Keep generic > plane-state validation before TVE200-specific mode, alignment, pitch and > format-change checks, and install GEM framebuffer prepare helper explicitly. > >> diff --git a/drivers/gpu/drm/tve200/tve200_display.c >> b/drivers/gpu/drm/tve200/tve200_display.c >> --- a/drivers/gpu/drm/tve200/tve200_display.c >> +++ b/drivers/gpu/drm/tve200/tve200_display.c > [ ... ] >> @@ -119,16 +140,72 @@ static int tve200_display_check(struct >> drm_simple_display_pipe *pipe, > [ ... ] >> +static const struct drm_plane_helper_funcs tve200_plane_helper_funcs = { >> + .prepare_fb = drm_gem_plane_helper_prepare_fb, >> + .atomic_check = tve200_plane_helper_atomic_check, >> + .atomic_update = tve200_plane_helper_atomic_update, >> +}; > > [Severity: High] > Since .prepare_fb is assigned to drm_gem_plane_helper_prepare_fb, will the > lack of a corresponding .cleanup_fb callback lead to memory and resource > leaks of pinned GEM objects and DMA-buf mappings when the framebuffer is > swapped out?
The documentation for drm_gem_plane_helper_prepare_fb() [1] explicitly says that simple GEM framebuffer drivers do not need a matching cleanup_fb hook. [1] https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_gem_atomic_helper.c#L107 > > [ ... ] >> -static void tve200_display_enable(struct drm_simple_display_pipe *pipe, >> - struct drm_crtc_state *cstate, >> - struct drm_plane_state *plane_state) >> +static void tve200_crtc_helper_atomic_enable(struct drm_crtc *crtc, >> + struct drm_atomic_commit *commit) >> { >> - struct drm_crtc *crtc = &pipe->crtc; >> - struct drm_plane *plane = &pipe->plane; >> struct drm_device *drm = crtc->dev; >> struct tve200_drm_dev_private *priv = drm->dev_private; >> + struct drm_crtc_state *cstate = drm_atomic_get_new_crtc_state(commit, >> crtc); >> const struct drm_display_mode *mode = &cstate->mode; >> - struct drm_framebuffer *fb = plane->state->fb; >> + struct drm_plane_state *plane_state = priv->plane.state; >> + struct drm_framebuffer *fb = plane_state->fb; > > [Severity: High] > Could this direct access to priv->plane.state introduce a race condition > with subsequent non-blocking commits? A fast sequence of commits might swap > the global state before a previous commit's worker executes this function. > > Would it be safer to extract the current commit's plane state using > drm_atomic_get_new_plane_state(commit, &priv->plane) instead to guarantee > it reads the correct snapshot? Will do > > [Severity: High] > This is a pre-existing issue, but does tve200_crtc_helper_atomic_enable() > call sleeping functions in an atomic context? > > Looking further down in this function, it calls clk_prepare_enable() and > usleep_range(). Similarly, tve200_crtc_helper_atomic_disable() calls > clk_disable_unprepare(). Since atomic commit callbacks can run in an > atomic context where sleeping is forbidden, could this cause system > instability or deadlocks? It is a false positive.
