[PATCH v2 0/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[PATCH v2 0/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Thomas Zimmermann]

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\x153140585411176&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

[PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Thomas Zimmermann]

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)
-- 
2.18.0

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Hans de Goede]

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)
> 

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4571 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24373 bytes --]

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Petr Mladek]

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

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[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].

	-ss

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1159 bytes --]

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
> 

-- 
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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Sergey Senozhatsky]

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

Re: [PATCH v2 1/1] fbdev/core: Disable console-lock warnings when fb.lockless_register_fb is set

[Hans de Goede]

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
>