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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] Ignored return value of `get_cl_rdma_buf()` in `spacemit_plane_atomic_update` leads to Use-After-Free and Double-Free.
- [High] Sleeping functions called from CRTC atomic commit callbacks (`atomic_enable` / `atomic_disable`), violating atomic context requirements.
- [High] `spacemit_plane_reset` open-codes state destruction, permanently leaking the plane's DMA allocations.
- [High] Dynamic memory allocation occurs in the `atomic_update` fast path.
- [High] Enabling the DPU IRQ before hardware reset causes an interrupt storm.
- [High] TOCTOU race condition in `spacemit_crtc_atomic_disable` allows MMIO access while clocks are gated.
- [High] Unbounded DMA allocations in `atomic_check` lead to memory leaks on retries and unprivileged Denial of Service.
- [High] Incorrect teardown order in `spacemit_drm_master_unbind` causes VBLANK timeouts.
- [High] DRM ioctls can access freed `priv` and `hwdev` after driver unbind due to missing unplug state.
- [Medium] TOCTOU race setting `a_crtc->clocks_on = true` before the hardware clocks are actually resumed.
- [Medium] Unbounded accumulation of `use_cnt` in `saturn_count_rdma_use`.
- [Medium] `spacemit_crtc_duplicate_state` leaks DRM property blobs on error path.
- [Medium] Hardware resets are deasserted before clocks are enabled during PM resume.
--

--- Patch [10]: [PATCH 10/17] drm/spacemit: add Saturn DPU KMS pipeline ---
commit f6c949f848ca8fa43777ef224d0be429d2966ff6
Author: Cody Kang <codykang.hk@gmail.com>

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
> --- a/drivers/gpu/drm/spacemit/spacemit_crtc.c
> +++ b/drivers/gpu/drm/spacemit/spacemit_crtc.c
> @@ -43,18 +43,15 @@
>  static void saturn_count_rdma_use(struct drm_crtc_state *crtc_state)
>  {
>  	struct drm_plane *plane;
>  	struct spacemit_crtc_state *spacemit_state =
>  			to_spacemit_crtc_state(crtc_state);
>  	struct spacemit_crtc_rdma *rdmas = spacemit_state->rdmas;
>  	u32 rdma_id = 0;
>  	const struct drm_plane_state *pstate;
>  
>  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
>  		rdma_id = to_spacemit_plane_state(pstate)->rdma_id;
>  		if (rdma_id != RDMA_INVALID_ID)
>  			rdmas[rdma_id].use_cnt++;

[Severity: Medium]
Will this counter accumulate values incorrectly if atomic_check is called
multiple times on the same CRTC state? If the DRM core retries the commit due
to -EDEADLK backoff, it seems the counter might increment repeatedly since it
is not cleared first.

>  	}
>  }
>  
[ ... ]
> @@ -95,21 +92,20 @@
>  static void spacemit_crtc_atomic_enable(struct drm_crtc *crtc,
>  					struct drm_atomic_commit *old_state)
>  {
>  	struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc);
>  	int idx;
>  
>  	if (!drm_dev_enter(crtc->dev, &idx))
>  		return;
>  
>  	trace_spacemit_crtc_atomic_enable(a_crtc->dev_id);
>  
>  	/*
>  	 * U-Boot leaves its boot-logo pipeline running. Hard-stop it with the
>  	 * resets alone; the clocks are not ours to gate until we have enabled
>  	 * them.
>  	 */
>  	if (unlikely(a_crtc->first_modeset)) {
>  		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]
Is it safe to call usleep_range() here? CRTC atomic enable callbacks are
invoked in atomic context during non-blocking commits, where sleeping functions
are strictly forbidden and can cause kernel panics or system instability.

>  		a_crtc->first_modeset = false;
>  	}
>  
>  	if (!a_crtc->clocks_on) {
>  		a_crtc->clocks_on = true;
>  		dpu_pm_resume(a_crtc->dev);

[Severity: Medium]
Does setting a_crtc->clocks_on to true before the hardware clocks are actually
resumed in dpu_pm_resume() introduce a TOCTOU race? If a spurious or pending
interrupt fires in this window, the ISR might read MMIO registers before the
clocks are fully resumed, which could lead to a bus stall.

>  
>  		/*
>  		 * The DPU must settle after reset deassert or CFG_RDY races
>  		 * the flush.
>  		 */
>  		usleep_range(10000, 20000);
>  	}
>  
[ ... ]
> @@ -124,31 +120,29 @@
>  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;
>  
>  	trace_spacemit_crtc_atomic_disable(a_crtc->dev_id);
>  
>  	spacemit_crtc_uninit(a_crtc);
>  
>  	drm_crtc_vblank_off(&a_crtc->crtc);
>  
>  	/*
>  	 * 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);
>  
>  	/*
>  	 * 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]
Are there calls to multiple blocking and sleeping functions within this atomic
disable path? Functions like synchronize_irq(), timer_delete_sync(), and any
potential sleeping inside dpu_pm_suspend() violate atomic context requirements.

[Severity: High]
Could setting a_crtc->clocks_on = false after calling synchronize_irq() create
a TOCTOU race window? A new interrupt could fire immediately after
synchronize_irq() returns, and the new ISR instance would see clocks_on as
true and access MMIO concurrently while dpu_pm_suspend() gates the clocks,
potentially hanging the bus.

>  
>  	spin_lock_irq(&drm->event_lock);
>  	if (crtc->state->event) {
>  		drm_crtc_send_vblank_event(crtc, crtc->state->event);
>  		crtc->state->event = NULL;
[ ... ]
> @@ -171,15 +165,14 @@
>  static int spacemit_crtc_atomic_check(struct drm_crtc *crtc,
>  				      struct drm_atomic_commit *atomic_state)
>  {
>  	struct drm_crtc_state *state =
>  			drm_atomic_get_new_crtc_state(atomic_state, crtc);
>  	struct spacemit_crtc_state *cstate = to_spacemit_crtc_state(state);
>  	struct spacemit_crtc *a_crtc = to_spacemit_crtc(crtc);
>  	int ret = 0;
>  
>  	trace_spacemit_crtc_atomic_check(a_crtc->dev_id);
>  
>  	ret = spacemit_crtc_atomic_check_fbmem(crtc, state);
>  	if (ret) {
>  		drm_dbg_kms(crtc->dev,
>  			    "failed to satisfy fbcmem size for all rdmas\n");
>  		return ret;
>  	}
>  
>  	saturn_count_rdma_use(state);
>  
>  	ret = spacemit_cmdlist_alloc(a_crtc->dev, &cstate->cl);
>  	if (ret)
>  		return ret;

[Severity: High]
Is it possible for this DMA allocation in atomic_check() to cause memory leaks?
If the DRM core retries the commit (for example, due to -EDEADLK backoff),
atomic_check() is called again on the same state. This would overwrite the
pointer and permanently leak the previous allocation, allowing unprivileged
userspace to trigger a denial of service via TEST_ONLY commits.

>  
>  	return 0;
>  }
>  
[ ... ]
> @@ -211,21 +204,18 @@
>  static struct drm_crtc_state *
>  spacemit_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
>  	struct spacemit_crtc_state *state;
>  	struct spacemit_drm_private *priv = crtc->dev->dev_private;
>  	struct spacemit_hw_device *hwdev = priv->hwdev;
>  	u8 n_rdma;
>  
>  	if (WARN_ON(!crtc->state))
>  		return NULL;
>  
>  	state = kzalloc_obj(*state);
>  	if (!state)
>  		return NULL;
>  
>  	__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]
Will this error path leak DRM property blob references? The call to
__drm_atomic_helper_crtc_duplicate_state() increments blob refcounts, but if
kzalloc_objs() fails, kfree(state) is called without dropping those blob
references via __drm_atomic_helper_crtc_destroy_state().

>  
>  	state->cl.type = CMDLIST_CRTC;
>  
>  	return &state->base;
>  }
[ ... ]
> @@ -481,26 +471,21 @@
>  static int spacemit_drm_master_bind(struct device *dev)
>  {
>  	struct spacemit_drm_private *priv = dev_get_drvdata(dev);
>  	struct device_node *np = dev->of_node;
>  	struct device_node *ports = NULL, *port = NULL;
>  	struct spacemit_crtc *a_crtc;
>  	struct drm_plane *plane;
>  	struct spacemit_kms *kms;
>  	struct drm_device *drm;
>  	int ret;
>  
>  	ret = aperture_remove_all_conflicting_devices(spacemit_drm_drv.name);
>  	if (ret) {
>  		dev_err(dev, "failed to remove existing framebuffers: %d\n",
>  			ret);
>  		return ret;
>  	}
>  
>  	kms = devm_drm_dev_alloc(dev, &spacemit_drm_drv,
>  				 struct spacemit_kms, drm);
>  	if (IS_ERR(kms))
>  		return PTR_ERR(kms);
>  	drm = &kms->drm;
>  	priv->ddev = drm;
>  	drm->dev_private = priv;
>  
>  	ret = spacemit_drm_mode_config_init(drm);
>  	if (ret)
>  		return ret;
>  
>  	a_crtc = drmm_kzalloc(drm, sizeof(*a_crtc), GFP_KERNEL);
>  	if (!a_crtc) {
>  		ret = -ENOMEM;
>  		goto err_dc;
>  	}
>  	a_crtc->dev = dev;
>  	a_crtc->first_modeset = true;
>  	priv->a_crtc = a_crtc;
>  
>  	a_crtc->clk_ctx = priv->clk_ctx;
>  	a_crtc->aclk = DPU_AXICLK_DEFAULT;
>  	a_crtc->mclk_reset = priv->mclk_reset;
>  	a_crtc->esc_reset = priv->esc_reset;
>  	a_crtc->lcd_reset = priv->lcd_reset;
>  	a_crtc->aclk_reset = priv->aclk_reset;
>  	a_crtc->dsc_reset = priv->dsc_reset;
>  	a_crtc->irq_online = priv->irq_online;
>  
>  	/* Both K3 DPU instances are COMPOSER1 in the Saturn IRQ tables. */
>  	a_crtc->dev_id = COMPOSER1;
>  
>  	/* The K3 pipeline always hands RGB888 to the DP/eDP encoder. */
>  	a_crtc->out_format = OUTFMT_RGB888;
>  
>  	a_crtc->core = &dpu_saturn_ops;
>  
>  	timer_setup(&a_crtc->cfg_rdy_timer, spacemit_cfg_rdy_timer_handler, 0);
>  
>  	plane = spacemit_plane_init(drm, a_crtc);
>  	if (IS_ERR(plane)) {
>  		ret = PTR_ERR(plane);
>  		goto err_dc;
>  	}
>  
>  	ports = of_get_child_by_name(np, "ports");
>  	if (!ports) {
>  		dev_err(dev, "CRTC %pOF has no ports node\n", np);
>  		ret = -EINVAL;
>  		goto err_dc;
>  	}
>  
>  	port = of_get_child_by_name(ports, "port");
>  	if (!port) {
>  		dev_err(dev, "CRTC %pOF has no port@X node\n", np);
>  		ret = -EINVAL;
>  		goto err_ports_put;
>  	}
>  
>  	ret = spacemit_crtc_bind_init(drm, &a_crtc->crtc, plane, port);
>  	of_node_put(port);
>  	of_node_put(ports);
>  	port = NULL;
>  	ports = NULL;
>  	if (ret)
>  		goto err_dc;
>  
>  	/*
>  	 * component_bind_all() unbinds what it bound before returning an error.
>  	 */
>  	ret = component_bind_all(dev, drm);
>  	if (ret) {
>  		dev_err(dev, "failed to bind all components\n");
>  		goto err_dc;
>  	}
>  
>  	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize vblank\n");
>  		goto err_unbind_all;
>  	}
>  
>  	drm_mode_config_reset(drm);
>  	drmm_kms_helper_poll_init(drm);
>  	drm_helper_hpd_irq_event(drm);
>  
>  	enable_irq(priv->irq_online);

[Severity: High]
Does unmasking the OS IRQ here before the hardware state is prepared cause an
interrupt storm? If the bootloader left the display pipeline active, an
interrupt fires immediately. The ISR in spacemit_dpu_isr() will see
clocks_on == false and bail out with IRQ_NONE without acknowledging the
interrupt, forcing the kernel to permanently disable the IRQ line.

>  
>  	ret = drm_dev_register(drm, 0);
>  	if (ret < 0)
>  		goto err_disable_irq;
>  	drm_client_setup(drm, NULL);
>  
>  	return 0;
[ ... ]
> @@ -591,16 +576,14 @@
>  static void spacemit_drm_master_unbind(struct device *dev)
>  {
>  	struct spacemit_drm_private *priv = dev_get_drvdata(dev);
>  	struct drm_device *drm = priv->ddev;
>  
>  	disable_irq(priv->irq_online);
>  
>  	drm_dev_unregister(drm);
>  	drm_atomic_helper_shutdown(drm);

[Severity: High]
Is the teardown order correct here? Calling disable_irq() before
drm_atomic_helper_shutdown() can cause multi-second stalls, as the shutdown
process issues a full modeset to disable CRTCs and waits for vblank interrupts
that will never arrive since the IRQ is disabled.

[Severity: High]
Could calling drm_dev_unregister() without setting the unplugged state via
drm_dev_unplug() leave ioctl entry paths open while devm resources are freed?
If userspace holds an open DRM file descriptor, ioctls can still execute
because drm_dev_enter() will succeed, leading to a Use-After-Free when they
dereference the freed priv pointer.

>  	component_unbind_all(dev, drm);
>  
>  	/* .shutdown and a later re-bind must not touch the stale instance. */
>  	priv->ddev = NULL;
>  	priv->a_crtc = NULL;
>  }
[ ... ]
> @@ -747,21 +730,17 @@
>  static int dpu_pm_resume(struct device *dev)
>  {
>  	struct spacemit_drm_private *priv = dev_get_drvdata(dev);
>  	struct spacemit_crtc *a_crtc = priv->a_crtc;
>  
>  	dpu_reset_deassert(dev, "mclk", a_crtc->mclk_reset);
>  	dpu_reset_deassert(dev, "esc", a_crtc->esc_reset);
>  	dpu_reset_deassert(dev, "lcd", a_crtc->lcd_reset);
>  	dpu_reset_deassert(dev, "aclk", a_crtc->aclk_reset);
>  	dpu_reset_deassert(dev, "dsc", a_crtc->dsc_reset);
>  
>  	if (a_crtc->core && a_crtc->core->enable_clk)
>  		a_crtc->core->enable_clk(a_crtc);

[Severity: Medium]
Are hardware resets being deasserted before clocks are enabled? Reset edges
cannot propagate through gated hardware blocks, so the clocks should likely be
enabled before deasserting resets to ensure a clean initialization and avoid
hardware state machine violations.

>  
>  	return 0;
>  }
>  
>  static const struct of_device_id dpu_match_table[] = {
>  	{
>  		.compatible = "spacemit,k3-saturn-dpu",
>  		.data = &k3_saturn_dpu,
>  	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dpu_match_table);
>  
>  static struct platform_driver spacemit_dpu_driver = {
>  	.probe = spacemit_dpu_probe,
>  	.remove = spacemit_dpu_remove,
>  	.shutdown = spacemit_dpu_shutdown,
>  	.driver = {
>  		.name = "spacemit-dpu-drv",
>  		.of_match_table = dpu_match_table,
>  		.suppress_bind_attrs = true,
>  	},
>  };
>  module_platform_driver(spacemit_dpu_driver);
>  
>  MODULE_DESCRIPTION("SpacemiT DRM KMS master driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/spacemit/spacemit_planes.c b/drivers/gpu/drm/spacemit/spacemit_planes.c
> --- a/drivers/gpu/drm/spacemit/spacemit_planes.c
> +++ b/drivers/gpu/drm/spacemit/spacemit_planes.c
> @@ -107,31 +104,27 @@
>  static int spacemit_plane_atomic_check(struct drm_plane *plane,
>  				       struct drm_atomic_commit *atomic_state)
>  {
>  	struct drm_plane_state *state =
>  			drm_atomic_get_new_plane_state(atomic_state, plane);
>  	struct drm_framebuffer *fb = state->fb;
>  	u32 src_w, src_h, src_x, src_y;
>  	u32 crtc_w, crtc_h;
>  	struct spacemit_plane_state *cur_state = to_spacemit_plane_state(state);
>  	u32 cur_rdma_id = cur_state->rdma_id;
>  	struct spacemit_drm_private *priv = plane->dev->dev_private;
>  	struct spacemit_hw_device *hwdev = priv->hwdev;
>  	struct spacemit_crtc *a_crtc = NULL;
>  
>  	struct drm_crtc_state *crtc_state = NULL;
>  	int ret;
>  
>  	if (state->crtc)
>  		crtc_state = drm_atomic_get_new_crtc_state(atomic_state,
>  							   state->crtc);
>  
>  	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
>  						  DRM_PLANE_NO_SCALING,
>  						  DRM_PLANE_NO_SCALING,
>  						  true, false);
>  	if (ret)
>  		return ret;
>  
>  	if (!state->visible)
>  		return 0;
>  
>  	a_crtc = to_spacemit_crtc(state->crtc);
>  	trace_spacemit_plane_atomic_check(a_crtc->dev_id);
>  
>  	src_x = state->src_x >> 16;
>  	src_y = state->src_y >> 16;
>  	src_w = state->src_w >> 16;
>  	src_h = state->src_h >> 16;
>  	crtc_w = state->crtc_w;
>  	crtc_h = state->crtc_h;
>  
>  	/* The programming paths use the raw rectangles, so refuse to clip. */
>  	if ((state->src.x1 >> 16) != src_x || (state->src.y1 >> 16) != src_y ||
>  	    (drm_rect_width(&state->src) >> 16) != src_w ||
>  	    (drm_rect_height(&state->src) >> 16) != src_h ||
>  	    state->dst.x1 != state->crtc_x || state->dst.y1 != state->crtc_y ||
>  	    drm_rect_width(&state->dst) != crtc_w ||
>  	    drm_rect_height(&state->dst) != crtc_h) {
>  		drm_dbg_kms(plane->dev,
>  			    "clipped plane rectangles not supported\n");
>  		return -EINVAL;
>  	}
>  
>  	/* One-dimensional sources are invalid (only 0x0 = constant fill) */
>  	if ((src_w == 0) != (src_h == 0)) {
>  		drm_dbg_kms(plane->dev, "unsupported src_w %d src_h %d\n",
>  			    src_w, src_h);
>  		return -EINVAL;
>  	}
>  
>  	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;
>  
>  	if (cur_rdma_id != RDMA_INVALID_ID) {
>  		if (cur_rdma_id >= hwdev->rdma_nums) {
>  			drm_dbg_kms(plane->dev, "invalid rdma id %d\n",
>  				    cur_rdma_id);
>  			return -EINVAL;
>  		}
>  
>  		if (a_crtc->core->cal_layer_fbcmem_size(plane, state)) {
>  			drm_dbg_kms(plane->dev,
>  				    "plane %d: invalid fbcmem size\n",
>  				    state->zpos);
>  			return -EINVAL;
>  		}
>  
>  		cur_state->mmu_tbl.size =
>  			((PAGE_ALIGN(fb->obj[0]->size) >> PAGE_SHIFT) +
>  			 HW_ALIGN_TTB_NUM) * 4;
>  		cur_state->mmu_tbl.va =
>  			dma_alloc_coherent(a_crtc->dev, cur_state->mmu_tbl.size,
>  					   &cur_state->mmu_tbl.pa,
>  					   GFP_KERNEL | __GFP_ZERO);
>  		if (!cur_state->mmu_tbl.va)
>  			return -ENOMEM;

[Severity: High]
Does allocating DMA memory directly into the state here in atomic_check()
cause memory leaks on retries? Just like with spacemit_cmdlist_alloc(), if the
commit is retried due to -EDEADLK backoff, this will overwrite the pointer and
permanently leak the previous allocation.

>  	}
>  
>  	/*
>  	 * The commit cannot fail, so take the DMA buffer while -ENOMEM is an
>  	 * answer.
>  	 */
>  	ret = spacemit_cmdlist_alloc(a_crtc->dev, &cur_state->cl);
>  	if (ret)
>  		return ret;
>  
>  	cur_state->format = spacemit_plane_hw_get_format_id(fb->format->format);
>  	if (cur_state->format == SPACEMIT_DPU_INVALID_FORMAT_ID) {
>  		drm_dbg_kms(plane->dev, "unsupported format %p4cc\n",
>  			    &fb->format->format);
>  		return -EINVAL;
>  	}
>  
>  	return 0;
>  }
>  
>  static void spacemit_plane_atomic_update(struct drm_plane *plane,
>  					 struct drm_atomic_commit *state)
>  {
>  	int ret = 0;
>  	struct spacemit_crtc *a_crtc = to_spacemit_crtc(plane->state->crtc);
>  	struct spacemit_plane_state *spacemit_pstate =
>  			to_spacemit_plane_state(plane->state);
>  	struct spacemit_drm_private *priv = plane->dev->dev_private;
>  	struct spacemit_hw_device *hwdev = priv->hwdev;
>  	u32 rdma_id = spacemit_pstate->rdma_id;
>  	struct cmdlist *cl = NULL;
>  	int idx;
>  
>  	if (!drm_dev_enter(plane->dev, &idx))
>  		return;
>  
>  	trace_spacemit_plane_atomic_update(a_crtc->dev_id);
>  
>  	hwdev->get_cl_rdma_buf(a_crtc);

[Severity: High]
Is it safe to perform dynamic memory allocation in the atomic_update fast path?
The DRM atomic update phase is forbidden to fail or sleep. Any temporary memory
should be pre-allocated in atomic_check() or prepare_fb().

[Severity: Critical]
Does ignoring the return value of get_cl_rdma_buf() lead to a Use-After-Free
or Double-Free? If it fails, a_crtc->cl_rdma retains its previous value. When
free_cmdlist_regs(a_crtc->cl_rdma) is called later, it frees the memory but
does not set the pointer to NULL, making a_crtc->cl_rdma a dangling pointer on
subsequent failures.

>  
>  	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);
>  		if (!ret)
>  			cmdlist_regs_packing(plane_to_cl(plane),
>  					     CMDLIST_MOD_RDMA, a_crtc->cl_rdma);
>  		else
>  			drm_err(plane->dev, "failed to map plane: %d\n", ret);
>  	}
>  
>  	cl = &spacemit_pstate->cl;
>  	cl->rch_start_cmps_y = CMDLIST_SENTINEL_POISON;
>  	cl->cmdlist_ch_y_other = CMDLIST_SENTINEL_POISON;
>  	free_cmdlist_regs(a_crtc->cl_rdma);

[Severity: Critical]
Does this free_cmdlist_regs() call leave a_crtc->cl_rdma as a dangling pointer?
As mentioned above, since the pointer is not cleared after being freed, a
subsequent allocation failure will leave the pointer pointing to freed memory.

>  
>  	drm_dev_exit(idx);
>  }
>  
[ ... ]
> @@ -201,15 +190,14 @@
>  static void spacemit_plane_reset(struct drm_plane *plane)
>  {
>  	struct spacemit_plane_state *s;
>  
>  	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]
Should this cleanup logic utilize the driver-specific custom cleanup hook? By
open-coding the state destruction and calling kfree(s) directly, it bypasses
spacemit_plane_atomic_destroy_state(). This permanently leaks the plane's DMA
allocations (mmu_tbl.va and cl.va) when spacemit_plane_reset() executes on a
plane that already has a state.

>  
>  	s = kzalloc(sizeof(*s), GFP_KERNEL);
>  	if (s) {
>  		__drm_atomic_helper_plane_reset(plane, &s->state);
>  		s->rdma_id = RDMA_INVALID_ID;
>  	}
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-k3-display-v1-0-f22a10a52867@gmail.com?part=10

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy