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 <[email protected]> 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/[email protected]?part=2
