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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: sleeping in atomic context during plane atomic_update
- [High] drm/glanda: incorrect teardown order in glanda_drm_fini causes IRQ storm
--

commit 282719415a14034814111dcb1dd6288f05170cc2
Author: Leander Kieweg <kieweg.leander@gmail.com>

drm/glanda: Add initial DRM driver for GlandaGPU

Introduce the core DRM/KMS driver for GlandaGPU. This driver
supports basic modesetting, atomic updates via shadow plane helpers,
and optional QEMU PCI probing alongside the platform driver.

> diff --git a/drivers/gpu/drm/tiny/glandagpu.c b/drivers/gpu/drm/tiny/glandagpu.c
> new file mode 100644
> index 0000000000000..d133da93999f8
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/glandagpu.c
[ ... ]
> +static void glanda_blit_rect(struct glanda_device *gdev,
> +			     const struct drm_rect *dst_clip,
> +			     const struct iosys_map *src,
> +			     struct drm_framebuffer *fb,
> +			     int dst_off_x, int dst_off_y,
> +				 struct drm_format_conv_state *fmtcnv_state)
> +{
> +	unsigned int src_pitch = fb->pitches[0];
> +	unsigned int width = drm_rect_width(dst_clip);
> +	unsigned int height = drm_rect_height(dst_clip);
> +	unsigned int x, y;
> +	size_t len = width * sizeof(u32);
> +	u32 *sbuf;
> +
> +	sbuf = drm_format_conv_state_reserve(fmtcnv_state, len, GFP_KERNEL);

[Severity: High]
Is it safe to use GFP_KERNEL in this context?

glanda_blit_rect() is called from glanda_plane_atomic_update(), which is
registered as the atomic_update callback for the primary plane.

As per DRM subsystem rules, plane atomic update callbacks execute in atomic
context (e.g., inside drm_atomic_helper_commit_tail with locks held/preemption
disabled) where sleeping is strictly forbidden.

GFP_KERNEL allows the memory allocator to block and reclaim memory.

Could this allocation be deferred to a sleepable hook like atomic_check?

> +	if (!sbuf)
> +		return;
[ ... ]
> +static void glanda_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_state = to_drm_shadow_plane_state(new_state);
> +	struct drm_rect vram_clip = DRM_RECT_INIT(0, 0, GLANDA_WIDTH, GLANDA_HEIGHT);
> +	struct glanda_device *gdev = to_glanda(plane->dev);
> +	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_rect damage;
> +	int dst_off_x, dst_off_y;
> +	int ret, idx;
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);

[Severity: High]
Can this call sleep while executing in an atomic context?

As noted above, glanda_plane_atomic_update() runs in an atomic context where
sleeping is forbidden.

drm_gem_fb_begin_cpu_access() can sleep while acquiring DMA reservation locks
and waiting on fences. Calling sleepable functions in the atomic display
commit path will cause kernel warnings (scheduling while atomic BUGs),
deadlocks, and system instability.

Should CPU access preparation be deferred to the sleepable prepare_fb hook?

> +	if (ret)
> +		return;
[ ... ]
> +static void glanda_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct glanda_device *gdev = to_glanda(crtc->dev);
> +	u32 ier;
> +	int idx;
> +
> +	if (!drm_dev_enter(crtc->dev, &idx))
> +		return;

[Severity: High]
Will this early return prevent the hardware VSYNC interrupt from being disabled
when the module unloads?

If drm_dev_unplug() is called before drm_atomic_helper_shutdown(),
drm_dev_enter() will return false because the device is already marked as
unplugged.

The early return here would then cause the hardware interrupt disable logic
(clearing INT_VSYNC in REG_IER) to be skipped.

If the driver finishes removal, devres frees the IRQ handler while the hardware
continues to fire interrupts. Because the IRQ is requested with IRQF_SHARED, an
unhandled hardware interrupt causes an IRQ storm that will force the kernel to
permanently disable the shared interrupt line, potentially causing a denial of
service for other devices sharing the line.

> +
> +	ier = readl(gdev->mmio_base + REG_IER);
> +	writel(ier & ~INT_VSYNC, gdev->mmio_base + REG_IER);
> +
> +	drm_dev_exit(idx);
> +}
[ ... ]
> +/* Shared teardown, mirrors glanda_drm_init() */
> +static void glanda_drm_fini(struct glanda_device *gdev)
> +{
> +	drm_dev_unplug(&gdev->drm);
> +	drm_atomic_helper_shutdown(&gdev->drm);

[Severity: High]
Are these teardown operations ordered correctly to prevent an IRQ storm on
module unload?

Because drm_dev_unplug() marks the device as unplugged (dev->unplugged = true)
before drm_atomic_helper_shutdown() is called, any drm_dev_enter() calls in the
shutdown path (like in glanda_crtc_disable_vblank() and
glanda_plane_atomic_disable()) will immediately return false.

This short-circuits the hardware disable logic, leaving the VSYNC interrupt
enabled when the module unloads.

Should drm_atomic_helper_shutdown() be called before drm_dev_unplug() to ensure
hardware teardown logic executes properly?

> +}

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