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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Early return in atomic_disable due to drm_dev_enter skips hardware teardown and timer cancellation.
- [Critical] Permanent physical memory leak due to unbalanced page pinning in spacemit_dmmu_map.
- [High] Sleeping functions called from atomic context in CRTC atomic_enable and atomic_disable callbacks.
- [High] Plane atomic_update callback invokes sleeping GEM helpers.
- [High] Interrupt storm due to unhandled IRQ during device bind.
- [High] Race condition between IRQ synchronization and clock gating.
- [High] Memory leak of DRM device on component unbind.
- [High] Unconditional interrupt enable via spin_unlock_irq in atomic context.
- [High] Ignored return value of get_cl_rdma_buf leads to Use-After-Free of command list registers.
--

--- Patch [10]: [PATCH v2 10/17] drm/spacemit: add Saturn DPU KMS pipeline ---
commit f9572a94b08eefdd84a13496054fbc00b42837b7
Author: Cody Kang <codykang.hk@gmail.com>
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; atomic commit builds the cmdlist batches, maps the
framebuffer through the DMMU and arms the cfg-ready handshake, with
vblank events driven from the DPU interrupt.

> diff --git a/drivers/gpu/drm/spacemit/spacemit_crtc.c b/drivers/gpu/drm/spacemit/spacemit_crtc.c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_crtc.c

[ ... ]

> @@ -96,25 +96,25 @@ static void spacemit_crtc_atomic_enable(struct drm_crtc *crtc,
> 					struct drm_atomic_commit *old_state)
> {

[ ... ]

> 	if (unlikely(a_crtc->first_modeset)) {
> 		ret = reset_control_bulk_assert(ARRAY_SIZE(priv->pipe_resets),
> 						priv->pipe_resets);
> 		if (ret)
> 			dev_warn(a_crtc->dev,
> 				 "failed to assert resets: %d\n", ret);
> 		usleep_range(10000, 20000);
> 		a_crtc->first_modeset = false;
> 	}
> 
> 	if (!a_crtc->clocks_on) {
> 		dpu_pm_resume(a_crtc->dev);
> 		a_crtc->clocks_on = true;
> 
> 		/*
> 		 * The DPU must settle after reset deassert or CFG_RDY races
> 		 * the flush.
> 		 */
> 		usleep_range(10000, 20000);
> 	}

[Severity: High]
Could the usleep_range() calls cause system instability?

The DRM subsystem requires CRTC atomic_enable and atomic_disable callbacks to
execute in atomic context, where sleeping is strictly forbidden.

Because usleep_range() sleeps, calling it directly here violates the atomic
context constraints.

[ ... ]

> @@ -131,23 +129,20 @@ static void spacemit_crtc_atomic_disable(struct drm_crtc *crtc,
> 					 struct drm_atomic_commit *old_state)
> {
> 	struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc);
> 	struct drm_device *drm = a_crtc->crtc.dev;
> 	int idx;
> 
> 	if (!drm_dev_enter(drm, &idx))
> 		return;

[Severity: Critical]
Does checking drm_dev_enter() here inadvertently skip hardware teardown
during unbind?

During driver unbind, spacemit_drm_master_unbind() calls drm_dev_unregister(),
which sets the unplugged flag. It then calls drm_atomic_helper_shutdown(),
which invokes spacemit_crtc_atomic_disable().

Because the device is already marked unplugged, drm_dev_enter() evaluates to
false. This causes an immediate return, bypassing the timer cancellation
(which leaves a pending timer pointing to freed memory) and the VBLANK event
signaling (which can stall the teardown commit).

[ ... ]

> 	/*
> 	 * MMIO on a clock-gated DPU stalls the bus, so an in-flight handler
> 	 * 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]
Is it safe to call synchronize_irq() in atomic context?

This function can sleep, which violates the requirement that atomic_disable
callbacks execute in atomic context without sleeping.

> 	/*
> 	 * 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]
Might this sequence allow a new interrupt to slip in?

If synchronize_irq() completes, it only guarantees that in-flight handlers
have finished. Because a_crtc->clocks_on is still true when it returns, a
newly triggered interrupt could immediately enter the ISR.

Meanwhile, this thread continues, setting a_crtc->clocks_on to false and
calling dpu_pm_suspend() to gate the clocks. If the racing ISR attempts
MMIO reads on the clock-gated hardware, it could stall the system bus.

Additionally, does dpu_pm_suspend() sleep in this atomic context? It
invokes usleep_range() internally, which would violate the atomic_disable
constraints.

> 	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]
Will this unconditionally re-enable local interrupts?

Because spacemit_crtc_atomic_disable() executes in atomic context, the caller
might have already disabled interrupts. Calling spin_unlock_irq() re-enables
them unconditionally.

Would it be better to use spin_unlock_irqrestore() to preserve the original
CPU interrupt state?

[ ... ]

> @@ -467,20 +465,19 @@ 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))
> 		return PTR_ERR(kms);

[Severity: High]
Will this leak the DRM device structure on component unbind?

The KMS structure is allocated with devm_drm_dev_alloc() inside
spacemit_drm_master_bind(). The devm_* lifecycle is tied to the platform
device's probe and remove cycle, not the component bind and unbind cycle.

When spacemit_drm_master_unbind() is called, the DRM device is unregistered
but never freed because the platform device is not removed. A subsequent
re-bind would allocate a new instance, permanently leaking the old one along
with all drmm_* managed allocations.

[ ... ]

> 	enable_irq(priv->irq_online);
> 
> 	ret = drm_dev_register(drm, 0);

[Severity: High]
Could enabling the IRQ here lead to an interrupt storm?

At this point during bind, the hardware might not be fully reset and
a_crtc->clocks_on is still false. If a bootloader left the display pipeline
active, the hardware will continuously assert the interrupt.

When the ISR fires, it sees !a_crtc->clocks_on and immediately returns
IRQ_NONE without clearing the hardware interrupt status. This causes the
interrupt to fire repeatedly, potentially hanging the system.

> diff --git a/drivers/gpu/drm/spacemit/spacemit_planes.c b/drivers/gpu/drm/spacemit/spacemit_planes.c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_planes.c

[ ... ]

> @@ -170,25 +170,24 @@ static void spacemit_plane_atomic_update(struct drm_plane *plane,
> 					 struct drm_atomic_commit *state)
> {

[ ... ]

> 	hwdev->get_cl_rdma_buf(a_crtc);

[Severity: High]
What happens if hwdev->get_cl_rdma_buf() fails here?

If the return value is ignored and it fails to allocate, a_crtc->cl_rdma could
remain uninitialized or retain a dangling pointer from a previous iteration.

Later, free_cmdlist_regs(a_crtc->cl_rdma) frees the pointer but does not set
it to NULL. If a subsequent plane update fails its allocation, it would
double-free the old pointer.

> 	hwdev->plane_update_hw_channel(plane);
> 
> 	/* Solid-fill layers have no RDMA and no page table */
> 	if (rdma_id < hwdev->rdma_nums && spacemit_pstate->mmu_tbl.va) {
> 		u8 tbu_id = rdma_id * 2;
> 
> 		ret = spacemit_dmmu_map(plane->state->fb,
> 					&spacemit_pstate->mmu_tbl, tbu_id,
> 					a_crtc->cl_rdma, plane);

[Severity: High]
Does this path invoke sleeping functions from atomic context?

The spacemit_plane_atomic_update() callback executes in atomic context.
However, it calls spacemit_dmmu_map(), which subsequently calls
drm_gem_shmem_get_pages_sgt().

The drm_gem_shmem_get_pages_sgt() helper acquires the pages_lock mutex and
can block for disk I/O or GFP_KERNEL allocations, which is unsafe in
atomic context. Should this mapping be deferred to the prepare_fb phase?

[Severity: Critical]
Is there a missing unpin for the pages acquired inside the mapping function?

Calling drm_gem_shmem_get_pages_sgt() retrieves the scatter-gather table
and implicitly pins the backing physical pages by incrementing the object's
pages_use_count.

Since this occurs on every plane atomic update, and there appears to be no
matching drm_gem_shmem_put_pages() call during cleanup, the pages_use_count
will continually inflate. This prevents the GEM SHMEM core from releasing the
physical pages when the framebuffer is destroyed, leading to a permanent
memory leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-k3-display-v2-0-327d7910bf71@gmail.com?part=10