static_call: Fix unused variable warning
diff mbox series

Message ID 20210422003334.139452-1-linux@roeck-us.net
State New
Headers show
Series
  • static_call: Fix unused variable warning
Related show

Commit Message

Guenter Roeck April 22, 2021, 12:33 a.m. UTC
If CONFIG_MODULES=n, the following build warning is reported.

kernel/static_call.c: In function ‘__static_call_update’:
kernel/static_call.c:153:18: warning: unused variable ‘mod’

Mark the variable as __maybe_unused to fix the problem.

Fixes: 9183c3f9ed71 ("static_call: Add inline static call infrastructure")
Reported-by: Zach Reizner <zachr@google.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 kernel/static_call.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Rostedt April 22, 2021, 12:41 a.m. UTC | #1
Second patch with the exact same update. Perhaps we should take one
before we get more of them ;-)

https://lore.kernel.org/lkml/20210416194300.3952208-1-cmllamas@google.com/

-- Steve


On Wed, 21 Apr 2021 17:33:34 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> If CONFIG_MODULES=n, the following build warning is reported.
> 
> kernel/static_call.c: In function ‘__static_call_update’:
> kernel/static_call.c:153:18: warning: unused variable ‘mod’
> 
> Mark the variable as __maybe_unused to fix the problem.
> 
> Fixes: 9183c3f9ed71 ("static_call: Add inline static call infrastructure")
> Reported-by: Zach Reizner <zachr@google.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  kernel/static_call.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index 2c5950b0b90e..8211a34251f8 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -150,7 +150,7 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
>  
>  	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
>  		bool init = system_state < SYSTEM_RUNNING;
> -		struct module *mod = site_mod->mod;
> +		struct module __maybe_unused *mod = site_mod->mod;
>  
>  		if (!site_mod->sites) {
>  			/*
Guenter Roeck April 22, 2021, 1:24 a.m. UTC | #2
On 4/21/21 5:41 PM, Steven Rostedt wrote:
> 
> Second patch with the exact same update. Perhaps we should take one
> before we get more of them ;-)
> 
> https://lore.kernel.org/lkml/20210416194300.3952208-1-cmllamas@google.com/
> 

Sorry, I missed the other one.

Guenter

> -- Steve
> 
> 
> On Wed, 21 Apr 2021 17:33:34 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> If CONFIG_MODULES=n, the following build warning is reported.
>>
>> kernel/static_call.c: In function ‘__static_call_update’:
>> kernel/static_call.c:153:18: warning: unused variable ‘mod’
>>
>> Mark the variable as __maybe_unused to fix the problem.
>>
>> Fixes: 9183c3f9ed71 ("static_call: Add inline static call infrastructure")
>> Reported-by: Zach Reizner <zachr@google.com>
>> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  kernel/static_call.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/static_call.c b/kernel/static_call.c
>> index 2c5950b0b90e..8211a34251f8 100644
>> --- a/kernel/static_call.c
>> +++ b/kernel/static_call.c
>> @@ -150,7 +150,7 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
>>  
>>  	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
>>  		bool init = system_state < SYSTEM_RUNNING;
>> -		struct module *mod = site_mod->mod;
>> +		struct module __maybe_unused *mod = site_mod->mod;
>>  
>>  		if (!site_mod->sites) {
>>  			/*
>
Steven Rostedt April 22, 2021, 1:33 a.m. UTC | #3
On Wed, 21 Apr 2021 18:24:15 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 4/21/21 5:41 PM, Steven Rostedt wrote:
> > 
> > Second patch with the exact same update. Perhaps we should take one
> > before we get more of them ;-)
> > 
> > https://lore.kernel.org/lkml/20210416194300.3952208-1-cmllamas@google.com/
> >   
> 
> Sorry, I missed the other one.
> 


That's OK, I just thought you might have been doing some "research" on us.

What? Too soon?

-- Steve
Guenter Roeck April 22, 2021, 1:45 a.m. UTC | #4
On 4/21/21 6:33 PM, Steven Rostedt wrote:
> On Wed, 21 Apr 2021 18:24:15 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 4/21/21 5:41 PM, Steven Rostedt wrote:
>>>
>>> Second patch with the exact same update. Perhaps we should take one
>>> before we get more of them ;-)
>>>
>>> https://lore.kernel.org/lkml/20210416194300.3952208-1-cmllamas@google.com/
>>>   
>>
>> Sorry, I missed the other one.
>>
> 
> 
> That's OK, I just thought you might have been doing some "research" on us.
> 

Not amused. I don't play such games.

No, this causes a compile error for us for a build with CONFIG_MODULES=n
in combination with -Werror. The problem was backported to v5.10.y with
the merge of v5.10.28, which makes it worse because it introduced a
regression into that branch. A regression causing a compile error isn't
really improving confidence in the test coverage of stable releases.

Unfortunately there was a miscommunication internally; my understanding
was that no patch was submitted upstream.

Guenter
Steven Rostedt April 22, 2021, 2:38 a.m. UTC | #5
On Wed, 21 Apr 2021 18:45:20 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > 
> > That's OK, I just thought you might have been doing some "research" on us.
> >   
> 
> Not amused. I don't play such games.

I know you don't, but I just couldn't resist saying it. You should have
known that I was joking by the part of my reply that you cut off. ;-)

-- Steve
Peter Zijlstra April 22, 2021, 7:20 a.m. UTC | #6
On Wed, Apr 21, 2021 at 08:41:39PM -0400, Steven Rostedt wrote:
> 
> Second patch with the exact same update. Perhaps we should take one
> before we get more of them ;-)
> 

I thought we already fixed that...

---
commit 7d95f22798ecea513f37b792b39fec4bcf20fec3
Author: Matthieu Baerts <matthieu.baerts@tessares.net>
Date:   Fri Mar 26 11:50:23 2021 +0100

    static_call: Fix unused variable warn w/o MODULE
    
    Here is the warning converted as error and reported by GCC:
    
      kernel/static_call.c: In function ‘__static_call_update’:
      kernel/static_call.c:153:18: error: unused variable ‘mod’ [-Werror=unused-variable]
        153 |   struct module *mod = site_mod->mod;
            |                  ^~~
      cc1: all warnings being treated as errors
      make[1]: *** [scripts/Makefile.build:271: kernel/static_call.o] Error 1
    
    This is simply because since recently, we no longer use 'mod' variable
    elsewhere if MODULE is unset.
    
    When using 'make tinyconfig' to generate the default kconfig, MODULE is
    unset.
    
    There are different ways to fix this warning. Here I tried to minimised
    the number of modified lines and not add more #ifdef. We could also move
    the declaration of the 'mod' variable inside the if-statement or
    directly use site_mod->mod.
    
    Fixes: 698bacefe993 ("static_call: Align static_call_is_init() patching condition")
    Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
    Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    Link: https://lkml.kernel.org/r/20210326105023.2058860-1-matthieu.baerts@tessares.net

diff --git a/kernel/static_call.c b/kernel/static_call.c
index 2c5950b0b90e..723fcc9d20db 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -165,13 +165,13 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 
 		stop = __stop_static_call_sites;
 
-#ifdef CONFIG_MODULES
 		if (mod) {
+#ifdef CONFIG_MODULES
 			stop = mod->static_call_sites +
 			       mod->num_static_call_sites;
 			init = mod->state == MODULE_STATE_COMING;
-		}
 #endif
+		}
 
 		for (site = site_mod->sites;
 		     site < stop && static_call_key(site) == key; site++) {
Guenter Roeck April 22, 2021, 2:01 p.m. UTC | #7
On 4/22/21 12:20 AM, Peter Zijlstra wrote:
> On Wed, Apr 21, 2021 at 08:41:39PM -0400, Steven Rostedt wrote:
>>
>> Second patch with the exact same update. Perhaps we should take one
>> before we get more of them ;-)
>>
> 
> I thought we already fixed that...

Not in v5.12-rc8-6-g4bdafe832681, which is ToT right now.

Ah, I do see it in -next, but that doesn't help me in mainline,
nor in stable branches where the patch introducing the problem
has been backported to.

Guenter

> 
> ---
> commit 7d95f22798ecea513f37b792b39fec4bcf20fec3
> Author: Matthieu Baerts <matthieu.baerts@tessares.net>
> Date:   Fri Mar 26 11:50:23 2021 +0100
> 
>     static_call: Fix unused variable warn w/o MODULE
>     
>     Here is the warning converted as error and reported by GCC:
>     
>       kernel/static_call.c: In function ‘__static_call_update’:
>       kernel/static_call.c:153:18: error: unused variable ‘mod’ [-Werror=unused-variable]
>         153 |   struct module *mod = site_mod->mod;
>             |                  ^~~
>       cc1: all warnings being treated as errors
>       make[1]: *** [scripts/Makefile.build:271: kernel/static_call.o] Error 1
>     
>     This is simply because since recently, we no longer use 'mod' variable
>     elsewhere if MODULE is unset.
>     
>     When using 'make tinyconfig' to generate the default kconfig, MODULE is
>     unset.
>     
>     There are different ways to fix this warning. Here I tried to minimised
>     the number of modified lines and not add more #ifdef. We could also move
>     the declaration of the 'mod' variable inside the if-statement or
>     directly use site_mod->mod.
>     
>     Fixes: 698bacefe993 ("static_call: Align static_call_is_init() patching condition")
>     Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>     Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>     Link: https://lkml.kernel.org/r/20210326105023.2058860-1-matthieu.baerts@tessares.net
> 
> diff --git a/kernel/static_call.c b/kernel/static_call.c
> index 2c5950b0b90e..723fcc9d20db 100644
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -165,13 +165,13 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
>  
>  		stop = __stop_static_call_sites;
>  
> -#ifdef CONFIG_MODULES
>  		if (mod) {
> +#ifdef CONFIG_MODULES
>  			stop = mod->static_call_sites +
>  			       mod->num_static_call_sites;
>  			init = mod->state == MODULE_STATE_COMING;
> -		}
>  #endif
> +		}
>  
>  		for (site = site_mod->sites;
>  		     site < stop && static_call_key(site) == key; site++) {
>
Peter Zijlstra April 22, 2021, 4:13 p.m. UTC | #8
On Thu, Apr 22, 2021 at 07:01:47AM -0700, Guenter Roeck wrote:
> On 4/22/21 12:20 AM, Peter Zijlstra wrote:
> > On Wed, Apr 21, 2021 at 08:41:39PM -0400, Steven Rostedt wrote:
> >>
> >> Second patch with the exact same update. Perhaps we should take one
> >> before we get more of them ;-)
> >>
> > 
> > I thought we already fixed that...
> 
> Not in v5.12-rc8-6-g4bdafe832681, which is ToT right now.
> 
> Ah, I do see it in -next, but that doesn't help me in mainline,
> nor in stable branches where the patch introducing the problem
> has been backported to.

Given it's a silly warning I didn't figure it was urgent material. I
suppose we can backport it if someone (you apperntly) cares.
Guenter Roeck April 22, 2021, 6:01 p.m. UTC | #9
On Thu, Apr 22, 2021 at 06:13:32PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 22, 2021 at 07:01:47AM -0700, Guenter Roeck wrote:
> > On 4/22/21 12:20 AM, Peter Zijlstra wrote:
> > > On Wed, Apr 21, 2021 at 08:41:39PM -0400, Steven Rostedt wrote:
> > >>
> > >> Second patch with the exact same update. Perhaps we should take one
> > >> before we get more of them ;-)
> > >>
> > > 
> > > I thought we already fixed that...
> > 
> > Not in v5.12-rc8-6-g4bdafe832681, which is ToT right now.
> > 
> > Ah, I do see it in -next, but that doesn't help me in mainline,
> > nor in stable branches where the patch introducing the problem
> > has been backported to.
> 
> Given it's a silly warning I didn't figure it was urgent material. I
> suppose we can backport it if someone (you apperntly) cares.

We build release images with -Werror, and some of those images have
CONFIG_MODULES=n. So this wasn't silly for us. It was catastrophic
for the affected images. We already applied my proposed fix, so it
isn't specifically urgent for us anymore. However, the presence of
such "silly" warnings in stable releases (and backporting of patches
introducing such warnings into stable releases) gives ammunition
to those arguing that we should not merge stable releases because
they introduce regressions. And your statement that the warning is
"silly" doesn't help either, sorry.

Guenter

Patch
diff mbox series

diff --git a/kernel/static_call.c b/kernel/static_call.c
index 2c5950b0b90e..8211a34251f8 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -150,7 +150,7 @@  void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 
 	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
 		bool init = system_state < SYSTEM_RUNNING;
-		struct module *mod = site_mod->mod;
+		struct module __maybe_unused *mod = site_mod->mod;
 
 		if (!site_mod->sites) {
 			/*