| Message ID | 20240613154041.325964-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panic: depends on !VT_CONSOLE | expand |
Jocelyn Falempe <jfalempe@redhat.com> writes: Hello Jocelyn, > The race condition between fbcon and drm_panic can only occurs if > VT_CONSOLE is set. So update drm_panic dependency accordingly. > This will make it easier for Linux distributions to enable drm_panic > by disabling VT_CONSOLE, and keeping fbcon terminal. > The only drawback is that fbcon won't display the boot kmsg, so it > should rely on userspace to do that. > At least plymouth already handle this case with > https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index a9df94291622..f5c989aed7e9 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -107,7 +107,7 @@ config DRM_KMS_HELPER > > config DRM_PANIC > bool "Display a user-friendly message when a kernel panic occurs" > - depends on DRM && !FRAMEBUFFER_CONSOLE > + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) I thought the idea was to only make it depend on !VT_CONSOLE, so that distros could also enable fbcon / VT but prevent the race condition to happen due the VT not being a system console for the kernel to print messages ? In other words, my understanding from the discussion with Sima was that this should be instead: + depends on DRM && !VT_CONSOLE I've tested that and at least I see that a framebuffer console is present and `echo c > /proc/sysrq-trigger` triggers the DRM panic handler message (but don't know if the race exists and is just that I was not hitting it). If my understanding is correct and should only be a depends on !VT_CONSOLE then feel free to add my: Tested-by: Javier Martinez Canillas <javierm@redhat.com>
On 16/06/2024 14:43, Javier Martinez Canillas wrote: > Jocelyn Falempe <jfalempe@redhat.com> writes: > > Hello Jocelyn, > >> The race condition between fbcon and drm_panic can only occurs if >> VT_CONSOLE is set. So update drm_panic dependency accordingly. >> This will make it easier for Linux distributions to enable drm_panic >> by disabling VT_CONSOLE, and keeping fbcon terminal. >> The only drawback is that fbcon won't display the boot kmsg, so it >> should rely on userspace to do that. >> At least plymouth already handle this case with >> https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 >> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index a9df94291622..f5c989aed7e9 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER >> >> config DRM_PANIC >> bool "Display a user-friendly message when a kernel panic occurs" >> - depends on DRM && !FRAMEBUFFER_CONSOLE >> + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) > > I thought the idea was to only make it depend on !VT_CONSOLE, so that > distros could also enable fbcon / VT but prevent the race condition to > happen due the VT not being a system console for the kernel to print > messages ? Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and drm_panic can be enabled safely. I don't know if that really matters, and if VT_CONSOLE has any usage apart from fbcon. > > In other words, my understanding from the discussion with Sima was that > this should be instead: > > + depends on DRM && !VT_CONSOLE > > I've tested that and at least I see that a framebuffer console is present > and `echo c > /proc/sysrq-trigger` triggers the DRM panic handler message > (but don't know if the race exists and is just that I was not hitting it). > > If my understanding is correct and should only be a depends on !VT_CONSOLE > then feel free to add my: > > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > Best regards,
Hi Jocelyn, On Mon, Jun 17, 2024 at 10:16 AM Jocelyn Falempe <jfalempe@redhat.com> wrote: > On 16/06/2024 14:43, Javier Martinez Canillas wrote: > > Jocelyn Falempe <jfalempe@redhat.com> writes: > >> The race condition between fbcon and drm_panic can only occurs if > >> VT_CONSOLE is set. So update drm_panic dependency accordingly. > >> This will make it easier for Linux distributions to enable drm_panic > >> by disabling VT_CONSOLE, and keeping fbcon terminal. > >> The only drawback is that fbcon won't display the boot kmsg, so it > >> should rely on userspace to do that. > >> At least plymouth already handle this case with > >> https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 > >> > >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > >> --- > >> drivers/gpu/drm/Kconfig | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > >> index a9df94291622..f5c989aed7e9 100644 > >> --- a/drivers/gpu/drm/Kconfig > >> +++ b/drivers/gpu/drm/Kconfig > >> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER > >> > >> config DRM_PANIC > >> bool "Display a user-friendly message when a kernel panic occurs" > >> - depends on DRM && !FRAMEBUFFER_CONSOLE > >> + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) > > > > I thought the idea was to only make it depend on !VT_CONSOLE, so that > > distros could also enable fbcon / VT but prevent the race condition to > > happen due the VT not being a system console for the kernel to print > > messages ? > > Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y > and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and > drm_panic can be enabled safely. > I don't know if that really matters, and if VT_CONSOLE has any usage > apart from fbcon. It is used for any kind of virtual terminal, so also for vgacon. Gr{oetje,eeting}s, Geert
Jocelyn Falempe <jfalempe@redhat.com> writes: > On 16/06/2024 14:43, Javier Martinez Canillas wrote: >> Jocelyn Falempe <jfalempe@redhat.com> writes: >> >> Hello Jocelyn, >> >>> The race condition between fbcon and drm_panic can only occurs if >>> VT_CONSOLE is set. So update drm_panic dependency accordingly. >>> This will make it easier for Linux distributions to enable drm_panic >>> by disabling VT_CONSOLE, and keeping fbcon terminal. >>> The only drawback is that fbcon won't display the boot kmsg, so it >>> should rely on userspace to do that. >>> At least plymouth already handle this case with >>> https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 >>> >>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> --- >>> drivers/gpu/drm/Kconfig | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>> index a9df94291622..f5c989aed7e9 100644 >>> --- a/drivers/gpu/drm/Kconfig >>> +++ b/drivers/gpu/drm/Kconfig >>> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER >>> >>> config DRM_PANIC >>> bool "Display a user-friendly message when a kernel panic occurs" >>> - depends on DRM && !FRAMEBUFFER_CONSOLE >>> + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) >> >> I thought the idea was to only make it depend on !VT_CONSOLE, so that >> distros could also enable fbcon / VT but prevent the race condition to >> happen due the VT not being a system console for the kernel to print >> messages ? > > Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y > and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and > drm_panic can be enabled safely. Ah, I see what you mean. > I don't know if that really matters, and if VT_CONSOLE has any usage > apart from fbcon. > Right. I don't know...
On 17/06/2024 10:25, Geert Uytterhoeven wrote: > Hi Jocelyn, > > On Mon, Jun 17, 2024 at 10:16 AM Jocelyn Falempe <jfalempe@redhat.com> wrote: >> On 16/06/2024 14:43, Javier Martinez Canillas wrote: >>> Jocelyn Falempe <jfalempe@redhat.com> writes: >>>> The race condition between fbcon and drm_panic can only occurs if >>>> VT_CONSOLE is set. So update drm_panic dependency accordingly. >>>> This will make it easier for Linux distributions to enable drm_panic >>>> by disabling VT_CONSOLE, and keeping fbcon terminal. >>>> The only drawback is that fbcon won't display the boot kmsg, so it >>>> should rely on userspace to do that. >>>> At least plymouth already handle this case with >>>> https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 >>>> >>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>> --- >>>> drivers/gpu/drm/Kconfig | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>>> index a9df94291622..f5c989aed7e9 100644 >>>> --- a/drivers/gpu/drm/Kconfig >>>> +++ b/drivers/gpu/drm/Kconfig >>>> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER >>>> >>>> config DRM_PANIC >>>> bool "Display a user-friendly message when a kernel panic occurs" >>>> - depends on DRM && !FRAMEBUFFER_CONSOLE >>>> + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) >>> >>> I thought the idea was to only make it depend on !VT_CONSOLE, so that >>> distros could also enable fbcon / VT but prevent the race condition to >>> happen due the VT not being a system console for the kernel to print >>> messages ? >> >> Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y >> and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and >> drm_panic can be enabled safely. >> I don't know if that really matters, and if VT_CONSOLE has any usage >> apart from fbcon. > > It is used for any kind of virtual terminal, so also for vgacon. Ah thanks, but I think vgacon is safe to use with drm_panic. The problem with fbcon, is that it can also uses the fbdev emulation from the drm driver, and in this case, drm_panic will write to the same framebuffer as fbcon during a panic, leading to corrupted output. This can't occur with other graphical console, so !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) is probably good.
Jocelyn Falempe <jfalempe@redhat.com> writes: > On 17/06/2024 10:25, Geert Uytterhoeven wrote: >> Hi Jocelyn, >> >> On Mon, Jun 17, 2024 at 10:16 AM Jocelyn Falempe <jfalempe@redhat.com> wrote: >>> On 16/06/2024 14:43, Javier Martinez Canillas wrote: >>>> Jocelyn Falempe <jfalempe@redhat.com> writes: >>>>> The race condition between fbcon and drm_panic can only occurs if >>>>> VT_CONSOLE is set. So update drm_panic dependency accordingly. >>>>> This will make it easier for Linux distributions to enable drm_panic >>>>> by disabling VT_CONSOLE, and keeping fbcon terminal. >>>>> The only drawback is that fbcon won't display the boot kmsg, so it >>>>> should rely on userspace to do that. >>>>> At least plymouth already handle this case with >>>>> https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 >>>>> >>>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>>>> --- >>>>> drivers/gpu/drm/Kconfig | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>>>> index a9df94291622..f5c989aed7e9 100644 >>>>> --- a/drivers/gpu/drm/Kconfig >>>>> +++ b/drivers/gpu/drm/Kconfig >>>>> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER >>>>> >>>>> config DRM_PANIC >>>>> bool "Display a user-friendly message when a kernel panic occurs" >>>>> - depends on DRM && !FRAMEBUFFER_CONSOLE >>>>> + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) >>>> >>>> I thought the idea was to only make it depend on !VT_CONSOLE, so that >>>> distros could also enable fbcon / VT but prevent the race condition to >>>> happen due the VT not being a system console for the kernel to print >>>> messages ? >>> >>> Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y >>> and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and >>> drm_panic can be enabled safely. >>> I don't know if that really matters, and if VT_CONSOLE has any usage >>> apart from fbcon. >> >> It is used for any kind of virtual terminal, so also for vgacon. > > Ah thanks, but I think vgacon is safe to use with drm_panic. > > The problem with fbcon, is that it can also uses the fbdev emulation > from the drm driver, and in this case, drm_panic will write to the same > framebuffer as fbcon during a panic, leading to corrupted output. > This can't occur with other graphical console, so !(FRAMEBUFFER_CONSOLE > && VT_CONSOLE) is probably good. > Yeah, I agre that !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) as you have in your patch is what makes sense. !VT_CONSOLE alone as I argued in my first email isn't correct since as you pointed out, it shouldn't affect other consoles besides fbcon. So please feel free to also add: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9df94291622..f5c989aed7e9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -107,7 +107,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" - depends on DRM && !FRAMEBUFFER_CONSOLE + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) select DRM_KMS_HELPER select FONT_SUPPORT help
The race condition between fbcon and drm_panic can only occurs if VT_CONSOLE is set. So update drm_panic dependency accordingly. This will make it easier for Linux distributions to enable drm_panic by disabling VT_CONSOLE, and keeping fbcon terminal. The only drawback is that fbcon won't display the boot kmsg, so it should rely on userspace to do that. At least plymouth already handle this case with https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 2bae076f3e35234e42bd7c90acd8caae8368ba90