[PATCH v2 0/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set
Hi,
this is a follow-up patch for the recent addition of 'ignore_console_lock_warning' to the console module [1] and should probably go through the same tree. When debugging fbdev console registration, the console-lock warnings will now be disabled temporarily.
Best regards Thomas
[1] https://marc.info/?l=dri-devel&m=153140585411176&w=2
v2: - restore ignore_console_lock_warning if lock_fb_info() fails
Thomas Zimmermann (1): fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set
drivers/video/fbdev/core/fbmem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
-- 2.18.0
If the console is unlocked during registration, the console subsystem generates significant amounts of warnings, which obfuscate actual debugging messages. Setting ignore_console_lock_warning while debugging console registration avoid the noise.
v2: - restore ignore_console_lock_warning if lock_fb_info() fails
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/video/fbdev/core/fbmem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 9e2f9d3c760e..432c26eeabfb 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1627,6 +1627,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) int i, ret; struct fb_event event; struct fb_videomode mode; + bool saved_ignore_console_lock_warning = ignore_console_lock_warning;
if (fb_check_foreignness(fb_info)) return -ENOSYS; @@ -1691,17 +1692,23 @@ static int do_register_framebuffer(struct fb_info *fb_info) event.info = fb_info; if (!lockless_register_fb) console_lock(); + else + ignore_console_lock_warning = true; if (!lock_fb_info(fb_info)) { - if (!lockless_register_fb) - console_unlock(); - return -ENODEV; + ret = -ENODEV; + goto unlock_console; } + ret = 0;
fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); unlock_fb_info(fb_info); +unlock_console: if (!lockless_register_fb) console_unlock(); - return 0; + else + ignore_console_lock_warning = + saved_ignore_console_lock_warning; + return ret; }
static int do_unregister_framebuffer(struct fb_info *fb_info)
Hi,
On 18-07-18 11:30, Thomas Zimmermann wrote:
If the console is unlocked during registration, the console subsystem generates significant amounts of warnings, which obfuscate actual debugging messages. Setting ignore_console_lock_warning while debugging console registration avoid the noise.
v2:
- restore ignore_console_lock_warning if lock_fb_info() fails
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Patch looks good to me, thanks:
Reviewed-by: Hans de Goede hdegoede@redhat.com
For "[PATCH v2] console: Replace #if 1 with a bool to ignore" Petr wrote:
"Acked-by: Petr Mladek pmladek@suse.com
I assume that it will go via fbdev tree with the other changes unless I hear otherwise."
So that patch and this one can both be picked up by Bartlomiej for merging through the fbdev tree when he is back.
Regards,
Hans
drivers/video/fbdev/core/fbmem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 9e2f9d3c760e..432c26eeabfb 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1627,6 +1627,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) int i, ret; struct fb_event event; struct fb_videomode mode;
bool saved_ignore_console_lock_warning = ignore_console_lock_warning;
if (fb_check_foreignness(fb_info)) return -ENOSYS;
@@ -1691,17 +1692,23 @@ static int do_register_framebuffer(struct fb_info *fb_info) event.info = fb_info; if (!lockless_register_fb) console_lock();
- else
ignore_console_lock_warning = true;
if (!lockless_register_fb)console_unlock();return -ENODEV;
ret = -ENODEV;goto unlock_console;}
ret = 0;
fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); unlock_fb_info(fb_info);
+unlock_console: if (!lockless_register_fb) console_unlock();
- return 0;
else
ignore_console_lock_warning =saved_ignore_console_lock_warning;return ret; }
static int do_unregister_framebuffer(struct fb_info *fb_info)
Hi Thomas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sof-driver-fuweitax/master] [also build test ERROR on v4.18-rc5 next-20180718] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/fbdev-core-Disabl... base: https://github.com/fuweitax/linux master config: i386-randconfig-s1-07181402 (attached as .config) compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 reproduce: # save the attached .config to linux build tree make ARCH=i386
All errors (new ones prefixed by >>):
drivers/video/fbdev/core/fbmem.c: In function 'do_register_framebuffer':
drivers/video/fbdev/core/fbmem.c:1642:43: error: 'ignore_console_lock_warning' undeclared (first use in this function)
bool saved_ignore_console_lock_warning = ignore_console_lock_warning; ^~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/video/fbdev/core/fbmem.c:1642:43: note: each undeclared identifier is reported only once for each function it appears in
vim +/ignore_console_lock_warning +1642 drivers/video/fbdev/core/fbmem.c
1631 1632 static bool lockless_register_fb; 1633 module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool, 0400); 1634 MODULE_PARM_DESC(lockless_register_fb, 1635 "Lockless framebuffer registration for debugging [default=off]"); 1636 1637 static int do_register_framebuffer(struct fb_info *fb_info) 1638 { 1639 int i, ret; 1640 struct fb_event event; 1641 struct fb_videomode mode;
1642 bool saved_ignore_console_lock_warning = ignore_console_lock_warning;
1643 1644 if (fb_check_foreignness(fb_info)) 1645 return -ENOSYS; 1646 1647 ret = do_remove_conflicting_framebuffers(fb_info->apertures, 1648 fb_info->fix.id, 1649 fb_is_primary_device(fb_info)); 1650 if (ret) 1651 return ret; 1652 1653 if (num_registered_fb == FB_MAX) 1654 return -ENXIO; 1655 1656 num_registered_fb++; 1657 for (i = 0 ; i < FB_MAX; i++) 1658 if (!registered_fb[i]) 1659 break; 1660 fb_info->node = i; 1661 atomic_set(&fb_info->count, 1); 1662 mutex_init(&fb_info->lock); 1663 mutex_init(&fb_info->mm_lock); 1664 1665 fb_info->dev = device_create(fb_class, fb_info->device, 1666 MKDEV(FB_MAJOR, i), NULL, "fb%d", i); 1667 if (IS_ERR(fb_info->dev)) { 1668 /* Not fatal */ 1669 printk(KERN_WARNING "Unable to create device for framebuffer %d; errno = %ld\n", i, PTR_ERR(fb_info->dev)); 1670 fb_info->dev = NULL; 1671 } else 1672 fb_init_device(fb_info); 1673 1674 if (fb_info->pixmap.addr == NULL) { 1675 fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL); 1676 if (fb_info->pixmap.addr) { 1677 fb_info->pixmap.size = FBPIXMAPSIZE; 1678 fb_info->pixmap.buf_align = 1; 1679 fb_info->pixmap.scan_align = 1; 1680 fb_info->pixmap.access_align = 32; 1681 fb_info->pixmap.flags = FB_PIXMAP_DEFAULT; 1682 } 1683 } 1684 fb_info->pixmap.offset = 0; 1685 1686 if (!fb_info->pixmap.blit_x) 1687 fb_info->pixmap.blit_x = ~(u32)0; 1688 1689 if (!fb_info->pixmap.blit_y) 1690 fb_info->pixmap.blit_y = ~(u32)0; 1691 1692 if (!fb_info->modelist.prev || !fb_info->modelist.next) 1693 INIT_LIST_HEAD(&fb_info->modelist); 1694 1695 if (fb_info->skip_vt_switch) 1696 pm_vt_switch_required(fb_info->dev, false); 1697 else 1698 pm_vt_switch_required(fb_info->dev, true); 1699 1700 fb_var_to_videomode(&mode, &fb_info->var); 1701 fb_add_videomode(&mode, &fb_info->modelist); 1702 registered_fb[i] = fb_info; 1703 1704 event.info = fb_info; 1705 if (!lockless_register_fb) 1706 console_lock(); 1707 else 1708 ignore_console_lock_warning = true; 1709 if (!lock_fb_info(fb_info)) { 1710 ret = -ENODEV; 1711 goto unlock_console; 1712 } 1713 ret = 0; 1714 1715 fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event); 1716 unlock_fb_info(fb_info); 1717 unlock_console: 1718 if (!lockless_register_fb) 1719 console_unlock(); 1720 else 1721 ignore_console_lock_warning = 1722 saved_ignore_console_lock_warning; 1723 return ret; 1724 } 1725
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed 2018-07-18 11:30:02, Thomas Zimmermann wrote:
If the console is unlocked during registration, the console subsystem generates significant amounts of warnings, which obfuscate actual debugging messages. Setting ignore_console_lock_warning while debugging console registration avoid the noise.
v2:
- restore ignore_console_lock_warning if lock_fb_info() fails
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/video/fbdev/core/fbmem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 9e2f9d3c760e..432c26eeabfb 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1627,6 +1627,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) int i, ret; struct fb_event event; struct fb_videomode mode;
- bool saved_ignore_console_lock_warning = ignore_console_lock_warning;
Hmm, this approach is racy if there are other users saving/setting/restoring ignore_console_lock_warning in parallel. I mean that this works only when the entire safe/set/restore operation is nested or sequential.
We might need another approach if there are more users, e.g. use an atomic counter for ignore_console_lock_warning.
On the other hand, I wonder if there ever will be other user. Also it is "just" for debugging. We could keep it simple for now. It might be enough to add a comment into include/linux/console.h, something like:
/* * Set ignore_console_lock_warning to true if you need to quiet * WARN_CONSOLE_UNLOCKED() for debugging purposes. Might need * another approach if manipulated by more users in parallel. */
Best Regards, Petr
On (07/19/18 10:53), Petr Mladek wrote:
Hmm, this approach is racy if there are other users saving/setting/restoring ignore_console_lock_warning in parallel. I mean that this works only when the entire safe/set/restore operation is nested or sequential.
Good point!
However, I tend to think that we don't need to care about it that much. Having a counter to permit nesting would probably be better, but, like you said, it's unlikely that we will see any problems with ignore_console_lock_warning anyway. So we can keep it simple [IOW - the way it is].
-ss
Hi
Am 19.07.2018 um 12:05 schrieb Sergey Senozhatsky:
On (07/19/18 10:53), Petr Mladek wrote:
Hmm, this approach is racy if there are other users saving/setting/restoring ignore_console_lock_warning in parallel. I mean that this works only when the entire safe/set/restore operation is nested or sequential.
Good point!
However, I tend to think that we don't need to care about it that much. Having a counter to permit nesting would probably be better, but, like you said, it's unlikely that we will see any problems with ignore_console_lock_warning anyway. So we can keep it simple [IOW - the way it is].
I just sent a new patch set based on atomic_t and TBH it's easier to use that this version. I only had to introduce the save-state variable in the caller because I couldn't do inc/dec.
Best regards Thomas
-ss
Hi,
On (07/19/18 12:20), Thomas Zimmermann wrote:
Am 19.07.2018 um 12:05 schrieb Sergey Senozhatsky:
On (07/19/18 10:53), Petr Mladek wrote:
Hmm, this approach is racy if there are other users saving/setting/restoring ignore_console_lock_warning in parallel. I mean that this works only when the entire safe/set/restore operation is nested or sequential.
Good point!
However, I tend to think that we don't need to care about it that much. Having a counter to permit nesting would probably be better, but, like you said, it's unlikely that we will see any problems with ignore_console_lock_warning anyway. So we can keep it simple [IOW - the way it is].
I just sent a new patch set based on atomic_t
Ah, just saw the new version.
and TBH it's easier to use that this version. I only had to introduce the save-state variable in the caller because I couldn't do inc/dec.
No objections, if it makes your life easier. Thanks.
-ss
Hi,
On 19-07-18 10:53, Petr Mladek wrote:
On Wed 2018-07-18 11:30:02, Thomas Zimmermann wrote:
If the console is unlocked during registration, the console subsystem generates significant amounts of warnings, which obfuscate actual debugging messages. Setting ignore_console_lock_warning while debugging console registration avoid the noise.
v2:
- restore ignore_console_lock_warning if lock_fb_info() fails
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/video/fbdev/core/fbmem.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 9e2f9d3c760e..432c26eeabfb 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1627,6 +1627,7 @@ static int do_register_framebuffer(struct fb_info *fb_info) int i, ret; struct fb_event event; struct fb_videomode mode;
- bool saved_ignore_console_lock_warning = ignore_console_lock_warning;
Hmm, this approach is racy if there are other users saving/setting/restoring ignore_console_lock_warning in parallel. I mean that this works only when the entire safe/set/restore operation is nested or sequential.
We might need another approach if there are more users, e.g. use an atomic counter for ignore_console_lock_warning.
I noticed this would be racy too, but this only gets used when console-locking should be disabled when registering fbdev-s for debugging purposes at which point everything console related is racy already anyways, so I think this is fine as is.
On the other hand, I wonder if there ever will be other user. Also it is "just" for debugging. We could keep it simple for now. It might be enough to add a comment into include/linux/console.h, something like:
Ack, lets keep this simple / as is in v2 of the patch.
Regards,
Hans
/*
- Set ignore_console_lock_warning to true if you need to quiet
- WARN_CONSOLE_UNLOCKED() for debugging purposes. Might need
- another approach if manipulated by more users in parallel.
*/
Best Regards, Petr
dri-devel@lists.freedesktop.org
-
Hans de Goede -
kbuild test robot -
Petr Mladek -
Sergey Senozhatsky -
Thomas Zimmermann