| Message ID | 20240603095343.39588-4-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a kmsg dump screen | expand |
Hi Jocelyn,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-panic-only-draw-the-foreground-color-in-drm_panic_blit/20240603-181247
base: 86266829ea755f737762ebda614c59b136c8feac
patch link: https://lore.kernel.org/r/20240603095343.39588-4-jfalempe%40redhat.com
patch subject: [PATCH v2 3/3] drm/panic: Add a kmsg panic screen
config: i386-randconfig-141-20240604 (https://download.01.org/0day-ci/archive/20240604/202406041051.KuVqtZCd-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202406041051.KuVqtZCd-lkp@intel.com/
smatch warnings:
drivers/gpu/drm/drm_panic.c:531 draw_panic_static_kmsg() warn: variable dereferenced before check 'font' (see line 529)
vim +/font +531 drivers/gpu/drm/drm_panic.c
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 519 static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb)
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 520 {
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 521 u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format);
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 522 u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 523 const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 524 struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 525 struct kmsg_dump_iter iter;
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 526 char kmsg_buf[512];
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 527 size_t kmsg_len;
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 528 struct drm_panic_line line;
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 @529 int yoffset = sb->height - font->height - (sb->height % font->height) / 2;
^^^^^^^^^^^^ ^^^^^^^^^^^^
Unchecked dereferences
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 530
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 @531 if (!font)
^^^^
Checked too late
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 532 return;
c259bba1e69ff2 Jocelyn Falempe 2024-06-03 533
Jocelyn Falempe <jfalempe@redhat.com> writes: > Add a kmsg option, which will display the last lines of kmsg, > and should be similar to fbcon. > Add a drm.panic_screen module parameter, so you can choose between > the different panic screens available. > two options currently, but more will be added later: > * "user": a short message telling the user to reboot the machine. > * "kmsg": fill the screen with the last lines of kmsg. > > You can even change it at runtime by writing to > /sys/module/drm/parameters/panic_screen > Great! > v2: > * use module parameter instead of Kconfig choice > (Javier Martinez Canillas) > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/Kconfig | 11 ++++ > drivers/gpu/drm/drm_panic.c | 108 ++++++++++++++++++++++++++++++++---- > 2 files changed, 109 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 9703429de6b9..944815cee080 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG > This is unsafe and should not be enabled on a production build. > If in doubt, say "N". > > +config DRM_PANIC_SCREEN > + string "Panic screen formater" > + default "user" > + depends on DRM_PANIC > + help > + This option enable to choose what will be displayed when a kernel > + panic occurs. You can choose between "user", a short message telling > + the user to reboot the system, or "kmsg" which will display the last > + lines of kmsg. Maybe I would mention here that this is only about the default, but that can be changed using the "drm.panic_screen=" kernel cmdline parameter or writting to the /sys/module/drm/parameters/panic_screen sysfs entry. [...] > +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb) > +{ > + u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); > + u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); > + const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); Dan reported that get_default_font() can return NULL.... > + struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); > + struct kmsg_dump_iter iter; > + char kmsg_buf[512]; > + size_t kmsg_len; > + struct drm_panic_line line; > + int yoffset = sb->height - font->height - (sb->height % font->height) / 2; > + > + if (!font) > + return; > + ... so you have to calculate yoffset after checking if the font is not NULL. with that fixed: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 07/06/2024 11:16, Javier Martinez Canillas wrote: > Jocelyn Falempe <jfalempe@redhat.com> writes: > >> Add a kmsg option, which will display the last lines of kmsg, >> and should be similar to fbcon. >> Add a drm.panic_screen module parameter, so you can choose between >> the different panic screens available. >> two options currently, but more will be added later: >> * "user": a short message telling the user to reboot the machine. >> * "kmsg": fill the screen with the last lines of kmsg. >> >> You can even change it at runtime by writing to >> /sys/module/drm/parameters/panic_screen >> > > Great! > >> v2: >> * use module parameter instead of Kconfig choice >> (Javier Martinez Canillas) >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/Kconfig | 11 ++++ >> drivers/gpu/drm/drm_panic.c | 108 ++++++++++++++++++++++++++++++++---- >> 2 files changed, 109 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index 9703429de6b9..944815cee080 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG >> This is unsafe and should not be enabled on a production build. >> If in doubt, say "N". >> >> +config DRM_PANIC_SCREEN >> + string "Panic screen formater" >> + default "user" >> + depends on DRM_PANIC >> + help >> + This option enable to choose what will be displayed when a kernel >> + panic occurs. You can choose between "user", a short message telling >> + the user to reboot the system, or "kmsg" which will display the last >> + lines of kmsg. > > Maybe I would mention here that this is only about the default, but that > can be changed using the "drm.panic_screen=" kernel cmdline parameter or > writting to the /sys/module/drm/parameters/panic_screen sysfs entry. > > [...] Done > >> +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb) >> +{ >> + u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); >> + u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); >> + const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); > > Dan reported that get_default_font() can return NULL.... > >> + struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); >> + struct kmsg_dump_iter iter; >> + char kmsg_buf[512]; >> + size_t kmsg_len; >> + struct drm_panic_line line; >> + int yoffset = sb->height - font->height - (sb->height % font->height) / 2; >> + >> + if (!font) >> + return; >> + > > ... so you have to calculate yoffset after checking if the font is not NULL. Yes I fixed that too. > > with that fixed: > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Thanks a lot. I just pushed this series to drm-misc-next. Best regards,
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 9703429de6b9..944815cee080 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG This is unsafe and should not be enabled on a production build. If in doubt, say "N". +config DRM_PANIC_SCREEN + string "Panic screen formater" + default "user" + depends on DRM_PANIC + help + This option enable to choose what will be displayed when a kernel + panic occurs. You can choose between "user", a short message telling + the user to reboot the system, or "kmsg" which will display the last + lines of kmsg. + Default is "user" + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 5dc9e98108ed..2e11273a8ad6 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -12,6 +12,7 @@ #include <linux/kmsg_dump.h> #include <linux/list.h> #include <linux/module.h> +#include <linux/printk.h> #include <linux/types.h> #include <drm/drm_drv.h> @@ -27,6 +28,12 @@ MODULE_AUTHOR("Jocelyn Falempe"); MODULE_DESCRIPTION("DRM panic handler"); MODULE_LICENSE("GPL"); +static char drm_panic_screen[16] = CONFIG_DRM_PANIC_SCREEN; +module_param_string(panic_screen, drm_panic_screen, sizeof(drm_panic_screen), 0644); +MODULE_PARM_DESC(panic_screen, + "Choose what will be displayed by drm_panic, 'user' or 'kmsg' [default=" + CONFIG_DRM_PANIC_SCREEN "]"); + /** * DOC: overview * @@ -437,24 +444,18 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, } } -/* - * Draw the panic message at the center of the screen - */ -static void draw_panic_static(struct drm_scanout_buffer *sb) +static void draw_panic_static_user(struct drm_scanout_buffer *sb) { size_t msg_lines = ARRAY_SIZE(panic_msg); size_t logo_lines = ARRAY_SIZE(logo); - u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR; - u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR; + u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); + u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); struct drm_rect r_screen, r_logo, r_msg; if (!font) return; - fg_color = convert_from_xrgb8888(fg_color, sb->format->format); - bg_color = convert_from_xrgb8888(bg_color, sb->format->format); - r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); r_logo = DRM_RECT_INIT(0, 0, @@ -477,6 +478,84 @@ static void draw_panic_static(struct drm_scanout_buffer *sb) draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color); } + +/* + * Draw one line of kmsg, and handle wrapping if it won't fit in the screen width. + * Return the y-offset of the next line. + */ +static int draw_line_with_wrap(struct drm_scanout_buffer *sb, const struct font_desc *font, + struct drm_panic_line *line, int yoffset, u32 fg_color) +{ + int chars_per_row = sb->width / font->width; + struct drm_rect r_txt = DRM_RECT_INIT(0, yoffset, sb->width, sb->height); + struct drm_panic_line line_wrap; + + if (line->len > chars_per_row) { + line_wrap.len = line->len % chars_per_row; + line_wrap.txt = line->txt + line->len - line_wrap.len; + draw_txt_rectangle(sb, font, &line_wrap, 1, false, &r_txt, fg_color); + r_txt.y1 -= font->height; + if (r_txt.y1 < 0) + return r_txt.y1; + while (line_wrap.txt > line->txt) { + line_wrap.txt -= chars_per_row; + line_wrap.len = chars_per_row; + draw_txt_rectangle(sb, font, &line_wrap, 1, false, &r_txt, fg_color); + r_txt.y1 -= font->height; + if (r_txt.y1 < 0) + return r_txt.y1; + } + } else { + draw_txt_rectangle(sb, font, line, 1, false, &r_txt, fg_color); + r_txt.y1 -= font->height; + } + return r_txt.y1; +} + +/* + * Draw the kmsg buffer to the screen, starting from the youngest message at the bottom, + * and going up until reaching the top of the screen. + */ +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb) +{ + u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); + u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); + const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); + struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); + struct kmsg_dump_iter iter; + char kmsg_buf[512]; + size_t kmsg_len; + struct drm_panic_line line; + int yoffset = sb->height - font->height - (sb->height % font->height) / 2; + + if (!font) + return; + + /* Fill with the background color, and draw text on top */ + drm_panic_fill(sb, &r_screen, bg_color); + + kmsg_dump_rewind(&iter); + while (kmsg_dump_get_buffer(&iter, false, kmsg_buf, sizeof(kmsg_buf), &kmsg_len)) { + char *start; + char *end; + + /* ignore terminating NUL and newline */ + start = kmsg_buf + kmsg_len - 2; + end = kmsg_buf + kmsg_len - 1; + while (start > kmsg_buf && yoffset >= 0) { + while (start > kmsg_buf && *start != '\n') + start--; + /* don't count the newline character */ + line.txt = start + (start == kmsg_buf ? 0 : 1); + line.len = end - line.txt; + + yoffset = draw_line_with_wrap(sb, font, &line, yoffset, fg_color); + end = start; + start--; + } + } +} + /* * drm_panic_is_format_supported() * @format: a fourcc color code @@ -491,6 +570,15 @@ static bool drm_panic_is_format_supported(const struct drm_format_info *format) return convert_from_xrgb8888(0xffffff, format->format) != 0; } +static void draw_panic_dispatch(struct drm_scanout_buffer *sb) +{ + if (!strcmp(drm_panic_screen, "kmsg")) { + draw_panic_static_kmsg(sb); + } else { + draw_panic_static_user(sb); + } +} + static void draw_panic_plane(struct drm_plane *plane) { struct drm_scanout_buffer sb; @@ -503,7 +591,7 @@ static void draw_panic_plane(struct drm_plane *plane) ret = plane->helper_private->get_scanout_buffer(plane, &sb); if (!ret && drm_panic_is_format_supported(sb.format)) { - draw_panic_static(&sb); + draw_panic_dispatch(&sb); if (plane->helper_private->panic_flush) plane->helper_private->panic_flush(plane); }
Add a kmsg option, which will display the last lines of kmsg, and should be similar to fbcon. Add a drm.panic_screen module parameter, so you can choose between the different panic screens available. two options currently, but more will be added later: * "user": a short message telling the user to reboot the machine. * "kmsg": fill the screen with the last lines of kmsg. You can even change it at runtime by writing to /sys/module/drm/parameters/panic_screen v2: * use module parameter instead of Kconfig choice (Javier Martinez Canillas) Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/Kconfig | 11 ++++ drivers/gpu/drm/drm_panic.c | 108 ++++++++++++++++++++++++++++++++---- 2 files changed, 109 insertions(+), 10 deletions(-)