Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11-07-18 17:28, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 11-07-18 17:07, Thomas Zimmermann wrote:
>>>
>>> Hi
>>>
>>> Am 11.07.2018 um 16:52 schrieb Steven Rostedt:
>>>>
>>>>
>>>> What if you make lockless_register_fb visible to fbcon, and then we can
>>>> have a macro:
>>>
>>>
>>> There are more of these macro invocations under drivers/tty/vt, which
>>> also mess up the log during debugging.
>>
>>
>> Hmm, so this option is already broken (in a way) then, my first reaction
>> to your mail was that we should just remove that option. But that seemed
>> a bit harsh to me so I've been working on a fix for the last 10 minutes
>> or so.
>>
>> But if it is already broken I'm tempted to just remove the option and
>> be done with it.  We really need less cruft in the fbdev/fbcon code not
>> more.
> 
> Please don't remove it, it makes debugging kms driver issues on
> initial modeset (which is usually run from framebuffer_register, while
> hodling the console_lock) impossible.

OK, so if we don't remove it, we should probably make it so that it
can be used without triggering any WARN_ONs, which would require changing
the existing WARN_CONSOLE_UNLOCKED() so that the calls from drivers/tty/vt/vt.c
also do not trigger it ?

I guess one can just ignore the oopses when debugging, but debugging surely
would be easier if there are just no oopses ?

Regards,

Hans




> -Daniel
> 
>>> WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
>>> ...' construct [1]. I thought about turning this into a config option.
>>
>>
>> Ah I noticed the #if but I did not notice that it was a "#if 1".
>>
>> If you want to fix lockless_register_fb it really should be replaced
>> with a runtime check, not a Kconfig option. This would require having
>> some lockless variable in the console code itself, which then gets
>> set by the fbdev code during init based on its lockless_register_fb
>> setting.
>>
>> But as said I think we should seriously consider just removing
>> lockless_register_fb.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1]
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/console.h#n203
>>>
>>>>
>>>> #define WARN_FB_CONSOLE_UNLOCKED()                      \
>>>>          do {                                            \
>>>>                  if (unlikely(!lockless_register_fb))    \
>>>>                          WARN_CONSOLE_UNLOCKED();        \
>>>>          } while (0)
>>>>
>>>> And use that instead?
>>>>
>>>> -- Steve
>>>>
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>> -New patch in v3 of this patchset
>>>>>>
>>>>>> Changes in v4:
>>>>>> -Keep the comments about which fbcon functions need locks in place
>>>>>> ---
>>>>>>    drivers/video/fbdev/core/fbcon.c | 11 +++++++++++
>>>>>>    1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/video/fbdev/core/fbcon.c
>>>>>> b/drivers/video/fbdev/core/fbcon.c
>>>>>> index c910e74d46ff..cd8d52a967aa 100644
>>>>>> --- a/drivers/video/fbdev/core/fbcon.c
>>>>>> +++ b/drivers/video/fbdev/core/fbcon.c
>>>>>> @@ -828,6 +828,8 @@ static int set_con2fb_map(int unit, int newidx, int
>>>>>> user)
>>>>>>          struct fb_info *oldinfo = NULL;
>>>>>>          int found, err = 0;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          if (oldidx == newidx)
>>>>>>                  return 0;
>>>>>>    @@ -3044,6 +3046,8 @@ static int fbcon_fb_unbind(int idx)
>>>>>>    {
>>>>>>          int i, new_idx = -1, ret = 0;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          if (!fbcon_has_console_bind)
>>>>>>                  return 0;
>>>>>>    @@ -3094,6 +3098,8 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>>> *info)
>>>>>>    {
>>>>>>          int i, idx;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          idx = info->node;
>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++) {
>>>>>>                  if (con2fb_map[i] == idx)
>>>>>> @@ -3131,6 +3137,9 @@ static int fbcon_fb_unregistered(struct fb_info
>>>>>> *info)
>>>>>>    static void fbcon_remap_all(int idx)
>>>>>>    {
>>>>>>          int i;
>>>>>> +
>>>>>> +       WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          for (i = first_fb_vc; i <= last_fb_vc; i++)
>>>>>>                  set_con2fb_map(i, idx, 0);
>>>>>>    @@ -3177,6 +3186,8 @@ static int fbcon_fb_registered(struct fb_info
>>>>>> *info)
>>>>>>    {
>>>>>>          int ret = 0, i, idx;
>>>>>>    +     WARN_CONSOLE_UNLOCKED();
>>>>>> +
>>>>>>          idx = info->node;
>>>>>>          fbcon_select_primary(info);
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
>