Re: [PATCH v2 07/13] drm/panic: Retry in dispatch function if panic output fails

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message claims a change that is not present in the code diff.
- [Medium] Fallback to user mode silently flushes a blank screen if the screen is narrower than a single character, defeating failure preservation.
--

--- Patch [7]: [PATCH v2 07/13] drm/panic: Retry in dispatch function if panic output fails ---
commit 43648cf544bdeed6f6c85be5e74f3e855c778814
Author: Thomas Zimmermann <[email protected]>

drm/panic: Retry in dispatch function if panic output fails

This patch introduces explicit fallback to DRM_PANIC_TYPE_USER in
drm_panic_dispatch() when QR code or kmsg outputs fail. It also increases the
minimum QR code test size to 494x494.

> Also pass module parameter drm_panic_type as a parameter to
> drm_panic_dispatch(). This simpifies the implementation. It will later
> allow for moving the function into a helper library.

[Severity: Medium]
This isn't a bug, but the commit message mentions passing drm_panic_type as a
parameter to drm_panic_dispatch(), which isn't present in this patch's code
changes. Was this already implemented in a preceding commit?

> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index f597ef04c5bcb..bc7c096c42363 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c

[ ... ]

> @@ -895,13 +886,22 @@ static int draw_panic_dispatch(struct drm_scanout_buffer *sb, enum drm_panic_typ
>  {
>  	int ret;
>  
> +retry:
>  	switch (panic_type) {
>  	case DRM_PANIC_TYPE_KMSG:
>  		ret = draw_panic_screen_kmsg(sb, fg_color, bg_color);
> +		if (ret) {
> +			panic_type = DRM_PANIC_TYPE_USER;
> +			goto retry;
> +		}

[Severity: Medium]
Does this fallback logic inadvertently cause the screen to be overwritten with
a blank screen if the screen is narrower than a single character?

If drm_panic_type is set to KMSG and sb->width < font->width,
draw_panic_screen_kmsg() returns -EINVAL. The retry logic here catches this
and falls back to DRM_PANIC_TYPE_USER.

However, looking at draw_panic_screen_user(), it lacks a similar width check.
It clears the screen with the background color:

drivers/gpu/drm/drm_panic.c:draw_panic_screen_user() {
    ...
    /* Fill with the background color, and draw text on top */
    drm_panic_fill(sb, &r_screen, bg_color);
    ...
}

It then returns 0 (success) without drawing any text. This success code causes
draw_panic_plane() to flush the blank screen, destroying the existing visual
evidence instead of preserving it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7