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