Re: [RFC PATCH 2/3] drm/glanda: Add initial DRM driver for GlandaGPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Plane atomic update callback calls sleeping functions, violating DRM atomic context rules.
- [High] Manual call to drm_mode_config_cleanup causes a double-free.
- [High] Missing drm_atomic_helper_shutdown() during driver removal.
- [High] Teardown logic creates a race condition causing ioctl threads to write to unmapped MMIO memory.
- [High] IRQ handler is registered before the CRTC is initialized, creating a window for a NULL pointer dereference.
- [Medium] Direct pointer dereference of `iosys_map` memory bypasses abstraction and breaks on I/O memory.
- [Medium] Mishandling of platform_get_irq return value breaks probe deferral.
--

--- Patch [2]: [RFC PATCH 2/3] drm/glanda: Add initial DRM driver for GlandaGPU ---
commit ee638d3912e8ed264bf2c9ee4b6fd9350bc3726b
Author: Leander Kieweg <kieweg.leander@gmail.com>
Subject: drm/glanda: Add initial DRM driver for GlandaGPU

This commit introduces the core DRM/KMS driver for GlandaGPU. This driver
supports basic modesetting, atomic updates, and custom 2D hardware
acceleration IOCTLs.

> diff --git a/drivers/gpu/drm/tiny/glandagpu.c b/drivers/gpu/drm/tiny/glandagpu.c
> new file mode 100644
> index 0000000000000..8f87ae096a2e3
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/glandagpu.c
> @@ -0,0 +1,769 @@
[ ... ]
> +static void glanda_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
[ ... ]
> +	dma_resv_lock(shmem->base.resv, NULL);
> +	ret = drm_gem_shmem_vmap_locked(shmem, &map);
> +	if (ret) {
> +		dma_resv_unlock(shmem->base.resv);
> +		drm_err(&gdev->drm,
> +			"GlandaGPU: failed to vmap GEM shmem object\n");
> +		return;
> +	}
> +
> +	mutex_lock(&gdev->lock);

[Severity: High]
DRM atomic commit callbacks execute in an atomic context and cannot sleep. 
Is it safe to call dma_resv_lock, drm_gem_shmem_vmap_locked, and mutex_lock 
here? These functions can sleep and might trigger a schedule while atomic
oops or deadlocks when this path is reached via non-blocking atomic commits.

> +
> +	ret = glanda_wait_idle(gdev);
> +	if (ret) {
> +		drm_err(&gdev->drm, "GlandaGPU: timed out waiting for idle\n");
> +		mutex_unlock(&gdev->lock);
> +		drm_gem_shmem_vunmap_locked(shmem, &map);
> +	dma_resv_unlock(shmem->base.resv);
> +		return;
> +	}
> +
> +	src_pitch = fb->pitches[0];
> +	width = min_t(u32, fb->width, GLANDA_WIDTH);
> +	height = min_t(u32, fb->height, GLANDA_HEIGHT);
> +
> +	u8 __iomem *dst_base = gdev->vram_base;
> +	u8 *src_base = map.vaddr;
> +	u32 y;
> +
> +	for (y = 0; y < height; y++) {
> +		u32 *src = (u32 *)(src_base + y * src_pitch);

[Severity: Medium]
Does directly dereferencing map.vaddr break abstraction on architectures 
that do not support direct memory dereferencing for I/O memory? If the 
mapped memory is in I/O space (like an imported dma-buf), this might cause 
a panic. Should this use iosys_map helpers like iosys_map_rd instead?

[ ... ]
> +static int glandagpu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	writel(0, gdev->mmio_base + REG_IER);
> +	writel(0xFFFFFFFF, gdev->mmio_base + REG_ISR);	/* clear flags */
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret > 0) {
> +		gdev->irq = ret;
> +		ret = devm_request_irq(&pdev->dev, gdev->irq, glanda_irq_handler,
> +				       IRQF_SHARED, "glandagpu", gdev);

[Severity: Medium]
What happens if platform_get_irq returns -EPROBE_DEFER or another negative 
error code? The check for ret > 0 seems to treat all negative error codes 
as a missing IRQ, falling back to polling permanently rather than returning 
the error to the driver core to retry.

[Severity: High]
Since devm_request_irq is called with IRQF_SHARED here, could the IRQ 
handler fire immediately before the CRTC is initialized later in probe? 
If the shared interrupt fires early and the INT_VSYNC bit is set, 
glanda_irq_handler would call drm_crtc_handle_vblank on an uninitialized 
crtc object, which could lead to a NULL pointer dereference.

[ ... ]
> +err_mode_cleanup:
> +	drm_mode_config_cleanup(&gdev->drm);
> +	return ret;
> +}

[Severity: High]
The mode config is initialized using drm_mode_config_init, which wraps 
drmm_mode_config_init and automatically registers a devres action for 
cleanup. Does calling drm_mode_config_cleanup manually here cause a 
double-free on probe failure?

> +
> +static void glandagpu_remove(struct platform_device *pdev)
> +{
> +	struct glanda_device *gdev = platform_get_drvdata(pdev);
> +
> +	/* Disable interrupts first so no new IRQ work can race the teardown
> +	 * below, and wake up anyone still blocked in glanda_wait_idle().
> +	 */
> +	writel(0, gdev->mmio_base + REG_IER);
> +	gdev->cmd_done = true;
> +	wake_up_interruptible(&gdev->cmd_wq);

[Severity: High]
By faking a successful completion instead of signaling an abort condition, 
any ioctl threads sleeping in glanda_wait_idle will wake up and proceed 
to write to mmio_base. Without proper synchronization or a call to 
drm_dev_enter/exit to ensure threads finish, can these awakened threads 
end up writing to unmapped MMIO memory as the devres teardown proceeds?

> +
> +	drm_info(&gdev->drm, "GlandaGPU DRM Driver removed\n");
> +	drm_dev_unregister(&gdev->drm);

[Severity: High]
Should this removal sequence include a call to drm_atomic_helper_shutdown 
to quiesce the display hardware? Without it, hardware pipelines might remain 
active after the module is unloaded, which could result in memory corruption 
or resource leaks.

> +	drm_mode_config_cleanup(&gdev->drm);
> +}

[Severity: High]
Similar to the probe error path, does this manual call to 
drm_mode_config_cleanup cause a double-free on module unload, given that 
drm_mode_config_init already registered a devres cleanup handler?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714101146.200416-1-kieweg.leander@gmail.com?part=2