[PATCH v2 0/6] panic: sys_info: Refactor and fix a potential issue

[PATCH v2 0/6] panic: sys_info: Refactor and fix a potential issue

[Andy Shevchenko]

While targeting the compilation issue due to dangling variable,
I have noticed more opportunities for refactoring that helps to
avoid above mentioned compilation issue in a cleaner way and
also fixes a potential problem with global variable access.
Please, give it a try.

Changelog v2:
- rebased on top of the current codebase
- addressed an issue when converting to match_string() (Feng)
- Cc'ed to Petr (requested by Feng)

v1: https://lore.kernel.org/r/20250711095413.1472448-1-andriy.shevchenko@linux.intel.com

Andy Shevchenko (6):
  panic: sys_info: Capture si_bits_global before iterating over it
  panic: sys_info: Align constant definition names with parameters
  panic: sys_info: Replace struct sys_info_name with plain array of
    strings
  panic: sys_info: Rewrite a fix for a compilation error (`make W=1`)
  panic: sys_info: Deduplicate local variable 'table; assignments
  panic: sys_info: Factor out read and write handlers

 include/linux/sys_info.h |   2 +-
 kernel/panic.c           |   2 +-
 lib/sys_info.c           | 133 ++++++++++++++++++++++-----------------
 3 files changed, 76 insertions(+), 61 deletions(-)

-- 
2.50.1

[PATCH v2 1/6] panic: sys_info: Capture si_bits_global before iterating over it

[Andy Shevchenko]

The for-loop might re-read the content of the memory the si_bits_global
points to on each iteration. Instead, just capture it for the sake of
consistency and use that instead.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 496f9151c9b6..d542a024406a 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -58,11 +58,11 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 	char names[sizeof(sys_info_avail)];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
+	unsigned long si_bits;
 
 	si_bits_global = ro_table->data;
 
 	if (write) {
-		unsigned long si_bits;
 		int ret;
 
 		table = *ro_table;
@@ -81,9 +81,12 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 		char *delim = "";
 		int i, len = 0;
 
+		/* The access to the global value is not synchronized. */
+		si_bits = READ_ONCE(*si_bits_global);
+
 		names[0] = '\0';
 		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
-			if (*si_bits_global & si_names[i].bit) {
+			if (si_bits & si_names[i].bit) {
 				len += scnprintf(names + len, sizeof(names) - len,
 					"%s%s", delim, si_names[i].name);
 				delim = ",";
-- 
2.50.1

Re: [PATCH v2 1/6] panic: sys_info: Capture si_bits_global before iterating over it

[Feng Tang]

On Wed, Oct 29, 2025 at 12:07:36PM +0100, Andy Shevchenko wrote:
> The for-loop might re-read the content of the memory the si_bits_global
> points to on each iteration. Instead, just capture it for the sake of
> consistency and use that instead.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>

Thanks!

[PATCH v2 2/6] panic: sys_info: Align constant definition names with parameters

[Andy Shevchenko]

Align constant definition names with parameters to make it easier
to map. It's also better to maintain and extend the names while
keeping their uniqueness.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/sys_info.h | 2 +-
 kernel/panic.c           | 2 +-
 lib/sys_info.c           | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
index 89d77dc4f2ed..a5bc3ea3d44b 100644
--- a/include/linux/sys_info.h
+++ b/include/linux/sys_info.h
@@ -14,7 +14,7 @@
 #define SYS_INFO_LOCKS			0x00000008
 #define SYS_INFO_FTRACE			0x00000010
 #define SYS_INFO_PANIC_CONSOLE_REPLAY	0x00000020
-#define SYS_INFO_ALL_CPU_BT		0x00000040
+#define SYS_INFO_ALL_BT			0x00000040
 #define SYS_INFO_BLOCKED_TASKS		0x00000080
 
 void sys_info(unsigned long si_mask);
diff --git a/kernel/panic.c b/kernel/panic.c
index 341c66948dcb..0d52210a9e2b 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -401,7 +401,7 @@ static void panic_trigger_all_cpu_backtrace(void)
  */
 static void panic_other_cpus_shutdown(bool crash_kexec)
 {
-	if (panic_print & SYS_INFO_ALL_CPU_BT)
+	if (panic_print & SYS_INFO_ALL_BT)
 		panic_trigger_all_cpu_backtrace();
 
 	/*
diff --git a/lib/sys_info.c b/lib/sys_info.c
index d542a024406a..6b0188b30227 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -23,7 +23,7 @@ static const struct sys_info_name  si_names[] = {
 	{ SYS_INFO_TIMERS,		"timers" },
 	{ SYS_INFO_LOCKS,		"locks" },
 	{ SYS_INFO_FTRACE,		"ftrace" },
-	{ SYS_INFO_ALL_CPU_BT,		"all_bt" },
+	{ SYS_INFO_ALL_BT,		"all_bt" },
 	{ SYS_INFO_BLOCKED_TASKS,	"blocked_tasks" },
 };
 
@@ -118,7 +118,7 @@ void sys_info(unsigned long si_mask)
 	if (si_mask & SYS_INFO_FTRACE)
 		ftrace_dump(DUMP_ALL);
 
-	if (si_mask & SYS_INFO_ALL_CPU_BT)
+	if (si_mask & SYS_INFO_ALL_BT)
 		trigger_all_cpu_backtrace();
 
 	if (si_mask & SYS_INFO_BLOCKED_TASKS)
-- 
2.50.1

Re: [PATCH v2 2/6] panic: sys_info: Align constant definition names with parameters

[Feng Tang]

On Wed, Oct 29, 2025 at 12:07:37PM +0100, Andy Shevchenko wrote:
> Align constant definition names with parameters to make it easier
> to map. It's also better to maintain and extend the names while
> keeping their uniqueness.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>

Thanks,
Feng

>
> ---
>  include/linux/sys_info.h | 2 +-
>  kernel/panic.c           | 2 +-
>  lib/sys_info.c           | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
> index 89d77dc4f2ed..a5bc3ea3d44b 100644
> --- a/include/linux/sys_info.h
> +++ b/include/linux/sys_info.h
> @@ -14,7 +14,7 @@
>  #define SYS_INFO_LOCKS			0x00000008
>  #define SYS_INFO_FTRACE			0x00000010
>  #define SYS_INFO_PANIC_CONSOLE_REPLAY	0x00000020
> -#define SYS_INFO_ALL_CPU_BT		0x00000040
> +#define SYS_INFO_ALL_BT			0x00000040
>  #define SYS_INFO_BLOCKED_TASKS		0x00000080
>  
>  void sys_info(unsigned long si_mask);
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 341c66948dcb..0d52210a9e2b 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -401,7 +401,7 @@ static void panic_trigger_all_cpu_backtrace(void)
>   */
>  static void panic_other_cpus_shutdown(bool crash_kexec)
>  {
> -	if (panic_print & SYS_INFO_ALL_CPU_BT)
> +	if (panic_print & SYS_INFO_ALL_BT)
>  		panic_trigger_all_cpu_backtrace();
>  
>  	/*
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index d542a024406a..6b0188b30227 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -23,7 +23,7 @@ static const struct sys_info_name  si_names[] = {
>  	{ SYS_INFO_TIMERS,		"timers" },
>  	{ SYS_INFO_LOCKS,		"locks" },
>  	{ SYS_INFO_FTRACE,		"ftrace" },
> -	{ SYS_INFO_ALL_CPU_BT,		"all_bt" },
> +	{ SYS_INFO_ALL_BT,		"all_bt" },
>  	{ SYS_INFO_BLOCKED_TASKS,	"blocked_tasks" },
>  };
>  
> @@ -118,7 +118,7 @@ void sys_info(unsigned long si_mask)
>  	if (si_mask & SYS_INFO_FTRACE)
>  		ftrace_dump(DUMP_ALL);
>  
> -	if (si_mask & SYS_INFO_ALL_CPU_BT)
> +	if (si_mask & SYS_INFO_ALL_BT)
>  		trigger_all_cpu_backtrace();
>  
>  	if (si_mask & SYS_INFO_BLOCKED_TASKS)
> -- 
> 2.50.1

[PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Andy Shevchenko]

There is no need to keep a custom structure just for the need of
a plain array of strings. Replace struct sys_info_name with plain
array of strings.

With that done, simplify the code, in particular, naturally use
for_each_set_bit() when iterating over si_bits_global bitmap.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 6b0188b30227..5aecf4b6025f 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -1,30 +1,29 @@
 // SPDX-License-Identifier: GPL-2.0-only
-#include <linux/sched/debug.h>
+#include <linux/bitops.h>
 #include <linux/console.h>
+#include <linux/log2.h>
 #include <linux/kernel.h>
 #include <linux/ftrace.h>
-#include <linux/sysctl.h>
 #include <linux/nmi.h>
+#include <linux/sched/debug.h>
+#include <linux/string.h>
+#include <linux/sysctl.h>
 
 #include <linux/sys_info.h>
 
-struct sys_info_name {
-	unsigned long bit;
-	const char *name;
-};
-
 /*
  * 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" },
-	{ SYS_INFO_TIMERS,		"timers" },
-	{ SYS_INFO_LOCKS,		"locks" },
-	{ SYS_INFO_FTRACE,		"ftrace" },
-	{ SYS_INFO_ALL_BT,		"all_bt" },
-	{ SYS_INFO_BLOCKED_TASKS,	"blocked_tasks" },
+static const char * const si_names[] = {
+	[ilog2(SYS_INFO_TASKS)]			= "tasks",
+	[ilog2(SYS_INFO_MEM)]			= "mem",
+	[ilog2(SYS_INFO_TIMERS)]		= "timers",
+	[ilog2(SYS_INFO_LOCKS)]			= "locks",
+	[ilog2(SYS_INFO_FTRACE)]		= "ftrace",
+	[ilog2(SYS_INFO_PANIC_CONSOLE_REPLAY)]	= "",
+	[ilog2(SYS_INFO_ALL_BT)]		= "all_bt",
+	[ilog2(SYS_INFO_BLOCKED_TASKS)]		= "blocked_tasks",
 };
 
 /* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
@@ -36,12 +35,9 @@ unsigned long sys_info_parse_param(char *str)
 
 	s = str;
 	while ((name = strsep(&s, ",")) && *name) {
-		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
-			if (!strcmp(name, si_names[i].name)) {
-				si_bits |= si_names[i].bit;
-				break;
-			}
-		}
+		i = match_string(si_names, ARRAY_SIZE(si_names), name);
+		if (i >= 0)
+			__set_bit(i, &si_bits);
 	}
 
 	return si_bits;
@@ -85,12 +81,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 		si_bits = READ_ONCE(*si_bits_global);
 
 		names[0] = '\0';
-		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
-			if (si_bits & si_names[i].bit) {
-				len += scnprintf(names + len, sizeof(names) - len,
-					"%s%s", delim, si_names[i].name);
-				delim = ",";
-			}
+		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
+			len += scnprintf(names + len, sizeof(names) - len,
+					 "%s%s", delim, si_names[i]);
+			delim = ",";
 		}
 
 		table = *ro_table;
-- 
2.50.1

Re: [PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Feng Tang]

On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> There is no need to keep a custom structure just for the need of
> a plain array of strings. Replace struct sys_info_name with plain
> array of strings.
> 
> With that done, simplify the code, in particular, naturally use
> for_each_set_bit() when iterating over si_bits_global bitmap.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/sys_info.c | 48 +++++++++++++++++++++---------------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 6b0188b30227..5aecf4b6025f 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -1,30 +1,29 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -#include <linux/sched/debug.h>
> +#include <linux/bitops.h>
>  #include <linux/console.h>
> +#include <linux/log2.h>
>  #include <linux/kernel.h>
>  #include <linux/ftrace.h>
> -#include <linux/sysctl.h>
>  #include <linux/nmi.h>
> +#include <linux/sched/debug.h>
> +#include <linux/string.h>
> +#include <linux/sysctl.h>
>  
>  #include <linux/sys_info.h>
>  
> -struct sys_info_name {
> -	unsigned long bit;
> -	const char *name;
> -};
> -
>  /*
>   * 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" },
> -	{ SYS_INFO_TIMERS,		"timers" },
> -	{ SYS_INFO_LOCKS,		"locks" },
> -	{ SYS_INFO_FTRACE,		"ftrace" },
> -	{ SYS_INFO_ALL_BT,		"all_bt" },
> -	{ SYS_INFO_BLOCKED_TASKS,	"blocked_tasks" },
> +static const char * const si_names[] = {
> +	[ilog2(SYS_INFO_TASKS)]			= "tasks",
> +	[ilog2(SYS_INFO_MEM)]			= "mem",
> +	[ilog2(SYS_INFO_TIMERS)]		= "timers",
> +	[ilog2(SYS_INFO_LOCKS)]			= "locks",
> +	[ilog2(SYS_INFO_FTRACE)]		= "ftrace",
> +	[ilog2(SYS_INFO_PANIC_CONSOLE_REPLAY)]	= "",
> +	[ilog2(SYS_INFO_ALL_BT)]		= "all_bt",
> +	[ilog2(SYS_INFO_BLOCKED_TASKS)]		= "blocked_tasks",
>  };
>  
>  /* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
> @@ -36,12 +35,9 @@ unsigned long sys_info_parse_param(char *str)
>  
>  	s = str;
>  	while ((name = strsep(&s, ",")) && *name) {
> -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> -			if (!strcmp(name, si_names[i].name)) {
> -				si_bits |= si_names[i].bit;
> -				break;
> -			}
> -		}
> +		i = match_string(si_names, ARRAY_SIZE(si_names), name);
> +		if (i >= 0)
> +			__set_bit(i, &si_bits);
>  	}
>  
>  	return si_bits;
> @@ -85,12 +81,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  		si_bits = READ_ONCE(*si_bits_global);
>  
>  		names[0] = '\0';
> -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> -			if (si_bits & si_names[i].bit) {
> -				len += scnprintf(names + len, sizeof(names) - len,
> -					"%s%s", delim, si_names[i].name);
> -				delim = ",";
> -			}
> +		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> +			len += scnprintf(names + len, sizeof(names) - len,
> +					 "%s%s", delim, si_names[i]);
> +			delim = ",";

For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
an extra ','?

Thanks,
Feng

Re: [PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > There is no need to keep a custom structure just for the need of
> > a plain array of strings. Replace struct sys_info_name with plain
> > array of strings.
> > 
> > With that done, simplify the code, in particular, naturally use
> > for_each_set_bit() when iterating over si_bits_global bitmap.

...

> >  		names[0] = '\0';
> > -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > -			if (si_bits & si_names[i].bit) {
> > -				len += scnprintf(names + len, sizeof(names) - len,
> > -					"%s%s", delim, si_names[i].name);
> > -				delim = ",";
> > -			}
> > +		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > +			len += scnprintf(names + len, sizeof(names) - len,
> > +					 "%s%s", delim, si_names[i]);
> > +			delim = ",";
> 
> For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> an extra ','?

If you look closer to the original code, the behaviour is the same. Feel free
to update behaviour separately as I tried to keep the functionality to be not
changed with my series (with the exceptions of the fetching issue).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Feng Tang]

On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > There is no need to keep a custom structure just for the need of
> > > a plain array of strings. Replace struct sys_info_name with plain
> > > array of strings.
> > > 
> > > With that done, simplify the code, in particular, naturally use
> > > for_each_set_bit() when iterating over si_bits_global bitmap.
> 
> ...
> 
> > >  		names[0] = '\0';
> > > -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > -			if (si_bits & si_names[i].bit) {
> > > -				len += scnprintf(names + len, sizeof(names) - len,
> > > -					"%s%s", delim, si_names[i].name);
> > > -				delim = ",";
> > > -			}
> > > +		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > +			len += scnprintf(names + len, sizeof(names) - len,
> > > +					 "%s%s", delim, si_names[i]);
> > > +			delim = ",";
> > 
> > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > an extra ','?
> 
> If you look closer to the original code, the behaviour is the same. Feel free
> to update behaviour separately as I tried to keep the functionality to be not
> changed with my series (with the exceptions of the fetching issue).

I gave the comment by code-reading.

And to double check it, I just run a simple test by adding "panic_print=0xff"
to cmdline, with the current kernel, by running "sysctl  kernel.panic_sys_info"
on current kernel, it will get:

	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'

And after applying your first 3 patches, it will show:
	
	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'

Thanks,
Feng

Re: [PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 04:45:24PM +0800, Feng Tang wrote:
> On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > > There is no need to keep a custom structure just for the need of
> > > > a plain array of strings. Replace struct sys_info_name with plain
> > > > array of strings.
> > > > 
> > > > With that done, simplify the code, in particular, naturally use
> > > > for_each_set_bit() when iterating over si_bits_global bitmap.

...

> > > >  		names[0] = '\0';
> > > > -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > > -			if (si_bits & si_names[i].bit) {
> > > > -				len += scnprintf(names + len, sizeof(names) - len,
> > > > -					"%s%s", delim, si_names[i].name);
> > > > -				delim = ",";
> > > > -			}
> > > > +		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > > +			len += scnprintf(names + len, sizeof(names) - len,
> > > > +					 "%s%s", delim, si_names[i]);
> > > > +			delim = ",";
> > > 
> > > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > > an extra ','?
> > 
> > If you look closer to the original code, the behaviour is the same. Feel free
> > to update behaviour separately as I tried to keep the functionality to be not
> > changed with my series (with the exceptions of the fetching issue).
> 
> I gave the comment by code-reading.
> 
> And to double check it, I just run a simple test by adding "panic_print=0xff"
> to cmdline, with the current kernel, by running "sysctl  kernel.panic_sys_info"
> on current kernel, it will get:
> 
> 	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'
> 
> And after applying your first 3 patches, it will show:
> 	
> 	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'

Thanks for the test, now I see the issue! Before si_names has no entry for that bit.
I will address this in next version. Sorry for the confusion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Feng Tang]

On Thu, Oct 30, 2025 at 11:05:59AM +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 04:45:24PM +0800, Feng Tang wrote:
> > On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> > > On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > > > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > > > There is no need to keep a custom structure just for the need of
> > > > > a plain array of strings. Replace struct sys_info_name with plain
> > > > > array of strings.
> > > > > 
> > > > > With that done, simplify the code, in particular, naturally use
> > > > > for_each_set_bit() when iterating over si_bits_global bitmap.
> 
> ...
> 
> > > > >  		names[0] = '\0';
> > > > > -		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > > > -			if (si_bits & si_names[i].bit) {
> > > > > -				len += scnprintf(names + len, sizeof(names) - len,
> > > > > -					"%s%s", delim, si_names[i].name);
> > > > > -				delim = ",";
> > > > > -			}
> > > > > +		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > > > +			len += scnprintf(names + len, sizeof(names) - len,
> > > > > +					 "%s%s", delim, si_names[i]);
> > > > > +			delim = ",";
> > > > 
> > > > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > > > an extra ','?
> > > 
> > > If you look closer to the original code, the behaviour is the same. Feel free
> > > to update behaviour separately as I tried to keep the functionality to be not
> > > changed with my series (with the exceptions of the fetching issue).
> > 
> > I gave the comment by code-reading.
> > 
> > And to double check it, I just run a simple test by adding "panic_print=0xff"
> > to cmdline, with the current kernel, by running "sysctl  kernel.panic_sys_info"
> > on current kernel, it will get:
> > 
> > 	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'
> > 
> > And after applying your first 3 patches, it will show:
> > 	
> > 	'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'
> 
> Thanks for the test, now I see the issue! Before si_names has no entry for that bit.
> I will address this in next version. Sorry for the confusion.

No problem. Thanks for helping to improve the code!

- Feng

[PATCH v2 4/6] panic: sys_info: Rewrite a fix for a compilation error (`make W=1`)

[Andy Shevchenko]

Compiler was not happy about dead variable in use:

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";
      |                   ^~~~~~~~~~~~~~

This was fixed by adding __maybe_unused attribute that just hides the issue
and didn't actually fix the root cause. Rewrite the fix by moving the local
variable from stack to a heap.

As a side effect this drops unneeded "synchronisation" of duplicative info
and also makes code ready for the further refactoring.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 5aecf4b6025f..027b2c432d07 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/array_size.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/console.h>
 #include <linux/log2.h>
 #include <linux/kernel.h>
@@ -11,10 +13,6 @@
 
 #include <linux/sys_info.h>
 
-/*
- * When 'si_names' gets updated,  please make sure the 'sys_info_avail'
- * below is updated accordingly.
- */
 static const char * const si_names[] = {
 	[ilog2(SYS_INFO_TASKS)]			= "tasks",
 	[ilog2(SYS_INFO_MEM)]			= "mem",
@@ -45,25 +43,32 @@ unsigned long sys_info_parse_param(char *str)
 
 #ifdef CONFIG_SYSCTL
 
-static const char sys_info_avail[] __maybe_unused = "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)];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
 	unsigned long si_bits;
+	unsigned int i;
+	size_t maxlen;
 
 	si_bits_global = ro_table->data;
 
+	maxlen = 0;
+	for (i = 0; i < ARRAY_SIZE(si_names); i++)
+		maxlen += strlen(si_names[i]) + 1;
+
+	char *names __free(kfree) = kzalloc(maxlen, GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
 	if (write) {
 		int ret;
 
 		table = *ro_table;
 		table.data = names;
-		table.maxlen = sizeof(names);
+		table.maxlen = maxlen;
 		ret = proc_dostring(&table, write, buffer, lenp, ppos);
 		if (ret)
 			return ret;
@@ -74,22 +79,21 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 		return 0;
 	} else {
 		/* for 'read' operation */
+		unsigned int len = 0;
 		char *delim = "";
-		int i, len = 0;
 
 		/* The access to the global value is not synchronized. */
 		si_bits = READ_ONCE(*si_bits_global);
 
-		names[0] = '\0';
 		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
-			len += scnprintf(names + len, sizeof(names) - len,
+			len += scnprintf(names + len, maxlen - len,
 					 "%s%s", delim, si_names[i]);
 			delim = ",";
 		}
 
 		table = *ro_table;
 		table.data = names;
-		table.maxlen = sizeof(names);
+		table.maxlen = maxlen;
 		return proc_dostring(&table, write, buffer, lenp, ppos);
 	}
 }
-- 
2.50.1

[PATCH v2 5/6] panic: sys_info: Deduplicate local variable 'table; assignments

[Andy Shevchenko]

The both handlers use the local 'table' variable and assign
the same data to it, deduplicate that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 027b2c432d07..c86f52644857 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -63,12 +63,13 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 	if (!names)
 		return -ENOMEM;
 
+	table = *ro_table;
+	table.data = names;
+	table.maxlen = maxlen;
+
 	if (write) {
 		int ret;
 
-		table = *ro_table;
-		table.data = names;
-		table.maxlen = maxlen;
 		ret = proc_dostring(&table, write, buffer, lenp, ppos);
 		if (ret)
 			return ret;
@@ -91,9 +92,6 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 			delim = ",";
 		}
 
-		table = *ro_table;
-		table.data = names;
-		table.maxlen = maxlen;
 		return proc_dostring(&table, write, buffer, lenp, ppos);
 	}
 }
-- 
2.50.1

Re: [PATCH v2 5/6] panic: sys_info: Deduplicate local variable 'table; assignments

[Feng Tang]

On Wed, Oct 29, 2025 at 12:07:40PM +0100, Andy Shevchenko wrote:
> The both handlers use the local 'table' variable and assign
> the same data to it, deduplicate that.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>

Thanks!

[PATCH v2 6/6] panic: sys_info: Factor out read and write handlers

[Andy Shevchenko]

For the sake of the code readability and easier maintenance
factor out read and write sys_info handlers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/sys_info.c | 72 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index c86f52644857..8ed3b4b55854 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -43,13 +43,52 @@ unsigned long sys_info_parse_param(char *str)
 
 #ifdef CONFIG_SYSCTL
 
+static int sys_info_write_handler(struct ctl_table *table,
+				  void *buffer, size_t *lenp, loff_t *ppos,
+				  unsigned long *si_bits_global)
+{
+	unsigned long si_bits;
+	int ret;
+
+	ret = proc_dostring(table, 1, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+
+	si_bits = sys_info_parse_param(table->data);
+
+	/* The access to the global value is not synchronized. */
+	WRITE_ONCE(*si_bits_global, si_bits);
+
+	return 0;
+}
+
+static int sys_info_read_handler(struct ctl_table *table,
+				 void *buffer, size_t *lenp, loff_t *ppos,
+				 unsigned long *si_bits_global)
+{
+	unsigned long si_bits;
+	unsigned int len = 0;
+	char *delim = "";
+	unsigned int i;
+
+	/* The access to the global value is not synchronized. */
+	si_bits = READ_ONCE(*si_bits_global);
+
+	for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
+		len += scnprintf(table->data + len, table->maxlen - len,
+				 "%s%s", delim, si_names[i]);
+		delim = ",";
+	}
+
+	return proc_dostring(table, 0, buffer, lenp, ppos);
+}
+
 int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 					  void *buffer, size_t *lenp,
 					  loff_t *ppos)
 {
 	struct ctl_table table;
 	unsigned long *si_bits_global;
-	unsigned long si_bits;
 	unsigned int i;
 	size_t maxlen;
 
@@ -67,33 +106,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 	table.data = names;
 	table.maxlen = maxlen;
 
-	if (write) {
-		int ret;
-
-		ret = proc_dostring(&table, write, buffer, lenp, ppos);
-		if (ret)
-			return ret;
-
-		si_bits = sys_info_parse_param(names);
-		/* The access to the global value is not synchronized. */
-		WRITE_ONCE(*si_bits_global, si_bits);
-		return 0;
-	} else {
-		/* for 'read' operation */
-		unsigned int len = 0;
-		char *delim = "";
-
-		/* The access to the global value is not synchronized. */
-		si_bits = READ_ONCE(*si_bits_global);
-
-		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
-			len += scnprintf(names + len, maxlen - len,
-					 "%s%s", delim, si_names[i]);
-			delim = ",";
-		}
-
-		return proc_dostring(&table, write, buffer, lenp, ppos);
-	}
+	if (write)
+		return sys_info_write_handler(&table, buffer, lenp, ppos, si_bits_global);
+	else
+		return sys_info_read_handler(&table, buffer, lenp, ppos, si_bits_global);
 }
 #endif
 
-- 
2.50.1

Re: [PATCH v2 6/6] panic: sys_info: Factor out read and write handlers

[Feng Tang]

On Wed, Oct 29, 2025 at 12:07:41PM +0100, Andy Shevchenko wrote:
> For the sake of the code readability and easier maintenance
> factor out read and write sys_info handlers.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/sys_info.c | 72 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index c86f52644857..8ed3b4b55854 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -43,13 +43,52 @@ unsigned long sys_info_parse_param(char *str)
>  
>  #ifdef CONFIG_SYSCTL
>  
> +static int sys_info_write_handler(struct ctl_table *table,
> +				  void *buffer, size_t *lenp, loff_t *ppos,
> +				  unsigned long *si_bits_global)
> +{
> +	unsigned long si_bits;
> +	int ret;
> +
> +	ret = proc_dostring(table, 1, buffer, lenp, ppos);
> +	if (ret)
> +		return ret;
> +
> +	si_bits = sys_info_parse_param(table->data);
> +
> +	/* The access to the global value is not synchronized. */
> +	WRITE_ONCE(*si_bits_global, si_bits);
> +
> +	return 0;
> +}
> +
> +static int sys_info_read_handler(struct ctl_table *table,
> +				 void *buffer, size_t *lenp, loff_t *ppos,
> +				 unsigned long *si_bits_global)
> +{
> +	unsigned long si_bits;
> +	unsigned int len = 0;
> +	char *delim = "";
> +	unsigned int i;
> +
> +	/* The access to the global value is not synchronized. */
> +	si_bits = READ_ONCE(*si_bits_global);
> +
> +	for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> +		len += scnprintf(table->data + len, table->maxlen - len,
> +				 "%s%s", delim, si_names[i]);
> +		delim = ",";
> +	}
> +
> +	return proc_dostring(table, 0, buffer, lenp, ppos);
> +}
> +
>  int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  					  void *buffer, size_t *lenp,
>  					  loff_t *ppos)
>  {
>  	struct ctl_table table;
>  	unsigned long *si_bits_global;
> -	unsigned long si_bits;
>  	unsigned int i;
>  	size_t maxlen;
>  
> @@ -67,33 +106,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  	table.data = names;
>  	table.maxlen = maxlen;
>  
> -	if (write) {
> -		int ret;
> -
> -		ret = proc_dostring(&table, write, buffer, lenp, ppos);
> -		if (ret)
> -			return ret;
> -
> -		si_bits = sys_info_parse_param(names);
> -		/* The access to the global value is not synchronized. */
> -		WRITE_ONCE(*si_bits_global, si_bits);
> -		return 0;
> -	} else {
> -		/* for 'read' operation */
> -		unsigned int len = 0;
> -		char *delim = "";
> -
> -		/* The access to the global value is not synchronized. */
> -		si_bits = READ_ONCE(*si_bits_global);
> -
> -		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> -			len += scnprintf(names + len, maxlen - len,
> -					 "%s%s", delim, si_names[i]);
> -			delim = ",";
> -		}
> -
> -		return proc_dostring(&table, write, buffer, lenp, ppos);
> -	}

As the if and else block each has only round 10 lines of code, and fit a
screen well, I'm not sure whether adding 2 new functions will improve
readabilty, but I'm fine with the change.

Thanks,
Feng

Re: [PATCH v2 6/6] panic: sys_info: Factor out read and write handlers

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 10:06:14AM +0800, Feng Tang wrote:
> On Wed, Oct 29, 2025 at 12:07:41PM +0100, Andy Shevchenko wrote:
> > For the sake of the code readability and easier maintenance
> > factor out read and write sys_info handlers.

...

> As the if and else block each has only round 10 lines of code, and fit a
> screen well, I'm not sure whether adding 2 new functions will improve
> readabilty, but I'm fine with the change.

It increases readability and understanding as, for example, one don't need to
check what "write" is when giving as a parameter, Also if you noticed one of my
patch actually made it short by a few lines. So, better to compare the before
_the series_ and after. And even so, the size is not often the main metric for
the readability. I made this patch as I obviously see the benefit of it.

P.S. Thanks for your review!

-- 
With Best Regards,
Andy Shevchenko