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