On Sat, 25 Jul 2026 05:04:26 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential
> issue(s) to consider:
> - [High] Sleeping functions are called from CRTC atomic enable and disable
> callbacks, which execute in atomic context.
> - [High] TOCTOU race when updating the `clocks_on` flag and gating hardware
> clocks leads to MMIO stalls.
> - [High] Calling `devm_drm_dev_alloc` inside a component `bind` callback
> leaks the DRM device on rebinds.
> - [High] Enabling the DPU interrupt before hardware is initialized leads to
> an unhandled IRQ storm.
> - [High] `spacemit_plane_reset` bypasses the custom state destructor, leaking
> DMA coherent memory.
> - [High] `disable_irq()` is called before `drm_atomic_helper_shutdown()`
> during driver unbind, causing shutdown to hang.
> - [High] `spacemit_plane_atomic_check()` assigns RDMA channels based on
> unvalidated `zpos`, leading to channel collisions.
> - [High] Unconditional `spin_unlock_irq` used in atomic context improperly
> re-enables interrupts.
> - [Medium] Leaking DRM property blob references in
> `spacemit_crtc_duplicate_state` on error.
> - [Medium] Missing NULL check on `plane->state` in
> `spacemit_plane_atomic_duplicate_state` causes a crash if the state is
> missing.
> --
>
> --- Patch [10]: [PATCH RESEND 10/17] drm/spacemit: add Saturn DPU KMS
> pipeline ---
> commit cf6665928c2c5fcf07a88c5cd2141feb18245e17
> Author: Cody Kang <[email protected]>
> Subject: drm/spacemit: add Saturn DPU KMS pipeline
>
> Add the atomic KMS implementation on top of the hardware backend: one
> CRTC and one primary plane per DPU instance. atomic_check validates
> the plane rectangle 1:1 against the mode and sizes the per-channel FBC
> line buffer.
>
> > diff --git a/drivers/gpu/drm/spacemit/spacemit_crtc.c
> > b/drivers/gpu/drm/spacemit/spacemit_crtc.c
> > new file mode 100644
> > index 0000000000000..b75ff63205011
> > --- /dev/null
> > +++ b/drivers/gpu/drm/spacemit/spacemit_crtc.c
>
> [ ... ]
>
> > @@ -91,7 +91,7 @@ static void spacemit_crtc_atomic_enable(struct drm_crtc
> > *crtc,
> > dpu_reset_assert(a_crtc->dev, "lcd", a_crtc->lcd_reset);
> > dpu_reset_assert(a_crtc->dev, "esc", a_crtc->esc_reset);
> > dpu_reset_assert(a_crtc->dev, "mclk", a_crtc->mclk_reset);
> > dpu_reset_assert(a_crtc->dev, "aclk", a_crtc->aclk_reset);
> > dpu_reset_assert(a_crtc->dev, "dsc", a_crtc->dsc_reset);
> > usleep_range(10000, 20000);
>
> [Severity: High]
> Does this sleep in an atomic context? The spacemit_crtc_atomic_enable
> callback is executed during a non-blocking DRM atomic commit, which runs
> in an atomic context. Calling usleep_range() here might violate atomic
> context rules and lead to scheduling-while-atomic warnings.
atomic_enable runs from the commit tail worker, which is process
context; sleeping is allowed.
> [ ... ]
>
> > @@ -145,13 +145,13 @@ static void spacemit_crtc_atomic_disable(struct
> > drm_crtc *crtc,
> > * must finish its status reads before dpu_pm_suspend() gates the
> > * clocks.
> > */
> > if (a_crtc->irq_online > 0)
> > synchronize_irq(a_crtc->irq_online);
>
> [Severity: High]
> Can synchronize_irq() safely execute here? This callback runs in an atomic
> context, but synchronize_irq() blocks waiting for executing handlers.
atomic_disable also runs in the commit tail (process context);
synchronize_irq() is legal there.
> [Severity: High]
> Does this code leave a race window before the flags are updated? An interrupt
> could fire immediately after synchronize_irq() returns but before clocks_on
> is toggled to false, leading the ISR to access MMIO while dpu_pm_suspend()
> is shutting down the clocks.
The DPU cannot raise its interrupt while its own clocks are off, so
the window is not exploitable; still, will move the flag update
after dpu_pm_resume() in v2.
> >
> > /*
> > * Nothing else cancels the watchdog; it would fire against freed state.
> > */
> > timer_delete_sync(&a_crtc->cfg_rdy_timer);
> >
> > /* Close the ISR's MMIO gate before the clocks actually stop. */
> > a_crtc->clocks_on = false;
> > dpu_pm_suspend(a_crtc->dev);
>
> [Severity: High]
> Does dpu_pm_suspend() sleep in this atomic context? It calls usleep_range()
> which could trigger kernel warnings and deadlocks when reached via
> spacemit_crtc_atomic_disable().
Process context, as above.
> >
> > spin_lock_irq(&drm->event_lock);
> > if (crtc->state->event) {
> > drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > crtc->state->event = NULL;
> > }
> > spin_unlock_irq(&drm->event_lock);
>
> [Severity: High]
> Is it safe to use unconditional spin_unlock_irq() here? Since this callback
> executes in an atomic context (e.g. from drm_atomic_helper_commit_tail),
> unconditionally re-enabling interrupts breaks the caller's atomic assumptions.
> Should this use spin_lock_irqsave() and spin_unlock_irqrestore() instead?
The callback is not entered with interrupts disabled (commit tail);
the event_lock pattern here matches the drm helpers' own usage.
> [ ... ]
>
> > @@ -231,6 +231,5 @@ static struct drm_crtc_state *
> > __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
> >
> > n_rdma = hwdev->rdma_nums;
> > state->rdmas = kzalloc_objs(*state->rdmas, n_rdma);
> > if (!state->rdmas) {
> > kfree(state);
> > return NULL;
> > }
>
> [Severity: Medium]
> Does this error path leak DRM property blob references?
> __drm_atomic_helper_crtc_duplicate_state() increments references on blobs
> like mode_blob, ctm, and gamma_lut. Returning NULL here without releasing
> them could leak those references.
Real; will fix in v2 with __drm_atomic_helper_crtc_destroy_state()
on that error path.
> [ ... ]
>
> > @@ -418,6 +418,5 @@ static irqreturn_t spacemit_dpu_isr(int irq, void *data)
> > /* Reading a clock-gated DPU's status registers stalls the bus. */
> > if (unlikely(!a_crtc->clocks_on)) {
> > drm_dev_exit(idx);
> > return IRQ_NONE;
> > }
>
> [Severity: High]
> Does this code cause an IRQ storm? By returning IRQ_NONE without clearing
> the hardware interrupt status when clocks_on is false, the interrupt
> controller might continuously re-trigger the ISR if the bootloader left the
> interrupt asserted.
Only reachable in the unbind window where the line is being torn
down; otherwise the ISR acks all status it observes.
> [ ... ]
>
> > @@ -496,6 +496,5 @@ static int spacemit_drm_master_bind(struct device *dev)
> > }
> >
> > kms = devm_drm_dev_alloc(dev, &spacemit_drm_drv,
> > struct spacemit_kms, drm);
> > if (IS_ERR(kms))
>
> [Severity: High]
> Will this leak the DRM device on rebind? devm_drm_dev_alloc ties the
> allocation lifetime to the platform device's probe/remove cycle. If the
> component unbinds and rebinds, new DRM devices are allocated but the old
> ones are never freed.
A known trade-off of devm on the component master: the device is
unregistered at unbind and the memory is bounded by the platform
device lifetime.
> [ ... ]
>
> > @@ -573,6 +573,5 @@ static int spacemit_drm_master_bind(struct device *dev)
> > drm_mode_config_reset(drm);
> > drmm_kms_helper_poll_init(drm);
> > drm_helper_hpd_irq_event(drm);
> >
> > enable_irq(priv->irq_online);
>
> [Severity: High]
> Is it safe to enable the IRQ before the hardware state is fully initialized?
> If the bootloader left the display hardware active, the ISR might fire
> immediately while a_crtc->clocks_on is still false, leading to the IRQ storm
> mentioned above.
The ISR is gated on clocks_on and drm_dev_enter, and every board
cycle boots through this path; no storm has been observed.
> [ ... ]
>
> > @@ -594,6 +594,5 @@ static void spacemit_drm_master_unbind(struct device
> > *dev)
> >
> > disable_irq(priv->irq_online);
> >
> > drm_dev_unregister(drm);
> > drm_atomic_helper_shutdown(drm);
> > component_unbind_all(dev, drm);
>
> [Severity: High]
> Will calling drm_atomic_helper_shutdown() hang if the IRQ is already
> disabled? The shutdown helper flushes pending atomic commits and disables
> CRTCs, which requires the hardware vblank interrupt to signal completion.
> Without it, the commit wait could time out.
Confirmed on hardware: the fbdev-restore commit arms the CFG_RDY
handshake after the line is dead, and every unload rode two watchdog
timeouts. Will reorder in v2: unregister and shutdown before
disable_irq().
> > diff --git a/drivers/gpu/drm/spacemit/spacemit_planes.c
> > b/drivers/gpu/drm/spacemit/spacemit_planes.c
> > new file mode 100644
> > index 0000000000000..3442013b0c37f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/spacemit/spacemit_planes.c
>
> [ ... ]
>
> > @@ -98,6 +98,5 @@ static int spacemit_plane_atomic_check(struct drm_plane
> > *plane,
> > if (src_w == 0 && src_h == 0)
> > cur_rdma_id = RDMA_INVALID_ID; /* constant-fill layer, no RDMA
> > */
> > else if (cur_rdma_id == RDMA_INVALID_ID)
> > cur_rdma_id = state->zpos; /* first commit: bind channel
> > by zpos */
> > cur_state->rdma_id = cur_rdma_id;
>
> [Severity: High]
> Does this code assign RDMA channels based on unvalidated zpos? Userspace
> could submit a configuration with duplicate zpos values across multiple
> planes, resulting in channel collisions where planes overwrite each other's
> hardware registers during spacemit_plane_atomic_update().
There is a single primary plane per CRTC, so no collision is
possible; rdma_id is also bounds-checked in atomic_check.
> [ ... ]
>
> > @@ -193,7 +193,6 @@ static void spacemit_plane_reset(struct drm_plane
> > *plane)
> > if (plane->state) {
> > s = to_spacemit_plane_state(plane->state);
> > __drm_atomic_helper_plane_destroy_state(plane->state);
> > kfree(s);
> > plane->state = NULL;
> > }
>
> [Severity: High]
> Does freeing the state directly bypass the driver's custom destructor?
> Calling kfree(s) directly in spacemit_plane_reset() skips
> spacemit_plane_atomic_destroy_state(), which means the DMA coherent buffers
> for mmu_tbl.va and cl.va could be permanently leaked when a plane is reset.
Real; will route .reset through the custom destroy in v2, the same
way the CRTC side already does.
> [ ... ]
>
> > @@ -211,6 +211,5 @@ static struct drm_plane_state *
> > spacemit_plane_atomic_duplicate_state(struct drm_plane *plane)
> > {
> > struct spacemit_plane_state *s;
> > struct spacemit_plane_state *old_state =
> > to_spacemit_plane_state(plane->state);
> > struct spacemit_crtc *a_crtc = NULL;
>
> [Severity: Medium]
> Will this crash if plane->state is NULL? The to_spacemit_plane_state()
> macro uses container_of, which will produce a negative pointer if
> plane->state is NULL. Dereferencing old_state->rdma_id later would trigger
> a fault.
The core only calls duplicate_state with an existing state; this is
the same contract the helpers themselves rely on.
Cody
