Re: [PATCH v2 09/13] drm/panic: Restrict to primary planes; unconditionally unregister

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>

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/20260824125201.218863-1-tzimmermann@suse.de?part=9