Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Am 11.07.2018 um 21:19 schrieb Steven Rostedt: > I just sent the patch. If the printk maintainers take it, then you can > update the fb driver to set the ignore_console_lock_warning when > lockless_fb_register is set. Thank you. > > -- Steve > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg Tel: +49-911-74053-0; Fax: +49-911-7417755; https://www.suse.com/ SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi, On (07/11/18 16:46), Thomas Zimmermann wrote: > Am 28.06.2018 um 11:03 schrieb Hans de Goede: > > Replace comments about places where the console lock should be held with > > calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held. > > Debugging fbcon sometimes requires to not take the console lock. This > patch breaks the debugging workaround provided by > 'fb.lockless_register_fb'. The dmesg is now filled with warnings about > the missing lock. Hmm. I once dealt with WARN_CONSOLE_UNLOCKED(), and back then, IIRC, I really wanted to turn it into WARN_ONCE_CONSOLE_UNLOCKED(), which would WARN_ON_ONCE() instead of WARN_ON(). It's just a bit too noisy and verbose and a single backtrace was already enough. -ss ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi, On 11-07-18 21:19, Steven Rostedt wrote: On Wed, 11 Jul 2018 19:56:02 +0200 Daniel Vetter wrote: Have you seen Steven's suggestion which he send about the same time as your mail I'm replying to here ? I personally think that doing something like that makes sense (for as long as we have the need for the lockless_fb_register debug hack). Note I've 2 patches ready to go to only fix this in fbcon.c, but I think a more thorough fix makes sense. Yeah Steven's suggestion looks reasonable to fix this all for good. The #if 1 predates git history, so no idea why it was added or by whom :-) I just sent the patch. If the printk maintainers take it, then you can update the fb driver to set the ignore_console_lock_warning when lockless_fb_register is set. Thanks for doing this. I will wait with sending a fbcon / fbdev patch till the fate of your patch is clear then. Regards, Hans ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, 11 Jul 2018 19:56:02 +0200 Daniel Vetter wrote: > > Have you seen Steven's suggestion which he send about the same time > > as your mail I'm replying to here ? I personally think that doing > > something like that makes sense (for as long as we have the need > > for the lockless_fb_register debug hack). > > > > Note I've 2 patches ready to go to only fix this in fbcon.c, but I > > think a more thorough fix makes sense. > > Yeah Steven's suggestion looks reasonable to fix this all for good. > The #if 1 predates git history, so no idea why it was added or by whom > :-) I just sent the patch. If the printk maintainers take it, then you can update the fb driver to set the ignore_console_lock_warning when lockless_fb_register is set. -- Steve ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, Jul 11, 2018 at 7:35 PM, Hans de Goede wrote: > Hi, > > > On 11-07-18 17:42, Daniel Vetter wrote: >> >> On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede >> wrote: >>> >>> Hi, >>> >>> On 11-07-18 17:28, Daniel Vetter wrote: On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede 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 ? >> >> >> I'd say let's only bother with the ones in fbcon.c. Avoids the trouble >> with having to expose the fb module option to vt.c somehow. > > > The plan was actually do the things the other way around, add a flag to > vt.c which when set disables the WARN_ON calls and then have fbdev[.ko] > set that when the fb.lockless_fb_register option is set. > >> The ones >> in vt.c are as old as the git history (from a quick check at least), >> and in my debugging they never have been annoying (or I somehow didn't >> ever hit them, not idea). > > > There is a #if 1 #define #else #define empty around the > WARN_CONSOLE_UNLOCKED() > call in include/linux/console.h I've the feeling that is there as a hack > to be able to quickly disable the WARN_ONs when debugging. > > Have you seen Steven's suggestion which he send about the same time > as your mail I'm replying to here ? I personally think that doing > something like that makes sense (for as long as we have the need > for the lockless_fb_register debug hack). > > Note I've 2 patches ready to go to only fix this in fbcon.c, but I > think a more thorough fix makes sense. Yeah Steven's suggestion looks reasonable to fix this all for good. The #if 1 predates git history, so no idea why it was added or by whom :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi, On 11-07-18 17:42, Daniel Vetter wrote: On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede wrote: Hi, On 11-07-18 17:28, Daniel Vetter wrote: On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede 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 ? I'd say let's only bother with the ones in fbcon.c. Avoids the trouble with having to expose the fb module option to vt.c somehow. The plan was actually do the things the other way around, add a flag to vt.c which when set disables the WARN_ON calls and then have fbdev[.ko] set that when the fb.lockless_fb_register option is set. The ones in vt.c are as old as the git history (from a quick check at least), and in my debugging they never have been annoying (or I somehow didn't ever hit them, not idea). There is a #if 1 #define #else #define empty around the WARN_CONSOLE_UNLOCKED() call in include/linux/console.h I've the feeling that is there as a hack to be able to quickly disable the WARN_ONs when debugging. Have you seen Steven's suggestion which he send about the same time as your mail I'm replying to here ? I personally think that doing something like that makes sense (for as long as we have the need for the lockless_fb_register debug hack). Note I've 2 patches ready to go to only fix this in fbcon.c, but I think a more thorough fix makes sense. Regards, Hans ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, Jul 11, 2018 at 5:35 PM, Hans de Goede wrote:
> Hi,
>
> On 11-07-18 17:28, Daniel Vetter wrote:
>>
>> On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede
>> 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 ?
I'd say let's only bother with the ones in fbcon.c. Avoids the trouble
with having to expose the fb module option to vt.c somehow. The ones
in vt.c are as old as the git history (from a quick check at least),
and in my debugging they never have been annoying (or I somehow didn't
ever hit them, not idea).
Also note that none if this matters long-term because I have a really
fancy plan to fix up this entire console_locking mess, which will also
remove the need for the lockless_fb_register debug hack.
-Daniel
>
> 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)
>>> Reviewed-by: Daniel Vetter
>>> Reviewed-by: Sergey Senozhatsky
>>> Signed-off-by: Hans de Goede
>>> ---
>>> 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] ==
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, 11 Jul 2018 17:35:10 +0200
Hans de Goede wrote:
> 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 ?
What about adding this patch (untested, not even compiled), and then
set it from the fb module.
-- Steve
diff --git a/include/linux/console.h b/include/linux/console.h
index dfd6b0e97855..11cd4956a57f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -200,8 +200,10 @@ void vcs_make_sysfs(int index);
void vcs_remove_sysfs(int index);
/* Some debug stub to catch some of the obvious races in the VT code */
+extern bool ignore_console_lock_warning;
#if 1
-#define WARN_CONSOLE_UNLOCKED()WARN_ON(!is_console_locked() &&
!oops_in_progress)
+#define WARN_CONSOLE_UNLOCKED()
\
+ WARN_ON(!ignore_console_lock_warning && !is_console_locked() &&
!oops_in_progress)
#else
#define WARN_CONSOLE_UNLOCKED()
#endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..fa15d7ddf0c4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -66,6 +66,9 @@ int console_printk[4] = {
CONSOLE_LOGLEVEL_DEFAULT, /* default_console_loglevel */
};
+bool ignore_console_lock_warning;
+EXPORT_SYMBOL(ignore_console_lock_warning);
+
/*
* Low level drivers may need that to know if they can schedule in
* their unblank() callback or not. So let's export it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi,
On 11-07-18 17:28, Daniel Vetter wrote:
On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede 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)
Reviewed-by: Daniel Vetter
Reviewed-by: Sergey Senozhatsky
Signed-off-by: Hans de Goede
---
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, Jul 11, 2018 at 5:14 PM, Hans de Goede 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.
-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)
> Reviewed-by: Daniel Vetter
> Reviewed-by: Sergey Senozhatsky
> Signed-off-by: Hans de Goede
> ---
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
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.
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)
Reviewed-by: Daniel Vetter
Reviewed-by: Sergey Senozhatsky
Signed-off-by: Hans de Goede
---
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
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, Jul 11, 2018 at 5:01 PM, Hans de Goede wrote:
> Hi,
>
> On 11-07-18 16:52, Steven Rostedt wrote:
>>
>> On Wed, 11 Jul 2018 16:46:11 +0200
>> Thomas Zimmermann wrote:
>>
>>> Hi
>>>
>>> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
Replace comments about places where the console lock should be held with
calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
>>>
>>>
>>> Debugging fbcon sometimes requires to not take the console lock. This
>>> patch breaks the debugging workaround provided by
>>> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
>>> the missing lock.
>>>
>>
>> What if you make lockless_register_fb visible to fbcon, and then we can
>> have a macro:
>>
>> #define WARN_FB_CONSOLE_UNLOCKED() \
>> do {\
>> if (unlikely(!lockless_register_fb))\
>> WARN_CONSOLE_UNLOCKED();\
>> } while (0)
>>
>> And use that instead?
>
>
> Yes I'm already preparing a patch like that :)
>
> Without the unlikely though, this is not used in hot paths.
Note that you don't even need an EXPORT_SYMBOL since this is all in
the same module.
-Daniel
> Regards,
>
> Hans
>
>
>
>
>>
>> -- Steve
>>
>>
>>> Best regards
>>> Thomas
>>>
Acked-by: Steven Rostedt (VMware)
Reviewed-by: Daniel Vetter
Reviewed-by: Sergey Senozhatsky
Signed-off-by: Hans de Goede
---
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
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.
WARN_CONSOLE_UNLOCKED is already protected by an '#ifdef 1 ... #else
...' construct [1]. I thought about turning this into a config option.
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)
>>> Reviewed-by: Daniel Vetter
>>> Reviewed-by: Sergey Senozhatsky
>>> Signed-off-by: Hans de Goede
>>> ---
>>> 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);
>>>
>>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755; https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)
signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi,
On 11-07-18 16:52, Steven Rostedt wrote:
On Wed, 11 Jul 2018 16:46:11 +0200
Thomas Zimmermann wrote:
Hi
Am 28.06.2018 um 11:03 schrieb Hans de Goede:
Replace comments about places where the console lock should be held with
calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
Debugging fbcon sometimes requires to not take the console lock. This
patch breaks the debugging workaround provided by
'fb.lockless_register_fb'. The dmesg is now filled with warnings about
the missing lock.
What if you make lockless_register_fb visible to fbcon, and then we can
have a macro:
#define WARN_FB_CONSOLE_UNLOCKED() \
do {\
if (unlikely(!lockless_register_fb))\
WARN_CONSOLE_UNLOCKED();\
} while (0)
And use that instead?
Yes I'm already preparing a patch like that :)
Without the unlikely though, this is not used in hot paths.
Regards,
Hans
-- Steve
Best regards
Thomas
Acked-by: Steven Rostedt (VMware)
Reviewed-by: Daniel Vetter
Reviewed-by: Sergey Senozhatsky
Signed-off-by: Hans de Goede
---
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
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
On Wed, 11 Jul 2018 16:46:11 +0200
Thomas Zimmermann wrote:
> Hi
>
> Am 28.06.2018 um 11:03 schrieb Hans de Goede:
> > Replace comments about places where the console lock should be held with
> > calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
>
> Debugging fbcon sometimes requires to not take the console lock. This
> patch breaks the debugging workaround provided by
> 'fb.lockless_register_fb'. The dmesg is now filled with warnings about
> the missing lock.
>
What if you make lockless_register_fb visible to fbcon, and then we can
have a macro:
#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)
> > Reviewed-by: Daniel Vetter
> > Reviewed-by: Sergey Senozhatsky
> > Signed-off-by: Hans de Goede
> > ---
> > 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
Re: [PATCH v5 2/3] fbcon: Call WARN_CONSOLE_UNLOCKED() where applicable
Hi
Am 28.06.2018 um 11:03 schrieb Hans de Goede:
> Replace comments about places where the console lock should be held with
> calls to WARN_CONSOLE_UNLOCKED() to assert that it is actually held.
Debugging fbcon sometimes requires to not take the console lock. This
patch breaks the debugging workaround provided by
'fb.lockless_register_fb'. The dmesg is now filled with warnings about
the missing lock.
Best regards
Thomas
> Acked-by: Steven Rostedt (VMware)
> Reviewed-by: Daniel Vetter
> Reviewed-by: Sergey Senozhatsky
> Signed-off-by: Hans de Goede
> ---
> 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);
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755; https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)
signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
