On Sun Jul 5, 2026 at 2:44 AM CST, sashiko-bot wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) > to consider: > > New issues: > - [Critical] Uninitialized NULL pointer dereference when attaching encoder to > connector. > > Pre-existing issues: > - [High] Atomic callbacks directly access global `obj->state` pointers > instead of using the state passed in the `drm_atomic_commit` parameter. > -- > > commit 098f44a54758ae5b340970e3384cc9330909fb06 > Author: Ze Huang <[email protected]> > > drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers > > Convert MCDE to explicit plane, CRTC and encoder objects. > > Keep FIFO, event and framebuffer update sequencing intact, and install > GEM framebuffer prepare callback explicitly. > >> diff --git a/drivers/gpu/drm/mcde/mcde_display.c >> b/drivers/gpu/drm/mcde/mcde_display.c >> index 257a6e84dd583..4d86fa5030ebf 100644 >> --- a/drivers/gpu/drm/mcde/mcde_display.c >> +++ b/drivers/gpu/drm/mcde/mcde_display.c > > [ ... ] > >> @@ -1149,16 +1171,14 @@ static void mcde_setup_dsi(struct mcde *mcde, const >> struct drm_display_mode *mod >> *dsi_formatter_frame = formatter_frame; >> } >> >> -static void mcde_display_enable(struct drm_simple_display_pipe *pipe, >> - struct drm_crtc_state *cstate, >> - struct drm_plane_state *plane_state) >> +static void mcde_crtc_helper_atomic_enable(struct drm_crtc *crtc, >> + struct drm_atomic_commit *state) >> { >> - struct drm_crtc *crtc = &pipe->crtc; >> - struct drm_plane *plane = &pipe->plane; >> struct drm_device *drm = crtc->dev; >> struct mcde *mcde = to_mcde(drm); >> + struct drm_crtc_state *cstate = crtc->state; > > [Severity: High] > This isn't a bug introduced by this patch, as it was inherited from the > simple-kms implementation, but does this newly added line introduce a data > race? > > In non-blocking commits, the DRM core asynchronously executes the worker > thread while simultaneously swapping the global state pointers. Reading the > global crtc->state here instead of fetching the new state from the state > parameter (e.g. using drm_atomic_get_new_crtc_state()) can race with > subsequent commits, potentially programming the hardware with mismatched > states. > > [ ... ] > >> -static void mcde_display_update(struct drm_simple_display_pipe *pipe, >> - struct drm_plane_state *old_pstate) >> +static void mcde_plane_helper_atomic_update(struct drm_plane *plane, >> + struct drm_atomic_commit *state) >> { >> - struct drm_crtc *crtc = &pipe->crtc; >> - struct drm_device *drm = crtc->dev; >> - struct mcde *mcde = to_mcde(drm); >> - struct drm_pending_vblank_event *event = crtc->state->event; >> - struct drm_plane *plane = &pipe->plane; >> + struct drm_crtc *crtc = plane->state->crtc; >> + struct drm_device *drm; >> + struct mcde *mcde; >> + struct drm_pending_vblank_event *event; >> struct drm_plane_state *pstate = plane->state; >> struct drm_framebuffer *fb = pstate->fb; >> >> + if (!crtc) >> + return; >> + >> + drm = crtc->dev; >> + mcde = to_mcde(drm); >> + event = crtc->state->event; > > [Severity: High] > This is a pre-existing issue, but does accessing crtc->state->event here > continue to expose a time-of-check to time-of-use race? > > If a compositor queues multiple rapid non-blocking commits, reading the event > from the global state might clear the event belonging to a subsequent commit, > leaving the later commit's worker with a NULL event and hanging the compositor > waiting for a page flip. > > [ ... ] > >> @@ -1510,11 +1577,30 @@ int mcde_display_init(struct drm_device *drm) >> if (ret) >> return ret; >> >> - ret = drm_simple_display_pipe_init(drm, &mcde->pipe, >> - &mcde_display_funcs, >> - formats, ARRAY_SIZE(formats), >> - NULL, >> - mcde->connector); >> + ret = drm_universal_plane_init(drm, &mcde->plane, 0, >> + &mcde_plane_funcs, >> + formats, ARRAY_SIZE(formats), >> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); >> + if (ret) >> + return ret; >> + >> + drm_plane_helper_add(&mcde->plane, &mcde_plane_helper_funcs); >> + >> + ret = drm_crtc_init_with_planes(drm, &mcde->crtc, &mcde->plane, >> + NULL, &mcde_crtc_funcs, NULL); >> + if (ret) >> + return ret; >> + >> + drm_crtc_helper_add(&mcde->crtc, &mcde_crtc_helper_funcs); >> + >> + ret = drm_encoder_init(drm, &mcde->encoder, &mcde_encoder_funcs, >> + DRM_MODE_ENCODER_NONE, NULL); >> + if (ret) >> + return ret; >> + >> + mcde->encoder.possible_crtcs = drm_crtc_mask(&mcde->crtc); >> + >> + ret = drm_connector_attach_encoder(mcde->connector, &mcde->encoder); > > [Severity: Critical] > Will this lead to a NULL pointer dereference during probe? > > mcde->connector appears to be a zero-initialized struct member that is not > allocated or populated prior to this call. Because > drm_connector_attach_encoder() unconditionally dereferences the connector > argument, passing it a NULL pointer will lead to an immediate kernel oops.
Will drop drm_connector_attach_encoder() as mcde->connector is not used.
