Re: [PATCH v2 09/17] drm/spacemit: add Saturn DPU hardware backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] Dynamic memory allocation using GFP_KERNEL inside plane atomic update, disable, and CRTC flush paths.
- [High] If cmdlist memory allocation fails, the driver falls back to direct MMIO writes during atomic updates, breaking transaction semantics.
- [High] Unsynchronized lockless access to `flip_done` between ISR and timer handler allows double `drm_crtc_vblank_put()`.
- [High] Dereferencing `state->fb` without checking for NULL causes a crash when configuring pixel blending for solid fill planes.
- [High] Potential NULL pointer dereference of `state->fb` in `saturn_cal_layer_fbcmem_size`.
- [High] The `dpu_init` function uses the sleeping function `usleep_range()` while executing from an atomic commit path.
- [High] The `cfg_rdy_timer` is not synchronously cancelled during device teardown, leading to a potential use-after-free.
- [High] The VBLANK ISR and timer handler directly access `crtc->state->event`, causing a race condition with concurrent atomic commits.
- [High] Silently dropped cmdlist rows cause the hardware DMA to read out-of-bounds memory.
- [Medium] Hardware clocks are disabled in the same forward order they were enabled, violating clock dependencies.
- [Medium] C bitfields are used for defining hardware DMA descriptor layouts.
- [Medium] Unchecked `drm_crtc_vblank_get` return value leads to refcount underflow.
- [Medium] Double-clearing of underrun interrupt status due to stale irq_raw snapshot.
- [Medium] Useless and contextually incorrect read memory barrier (dma_rmb).
--

--- Patch [9]: [PATCH v2 09/17] drm/spacemit: add Saturn DPU hardware backend ---
commit 203a676effd210733651accaca4eebc02e32d9c2
Author: Cody Kang <codykang.hk@gmail.com>
Subject: drm/spacemit: add Saturn DPU hardware backend

This commit adds the register-level programming of the Saturn instance,
including clock and reset bring-up, scene control, and timing generator setup.
The backend is exposed as an ops table for the KMS layer.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn.c
[ ... ]
> +static int dpu_disable_clocks(struct spacemit_crtc *a_crtc)
> +{
> +	struct dpu_clk_context *clk_ctx = &a_crtc->clk_ctx;
> +
> +	trace_dpu_disable_clocks(a_crtc->dev_id);
> +
> +	clk_disable_unprepare(clk_ctx->pxclk);
> +	clk_disable_unprepare(clk_ctx->mclk);
> +	clk_disable_unprepare(clk_ctx->escclk);
> +
> +	clk_disable_unprepare(clk_ctx->aclk);
> +	clk_disable_unprepare(clk_ctx->dscclk);

[Severity: Medium]
Providers should usually be disabled after their consumers, but this matches
the exact forward order they were enabled in dpu_enable_clocks().

Are these clocks being disabled in the correct order?

[ ... ]
> +static int dpu_init(struct spacemit_crtc *a_crtc)
> +{
> +	unsigned int timeout = 1000;
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +
> +	if (!a_crtc->clocks_on)
> +		return 0;
> +
> +	while (timeout) {
> +		if (hwdev->get_cfg_rdy(a_crtc, hwdev) == 0)
> +			break;
> +		usleep_range(100, 200);

[Severity: High]
dpu_init() appears to be called from the CRTC's atomic enable sequence,
where sleeping is strictly forbidden and can cause kernel warnings or
deadlocks.

Is it safe to call a sleeping function like usleep_range() here?

[ ... ]
> +static void dpu_uninit(struct spacemit_crtc *a_crtc)
> +{
> +	struct spacemit_drm_private *priv = a_crtc->crtc.dev->dev_private;
> +	struct spacemit_hw_device *hwdev = priv->hwdev;
> +
> +	if (!a_crtc->clocks_on)
> +		return;
> +
> +	trace_dpu_uninit(a_crtc->dev_id);
> +	hwdev->irq_enable(a_crtc, false);
> +}

[Severity: High]
If the driver unbinds while the timer is pending, the timer callback could
execute on freed spacemit_crtc memory, leading to a use-after-free.

Should there be a synchronous cancellation of cfg_rdy_timer here?

[ ... ]
> +static inline void dpu_isr_vblank(struct spacemit_crtc *a_crtc, bool *flip)
> +{
> +	struct drm_crtc *crtc = &a_crtc->crtc;
> +	struct drm_device *drm = crtc->dev;
> +	struct drm_pending_vblank_event *event;
> +
> +	drm_crtc_handle_vblank(crtc);
> +
> +	if (*flip)
> +		return;
> +	*flip = true;
> +
> +	/*
> +	 * event_lock serialises this against the commit arming
> +	 * crtc->state->event.
> +	 */
> +	spin_lock(&drm->event_lock);
> +	event = crtc->state->event;
> +	if (event) {

[Severity: High]
The crtc->state can be swapped asynchronously by
drm_atomic_helper_swap_state(), which could cause this handler to point to a
new event and lose the old one if multiple commits are queued rapidly.

Is it safe to directly dereference crtc->state->event dynamically from an
asynchronous context?

[ ... ]
> +static u32 dpu_online_isr(struct spacemit_crtc *a_crtc)
> +{
[ ... ]
> +	irq_raw = hwdev->get_int_sts(hwdev, dev_id);
> +	trace_dpu_isr_status("ONLINE", irq_raw);
> +	irq_bit = hwdev->get_irq_bit(INT_UNDERRUN, dev_id);
> +	irq_ur_bit = irq_raw & irq_bit;
> +	if (irq_ur_bit && !a_crtc->ur_reported) {
> +		hwdev->clr_int_sts(a_crtc, irq_ur_bit, dev_id);
> +		trace_dpu_isr_status("underrun", irq_ur_bit);
> +		drm_err_ratelimited(a_crtc->crtc.dev, "underrun\n");
> +		a_crtc->ur_reported = true;
> +	}
> +	irq_bit = hwdev->get_irq_bit(INT_CFG_RDY, dev_id);
> +	if (irq_raw & irq_bit) {
> +		hwdev->clr_int_sts(a_crtc, irq_bit, dev_id);
> +		trace_dpu_isr_status("cfg_rdy_clr", irq_raw & irq_bit);
> +		/*
> +		 * A fired watchdog already sent the event and put this flip's
> +		 * vblank reference; re-arming the vsync path would put twice.
> +		 */
> +		if (timer_delete(&a_crtc->cfg_rdy_timer))
> +			a_crtc->flip_done = false;

[Severity: High]
If the timer executes on another CPU, timer_delete() returns 0 and both the
ISR and the timer handler could execute lockless modifications to flip_done
and call drm_crtc_vblank_put() twice.

Could this non-synchronous timer cancellation leave the callback running
concurrently?

[ ... ]
> +	irq_bit = hwdev->get_irq_bit(INT_REST, dev_id) | irq_ur_bit;
> +	if (irq_raw & irq_bit)
> +		hwdev->clr_int_sts(a_crtc, irq_bit, dev_id);

[Severity: Medium]
The code takes a single snapshot of irq_raw at the start of the ISR, clears
irq_ur_bit if present, and then clears it again here using the stale
snapshot. A new underrun firing in that window could be cleared without
being processed.

Does double-clearing the underrun interrupt status risk losing interrupts?

[ ... ]
> +static void dpu_run(struct drm_crtc *crtc,
> +		    struct drm_crtc_state *old_state)
> +{
[ ... ]
> +	/* flush all config register writes before CFG_RDY is raised */
> +	mb();
> +
> +	drm_crtc_vblank_get(crtc);

[Severity: Medium]
If getting the vblank fails, the reference is not taken, but the ISR still
unconditionally calls drm_crtc_vblank_put(), which can lead to a vblank
reference count underflow.

Should the return value of drm_crtc_vblank_get() be checked?

> +	if (hwdev->enable_cfg_irq)
> +		hwdev->enable_cfg_irq(a_crtc, hwdev, true);
> +
> +	saturn_ctrl_cfg_ready(a_crtc, true);
> +
> +	if (unlikely(a_crtc->is_1st_f)) {
> +		drm_dbg(a_crtc->crtc.dev, "DPU start\n");
> +		a_crtc->is_1st_f = false;
> +		saturn_ctrl_sw_start(a_crtc, true);
> +	}
> +	dma_rmb();
> +}

[Severity: Medium]
A read memory barrier strictly enforces ordering between CPU reads, but the
preceding operation writes to DMA control registers to start the engine.

Is this read memory barrier required here? It seems semantically out of
place.

> diff --git a/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/dpu_saturn_hee.c
[ ... ]
> +void saturn_hee_plane_update_hw_channel(struct drm_plane *plane)
> +{
[ ... ]
> +	if (state->fb->format && state->fb->format->has_alpha &&
> +	    pixel_alpha != DRM_MODE_BLEND_PIXEL_NONE) {

[Severity: High]
For solid fill planes, state->fb is typically NULL. Evaluating
state->fb->format without a NULL check could cause a kernel oops when
userspace submits an atomic commit for a solid-color plane.

Can state->fb be NULL here?

[ ... ]
> +	cl_cmp = alloc_cmdlist_regs(CMPS_X_REG);
> +
> +	base = CMP_BASE_ADDR[a_crtc->dev_id];
> +	dpu_write(hwdev, CMPS_X_REG, base, dst_w, mode->hdisplay, cl_cmp);
> +	dpu_write(hwdev, CMPS_X_REG, base, dst_h, mode->vdisplay, cl_cmp);
> +
> +	SATURN_WRITE_COMPOSER_LAYER(00);

[Severity: High]
The alloc_cmdlist_regs macro internally uses kzalloc with GFP_KERNEL.

Can this allocation sleep in an atomic context and trigger deadlock warnings,
since it is called from the plane atomic update callback?

Also, if this allocation fails and returns NULL, cl_cmp is passed directly to
SATURN_WRITE_COMPOSER_LAYER and dpu_write. The dpu_write logic falls back to
direct MMIO writes when the command list is NULL.

Does this bypass command list synchronization to VBLANK and risk applying
partial state immediately?

[ ... ]
> +struct cmdlist_header {
> +	u64 next_list_addr : 39;
> +	/* reserved */
> +	u32: 1;
> +	u32 nod_len : 16;
> +	/* 1: the last cmdlist node, 2: pending node, otherwise 0 */
> +	u32 nod_type : 2;
> +	u32 next_nod_secu : 1;
> +	u64 wait_event_low    : 5;
> +	u64 wait_event    : 59;
> +	/* reserved */
> +	u32: 5;
> +};

[Severity: Medium]
Bitfield packing and ordering are heavily compiler and architecture dependent,
which might produce incorrect structures in memory for the hardware DMA to
read.

Is it safe to use C bitfields for hardware DMA descriptor layouts?

[ ... ]
> +void saturn_hee_cmdlist_fill_data_row(struct cmdlist *cl, u32 strobe,
> +				      u32 offset, u32 value[])
> +{
> +	struct cmdlist_row *row;
> +	u8 i;
> +
> +	/*
> +	 * packing counts rows blindly; drop the row rather than the buffer end
> +	 */
> +	if (WARN_ONCE(CL_HEADER_SZ + (cl->nod_len + 1) * CL_ROW_SZ > cl->size,
> +		      "cmdlist buffer full (rows %u)\n", cl->nod_len))
> +		return;

[Severity: High]
Since this function silently drops the row and returns void, the caller
cmdlist_regs_packing() will blindly increment cl->nod_len regardless of
buffer truncation.

Could this inflated length cause the DMA engine to read past the end of the
initialized buffer?

> diff --git a/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/dpu/saturn_fbcmem.c
[ ... ]
> +int saturn_cal_layer_fbcmem_size(struct drm_plane *plane,
> +				 struct drm_plane_state *state)
> +{
> +	struct spacemit_plane_state *pstate = to_spacemit_plane_state(state);
> +	u32 drm_4cc_fmt = pstate->state.fb->format->format;

[Severity: High]
If a plane is being disabled or is in a solid fill mode, the framebuffer
pointer will be NULL, which would cause an immediate crash during the atomic
check phase.

Is it guaranteed that pstate->state.fb is non-NULL here?

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