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

[PATCH v3 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 v3:
- addressed an issue with empty parameter returned (Feng)
- gathered tags (Feng)

Changelog v2:
v2: https://lore.kernel.org/r/20251029111202.3217870-2-andriy.shevchenko@linux.intel.com
- 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           | 138 +++++++++++++++++++++------------------
 3 files changed, 78 insertions(+), 64 deletions(-)

-- 
2.50.1

[PATCH v3 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.

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>
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

[PATCH v3 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.

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>
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

[PATCH v3 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 | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 6b0188b30227..29a63238b31d 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,10 +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) {
+		for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
+			if (*si_names[i]) {
 				len += scnprintf(names + len, sizeof(names) - len,
-					"%s%s", delim, si_names[i].name);
+						 "%s%s", delim, si_names[i]);
 				delim = ",";
 			}
 		}
-- 
2.50.1

[PATCH v3 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 29a63238b31d..eb5c1226bfc8 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,16 +79,15 @@ 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)) {
 			if (*si_names[i]) {
-				len += scnprintf(names + len, sizeof(names) - len,
+				len += scnprintf(names + len, maxlen - len,
 						 "%s%s", delim, si_names[i]);
 				delim = ",";
 			}
@@ -91,7 +95,7 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 
 		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 v3 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.

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>
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 eb5c1226bfc8..94526de8482b 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;
@@ -93,9 +94,6 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 			}
 		}
 
-		table = *ro_table;
-		table.data = names;
-		table.maxlen = maxlen;
 		return proc_dostring(&table, write, buffer, lenp, ppos);
 	}
 }
-- 
2.50.1

[PATCH v3 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 | 79 +++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 33 deletions(-)

diff --git a/lib/sys_info.c b/lib/sys_info.c
index 94526de8482b..874b9471a068 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -43,18 +43,56 @@ 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)) {
+		if (*si_names[i]) {
+			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;
 
-	si_bits_global = ro_table->data;
-
 	maxlen = 0;
 	for (i = 0; i < ARRAY_SIZE(si_names); i++)
 		maxlen += strlen(si_names[i]) + 1;
@@ -67,35 +105,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)) {
-			if (*si_names[i]) {
-				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, ro_table->data);
+	else
+		return sys_info_read_handler(&table, buffer, lenp, ppos, ro_table->data);
 }
 #endif
 
-- 
2.50.1

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

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 12:44:16PM +0100, Andy Shevchenko wrote:

+Cc: Petr, sorry, forgot to add you to the Cc list.

> 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 v3:
> - addressed an issue with empty parameter returned (Feng)
> - gathered tags (Feng)
> 
> Changelog v2:
> v2: https://lore.kernel.org/r/20251029111202.3217870-2-andriy.shevchenko@linux.intel.com
> - 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

-- 
With Best Regards,
Andy Shevchenko

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

[Petr Mladek]

On Thu 2025-10-30 16:07:47, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 12:44:16PM +0100, Andy Shevchenko wrote:
> 
> +Cc: Petr, sorry, forgot to add you to the Cc list.

Thank you for Cc-ing me.

> > 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 v3:
> > - addressed an issue with empty parameter returned (Feng)
> > - gathered tags (Feng)

I like all the changes and do not see any problem there.
For the entire patchset, feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

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

[Andrew Morton]

On Thu, 30 Oct 2025 12:44:19 +0100 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.
> 
> ...
>
> -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",
>  };

We have this const_ilog2() which appears to exist to work around a
sparse shortcoming.  But only net/ethtool/common.c uses it - plenty of
other sites do what you've done.

Which makes me wonder whether const_ilog2() can be removed, switch
everything over to ilog2().

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

[Andrew Morton]

On Thu, 30 Oct 2025 12:44:22 +0100 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> For the sake of the code readability and easier maintenance
> factor out read and write sys_info handlers.
> 
> ...
>
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -43,18 +43,56 @@ 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)
> +{

checkpatch wanted this:

--- a/lib/sys_info.c~panic-sys_info-factor-out-read-and-write-handlers-checkpatch-fixes
+++ a/lib/sys_info.c
@@ -43,7 +43,7 @@ unsigned long sys_info_parse_param(char
 
 #ifdef CONFIG_SYSCTL
 
-static int sys_info_write_handler(struct ctl_table *table,
+static int sys_info_write_handler(const struct ctl_table *table,
 				  void *buffer, size_t *lenp, loff_t *ppos,
 				  unsigned long *si_bits_global)
 {
@@ -62,7 +62,7 @@ static int sys_info_write_handler(struct
 	return 0;
 }
 
-static int sys_info_read_handler(struct ctl_table *table,
+static int sys_info_read_handler(const struct ctl_table *table,
 				 void *buffer, size_t *lenp, loff_t *ppos,
 				 unsigned long *si_bits_global)
 {
_

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

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:16:12PM -0700, Andrew Morton wrote:
> On Thu, 30 Oct 2025 12:44:22 +0100 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> checkpatch wanted this:

Makes sense. Have you applied this update to your tree already? Or should I
send (re-send) a formal patch?

> --- a/lib/sys_info.c~panic-sys_info-factor-out-read-and-write-handlers-checkpatch-fixes
> +++ a/lib/sys_info.c
> @@ -43,7 +43,7 @@ unsigned long sys_info_parse_param(char
>  
>  #ifdef CONFIG_SYSCTL
>  
> -static int sys_info_write_handler(struct ctl_table *table,
> +static int sys_info_write_handler(const struct ctl_table *table,
>  				  void *buffer, size_t *lenp, loff_t *ppos,
>  				  unsigned long *si_bits_global)
>  {
> @@ -62,7 +62,7 @@ static int sys_info_write_handler(struct
>  	return 0;
>  }
>  
> -static int sys_info_read_handler(struct ctl_table *table,
> +static int sys_info_read_handler(const struct ctl_table *table,
>  				 void *buffer, size_t *lenp, loff_t *ppos,
>  				 unsigned long *si_bits_global)
>  {

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 08:06:16PM -0700, Andrew Morton wrote:
> On Thu, 30 Oct 2025 12:44:19 +0100 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > -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",
> >  };
> 
> We have this const_ilog2() which appears to exist to work around a
> sparse shortcoming.  But only net/ethtool/common.c uses it - plenty of
> other sites do what you've done.
> 
> Which makes me wonder whether const_ilog2() can be removed, switch
> everything over to ilog2().

Good question. I haven't noticed sparse errors, I always use make W=1 C=1 for
my local builds.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 09:22:16AM +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 08:16:12PM -0700, Andrew Morton wrote:
> > On Thu, 30 Oct 2025 12:44:22 +0100 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > checkpatch wanted this:
> 
> Makes sense. Have you applied this update to your tree already? Or should I
> send (re-send) a formal patch?

Never mind, I see it's there, thanks!

> Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 04:48:03PM +0100, Petr Mladek wrote:
> On Thu 2025-10-30 16:07:47, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 12:44:16PM +0100, Andy Shevchenko wrote:
> > 
> > +Cc: Petr, sorry, forgot to add you to the Cc list.
> 
> Thank you for Cc-ing me.
> 
> > > 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 v3:
> > > - addressed an issue with empty parameter returned (Feng)
> > > - gathered tags (Feng)
> 
> I like all the changes and do not see any problem there.
> For the entire patchset, feel free to use:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thank you!

-- 
With Best Regards,
Andy Shevchenko