Messages in this thread | | | Subject | Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable | From | Hans de Goede <> | Date | Wed, 11 Jul 2018 17:35:10 +0200 |
| |
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 > > >
| |