Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Plane atomic update callback calls sleeping functions, violating DRM atomic context rules. - [High] Manual call to drm_mode_config_cleanup causes a double-free. - [High] Missing drm_atomic_helper_shutdown() during driver removal. - [High] Teardown logic creates a race condition causing ioctl threads to write to unmapped MMIO memory. - [High] IRQ handler is registered before the CRTC is initialized, creating a window for a NULL pointer dereference. - [Medium] Direct pointer dereference of `iosys_map` memory bypasses abstraction and breaks on I/O memory. - [Medium] Mishandling of platform_get_irq return value breaks probe deferral. --
--- Patch [2]: [RFC PATCH 2/3] drm/glanda: Add initial DRM driver for GlandaGPU --- commit ee638d3912e8ed264bf2c9ee4b6fd9350bc3726b Author: Leander Kieweg <[email protected]> Subject: drm/glanda: Add initial DRM driver for GlandaGPU This commit introduces the core DRM/KMS driver for GlandaGPU. This driver supports basic modesetting, atomic updates, and custom 2D hardware acceleration IOCTLs. > diff --git a/drivers/gpu/drm/tiny/glandagpu.c > b/drivers/gpu/drm/tiny/glandagpu.c > new file mode 100644 > index 0000000000000..8f87ae096a2e3 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/glandagpu.c > @@ -0,0 +1,769 @@ [ ... ] > +static void glanda_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ [ ... ] > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_vmap_locked(shmem, &map); > + if (ret) { > + dma_resv_unlock(shmem->base.resv); > + drm_err(&gdev->drm, > + "GlandaGPU: failed to vmap GEM shmem object\n"); > + return; > + } > + > + mutex_lock(&gdev->lock); [Severity: High] DRM atomic commit callbacks execute in an atomic context and cannot sleep. Is it safe to call dma_resv_lock, drm_gem_shmem_vmap_locked, and mutex_lock here? These functions can sleep and might trigger a schedule while atomic oops or deadlocks when this path is reached via non-blocking atomic commits. > + > + ret = glanda_wait_idle(gdev); > + if (ret) { > + drm_err(&gdev->drm, "GlandaGPU: timed out waiting for idle\n"); > + mutex_unlock(&gdev->lock); > + drm_gem_shmem_vunmap_locked(shmem, &map); > + dma_resv_unlock(shmem->base.resv); > + return; > + } > + > + src_pitch = fb->pitches[0]; > + width = min_t(u32, fb->width, GLANDA_WIDTH); > + height = min_t(u32, fb->height, GLANDA_HEIGHT); > + > + u8 __iomem *dst_base = gdev->vram_base; > + u8 *src_base = map.vaddr; > + u32 y; > + > + for (y = 0; y < height; y++) { > + u32 *src = (u32 *)(src_base + y * src_pitch); [Severity: Medium] Does directly dereferencing map.vaddr break abstraction on architectures that do not support direct memory dereferencing for I/O memory? If the mapped memory is in I/O space (like an imported dma-buf), this might cause a panic. Should this use iosys_map helpers like iosys_map_rd instead? [ ... ] > +static int glandagpu_probe(struct platform_device *pdev) > +{ [ ... ] > + writel(0, gdev->mmio_base + REG_IER); > + writel(0xFFFFFFFF, gdev->mmio_base + REG_ISR); /* clear flags */ > + > + ret = platform_get_irq(pdev, 0); > + if (ret > 0) { > + gdev->irq = ret; > + ret = devm_request_irq(&pdev->dev, gdev->irq, > glanda_irq_handler, > + IRQF_SHARED, "glandagpu", gdev); [Severity: Medium] What happens if platform_get_irq returns -EPROBE_DEFER or another negative error code? The check for ret > 0 seems to treat all negative error codes as a missing IRQ, falling back to polling permanently rather than returning the error to the driver core to retry. [Severity: High] Since devm_request_irq is called with IRQF_SHARED here, could the IRQ handler fire immediately before the CRTC is initialized later in probe? If the shared interrupt fires early and the INT_VSYNC bit is set, glanda_irq_handler would call drm_crtc_handle_vblank on an uninitialized crtc object, which could lead to a NULL pointer dereference. [ ... ] > +err_mode_cleanup: > + drm_mode_config_cleanup(&gdev->drm); > + return ret; > +} [Severity: High] The mode config is initialized using drm_mode_config_init, which wraps drmm_mode_config_init and automatically registers a devres action for cleanup. Does calling drm_mode_config_cleanup manually here cause a double-free on probe failure? > + > +static void glandagpu_remove(struct platform_device *pdev) > +{ > + struct glanda_device *gdev = platform_get_drvdata(pdev); > + > + /* Disable interrupts first so no new IRQ work can race the teardown > + * below, and wake up anyone still blocked in glanda_wait_idle(). > + */ > + writel(0, gdev->mmio_base + REG_IER); > + gdev->cmd_done = true; > + wake_up_interruptible(&gdev->cmd_wq); [Severity: High] By faking a successful completion instead of signaling an abort condition, any ioctl threads sleeping in glanda_wait_idle will wake up and proceed to write to mmio_base. Without proper synchronization or a call to drm_dev_enter/exit to ensure threads finish, can these awakened threads end up writing to unmapped MMIO memory as the devres teardown proceeds? > + > + drm_info(&gdev->drm, "GlandaGPU DRM Driver removed\n"); > + drm_dev_unregister(&gdev->drm); [Severity: High] Should this removal sequence include a call to drm_atomic_helper_shutdown to quiesce the display hardware? Without it, hardware pipelines might remain active after the module is unloaded, which could result in memory corruption or resource leaks. > + drm_mode_config_cleanup(&gdev->drm); > +} [Severity: High] Similar to the probe error path, does this manual call to drm_mode_config_cleanup cause a double-free on module unload, given that drm_mode_config_init already registered a devres cleanup handler? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
