Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Unconditional calls to kmsg_dump_unregister() for all planes cause severe delays during device teardown due to sequential synchronize_rcu() calls. - [Medium] drm_panic_is_enabled() was not updated to enforce the primary plane restriction, leading to inconsistent panic handler state. --
--- Patch [9]: [PATCH v2 09/13] drm/panic: Restrict to primary planes; unconditionally unregister --- commit c3117dcbe59bcb2d0d31d68229c5ec24738e412e Author: Thomas Zimmermann <[email protected]> drm/panic: Restrict to primary planes; unconditionally unregister Restrict panic handling to primary displays. Overlays and cursors should not display panic output. If there are exceptions, they can be registered by the driver itself. For cleanup, unregister each plane unconditionally. The kmsg dumper already keeps track of the registration status so DRM doesn't have to. > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c > index 2048a3bf736f0..7dbbcc1c219ff 100644 > --- a/drivers/gpu/drm/drm_panic.c > +++ b/drivers/gpu/drm/drm_panic.c > @@ -1079,6 +1079,8 @@ void drm_panic_register(struct drm_device *dev) > return; > > drm_for_each_plane(plane, dev) { > + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > + continue; [Severity: Medium] Was drm_panic_is_enabled() updated to match this new primary plane restriction? If a driver implements the panic callback on a non-primary plane but not on a primary plane, drm_panic_is_enabled() will return true since it lacks this type check: drivers/gpu/drm/drm_panic.c:drm_panic_is_enabled() { ... drm_for_each_plane(plane, dev) if (plane->helper_private && plane->helper_private->get_scanout_buffer) return true; ... } Because drm_panic_register() skips the non-primary plane, it won't actually be registered. Does this false positive in drm_panic_is_enabled() introduce a regression where drm_fb_helper disables its fallback panic handling, leaving the system with no panic screen handler at all? > if (!plane->helper_private || > !plane->helper_private->get_scanout_buffer) > continue; > plane->kmsg_panic.dump = drm_panic; [ ... ] > @@ -1105,11 +1107,8 @@ void drm_panic_unregister(struct drm_device *dev) > if (!dev->mode_config.num_total_plane) > return; > > - drm_for_each_plane(plane, dev) { > - if (!plane->helper_private || > !plane->helper_private->get_scanout_buffer) > - continue; > + drm_for_each_plane(plane, dev) > kmsg_dump_unregister(&plane->kmsg_panic); > - } > } [Severity: Medium] Does this unconditional loop cause severe delays during device teardown? The function kmsg_dump_unregister() executes synchronize_rcu() at the end regardless of the dumper's registration status: kernel/printk/printk.c:kmsg_dump_unregister() { ... spin_unlock_irqrestore(&dump_list_lock, flags); synchronize_rcu(); ... } Looping sequentially over all planes without checking if they were actually registered forces the teardown path to block for multiple consecutive RCU grace periods. Can this regression potentially add hundreds of milliseconds to several seconds of delay during driver unbind or hot-unplug? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
