[rft, PATCH v1 0/7] panic: sys_info: Refactor and fix a compilation issue

[rft, PATCH v1 0/7] panic: sys_info: Refactor and fix a compilation 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 and make code cleaner in
general. Please, give it a try.

I put a revert as the first patch of the previous solution, but I believe
the previous solution may be pulled out without a problem.

Andy Shevchenko (7):
  Revert "panic: fix compilation error (`make W=1`)"
  panic: sys_info: Align constant definition names with parameters
  panic: sys_info: Capture si_bits_global before iterating over it
  panic: sys_info: Replace struct sys_info_name with plain array of
    strings
  panic: sys_info: Fix 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           | 129 ++++++++++++++++++++++-----------------
 3 files changed, 74 insertions(+), 59 deletions(-)

-- 
2.47.2

[PATCH v1 1/7] Revert "panic: fix compilation error (`make W=1`)"

[Andy Shevchenko]

This reverts commit d66eaf0a73fbe3dfbe95c473ef12c68d582e5ce2.

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

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 779f8725b194..46d6f4f1ad2a 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -49,11 +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";
+
 int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 					  void *buffer, size_t *lenp,
 					  loff_t *ppos)
 {
-	char names[sizeof("tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks") + 1];
+	char names[sizeof(sys_info_avail) + 1];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
 
-- 
2.47.2

[PATCH v1 2/7] 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 ccee04378d2e..1567f04a58fc 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -334,7 +334,7 @@ void check_panic_on_warn(const char *origin)
  */
 static void panic_other_cpus_shutdown(bool crash_kexec)
 {
-	if (panic_print & SYS_INFO_ALL_CPU_BT) {
+	if (panic_print & SYS_INFO_ALL_BT) {
 		/* Temporary allow non-panic CPUs to write their backtraces. */
 		panic_triggering_all_cpu_backtrace = true;
 		trigger_all_cpu_backtrace();
diff --git a/lib/sys_info.c b/lib/sys_info.c
index 46d6f4f1ad2a..44bc6d96b702 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" },
 };
 
@@ -114,7 +114,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.47.2

[PATCH v1 3/7] 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 44bc6d96b702..5d98560f3f53 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) + 1];
 	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,8 +81,11 @@ 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);
+
 		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.47.2

Re: [PATCH v1 3/7] panic: sys_info: Capture si_bits_global before iterating over it

[Feng Tang]

Hi Andy,

Thanks for the patch! please cc Petr Mladek <pmladek@suse.com> for changes
as I mentioned in the cover letter, he contributed a lot to the code and arch
from RFC to v3.

On Fri, Jul 11, 2025 at 12:51:09PM +0300, 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>
> ---
>  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 44bc6d96b702..5d98560f3f53 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) + 1];
>  	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,8 +81,11 @@ 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);

Good catch!

Thanks,
Feng

> +
>  		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.47.2
>

Re: [PATCH v1 3/7] panic: sys_info: Capture si_bits_global before iterating over it

[Andy Shevchenko]

+Cc: Petr (as requested by Feng).

On Fri, Jul 11, 2025 at 10:56:33PM +0800, Feng Tang wrote:
> 
> Thanks for the patch! please cc Petr Mladek <pmladek@suse.com> for changes
> as I mentioned in the cover letter, he contributed a lot to the code and arch
> from RFC to v3.

Sure, I can do that in next version if that one will be needed (otherwise I
would wait for your new version, depending of what we decide to do, because it
seems to me the 7 patches on a brand new feature which even doesn't compile is
too many).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/7] 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 | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 5d98560f3f53..7965984c6d67 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -1,30 +1,28 @@
 // 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_ALL_BT)]	= "all_bt",
+	[ilog2(SYS_INFO_BLOCKED_TASKS)]	= "blocked_tasks",
 };
 
 /* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
@@ -36,12 +34,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;
@@ -84,12 +79,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 		/* The access to the global value is not synchronized. */
 		si_bits = READ_ONCE(*si_bits_global);
 
-		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.47.2

Re: [PATCH v1 4/7] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Feng Tang]

On Fri, Jul 11, 2025 at 12:51:10PM +0300, 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.

My earlier reply seems to get lost, so I'll resend. (Please ignore this
if your alreay received the reply to 4/7 patch.)

IIUC, this will not work.

Actually there is a hole in the bitmap definition:

#define SYS_INFO_PANIC_CONSOLE_REPLAY	0x00000020

Ad Petr pointed in his review, it's only for panic use, that's why
we make it invisible in sys_info.c. Eventually, we plan to deprecate
'panic_print', and use 'panic_sys_info' and 'panic_console_replay' to
replace it. After that happens that user only see string interface,
we can change these bitmap definition and remove the hole, and use
your cleanup here.

Thanks,
Feng

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/sys_info.c | 47 ++++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 5d98560f3f53..7965984c6d67 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -1,30 +1,28 @@
>  // 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_ALL_BT)]	= "all_bt",
> +	[ilog2(SYS_INFO_BLOCKED_TASKS)]	= "blocked_tasks",
>  };
>  
>  /* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
> @@ -36,12 +34,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;
> @@ -84,12 +79,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
>  		/* The access to the global value is not synchronized. */
>  		si_bits = READ_ONCE(*si_bits_global);
>  
> -		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.47.2

Re: [PATCH v1 4/7] panic: sys_info: Replace struct sys_info_name with plain array of strings

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 11:21:20PM +0800, Feng Tang wrote:
> On Fri, Jul 11, 2025 at 12:51:10PM +0300, 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.
> 
> IIUC, this will not work.
> 
> Actually there is a hole in the bitmap definition:
> 
> #define SYS_INFO_PANIC_CONSOLE_REPLAY	0x00000020
> 
> Ad Petr pointed in his review, it's only for panic use, that's why
> we make it invisible in sys_info.c. Eventually, we plan to deprecate
> 'panic_print', and use 'panic_sys_info' and 'panic_console_replay' to
> replace it. After that happens that user only see string interface,
> we can change these bitmap definition and remove the hole, and use
> your cleanup here.

Yeah, the hole should be move to the end of the bitmap, so we can have all
visible entries be sequential.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/7] panic: sys_info: 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 by moving the local variable from stack to a heap.

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

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 7965984c6d67..08de4c5fcfca 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/console.h>
 #include <linux/log2.h>
 #include <linux/kernel.h>
@@ -11,10 +12,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",
@@ -43,26 +40,32 @@ 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];
 	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) = kmalloc(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;
@@ -73,21 +76,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);
 
 		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.47.2

[PATCH v1 6/7] 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 08de4c5fcfca..7483b6e9b30b 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -60,12 +60,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;
@@ -88,9 +89,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.47.2

[PATCH v1 7/7] 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 7483b6e9b30b..32bf639c4de2 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -40,13 +40,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;
 
@@ -64,33 +103,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.47.2

Re: [PATCH v1 7/7] panic: sys_info: Factor out read and write handlers

[Feng Tang]

On Fri, Jul 11, 2025 at 12:51:13PM +0300, Andy Shevchenko wrote:
> For the sake of the code readability and easier maintenance
> factor out read and write sys_info handlers.

IIRC, I did implement separate 'write' handler, but chose not
to do that to save some common definition. I guess it's personal
preference, and I'm fine with either one. 

Thanks,
Feng

> 
> 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 7483b6e9b30b..32bf639c4de2 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -40,13 +40,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;
>  
> @@ -64,33 +103,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.47.2

Re: [PATCH v1 7/7] panic: sys_info: Factor out read and write handlers

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 11:09:16PM +0800, Feng Tang wrote:
> On Fri, Jul 11, 2025 at 12:51:13PM +0300, Andy Shevchenko wrote:
> > For the sake of the code readability and easier maintenance
> > factor out read and write sys_info handlers.
> 
> IIRC, I did implement separate 'write' handler, but chose not
> to do that to save some common definition. I guess it's personal
> preference, and I'm fine with either one. 

It looks better, in my opinion, to be three functions.

-- 
With Best Regards,
Andy Shevchenko

Re: [rft, PATCH v1 0/7] panic: sys_info: Refactor and fix a compilation issue

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 12:51:06PM +0300, Andy Shevchenko wrote:
> While targeting the compilation issue due to dangling variable,
> I have noticed more opportunities for refactoring that helps to
> avoid above mentioned compilation issue and make code cleaner in
> general. Please, give it a try.
> 
> I put a revert as the first patch of the previous solution, but I believe
> the previous solution may be pulled out without a problem.

Btw, I can try to pop the fix upper in the series, but since this
whole feature was only a few days old, I propose to drop it completely
for now and start again. Please, Cc me for the review.

-- 
With Best Regards,
Andy Shevchenko

Re: [rft, PATCH v1 0/7] panic: sys_info: Refactor and fix a compilation issue

[Feng Tang]

On Fri, Jul 11, 2025 at 03:13:09PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 11, 2025 at 12:51:06PM +0300, Andy Shevchenko wrote:
> > While targeting the compilation issue due to dangling variable,
> > I have noticed more opportunities for refactoring that helps to
> > avoid above mentioned compilation issue and make code cleaner in
> > general. Please, give it a try.
> > 
> > I put a revert as the first patch of the previous solution, but I believe
> > the previous solution may be pulled out without a problem.
> 
> Btw, I can try to pop the fix upper in the series, but since this
> whole feature was only a few days old, I propose to drop it completely
> for now and start again. Please, Cc me for the review.

Can we just take your fix and nice cleanups? Do we really need to drop
the whole series? My gcc haven't raised warnings in the past several
versions, and I will install clang for more complete compiling test
beside functional test for future patches.

Anyway I don't think it has fundamental blocking issue, but I'm fine
if you insist to do so.

Thanks,
Feng

> -- 
> With Best Regards,
> Andy Shevchenko
> 

Re: [rft, PATCH v1 0/7] panic: sys_info: Refactor and fix a compilation issue

[Andy Shevchenko]

On Fri, Jul 11, 2025 at 11:42:13PM +0800, Feng Tang wrote:
> On Fri, Jul 11, 2025 at 03:13:09PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 11, 2025 at 12:51:06PM +0300, Andy Shevchenko wrote:
> > > While targeting the compilation issue due to dangling variable,
> > > I have noticed more opportunities for refactoring that helps to
> > > avoid above mentioned compilation issue and make code cleaner in
> > > general. Please, give it a try.
> > > 
> > > I put a revert as the first patch of the previous solution, but I believe
> > > the previous solution may be pulled out without a problem.
> > 
> > Btw, I can try to pop the fix upper in the series, but since this
> > whole feature was only a few days old, I propose to drop it completely
> > for now and start again. Please, Cc me for the review.
> 
> Can we just take your fix and nice cleanups? Do we really need to drop
> the whole series? My gcc haven't raised warnings in the past several
> versions, and I will install clang for more complete compiling test
> beside functional test for future patches.

> Anyway I don't think it has fundamental blocking issue, but I'm fine
> if you insist to do so.

Let's hear Andrew who took the series in the first place. As he says
I will follow. But personally I prefer to drop the whole set and start
from the scratch.

-- 
With Best Regards,
Andy Shevchenko