lkml.org 
[lkml]   [2018]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
From
Date
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
>
>
>
\
 
 \ /
  Last update: 2018-07-15 22:05    [W:0.073 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site