Re: [PATCH RESEND 10/17] drm/spacemit: add Saturn DPU KMS pipeline

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

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy