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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053...@oss.qualcomm.com?part=4
