[PATCH] drm/panic: depends on !VT_CONSOLE

[Jocelyn Falempe]

On 17/06/2024 11:27, Javier Martinez Canillas wrote:
> 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>
> 

Thanks all for your comments and reviews.

I just pushed it to drm-misc-next.

-- 

Jocelyn