Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Leaking vblank event and causing userspace to hang indefinitely on device unplug. - [High] Bailing out early from CRTC disable skips mandatory DRM software state cleanup (`drm_crtc_vblank_off`). - [High] Calling `drm_dev_unplug()` before `drm_atomic_helper_shutdown()` aborts hardware shutdown, leading to DMA use-after-free and system hangs. - [High] Omitting `drm_dev_enter()` in the IRQ handler risks unclocked register access and system crashes during device removal. - [Medium] The external DRM panel is not properly disabled and unprepared upon device unplug. --
--- Patch [2]: [PATCH v3 2/2] drm/logicvc: Avoid using DRM resources after device is unplugged --- commit 984a88d7dda613a6824feda7d8e4824b35006bb5 Author: Romain Gantois <[email protected]> Subject: drm/logicvc: Avoid using DRM resources after device is unplugged Some DRM resources such as plane, CRTC or encoder objects could remain in use after the DRM device is removed. Use the drm_dev_enter/exit() mechanism to ensure that the DRM device is not unplugged before using its resources. > diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.c > b/drivers/gpu/drm/logicvc/logicvc_crtc.c > index 3a4c347eaa648..e2575fa78ab25 100644 > --- a/drivers/gpu/drm/logicvc/logicvc_crtc.c > +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.c > @@ -36,6 +36,21 @@ logicvc_crtc_mode_valid(struct drm_crtc *drm_crtc, > return 0; > } > > +static void logicvc_crtc_drop_any_event(struct drm_device *drm_dev, > + struct drm_crtc *drm_crtc) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&drm_dev->event_lock, flags); > + > + if (drm_crtc->state->event) { > + drm_crtc->state->event = NULL; [Severity: High] Does setting this event to NULL leak the drm_pending_vblank_event memory? Without calling drm_crtc_send_vblank_event(), will userspace compositors waiting for the page flip completion hang indefinitely during an unplug? > + drm_warn(drm_crtc->dev, "Device is unplugged, ignoring pending > vblank event!"); > + } > + > + spin_unlock_irqrestore(&drm_dev->event_lock, flags); > +} [ ... ] > @@ -155,6 +186,12 @@ static void logicvc_crtc_atomic_disable(struct drm_crtc > *drm_crtc, > { > struct logicvc_drm *logicvc = logicvc_drm(drm_crtc->dev); > struct drm_device *drm_dev = drm_crtc->dev; > + int idx; > + > + if (!drm_dev_enter(drm_dev, &idx)) { > + logicvc_crtc_drop_any_event(drm_dev, drm_crtc); > + return; > + } [Severity: High] If we return early here during an unplug, we skip the call to drm_crtc_vblank_off(). Does this leave the DRM core's vblank state machine active, potentially orphaning waitqueues or leaving background works pending? > > drm_crtc_vblank_off(drm_crtc); [ ... ] > diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c > b/drivers/gpu/drm/logicvc/logicvc_drm.c > index bbebf4fc7f51a..fb66f5fb67937 100644 > --- a/drivers/gpu/drm/logicvc/logicvc_drm.c > +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c > @@ -72,6 +72,10 @@ static irqreturn_t logicvc_drm_irq_handler(int irq, void > *data) > irqreturn_t ret = IRQ_NONE; > u32 stat = 0; > > + /* The interrupt handler will be unregistered when the device is > + * removed. Therefore, there's no need for drm_dev_enter() here. > + */ > + > /* Get pending interrupt sources. */ > regmap_read(logicvc->regmap, LOGICVC_INT_STAT_REG, &stat); [Severity: High] Since the interrupt handler relies on devres for unregistration, it remains active after logicvc_drm_remove() completes. Inside logicvc_drm_remove(), logicvc_clocks_unprepare() is called which disables the clocks. If this handler fires afterwards (e.g., from a shared IRQ line), won't reading LOGICVC_INT_STAT_REG without clocks cause an imprecise external abort and crash the system? > > @@ -463,7 +467,7 @@ static void logicvc_drm_remove(struct platform_device > *pdev) > struct device *dev = &pdev->dev; > struct drm_device *drm_dev = &logicvc->drm_dev; > > - drm_dev_unregister(drm_dev); > + drm_dev_unplug(drm_dev); > drm_atomic_helper_shutdown(drm_dev); [Severity: High] Calling drm_dev_unplug() here sets the device unplugged state, which causes subsequent drm_dev_enter() checks to fail in functions like logicvc_plane_atomic_check(). If logicvc_plane_atomic_check() returns -ENODEV, won't this abort the drm_atomic_helper_shutdown() commit? If the hardware display controller is never stopped and continues fetching from memory, disabling the clocks later could cause an AXI bus lockup. > > logicvc_mode_fini(logicvc); [ ... ] > diff --git a/drivers/gpu/drm/logicvc/logicvc_interface.c > b/drivers/gpu/drm/logicvc/logicvc_interface.c > index 0d037f37b950f..aa13338a29535 100644 > --- a/drivers/gpu/drm/logicvc/logicvc_interface.c > +++ b/drivers/gpu/drm/logicvc/logicvc_interface.c > @@ -43,17 +47,25 @@ static void logicvc_encoder_enable(struct drm_encoder > *drm_encoder) [ ... ] > static void logicvc_encoder_disable(struct drm_encoder *drm_encoder) > { > struct logicvc_interface *interface = > logicvc_interface_from_drm_encoder(drm_encoder); > + int idx; > + > + if (!drm_dev_enter(drm_encoder->dev, &idx)) > + return; [Severity: Medium] Does returning early here skip disabling and unpreparing the DRM panel? If the device is unplugged, skipping drm_panel_disable() and drm_panel_unprepare() could leave the panel permanently powered on with its regulators and backlight enabled, causing a resource leak. > > if (interface->drm_panel) { > drm_panel_disable(interface->drm_panel); > drm_panel_unprepare(interface->drm_panel); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
