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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling a sleeping function in an atomic context (plane atomic update callback).
- [High] Missing drm_atomic_helper_shutdown() in driver removal and missing .shutdown hooks.
- [High] Infinite interrupt storm leading to a CPU hard lockup upon PCI device removal (missing 0xFFFFFFFF check).
- [High] Kernel panic via unaligned memory access in `glanda_blit_rect` due to unvalidated framebuffer pitch.
--

commit 17ab32ed9422ea6f511ee3ba92be0492625b8e06
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..bc954eb6d4412
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/glandagpu.c

[ ... ]

> @@ -107,17 +107,17 @@ static void glanda_blit_rect(struct glanda_device *gdev,
> +		u32 __iomem *dst = (u32 __iomem *)gdev->vram_base +
> +				   (size_t)dst_y * GLANDA_WIDTH + dst_clip->x1;
> +		size_t src_off = (size_t)src_y * src_pitch +
> +				 (size_t)(dst_clip->x1 - dst_off_x) * sizeof(u32);
> +
> +		for (x = 0; x < width; x++) {
> +			u32 pixel = iosys_map_rd(src, src_off + x * sizeof(u32), u32);

[Severity: High]
Does this code risk an unaligned memory access and a kernel panic?

Userspace can specify an arbitrary framebuffer pitch via the
DRM_IOCTL_MODE_ADDFB2 ioctl, and the DRM core does not enforce byte alignment
on fb->pitches[0].

If src_pitch is not a multiple of 4, src_off could be an unaligned byte
offset. Because the iosys_map_rd() macro casts directly to a 32-bit pointer
and dereferences it, this could cause an Alignment Fault and kernel panic on
architectures with strict alignment requirements.

[ ... ]

> @@ -136,10 +136,10 @@ static void glanda_plane_atomic_update(struct drm_plane *plane,
> +	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);
> +	if (ret)
> +		return;

[Severity: High]
Could this result in calling a sleeping function from an atomic context?

The glanda_plane_atomic_update() function executes in an atomic context
during non-blocking commits where sleeping is strictly forbidden.

Because drm_gem_fb_begin_cpu_access() waits on DMA fences and acquires
mutexes, it can sleep. This could lead to might_sleep() warnings, system
instability, or deadlocks. Should CPU access be prepared beforehand in
a begin_fb_access or prepare_fb hook instead?

[ ... ]

> @@ -162,6 +162,6 @@ static void glanda_plane_atomic_update(struct drm_plane *plane,
> +
> +	drm_dev_exit(idx);
> +out_drm_gem_fb_end_cpu_access:
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}

[Severity: High]
Can this end_cpu_access call also sleep in the atomic update callback?

[ ... ]

> @@ -282,10 +282,10 @@ static const struct drm_connector_funcs glanda_connector_funcs = {
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static const struct drm_mode_config_funcs glanda_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};

[Severity: High]
Does the lack of a custom validation wrapper here allow the unaligned pitch
issue in glanda_blit_rect() to occur?

Hooking fb_create directly to drm_gem_fb_create_with_dirty() without a
custom wrapper means the driver does not enforce pitch alignment during
framebuffer creation.

[ ... ]

> @@ -306,10 +306,10 @@ static irqreturn_t glanda_irq_handler(int irq, void *dev_id)
> +	struct glanda_device *gdev = dev_id;
> +	u32 isr, ier;
> +
> +	if (!gdev || !gdev->mmio_base)
> +		return IRQ_NONE;
> +
> +	isr = readl(gdev->mmio_base + REG_ISR);
> +	ier = readl(gdev->mmio_base + REG_IER);
> +
> +	if (!(isr & ier))
> +		return IRQ_NONE;

[Severity: High]
Could this cause an infinite interrupt storm and a CPU hard lockup if the PCI
device is removed?

When a PCI device is removed (e.g. hot-unplug), MMIO reads return all 1s
(0xFFFFFFFF). Since !(0xFFFFFFFF & 0xFFFFFFFF) evaluates to false, this
handler would not return IRQ_NONE.

Instead, it would process a fake VSYNC, write 0xFFFFFFFF back, and return
IRQ_HANDLED. If the IRQ line is shared and asserted by another device, the
kernel's spurious interrupt detector will not disable the broken line,
potentially causing a hard lockup.

[ ... ]

> @@ -522,10 +522,10 @@ static int glanda_drm_init(struct glanda_device *gdev, int irq)
> +	return 0;
> +}
> +
> +/* Shared teardown, mirrors glanda_drm_init() */
> +static void glanda_drm_fini(struct glanda_device *gdev)
> +{
> +	drm_dev_unplug(&gdev->drm);
> +}

[Severity: High]
Is a call to drm_atomic_helper_shutdown() missing here?

Unregistering the DRM device without shutting down the atomic helpers means
the display pipeline is not turned off. The active drm_crtc_state and
drm_plane_state would not transition to a disabled state, leaving
framebuffers pinned and resulting in memory leaks.

[ ... ]

> @@ -575,10 +575,10 @@ static const struct of_device_id glanda_of_match[] = {
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, glanda_of_match);
> +
> +static struct platform_driver glandagpu_driver = {
> +	.driver = {
> +		.name = "glandagpu",
> +		.of_match_table = glanda_of_match,
> +	},
> +	.probe = glandagpu_probe,
> +	.remove = glandagpu_remove,
> +};

[Severity: High]
Does this platform driver need a .shutdown callback?

Without a .shutdown callback, the display hardware remains active during
system halt or reboot, which can cause the device to continue scanning out
VRAM inappropriately across reboots or kexecs.

[ ... ]

> @@ -613,10 +613,10 @@ static const struct pci_device_id glanda_pci_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_DEVICE_ID_GLANDA_GPU) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, glanda_pci_ids);
> +
> +static struct pci_driver glandagpu_pci_driver = {
> +	.name = "glandagpu-pci",
> +	.id_table = glanda_pci_ids,
> +	.probe = glandagpu_pci_probe,
> +	.remove = glandagpu_pci_remove,
> +};

[Severity: High]
Is the .shutdown callback also missing from the PCI driver?

Similar to the platform driver, omitting this callback prevents the display
from being gracefully disabled on shutdown.

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