| Message ID | 20240723091553.286844-6-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Cleanup and add panic description | expand |
On Tue, Jul 23, 2024 at 11:11:34AM +0200, Jocelyn Falempe wrote: > Now that kmsg dump callback has the description parameter, use it in > the user panic screen. > This is the string passed to panic(), like "VFS: Unable to mount root > fs on xxx" or "Attempted to kill init! exitcode=0xxxx". > It gives a hint on why the panic occurred, without being too cryptic. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/drm_panic.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c > index 2efede7fa23a..fb283695f50e 100644 > --- a/drivers/gpu/drm/drm_panic.c > +++ b/drivers/gpu/drm/drm_panic.c > @@ -81,6 +81,8 @@ static struct drm_panic_line panic_msg[] = { > PANIC_LINE("KERNEL PANIC!"), > PANIC_LINE(""), > PANIC_LINE("Please reboot your computer."), > + PANIC_LINE(""), > + PANIC_LINE(""), /* will be replaced by the panic description */ > }; > > static const struct drm_panic_line logo_ascii[] = { > @@ -633,7 +635,20 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb) > } > } > > -static void draw_panic_plane(struct drm_plane *plane) > +static void drm_panic_set_description(const char *description) > +{ > + u32 len; > + if (description) { #define PANIC_REASON_LINE ARRAY_SIZE(panic_msg) instead of open coding a magic for. > + panic_msg[4].txt = description; > + len = strlen(description); > + /* ignore the last newline character */ > + if (len && description[len - 1] == '\n') > + len -= 1; > + panic_msg[4].len = len; > + } > +} > + > +static void draw_panic_plane(struct drm_plane *plane, const char *description) > { > struct drm_scanout_buffer sb = { }; > int ret; > @@ -642,6 +657,8 @@ static void draw_panic_plane(struct drm_plane *plane) > if (!drm_panic_trylock(plane->dev, flags)) > return; > > + drm_panic_set_description(description); I think a drm_panic_clear_description at the end of this function would be good, so we don't leve dangling pointers around (the passed-in line might be dynamically generated and stack allocated). > + > ret = plane->helper_private->get_scanout_buffer(plane, &sb); > > if (!ret && drm_panic_is_format_supported(sb.format)) { > @@ -662,7 +679,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai > struct drm_plane *plane = to_drm_plane(dumper); > > if (detail->reason == KMSG_DUMP_PANIC) > - draw_panic_plane(plane); > + draw_panic_plane(plane, detail->description); > } > > > @@ -682,7 +699,7 @@ static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_ > if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) { > struct drm_plane *plane = file->private_data; > > - draw_panic_plane(plane); > + draw_panic_plane(plane, "Test from debugfs"); Hm, should we make this line intentionally very long, to make sure we exercise the overflow logic for too long lines? Like so long that even on a 4k panel with a small font it will overflow. I worry otherwise that there will be a bug in that logic, taking down the machine at the worst possible moment ... Reminds me, I think a bunch of kunit testcases to make sure the drm panic code is absolutely solid (there's a bunch of math in there after all) would be really good to add. Cheers, Sima > } > return count; > } > -- > 2.45.2 >
On 07/08/2024 11:21, Daniel Vetter wrote: > On Tue, Jul 23, 2024 at 11:11:34AM +0200, Jocelyn Falempe wrote: >> Now that kmsg dump callback has the description parameter, use it in >> the user panic screen. >> This is the string passed to panic(), like "VFS: Unable to mount root >> fs on xxx" or "Attempted to kill init! exitcode=0xxxx". >> It gives a hint on why the panic occurred, without being too cryptic. >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/drm_panic.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c >> index 2efede7fa23a..fb283695f50e 100644 >> --- a/drivers/gpu/drm/drm_panic.c >> +++ b/drivers/gpu/drm/drm_panic.c >> @@ -81,6 +81,8 @@ static struct drm_panic_line panic_msg[] = { >> PANIC_LINE("KERNEL PANIC!"), >> PANIC_LINE(""), >> PANIC_LINE("Please reboot your computer."), >> + PANIC_LINE(""), >> + PANIC_LINE(""), /* will be replaced by the panic description */ >> }; >> >> static const struct drm_panic_line logo_ascii[] = { >> @@ -633,7 +635,20 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb) >> } >> } >> >> -static void draw_panic_plane(struct drm_plane *plane) >> +static void drm_panic_set_description(const char *description) >> +{ >> + u32 len; >> + if (description) { > > #define PANIC_REASON_LINE ARRAY_SIZE(panic_msg) > > instead of open coding a magic for. Agreed > >> + panic_msg[4].txt = description; >> + len = strlen(description); >> + /* ignore the last newline character */ >> + if (len && description[len - 1] == '\n') >> + len -= 1; >> + panic_msg[4].len = len; >> + } >> +} >> + >> +static void draw_panic_plane(struct drm_plane *plane, const char *description) >> { >> struct drm_scanout_buffer sb = { }; >> int ret; >> @@ -642,6 +657,8 @@ static void draw_panic_plane(struct drm_plane *plane) >> if (!drm_panic_trylock(plane->dev, flags)) >> return; >> >> + drm_panic_set_description(description); > > I think a drm_panic_clear_description at the end of this function would be > good, so we don't leve dangling pointers around (the passed-in line might > be dynamically generated and stack allocated). Yes it is stack allocated. Currently it works because you only use the panic_msg from this draw_panic_plane() function. But I agree clearing the description at the end is better. > >> + >> ret = plane->helper_private->get_scanout_buffer(plane, &sb); >> >> if (!ret && drm_panic_is_format_supported(sb.format)) { >> @@ -662,7 +679,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai >> struct drm_plane *plane = to_drm_plane(dumper); >> >> if (detail->reason == KMSG_DUMP_PANIC) >> - draw_panic_plane(plane); >> + draw_panic_plane(plane, detail->description); >> } >> >> >> @@ -682,7 +699,7 @@ static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_ >> if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) { >> struct drm_plane *plane = file->private_data; >> >> - draw_panic_plane(plane); >> + draw_panic_plane(plane, "Test from debugfs"); > > Hm, should we make this line intentionally very long, to make sure we > exercise the overflow logic for too long lines? Like so long that even on > a 4k panel with a small font it will overflow. I worry otherwise that > there will be a bug in that logic, taking down the machine at the worst > possible moment ... longer lines are truncated, so that the code stays simple, and ensure it won't draw out of the screen. > > Reminds me, I think a bunch of kunit testcases to make sure the drm panic > code is absolutely solid (there's a bunch of math in there after all) > would be really good to add. Yes, I need to add some kunit testcases. When writing the drawing code, I used a small C program and write the result to a png, so I was able to check for different resolutions, and it's easier to debug than in the kernel. I will send a v2. > > Cheers, Sima > >> } >> return count; >> } >> -- >> 2.45.2 >> >
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 2efede7fa23a..fb283695f50e 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -81,6 +81,8 @@ static struct drm_panic_line panic_msg[] = { PANIC_LINE("KERNEL PANIC!"), PANIC_LINE(""), PANIC_LINE("Please reboot your computer."), + PANIC_LINE(""), + PANIC_LINE(""), /* will be replaced by the panic description */ }; static const struct drm_panic_line logo_ascii[] = { @@ -633,7 +635,20 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb) } } -static void draw_panic_plane(struct drm_plane *plane) +static void drm_panic_set_description(const char *description) +{ + u32 len; + if (description) { + panic_msg[4].txt = description; + len = strlen(description); + /* ignore the last newline character */ + if (len && description[len - 1] == '\n') + len -= 1; + panic_msg[4].len = len; + } +} + +static void draw_panic_plane(struct drm_plane *plane, const char *description) { struct drm_scanout_buffer sb = { }; int ret; @@ -642,6 +657,8 @@ static void draw_panic_plane(struct drm_plane *plane) if (!drm_panic_trylock(plane->dev, flags)) return; + drm_panic_set_description(description); + ret = plane->helper_private->get_scanout_buffer(plane, &sb); if (!ret && drm_panic_is_format_supported(sb.format)) { @@ -662,7 +679,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai struct drm_plane *plane = to_drm_plane(dumper); if (detail->reason == KMSG_DUMP_PANIC) - draw_panic_plane(plane); + draw_panic_plane(plane, detail->description); } @@ -682,7 +699,7 @@ static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_ if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) { struct drm_plane *plane = file->private_data; - draw_panic_plane(plane); + draw_panic_plane(plane, "Test from debugfs"); } return count; }
Now that kmsg dump callback has the description parameter, use it in the user panic screen. This is the string passed to panic(), like "VFS: Unable to mount root fs on xxx" or "Attempted to kill init! exitcode=0xxxx". It gives a hint on why the panic occurred, without being too cryptic. Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/drm_panic.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)