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 <[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; 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org?part=10
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.