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/[email protected]?part=10
