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