Re: [PATCH 4/9] drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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 <ze.huang@oss.qualcomm.com>

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-b4e1ca053623@oss.qualcomm.com?part=4