[PATCH 0/3] Let sparse check for shadowed variables

[PATCH 0/3] Let sparse check for shadowed variables

[Sebastian Andrzej Siewior]

#1 enables checking for shadowed variables by sparse. Apparently this is
no longer default as of sparse 0.5.2.
#2 and #3 are just two fixes for what sparse reported. There are plenty
of others…

Sebastian

[PATCH 1/3] kbuild: Add -Wshadow to sparse

[Sebastian Andrzej Siewior]

I remember that `sparse' used to warn about shadowed variables but it
seems not to do it by default anymore. It was useful.

Enable `-Wshadow' while invoking sparse.

Cc: linux-sparse@vger.kernel.org
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bf3786e4ffece..0160c6b5dd45d 100644
--- a/Makefile
+++ b/Makefile
@@ -390,7 +390,7 @@ PYTHON3		= python3
 CHECK		= sparse
 
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
-		  -Wbitwise -Wno-return-void -Wno-unknown-attribute $(CF)
+		  -Wbitwise -Wno-return-void -Wno-unknown-attribute -Wshadow $(CF)
 NOSTDINC_FLAGS  =
 CFLAGS_MODULE   =
 AFLAGS_MODULE   =
-- 
2.19.1

Re: [PATCH 1/3] kbuild: Add -Wshadow to sparse

[Luc Van Oostenryck]

On Wed, Oct 17, 2018 at 07:05:52PM +0200, Sebastian Andrzej Siewior wrote:
> I remember that `sparse' used to warn about shadowed variables but it
> seems not to do it by default anymore. It was useful.
> 
> Enable `-Wshadow' while invoking sparse.

Hi,

I made a quick test and I saw it would add a lot of noise because of
(valid) warnings coming from macros using a statement expression
redeclaring variables like '__u', 'tmp', ...

BTW, as far as I can see, sparse never had it enabled by default.

Kind regards,
-- Luc Van Oostenryck

Re: [PATCH 1/3] kbuild: Add -Wshadow to sparse

[Sebastian Andrzej Siewior]

On 2018-10-17 21:25:02 [+0200], Luc Van Oostenryck wrote:
> Hi,
Hi,

> I made a quick test and I saw it would add a lot of noise because of
> (valid) warnings coming from macros using a statement expression
> redeclaring variables like '__u', 'tmp', ...
> 
> BTW, as far as I can see, sparse never had it enabled by default.

I must have seen those warnings with C=1 _years_ ago otherwise I
wouldn't know about that feature. Or it was enabled by the check process
and got disabled but I can't find evidence for it.

> Kind regards,
> -- Luc Van Oostenryck

Sebastian

[PATCH 2/3] x86/mcelog: Remove one mce_helper definition

[Sebastian Andrzej Siewior]

Commit 5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog
driver") moved the old interface into one file including mce_helper
definition as static and "extern".

Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: linux-edac <linux-edac@vger.kernel.org>
Cc: x86@kernel.org
Fixes:  5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog driver")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/cpu/mcheck/dev-mcelog.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
index 97685a0c31751..27f394ac983f6 100644
--- a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
@@ -38,9 +38,6 @@ static struct mce_log_buffer mcelog = {
 
 static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 
-/* User mode helper program triggered by machine check event */
-extern char			mce_helper[128];
-
 static int dev_mce_log(struct notifier_block *nb, unsigned long val,
 				void *data)
 {
-- 
2.19.1

Re: [PATCH 2/3] x86/mcelog: Remove one mce_helper definition

[Borislav Petkov]

On Wed, Oct 17, 2018 at 07:05:53PM +0200, Sebastian Andrzej Siewior wrote:
> Commit 5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog
> driver") moved the old interface into one file including mce_helper
> definition as static and "extern".
> 
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: linux-edac <linux-edac@vger.kernel.org>
> Cc: x86@kernel.org
> Fixes:  5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog driver")

I'm not sure about the Fixes: tag - it'll trigger a flood of backporting
for no reason AFAICT.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH 2/3] x86/mcelog: Remove one mce_helper definition

[Thomas Gleixner]

On Wed, 17 Oct 2018, Borislav Petkov wrote:

> On Wed, Oct 17, 2018 at 07:05:53PM +0200, Sebastian Andrzej Siewior wrote:
> > Commit 5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog
> > driver") moved the old interface into one file including mce_helper
> > definition as static and "extern".
> > 
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: linux-edac <linux-edac@vger.kernel.org>
> > Cc: x86@kernel.org
> > Fixes:  5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog driver")
> 
> I'm not sure about the Fixes: tag - it'll trigger a flood of backporting
> for no reason AFAICT.

Not when there is no Cc: stable. If the stable folks pick up stuff
nevertheless, shrug.

The fixes tag is interesting even for stuff which does not go into stable
because you can extract it properly and look at relations between original
commit and fix.

Thanks,

	tglx

[tip:ras/core] x86/mcelog: Remove one mce_helper definition

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  711f76a328cbe5b49164bb14bcb593fa52102051
Gitweb:     https://git.kernel.org/tip/711f76a328cbe5b49164bb14bcb593fa52102051
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Wed, 17 Oct 2018 19:05:53 +0200
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Thu, 18 Oct 2018 00:05:04 +0200

x86/mcelog: Remove one mce_helper definition

Commit

  5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog driver")

moved the old interface into one file including mce_helper definition as
static and "extern". Remove one.

Fixes: 5de97c9f6d85f ("x86/mce: Factor out and deprecate the /dev/mcelog driver")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Tony Luck <tony.luck@intel.com>
CC: linux-edac <linux-edac@vger.kernel.org>
CC: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/20181017170554.18841-3-bigeasy@linutronix.de
---
 arch/x86/kernel/cpu/mcheck/dev-mcelog.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
index 97685a0c3175..27f394ac983f 100644
--- a/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
+++ b/arch/x86/kernel/cpu/mcheck/dev-mcelog.c
@@ -38,9 +38,6 @@ static struct mce_log_buffer mcelog = {
 
 static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 
-/* User mode helper program triggered by machine check event */
-extern char			mce_helper[128];
-
 static int dev_mce_log(struct notifier_block *nb, unsigned long val,
 				void *data)
 {

[PATCH 3/3] kvm: don't redefine flags as something else

[Sebastian Andrzej Siewior]

The function irqfd_wakeup() has flags defined as __poll_t and then it
has additional flags which is used for irqflags.

Redefine the inner flags variable as iflags so it does not shadow the
outer flags.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 virt/kvm/eventfd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index b20b751286fc6..d15a51622d53e 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -214,9 +214,9 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
 
 	if (flags & EPOLLHUP) {
 		/* The eventfd is closing, detach from KVM */
-		unsigned long flags;
+		unsigned long iflags;
 
-		spin_lock_irqsave(&kvm->irqfds.lock, flags);
+		spin_lock_irqsave(&kvm->irqfds.lock, iflags);
 
 		/*
 		 * We must check if someone deactivated the irqfd before
@@ -230,7 +230,7 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
 		if (irqfd_is_active(irqfd))
 			irqfd_deactivate(irqfd);
 
-		spin_unlock_irqrestore(&kvm->irqfds.lock, flags);
+		spin_unlock_irqrestore(&kvm->irqfds.lock, iflags);
 	}
 
 	return 0;
-- 
2.19.1

Re: [PATCH 3/3] kvm: don't redefine flags as something else

[Paolo Bonzini]

On 17/10/2018 19:05, Sebastian Andrzej Siewior wrote:
> The function irqfd_wakeup() has flags defined as __poll_t and then it
> has additional flags which is used for irqflags.
> 
> Redefine the inner flags variable as iflags so it does not shadow the
> outer flags.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  virt/kvm/eventfd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index b20b751286fc6..d15a51622d53e 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -214,9 +214,9 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
>  
>  	if (flags & EPOLLHUP) {
>  		/* The eventfd is closing, detach from KVM */
> -		unsigned long flags;
> +		unsigned long iflags;
>  
> -		spin_lock_irqsave(&kvm->irqfds.lock, flags);
> +		spin_lock_irqsave(&kvm->irqfds.lock, iflags);
>  
>  		/*
>  		 * We must check if someone deactivated the irqfd before
> @@ -230,7 +230,7 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
>  		if (irqfd_is_active(irqfd))
>  			irqfd_deactivate(irqfd);
>  
> -		spin_unlock_irqrestore(&kvm->irqfds.lock, flags);
> +		spin_unlock_irqrestore(&kvm->irqfds.lock, iflags);
>  	}
>  
>  	return 0;
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 3/3] kvm: don't redefine flags as something else

[Sebastian Andrzej Siewior]

On 2018-10-17 19:06:33 [+0200], Paolo Bonzini wrote:
> On 17/10/2018 19:05, Sebastian Andrzej Siewior wrote:
> > The function irqfd_wakeup() has flags defined as __poll_t and then it
> > has additional flags which is used for irqflags.
> > 
> > Redefine the inner flags variable as iflags so it does not shadow the
> > outer flags.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

this touches only kvm and was acked by a kvm person. What did it miss to
get applied? :)

Sebastian

Re: [PATCH 3/3] kvm: don't redefine flags as something else

[Paolo Bonzini]

On 13/03/19 09:58, Sebastian Andrzej Siewior wrote:
> On 2018-10-17 19:06:33 [+0200], Paolo Bonzini wrote:
>> On 17/10/2018 19:05, Sebastian Andrzej Siewior wrote:
>>> The function irqfd_wakeup() has flags defined as __poll_t and then it
>>> has additional flags which is used for irqflags.
>>>
>>> Redefine the inner flags variable as iflags so it does not shadow the
>>> outer flags.
>>>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>>> Cc: kvm@vger.kernel.org
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> this touches only kvm and was acked by a kvm person. What did it miss to
> get applied? :)

I was expecting it to be applied together with the other patches.  I can
queue it too.

Paolo