[PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andy Shevchenko]

Compiler is not happy about the recently added code:

lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
   52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
      |                   ^~~~~~~~~~~~~~

Fix it in the same way how, for example, lib/vsprintf.c does in the similar
cases, i.e. by using string literal directly as sizeof() parameter.

Fixes: 9b5072c04c46 ("panic: add 'panic_sys_info' sysctl to take human readable string parameter")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 46d6f4f1ad2a..779f8725b194 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -49,13 +49,11 @@ unsigned long sys_info_parse_param(char *str)
 
 #ifdef CONFIG_SYSCTL
 
-static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
-
 int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 					  void *buffer, size_t *lenp,
 					  loff_t *ppos)
 {
-	char names[sizeof(sys_info_avail) + 1];
+	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
 
-- 
2.47.2

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andy Shevchenko]

On Thu, Jul 10, 2025 at 04:21:39PM +0300, Andy Shevchenko wrote:
> I don't understand why this mail was sent with LKML being dropped...
> 
> On Thu, Jul 10, 2025 at 06:41:29PM +0800, Feng Tang wrote:
> > On Thu, Jul 10, 2025 at 12:48:16PM +0300, Andy Shevchenko wrote:
> > > Compiler is not happy about the recently added code:
> > > 
> > > lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> > >    52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > >       |                   ^~~~~~~~~~~~~~
> > > 
> > > Fix it in the same way how, for example, lib/vsprintf.c does in the similar
> > > cases, i.e. by using string literal directly as sizeof() parameter.
> > > 
> > > Fixes: 9b5072c04c46 ("panic: add 'panic_sys_info' sysctl to take human readable string parameter")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Thanks for the fix! The issue has been reported, and the linux-mm tree already carried fix:
> > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-nonmm-unstable&id=0a080b90dc6fe0a1e7df954a0d8c7e486e83b189
> > 
> > which doesn't hit linux-next tree yet.
> 
> With all the respect to Nathan's fix, I prefer mine for a couple of reasons:
> 1) __maybe_unused is a linker time workaorund;
> 2) we still have unneeded variable dangling;

Should be...

"With my patch:"

> 3) less code to maintain;

1) ...

> 4) vsprintf.c has many similar cases and there the approach is different.

2) ...

> Note, the fix is still in unstable branch, hence can be pulled out.
> 
> Andrew, Nathan, what's your opinion?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andrew Morton]

On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Compiler is not happy about the recently added code:
> 
> lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
>    52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
>       |                   ^~~~~~~~~~~~~~
> 
> Fix it in the same way how, for example, lib/vsprintf.c does in the similar
> cases, i.e. by using string literal directly as sizeof() parameter.
> 
> ...
>

> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -49,13 +49,11 @@ unsigned long sys_info_parse_param(char *str)
>  
>  #ifdef CONFIG_SYSCTL
>  
> -static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> -
>  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  					  void *buffer, size_t *lenp,
>  					  loff_t *ppos)
>  {
> -	char names[sizeof(sys_info_avail) + 1];
> +	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
>  	struct ctl_table table;
>  	unsigned long *si_bits_global;
>  

Yes, that's neater than the fix we currently have.  I'll grab, thanks.

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Feng Tang]

On Thu, Jul 10, 2025 at 03:01:33PM -0700, Andrew Morton wrote:
> On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Compiler is not happy about the recently added code:
> > 
> > lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> >    52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> >       |                   ^~~~~~~~~~~~~~
> > 
> > Fix it in the same way how, for example, lib/vsprintf.c does in the similar
> > cases, i.e. by using string literal directly as sizeof() parameter.
> > 
> > ...
> >
> 
> > --- a/lib/sys_info.c
> > +++ b/lib/sys_info.c
> > @@ -49,13 +49,11 @@ unsigned long sys_info_parse_param(char *str)
> >  
> >  #ifdef CONFIG_SYSCTL
> >  
> > -static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > -
> >  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> >  					  void *buffer, size_t *lenp,
> >  					  loff_t *ppos)
> >  {
> > -	char names[sizeof(sys_info_avail) + 1];
> > +	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
> >  	struct ctl_table table;
> >  	unsigned long *si_bits_global;
> >  
> 
> Yes, that's neater than the fix we currently have.  I'll grab, thanks.

Hi Andrew, Andy,

sys_info_avail[] has another purpose for being a counterpart of si_names[],
which could be extended in future, so we make it obviously stand-alone. As
for definition of si_names[], we explicitly added comment:  

	/*
	 * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
	 * below is updated accordingly.
	 */
	static const struct sys_info_name  si_names[] = {
		{ SYS_INFO_TASKS,		"tasks" },
		{ SYS_INFO_MEM,			"mem" },
		
which has also been discussed in another thread:
https://lore.kernel.org/lkml/aG3o2RFHc5iXnJef@U-2FWC9VHC-2323.local/

And I suggest to keep sys_info_avail[], and either Nathan or Sergey's patch
works for me.

Sorry for the inconvenience, and I should upgrade my gcc :) 

Thanks,
Feng

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Nathan Chancellor]

On Fri, Jul 11, 2025 at 08:54:25AM +0800, Feng Tang wrote:
> On Thu, Jul 10, 2025 at 03:01:33PM -0700, Andrew Morton wrote:
> > On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > Compiler is not happy about the recently added code:
> > > 
> > > lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> > >    52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > >       |                   ^~~~~~~~~~~~~~
> > > 
> > > Fix it in the same way how, for example, lib/vsprintf.c does in the similar
> > > cases, i.e. by using string literal directly as sizeof() parameter.
> > > 
> > > ...
> > >
> > 
> > > --- a/lib/sys_info.c
> > > +++ b/lib/sys_info.c
> > > @@ -49,13 +49,11 @@ unsigned long sys_info_parse_param(char *str)
> > >  
> > >  #ifdef CONFIG_SYSCTL
> > >  
> > > -static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > > -
> > >  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> > >  					  void *buffer, size_t *lenp,
> > >  					  loff_t *ppos)
> > >  {
> > > -	char names[sizeof(sys_info_avail) + 1];
> > > +	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
> > >  	struct ctl_table table;
> > >  	unsigned long *si_bits_global;
> > >  
> > 
> > Yes, that's neater than the fix we currently have.  I'll grab, thanks.
> 
> Hi Andrew, Andy,
> 
> sys_info_avail[] has another purpose for being a counterpart of si_names[],
> which could be extended in future, so we make it obviously stand-alone. As
> for definition of si_names[], we explicitly added comment:  
> 
> 	/*
> 	 * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
> 	 * below is updated accordingly.
> 	 */
> 	static const struct sys_info_name  si_names[] = {
> 		{ SYS_INFO_TASKS,		"tasks" },
> 		{ SYS_INFO_MEM,			"mem" },
> 		
> which has also been discussed in another thread:
> https://lore.kernel.org/lkml/aG3o2RFHc5iXnJef@U-2FWC9VHC-2323.local/
> 
> And I suggest to keep sys_info_avail[], and either Nathan or Sergey's patch
> works for me.

We could do something like this to keep the sizeof() obvious and
separate, while still eliminating the variable? Happy to bike shed
aspects of it like the macro name and such.

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 46d6f4f1ad2a..c1df502a2c0d 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -14,7 +14,7 @@ struct sys_info_name {
 };
 
 /*
- * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
+ * When 'si_names' gets updated,  please make sure SYS_INFO_MAX_LEN
  * below is updated accordingly.
  */
 static const struct sys_info_name  si_names[] = {
@@ -49,13 +49,13 @@ unsigned long sys_info_parse_param(char *str)
 
 #ifdef CONFIG_SYSCTL
 
-static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
+#define SYS_INFO_MAX_LEN (sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1)
 
 int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 					  void *buffer, size_t *lenp,
 					  loff_t *ppos)
 {
-	char names[sizeof(sys_info_avail) + 1];
+	char names[SYS_INFO_MAX_LEN];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
 
---

> Sorry for the inconvenience, and I should upgrade my gcc :) 

I am not sure that GCC has this warning, I have only ever seen it with
clang.

Cheers,
Nathan

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Feng Tang]

On Thu, Jul 10, 2025 at 06:49:47PM -0700, Nathan Chancellor wrote:
> On Fri, Jul 11, 2025 at 08:54:25AM +0800, Feng Tang wrote:
> > On Thu, Jul 10, 2025 at 03:01:33PM -0700, Andrew Morton wrote:
> > > On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > 
> > > > Compiler is not happy about the recently added code:
> > > > 
> > > > lib/sys_info.c:52:19: error: variable 'sys_info_avail' is not needed and will not be emitted [-Werror,-Wunneeded-internal-declaration]
> > > >    52 | static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > > >       |                   ^~~~~~~~~~~~~~
> > > > 
> > > > Fix it in the same way how, for example, lib/vsprintf.c does in the similar
> > > > cases, i.e. by using string literal directly as sizeof() parameter.
> > > > 
> > > > ...
> > > >
> > > 
> > > > --- a/lib/sys_info.c
> > > > +++ b/lib/sys_info.c
> > > > @@ -49,13 +49,11 @@ unsigned long sys_info_parse_param(char *str)
> > > >  
> > > >  #ifdef CONFIG_SYSCTL
> > > >  
> > > > -static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > > > -
> > > >  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> > > >  					  void *buffer, size_t *lenp,
> > > >  					  loff_t *ppos)
> > > >  {
> > > > -	char names[sizeof(sys_info_avail) + 1];
> > > > +	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
> > > >  	struct ctl_table table;
> > > >  	unsigned long *si_bits_global;
> > > >  
> > > 
> > > Yes, that's neater than the fix we currently have.  I'll grab, thanks.
> > 
> > Hi Andrew, Andy,
> > 
> > sys_info_avail[] has another purpose for being a counterpart of si_names[],
> > which could be extended in future, so we make it obviously stand-alone. As
> > for definition of si_names[], we explicitly added comment:  
> > 
> > 	/*
> > 	 * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
> > 	 * below is updated accordingly.
> > 	 */
> > 	static const struct sys_info_name  si_names[] = {
> > 		{ SYS_INFO_TASKS,		"tasks" },
> > 		{ SYS_INFO_MEM,			"mem" },
> > 		
> > which has also been discussed in another thread:
> > https://lore.kernel.org/lkml/aG3o2RFHc5iXnJef@U-2FWC9VHC-2323.local/
> > 
> > And I suggest to keep sys_info_avail[], and either Nathan or Sergey's patch
> > works for me.
> 
> We could do something like this to keep the sizeof() obvious and
> separate, while still eliminating the variable? Happy to bike shed
> aspects of it like the macro name and such.
> 
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 46d6f4f1ad2a..c1df502a2c0d 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -14,7 +14,7 @@ struct sys_info_name {
>  };
>  
>  /*
> - * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
> + * When 'si_names' gets updated,  please make sure SYS_INFO_MAX_LEN
>   * below is updated accordingly.
>   */
>  static const struct sys_info_name  si_names[] = {
> @@ -49,13 +49,13 @@ unsigned long sys_info_parse_param(char *str)
>  
>  #ifdef CONFIG_SYSCTL
>  
> -static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> +#define SYS_INFO_MAX_LEN (sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1)
>  
>  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  					  void *buffer, size_t *lenp,
>  					  loff_t *ppos)
>  {
> -	char names[sizeof(sys_info_avail) + 1];
> +	char names[SYS_INFO_MAX_LEN];
>  	struct ctl_table table;
>  	unsigned long *si_bits_global;
  
Looks great to me, thanks!

We can even move the SYS_INFO_MAX_LEN definition close to si_names[],
initially sys_info_avail[] was next to si_names[], and was moved inside 
"#ifdef CONFIG_SYSCTL" region for compiling CONFIG_SYSCTL=n case.

> ---
> 
> > Sorry for the inconvenience, and I should upgrade my gcc :) 
> 
> I am not sure that GCC has this warning, I have only ever seen it with
> clang.

Got it.

Thanks,
Feng

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 10:48:29AM +0800, Feng Tang wrote:
> On Thu, Jul 10, 2025 at 06:49:47PM -0700, Nathan Chancellor wrote:
> > On Fri, Jul 11, 2025 at 08:54:25AM +0800, Feng Tang wrote:
> > > On Thu, Jul 10, 2025 at 03:01:33PM -0700, Andrew Morton wrote:
> > > > On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > > sys_info_avail[] has another purpose for being a counterpart of si_names[],
> > > which could be extended in future, so we make it obviously stand-alone. As
> > > for definition of si_names[], we explicitly added comment:  
> > > 
> > > 	/*
> > > 	 * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
> > > 	 * below is updated accordingly.
> > > 	 */
> > > 	static const struct sys_info_name  si_names[] = {
> > > 		{ SYS_INFO_TASKS,		"tasks" },
> > > 		{ SYS_INFO_MEM,			"mem" },
> > > 		
> > > which has also been discussed in another thread:
> > > https://lore.kernel.org/lkml/aG3o2RFHc5iXnJef@U-2FWC9VHC-2323.local/
> > > 
> > > And I suggest to keep sys_info_avail[], and either Nathan or Sergey's patch
> > > works for me.
> > 
> > We could do something like this to keep the sizeof() obvious and
> > separate, while still eliminating the variable? Happy to bike shed
> > aspects of it like the macro name and such.

...

> > +#define SYS_INFO_MAX_LEN (sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1)

The ' + 1' is batter to leave for the names below.
Or at bare minimum it needs a semantic explanation.
If it's for NUL, definitely shouldn't be part of the
definition.

> >  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> >  					  void *buffer, size_t *lenp,
> >  					  loff_t *ppos)
> >  {
> > -	char names[sizeof(sys_info_avail) + 1];
> > +	char names[SYS_INFO_MAX_LEN];
> >  	struct ctl_table table;
> >  	unsigned long *si_bits_global;
>   
> Looks great to me, thanks!
> 
> We can even move the SYS_INFO_MAX_LEN definition close to si_names[],

Agree.

> initially sys_info_avail[] was next to si_names[], and was moved inside 
> "#ifdef CONFIG_SYSCTL" region for compiling CONFIG_SYSCTL=n case.

But let me think a bit, perhaps we can come up with something even better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 09:04:29AM +0300, Andy Shevchenko wrote:
> On Fri, Jul 11, 2025 at 10:48:29AM +0800, Feng Tang wrote:
> > On Thu, Jul 10, 2025 at 06:49:47PM -0700, Nathan Chancellor wrote:

...

> > We can even move the SYS_INFO_MAX_LEN definition close to si_names[],
> 
> Agree.
> 
> > initially sys_info_avail[] was next to si_names[], and was moved inside
> > "#ifdef CONFIG_SYSCTL" region for compiling CONFIG_SYSCTL=n case.
> 
> But let me think a bit, perhaps we can come up with something even better.

Okay, guys, I have a better approach found. Let me mock up a patch series
for that (we need one prerequisite).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] panic: Fix compilation error (`make W=1`)

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 08:54:25AM +0800, Feng Tang wrote:
> On Thu, Jul 10, 2025 at 03:01:33PM -0700, Andrew Morton wrote:
> > On Thu, 10 Jul 2025 12:48:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > > -	char names[sizeof(sys_info_avail) + 1];
> > > +	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
> > 
> > Yes, that's neater than the fix we currently have.  I'll grab, thanks.
> 
> sys_info_avail[] has another purpose for being a counterpart of si_names[],
> which could be extended in future, so we make it obviously stand-alone. As
> for definition of si_names[], we explicitly added comment:  
> 
> 	/*
> 	 * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
> 	 * below is updated accordingly.
> 	 */
> 	static const struct sys_info_name  si_names[] = {
> 		{ SYS_INFO_TASKS,		"tasks" },
> 		{ SYS_INFO_MEM,			"mem" },
> 		
> which has also been discussed in another thread:
> https://lore.kernel.org/lkml/aG3o2RFHc5iXnJef@U-2FWC9VHC-2323.local/

Ah, this is interesting. Let's rethink about the solution.

> And I suggest to keep sys_info_avail[], and either Nathan or Sergey's patch
> works for me.

I definitely not suggest to leave build broken
(try with CONFOG_WERROR=y, which is default).

> Sorry for the inconvenience, and I should upgrade my gcc :) 

I have compiled with clang-19.

-- 
With Best Regards,
Andy Shevchenko