Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Lockless access to global atomic state pointers in commit path causes TOCTOU races and potential NULL pointer dereferences. - [High] CRTC vblank events are prematurely consumed in the plane's atomic_update callback. -- commit 7172c80142608f5bf7d738f64624b3135c44fdba Author: Ze Huang <[email protected]> Subject: drm/aspeed: replace struct drm_simple_display_pipe with regular atomic helpers This patch replaces the simple display pipe in the aspeed DRM driver with explicit plane, CRTC, and encoder objects. It removes the intermediate layer and wires up the standard atomic helpers for modesetting and page flips. > diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > index 7877a57b8e265..3294795c31c4c 100644 > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c [ ... ] > static int aspeed_gfx_set_pixel_fmt(struct aspeed_gfx *priv, u32 *bpp) > { > - struct drm_crtc *crtc = &priv->pipe.crtc; > + struct drm_crtc *crtc = &priv->crtc; > struct drm_device *drm = crtc->dev; > const u32 format = crtc->primary->state->fb->format->format; > u32 ctrl1; [Severity: High] This is a pre-existing issue, but does this lockless access to the global state pointers in aspeed_gfx_set_pixel_fmt risk a NULL pointer dereference or TOCTOU race? If a concurrent non-blocking atomic commit disables the primary plane while this runs in a worker thread, the global state could be swapped, potentially making the framebuffer pointer NULL. Would it be safer to extract the commit-specific state using the atomic helpers instead of reading directly from crtc->primary->state->fb? [ ... ] > static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv) > { > - struct drm_display_mode *m = &priv->pipe.crtc.state->adjusted_mode; > + struct drm_display_mode *m = &priv->crtc.state->adjusted_mode; > u32 ctrl1, d_offset, t_count, bpp; > int err; [Severity: High] This is also a pre-existing issue carried over during the conversion, but can reading directly from the global crtc.state cause a similar TOCTOU race? Reading the adjusted_mode directly from the global state pointer bypasses the atomic commit's local state. Could this function be updated to use the local state passed to the atomic hooks? [ ... ] > -static void aspeed_gfx_pipe_update(struct drm_simple_display_pipe *pipe, > - struct drm_plane_state *plane_state) > +static void aspeed_gfx_plane_helper_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit > *state) > { > - struct aspeed_gfx *priv = drm_pipe_to_aspeed_gfx(pipe); > - struct drm_crtc *crtc = &pipe->crtc; > - struct drm_framebuffer *fb = pipe->plane.state->fb; > + struct aspeed_gfx *priv = container_of(plane, struct aspeed_gfx, plane); > + struct drm_crtc *crtc = &priv->crtc; > + struct drm_framebuffer *fb = plane->state->fb; > struct drm_pending_vblank_event *event; > struct drm_gem_dma_object *gem; [Severity: High] This is a pre-existing issue, but does consuming the vblank event directly in the plane update phase risk premature signaling? Looking at aspeed_gfx_plane_helper_atomic_update, the driver steals the event from the global state (crtc->state->event) before the CRTC is fully enabled and programmed. Could userspace prematurely free or overwrite the framebuffer while the hardware is still scanning it out due to this early signaling? Would it be more appropriate to handle the event in an atomic_flush hook where standard atomic flush mechanisms can ensure hardware changes are fully complete? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053...@oss.qualcomm.com?part=2
