| Message ID | 20180718093002.4596-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 (!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 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-Disable-console-lock-warnings-when-fb-lockless_register_fb-is-set/20180719-023522
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 >
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)
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(-)