[PATCH] drm/panic: depends on !VT_CONSOLE

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe at 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 at redhat.com> wrote:
>>> On 16/06/2024 14:43, Javier Martinez Canillas wrote:
>>>> Jocelyn Falempe <jfalempe at 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 at ffwll.ch>
>>>>> Signed-off-by: Jocelyn Falempe <jfalempe at 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 at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat