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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Ignored return value of drm_mode_config_init().
- [High] Hardware interrupts are left enabled if drm_dev_register() fails, leading to an unhandled interrupt storm.
- [High] Missing resource size validation before I/O memory mapping allows out-of-bounds mapping and writes.
- [High] Userspace atomic commits hang when the driver falls back to polling mode because vblank events are armed but never signaled.
- [Medium] Unused INT_DONE interrupt is unconditionally enabled, risking an interrupt storm.
- [Medium] Native u32 read of little-endian pixel data breaks color conversions on big-endian CPUs.
- [Low] Missing dependencies in Kconfig for DRM_GLANDA.
- [Low] Using platform_get_irq() for an optional IRQ causes log spam.
--

--- Patch [2]: [PATCH v2 2/2] drm/glanda: Add initial DRM driver for GlandaGPU ---
commit 47c9d06e7235a576cf0a48e9d5a1011c1d5c7ced
Author: Leander Kieweg <kieweg.leander@gmail.com>
Subject: 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/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index f0e72d4b6a470..f5aaf499e34eb 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
[ ... ]
> @@ -56,6 +56,17 @@ config DRM_CIRRUS_QEMU
>  	   - qxl (DRM_QXL, qemu -vga qxl, works best with spice)
>  	   - virtio (DRM_VIRTIO_GPU), qemu -vga virtio)
>  
> +config DRM_GLANDA
> +	tristate "GlandaGPU DRM driver"
> +	depends on DRM
> +	select DRM_KMS_HELPER
> +	select DRM_GEM_SHMEM_HELPER

[Severity: Low]
Does this config need to depend on MMU and PCI || COMPILE_TEST? The driver
selects DRM_GEM_SHMEM_HELPER which depends on MMU, and it calls PCI devres
functions that lack non-PCI stubs when CONFIG_PCI is disabled.

> diff --git a/drivers/gpu/drm/tiny/glandagpu.c b/drivers/gpu/drm/tiny/glandagpu.c
> new file mode 100644
> index 0000000000000..e4dfddaba4e3b
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/glandagpu.c
[ ... ]
> +static void glanda_plane_atomic_update(struct drm_plane *plane,
> +				       struct drm_atomic_commit *state)
> +{
[ ... ]
> +	for (y = 0; y < height; y++) {
> +		size_t offset = y * GLANDA_WIDTH * sizeof(u32);
> +		u32 __iomem *dst = (u32 __iomem *)(gdev->vram_base + offset);
> +
> +		for (x = 0; x < width; x++) {
> +			u32 pixel = iosys_map_rd(&shadow_state->data[0],
> +						 y * src_pitch + x * sizeof(u32), u32);
> +			u32 packed = ((pixel >> 12) & 0x0F00) |
> +				((pixel >> 8) & 0x00F0) |
> +				((pixel >> 4) & 0x000F);
> +
> +			writel_relaxed(packed, &dst[x]);
> +		}
> +	}

[Severity: Medium]
Will this bitwise extraction read the correct color channels on big-endian
architectures? By using iosys_map_rd to read a native u32, the little-endian
DRM_FORMAT_XRGB8888 byte stream will have its bytes inverted on big-endian
CPUs.

[ ... ]
> +static void glanda_crtc_atomic_flush(struct drm_crtc *crtc,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_pending_vblank_event *event;
> +
> +	if (new_state->event) {
> +		event = new_state->event;
> +
> +		new_state->event = NULL;
> +
> +		spin_lock_irq(&crtc->dev->event_lock);
> +
> +		if (drm_crtc_vblank_get(crtc) == 0)
> +			drm_crtc_arm_vblank_event(crtc, event);
> +		else
> +			drm_crtc_send_vblank_event(crtc, event);

[Severity: High]
What happens to armed vblank events when the driver falls back to polling
mode? If there is no hardware interrupt to call drm_crtc_handle_vblank(),
these events might never be signaled, which can cause userspace page flips
to hang. Should the driver set crtc->state->no_vblank = true or bypass
drm_crtc_arm_vblank_event in polling mode?

[ ... ]
> +static int glanda_drm_init(struct glanda_device *gdev, int irq)
> +{
[ ... ]
> +	writel(0, gdev->mmio_base + REG_IER);
> +	writel(0xFFFFFFFF, gdev->mmio_base + REG_ISR);	/* clear flags */
> +
> +	/* DRM mode config */
> +	drm_mode_config_init(&gdev->drm);

[Severity: High]
Is it safe to ignore the integer return value of drm_mode_config_init? If
this fails due to memory allocation issues, the DRM mode configuration state
will be partially initialized, potentially leading to NULL pointer
dereferences later when the DRM core accesses it.

[ ... ]
> +	if (irq > 0) {
> +		gdev->irq = irq;
> +		ret = devm_request_irq(gdev->drm.dev, gdev->irq, glanda_irq_handler,
> +				       IRQF_SHARED, "glandagpu", gdev);
> +		if (ret) {
> +			drm_err(&gdev->drm, "Failed to request IRQ %d\n",
> +				gdev->irq);
> +			return ret;
> +		}
> +
> +		writel(INT_DONE, gdev->mmio_base + REG_IER);

[Severity: Medium]
Could enabling INT_DONE cause an interrupt storm? If the hardware asserts
INT_DONE when the 2D engine is idle, and the driver never submits commands
to it, the IRQ handler might get caught in a loop continuously clearing it.

> +		drm_info(&gdev->drm, "IRQ %d requested and enabled\n", gdev->irq);
> +	} else {
> +		drm_warn(&gdev->drm, "No IRQ found, falling back to polling\n");
> +	}
> +
> +	ret = drm_dev_register(&gdev->drm, 0);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this error path leave hardware interrupts enabled? If
drm_dev_register fails, the function returns without clearing REG_IER.
Since devres frees the IRQ handler, this might leave the hardware asserting
an interrupt with no kernel handler available to acknowledge it.

[ ... ]
> +static int glandagpu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	gdev->vram_phys = res->start;
> +	gdev->vram_base = devm_ioremap(&pdev->dev, res->start, GLANDA_VRAM_SIZE);
> +	gdev->mmio_base = devm_ioremap(&pdev->dev, res->start + GLANDA_MMIO_OFFSET,
> +				       GLANDA_MMIO_SIZE);

[Severity: High]
Should there be a check on the underlying resource length before mapping?
Blindly mapping GLANDA_VRAM_SIZE without verifying resource_size(res) could
cause out-of-bounds mappings if the hardware resource is undersized. Later,
the shadow plane write loop in glanda_plane_atomic_update() could overrun
the boundaries and corrupt physical memory.

> +	if (!gdev->vram_base || !gdev->mmio_base) {
> +		drm_err(&gdev->drm, "failed to ioremap\n");
> +		return -ENOMEM;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq == -ENXIO)
> +		irq = -1;	/* no IRQ resource, fall back to polling */

[Severity: Low]
Since the driver gracefully supports polling mode, could this use
platform_get_irq_optional instead? Using platform_get_irq logs a
false-positive error when the IRQ is not found.

[ ... ]
> +static int glandagpu_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
[ ... ]
> +	gdev->mmio_base = pcim_iomap_table(pdev)[0];
> +	gdev->vram_base = pcim_iomap_table(pdev)[1];
> +	gdev->vram_phys = pci_resource_start(pdev, 1);

[Severity: High]
Does this PCI probe path also need to validate the size of the mapped BARs
against the expected GLANDA_VRAM_SIZE and GLANDA_MMIO_SIZE to prevent
similar out-of-bounds writes?

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