On Thu Jul 16, 2026 at 5:26 PM CST, sashiko-bot wrote:
> 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 <[email protected]>
>
> 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.
>
As replied in patch 5, I'll switch to drm_atomic_get_new_plane_state()
>> 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.
I think it's a false positive.
>
> [ ... ]
>
>> /* 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.
>
ditto
> [ ... ]
>
>> @@ -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.
>
I don't think this is an issue here.
In drm_atomic_helper_commit_planes(), atomic_flush() is invoked after
atomic_disable(), and it is only skipped for inactive CRTCs when
active_only is set [1]:
if (active_only && !new_crtc_state->active)
continue;
funcs->atomic_flush(crtc, state);
However, in the default helper path active_only is not set [2], so
atomic_flush() should still be called in this case. Because of that, I
don't think an extra event handling path in atomic_disable() is
required here.
[1]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L3055
[2]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L2968
>> - 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.
I think this is a false positive.
