Re: [PATCH v2 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Pending vblank events are leaked during CRTC disable. The DRM subsystem requires that pending vblank events requested by userspace (e.g., during a pageflip) are consumed and sent when the CRTC is disabled. In this patch, event consumption is implemented in `pl111_crtc_helper_atomic_flush()`. However, the DRM core (`drm_atomic_helper_commit_planes`) only calls `atomic_flush` for active CRTCs. When a CRTC is being disabled, `atomic_flush` is skipped, and the event must be consumed in the `atomic_disable` hook. `pl111_crtc_helper_atomic_disable()` fails to check for or consume `cstate->event`. As a result, the event is leaked.
- [High] Unsafe access to global plane state in `pl111_crtc_helper_atomic_enable()`. The function retrieves the framebuffer using `priv->plane.state->fb` directly instead of using the committed atomic state via `drm_atomic_get_new_plane_state()`. In the DRM core, non-blocking atomic commits synchronously swap the global state pointers before the asynchronous commit workers run. This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability.
- [High] Sleeping functions are called from atomic context. The DRM subsystem guide explicitly defines CRTC `atomic_enable` and `atomic_disable` callbacks as executing in an atomic context during non-blocking commits, where sleeping is strictly forbidden. However, `pl111_crtc_helper_atomic_enable()` and `pl111_crtc_helper_atomic_disable()` call blocking functions: `clk_prepare_enable()`, `clk_disable_unprepare()`, and `msleep(20)`.
--

--- Patch [4]: [PATCH v2 4/8] drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit 07a755e3243c0f70ec87dc33910c7d68aa70590c
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/pl111: replace struct drm_simple_display_pipe with regular atomic helpers
    
Replace PL111 simple display pipe with explicit plane, CRTC and encoder
objects. Keep the existing PL111 timing, format and pitch validation, but
move it into the explicit atomic check paths. Move event handling to the CRTC
commit path.

> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index 5d10bc5fdf1f6..beb6423d4d439 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c

[ ... ]

> @@ -117,16 +137,14 @@ static int pl111_display_check(struct drm_simple_display_pipe *pipe,
> -static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
> -				 struct drm_crtc_state *cstate,
> -				 struct drm_plane_state *plane_state)
> +static void pl111_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 pl111_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_framebuffer *fb = priv->plane.state->fb;

[Severity: High]
This is a pre-existing issue, but does this direct access to the global plane
state in pl111_crtc_helper_atomic_enable() introduce a time-of-check to
time-of-use vulnerability?

By retrieving the framebuffer using priv->plane.state->fb directly instead
of fetching the committed atomic state via drm_atomic_get_new_plane_state(),
this might race with subsequent commits. If userspace submits a non-blocking
commit to enable the CRTC, followed immediately by another commit that
disables the primary plane (setting fb to NULL), the global state pointer
updates to the second commit. When the first commit's asynchronous worker
executes this function, it could read the new global state where fb is NULL,
causing a null pointer dereference when accessing fb->format->format.

>  	struct drm_connector *connector = priv->connector;
>  	struct drm_bridge *bridge = priv->bridge;
>  	bool grayscale = false;
>  	u32 cntl;
>  	u32 ppl, hsw, hfp, hbp;
>  	u32 lpp, vsw, vfp, vbp;
>  	u32 cpl, tim2;
>  	int ret;
>  
>  	ret = clk_set_rate(priv->clk, mode->clock * 1000);
>  	if (ret) {
>  		drm_err(drm,
>  			"Failed to set pixel clock rate to %d: %d\n",
>  			mode->clock * 1000, ret);
>  	}
>  
>  	clk_prepare_enable(priv->clk);

[Severity: High]
This is a pre-existing issue, but are blocking functions being called from an
atomic context here?

The DRM subsystem explicitly defines CRTC atomic_enable and atomic_disable
callbacks as executing in an atomic context during non-blocking commits,
where sleeping is strictly forbidden. However, 
pl111_crtc_helper_atomic_enable() calls blocking functions such as 
clk_prepare_enable() and msleep(). This might lead to system instability, 
kernel warnings, or deadlocks.

[ ... ]

>  	/* The PL110 in Integrator/Versatile does the BGR routing externally */
>  	if (priv->variant->external_bgr)
>  		cntl &= ~CNTL_BGR;
>  
>  	/* Power sequence: first enable and chill */
>  	writel(cntl, priv->regs + priv->ctrl);
>  
>  	/*
>  	 * We expect this delay to stabilize the contrast
>  	 * voltage Vee as stipulated by the manual
>  	 */
>  	msleep(20);

[Severity: High]
This is a pre-existing issue, but similar to the clk_prepare_enable() call,
does calling msleep() in pl111_crtc_helper_atomic_enable() violate the DRM
subsystem guidelines for atomic context execution?

Sleeping from this path could cause warnings or deadlocks during non-blocking
commits.

[ ... ]

> @@ -355,9 +373,9 @@ static void pl111_display_enable(struct drm_simple_display_pipe *pipe,
>  		drm_crtc_vblank_on(crtc);
>  }
>  
> -static void pl111_display_disable(struct drm_simple_display_pipe *pipe)
> +static void pl111_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> +					     struct drm_atomic_commit *commit)
>  {

[Severity: High]
This is a pre-existing issue, but does pl111_crtc_helper_atomic_disable() leak
pending vblank events during CRTC disable?

The DRM subsystem requires that pending vblank events requested by userspace
(like during a pageflip) are consumed and sent when the CRTC is disabled. In
this code, event consumption happens in pl111_crtc_helper_atomic_flush().
However, the DRM core skips atomic_flush() for disabled CRTCs, meaning the
event must be consumed in the atomic_disable hook.

Since pl111_crtc_helper_atomic_disable() fails to check for or consume
cstate->event, the event might be leaked. This could trigger a kernel warning
in drm_atomic_helper_commit_hw_done() and cause the userspace compositor to
hang indefinitely.

> -	struct drm_crtc *crtc = &pipe->crtc;
>  	struct drm_device *drm = crtc->dev;
>  	struct pl111_drm_dev_private *priv = drm->dev_private;
>  	u32 cntl;

[ ... ]

>  	/* Power Down */
>  	cntl = readl(priv->regs + priv->ctrl);
>  	if (cntl & CNTL_LCDPWR) {
>  		cntl &= ~CNTL_LCDPWR;
>  		writel(cntl, priv->regs + priv->ctrl);
>  	}
>  
>  	/*
>  	 * We expect this delay to stabilize the contrast voltage Vee as
>  	 * stipulated by the manual
>  	 */
>  	msleep(20);
>  
>  	if (priv->variant_display_disable)
>  		priv->variant_display_disable(drm);
>  
>  	/* Disable */
>  	writel(0, priv->regs + priv->ctrl);
>  
>  	clk_disable_unprepare(priv->clk);
>  }

[Severity: High]
This is a pre-existing issue, but does calling msleep() and
clk_disable_unprepare() in pl111_crtc_helper_atomic_disable() block execution
in an atomic context?

Since this callback runs in an atomic context during non-blocking commits,
these blocking calls might lead to system instability or deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-drm-simple-kms-removal-v2-0-1133a8fc3785@oss.qualcomm.com?part=4