Rework NMI "action" modparam handling:
1. Replace the uv_nmi_action string with an enum; and
2. Use sysfs_match_string() for string parsing in param_set_action()
Suggested-by: Steve Wahl <[email protected]>
Cc: Justin Stitt <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
Changes in v2:
- Also change uv_nmi_action to an enum to replace a bunch of
strcmp() calls
---
arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
1 file changed, 57 insertions(+), 47 deletions(-)
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index 45d0c17ce77c..b92f1b4adeb0 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
} while (0)
/* Valid NMI Actions */
-#define ACTION_LEN 16
-static struct nmi_action {
- char *action;
- char *desc;
-} valid_acts[] = {
- { "kdump", "do kernel crash dump" },
- { "dump", "dump process stack for each cpu" },
- { "ips", "dump Inst Ptr info for each cpu" },
- { "kdb", "enter KDB (needs kgdboc= assignment)" },
- { "kgdb", "enter KGDB (needs gdb target remote)" },
- { "health", "check if CPUs respond to NMI" },
+enum action_t {
+ nmi_act_kdump,
+ nmi_act_dump,
+ nmi_act_ips,
+ nmi_act_kdb,
+ nmi_act_kgdb,
+ nmi_act_health,
};
-typedef char action_t[ACTION_LEN];
-static action_t uv_nmi_action = { "dump" };
+
+static const char * const actions[] = {
+ [nmi_act_kdump] = "kdump",
+ [nmi_act_dump] = "dump",
+ [nmi_act_ips] = "ips",
+ [nmi_act_kdb] = "kdb",
+ [nmi_act_kgdb] = "kgdb",
+ [nmi_act_health] = "health",
+};
+
+static const char * const actions_desc[] = {
+ [nmi_act_kdump] = "do kernel crash dump",
+ [nmi_act_dump] = "dump process stack for each cpu",
+ [nmi_act_ips] = "dump Inst Ptr info for each cpu",
+ [nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
+ [nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
+ [nmi_act_health] = "check if CPUs respond to NMI",
+};
+
+static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
+
+static enum action_t uv_nmi_action = nmi_act_dump;
static int param_get_action(char *buffer, const struct kernel_param *kp)
{
- return sprintf(buffer, "%s\n", uv_nmi_action);
+ return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
}
static int param_set_action(const char *val, const struct kernel_param *kp)
{
- int i;
- int n = ARRAY_SIZE(valid_acts);
- char arg[ACTION_LEN];
+ int i, n = ARRAY_SIZE(actions);
- /* (remove possible '\n') */
- strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
-
- for (i = 0; i < n; i++)
- if (!strcmp(arg, valid_acts[i].action))
- break;
-
- if (i < n) {
- strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
- pr_info("UV: New NMI action:%s\n", uv_nmi_action);
+ i = sysfs_match_string(actions, val);
+ if (i >= 0) {
+ uv_nmi_action = i;
+ pr_info("UV: New NMI action:%s\n", actions[i]);
return 0;
}
- pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
+ pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
for (i = 0; i < n; i++)
- pr_err("UV: %-8s - %s\n",
- valid_acts[i].action, valid_acts[i].desc);
+ pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
+
return -EINVAL;
}
@@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
.get = param_get_action,
.set = param_set_action,
};
-#define param_check_action(name, p) __param_check(name, p, action_t)
+#define param_check_action(name, p) __param_check(name, p, enum action_t)
module_param_named(action, uv_nmi_action, action, 0644);
-static inline bool uv_nmi_action_is(const char *action)
-{
- return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
-}
-
/* Setup which NMI support is present in system */
static void uv_nmi_setup_mmrs(void)
{
@@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
if (cpu == 0)
uv_nmi_dump_cpu_ip_hdr();
- if (current->pid != 0 || !uv_nmi_action_is("ips"))
+ if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
uv_nmi_dump_cpu_ip(cpu, regs);
- if (uv_nmi_action_is("dump")) {
+ if (uv_nmi_action == nmi_act_dump) {
pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
show_regs(regs);
}
@@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
int saved_console_loglevel = console_loglevel;
pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
- uv_nmi_action_is("ips") ? "IPs" : "processes",
+ uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
atomic_read(&uv_nmi_cpus_in_nmi), cpu);
console_loglevel = uv_nmi_loglevel;
@@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
static inline int uv_nmi_kdb_reason(void)
{
/* Ensure user is expecting to attach gdb remote */
- if (uv_nmi_action_is("kgdb"))
+ if (uv_nmi_action == nmi_act_kgdb)
return 0;
pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
@@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
master = (atomic_read(&uv_nmi_cpu) == cpu);
/* If NMI action is "kdump", then attempt to do it */
- if (uv_nmi_action_is("kdump")) {
+ if (uv_nmi_action == nmi_act_kdump) {
uv_nmi_kdump(cpu, master, regs);
/* Unexpected return, revert action to "dump" */
if (master)
- strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
+ uv_nmi_action = nmi_act_dump;
}
/* Pause as all CPU's enter the NMI handler */
uv_nmi_wait(master);
/* Process actions other than "kdump": */
- if (uv_nmi_action_is("health")) {
+ switch (uv_nmi_action) {
+ case nmi_act_health:
uv_nmi_action_health(cpu, regs, master);
- } else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
+ break;
+ case nmi_act_ips:
+ case nmi_act_dump:
uv_nmi_dump_state(cpu, regs, master);
- } else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
+ break;
+ case nmi_act_kdb:
+ case nmi_act_kgdb:
uv_call_kgdb_kdb(cpu, regs, master);
- } else {
+ break;
+ default:
if (master)
- pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
+ pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
uv_nmi_sync_exit(master);
+ break;
}
/* Clear per_cpu "in_nmi" flag */
--
2.41.0
On Wed, Sep 13, 2023 at 08:01:11PM +0200, Hans de Goede wrote:
> Rework NMI "action" modparam handling:
>
> 1. Replace the uv_nmi_action string with an enum; and
> 2. Use sysfs_match_string() for string parsing in param_set_action()
>
> Suggested-by: Steve Wahl <[email protected]>
> Cc: Justin Stitt <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> Changes in v2:
> - Also change uv_nmi_action to an enum to replace a bunch of
> strcmp() calls
> ---
> arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 45d0c17ce77c..b92f1b4adeb0 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
> } while (0)
>
> /* Valid NMI Actions */
> -#define ACTION_LEN 16
> -static struct nmi_action {
> - char *action;
> - char *desc;
> -} valid_acts[] = {
> - { "kdump", "do kernel crash dump" },
> - { "dump", "dump process stack for each cpu" },
> - { "ips", "dump Inst Ptr info for each cpu" },
> - { "kdb", "enter KDB (needs kgdboc= assignment)" },
> - { "kgdb", "enter KGDB (needs gdb target remote)" },
> - { "health", "check if CPUs respond to NMI" },
> +enum action_t {
> + nmi_act_kdump,
> + nmi_act_dump,
> + nmi_act_ips,
> + nmi_act_kdb,
> + nmi_act_kgdb,
> + nmi_act_health,
> };
> -typedef char action_t[ACTION_LEN];
> -static action_t uv_nmi_action = { "dump" };
> +
> +static const char * const actions[] = {
> + [nmi_act_kdump] = "kdump",
> + [nmi_act_dump] = "dump",
> + [nmi_act_ips] = "ips",
> + [nmi_act_kdb] = "kdb",
> + [nmi_act_kgdb] = "kgdb",
> + [nmi_act_health] = "health",
> +};
> +
> +static const char * const actions_desc[] = {
> + [nmi_act_kdump] = "do kernel crash dump",
> + [nmi_act_dump] = "dump process stack for each cpu",
> + [nmi_act_ips] = "dump Inst Ptr info for each cpu",
> + [nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
> + [nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
> + [nmi_act_health] = "check if CPUs respond to NMI",
> +};
> +
> +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
> +
> +static enum action_t uv_nmi_action = nmi_act_dump;
>
> static int param_get_action(char *buffer, const struct kernel_param *kp)
> {
> - return sprintf(buffer, "%s\n", uv_nmi_action);
> + return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
> }
>
> static int param_set_action(const char *val, const struct kernel_param *kp)
> {
> - int i;
> - int n = ARRAY_SIZE(valid_acts);
> - char arg[ACTION_LEN];
> + int i, n = ARRAY_SIZE(actions);
>
> - /* (remove possible '\n') */
> - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
> -
> - for (i = 0; i < n; i++)
> - if (!strcmp(arg, valid_acts[i].action))
> - break;
> -
> - if (i < n) {
> - strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
> - pr_info("UV: New NMI action:%s\n", uv_nmi_action);
> + i = sysfs_match_string(actions, val);
> + if (i >= 0) {
> + uv_nmi_action = i;
> + pr_info("UV: New NMI action:%s\n", actions[i]);
> return 0;
> }
>
> - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
This is a very minor nit in an otherwise fine patch:
Testing by echoing to /sys/module/uv_nmi/parameters/action shows an
invalid action in val has a trailing newline that appears just before
the comma:
# echo "invalid" >/sys/module/uv_nmi/parameters/action
[ 1070.079303] UV: Invalid NMI action:invalid
[ 1070.079303] , valid actions are:
[ 1070.087485] UV: kdump - do kernel crash dump
[ 1070.092558] UV: dump - dump process stack for each cpu
[ 1070.098694] UV: ips - dump Inst Ptr info for each cpu
[ 1070.098697] UV: kdb - enter KDB (needs kgdboc= assignment)
[ 1070.098699] UV: kgdb - enter KGDB (needs gdb target remote)
[ 1070.098702] UV: health - check if CPUs respond to NMI
-bash: echo: write error: Invalid argument
#
There's no newline in val if it comes from the kernel command line, so
you can't just assume it's there.
It would be bad style to just overwrite the newline in place.
Allocating space for and copying a string seems a waste. Maybe rework
the message so a possible newline doesn't look so awkward, by removing
the comma?
> + pr_err("UV: Invalid NMI action:%s Valid actions are:\n", val);
Frankly, I approve of this patch going in, regardless of what, if
anything, is done about this.
Thanks.
Reveiwed-by: Steve Wahl <[email protected]>
Tested-by: Steve Wahl <[email protected]>
> for (i = 0; i < n; i++)
> - pr_err("UV: %-8s - %s\n",
> - valid_acts[i].action, valid_acts[i].desc);
> + pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
> +
> return -EINVAL;
> }
>
> @@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
> .get = param_get_action,
> .set = param_set_action,
> };
> -#define param_check_action(name, p) __param_check(name, p, action_t)
> +#define param_check_action(name, p) __param_check(name, p, enum action_t)
>
> module_param_named(action, uv_nmi_action, action, 0644);
>
> -static inline bool uv_nmi_action_is(const char *action)
> -{
> - return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
> -}
> -
> /* Setup which NMI support is present in system */
> static void uv_nmi_setup_mmrs(void)
> {
> @@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> if (cpu == 0)
> uv_nmi_dump_cpu_ip_hdr();
>
> - if (current->pid != 0 || !uv_nmi_action_is("ips"))
> + if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
> uv_nmi_dump_cpu_ip(cpu, regs);
>
> - if (uv_nmi_action_is("dump")) {
> + if (uv_nmi_action == nmi_act_dump) {
> pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
> show_regs(regs);
> }
> @@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
> int saved_console_loglevel = console_loglevel;
>
> pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
> - uv_nmi_action_is("ips") ? "IPs" : "processes",
> + uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
> atomic_read(&uv_nmi_cpus_in_nmi), cpu);
>
> console_loglevel = uv_nmi_loglevel;
> @@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
> static inline int uv_nmi_kdb_reason(void)
> {
> /* Ensure user is expecting to attach gdb remote */
> - if (uv_nmi_action_is("kgdb"))
> + if (uv_nmi_action == nmi_act_kgdb)
> return 0;
>
> pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
> @@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
> master = (atomic_read(&uv_nmi_cpu) == cpu);
>
> /* If NMI action is "kdump", then attempt to do it */
> - if (uv_nmi_action_is("kdump")) {
> + if (uv_nmi_action == nmi_act_kdump) {
> uv_nmi_kdump(cpu, master, regs);
>
> /* Unexpected return, revert action to "dump" */
> if (master)
> - strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
> + uv_nmi_action = nmi_act_dump;
> }
>
> /* Pause as all CPU's enter the NMI handler */
> uv_nmi_wait(master);
>
> /* Process actions other than "kdump": */
> - if (uv_nmi_action_is("health")) {
> + switch (uv_nmi_action) {
> + case nmi_act_health:
> uv_nmi_action_health(cpu, regs, master);
> - } else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
> + break;
> + case nmi_act_ips:
> + case nmi_act_dump:
> uv_nmi_dump_state(cpu, regs, master);
> - } else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
> + break;
> + case nmi_act_kdb:
> + case nmi_act_kgdb:
> uv_call_kgdb_kdb(cpu, regs, master);
> - } else {
> + break;
> + default:
> if (master)
> - pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
> + pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
> uv_nmi_sync_exit(master);
> + break;
> }
>
> /* Clear per_cpu "in_nmi" flag */
> --
> 2.41.0
>
--
Steve Wahl, Hewlett Packard Enterprise
On Wed, Sep 13, 2023 at 04:06:51PM -0500, Steve Wahl wrote:
> On Wed, Sep 13, 2023 at 08:01:11PM +0200, Hans de Goede wrote:
> > Rework NMI "action" modparam handling:
> >
> > 1. Replace the uv_nmi_action string with an enum; and
> > 2. Use sysfs_match_string() for string parsing in param_set_action()
> >
> > Suggested-by: Steve Wahl <[email protected]>
> > Cc: Justin Stitt <[email protected]>
> > Signed-off-by: Hans de Goede <[email protected]>
> > ---
> > Changes in v2:
> > - Also change uv_nmi_action to an enum to replace a bunch of
> > strcmp() calls
> > ---
> > arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
> > 1 file changed, 57 insertions(+), 47 deletions(-)
> >
> > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> > index 45d0c17ce77c..b92f1b4adeb0 100644
> > --- a/arch/x86/platform/uv/uv_nmi.c
> > +++ b/arch/x86/platform/uv/uv_nmi.c
> > @@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
> > } while (0)
> >
> > /* Valid NMI Actions */
> > -#define ACTION_LEN 16
> > -static struct nmi_action {
> > - char *action;
> > - char *desc;
> > -} valid_acts[] = {
> > - { "kdump", "do kernel crash dump" },
> > - { "dump", "dump process stack for each cpu" },
> > - { "ips", "dump Inst Ptr info for each cpu" },
> > - { "kdb", "enter KDB (needs kgdboc= assignment)" },
> > - { "kgdb", "enter KGDB (needs gdb target remote)" },
> > - { "health", "check if CPUs respond to NMI" },
> > +enum action_t {
> > + nmi_act_kdump,
> > + nmi_act_dump,
> > + nmi_act_ips,
> > + nmi_act_kdb,
> > + nmi_act_kgdb,
> > + nmi_act_health,
> > };
> > -typedef char action_t[ACTION_LEN];
> > -static action_t uv_nmi_action = { "dump" };
> > +
> > +static const char * const actions[] = {
> > + [nmi_act_kdump] = "kdump",
> > + [nmi_act_dump] = "dump",
> > + [nmi_act_ips] = "ips",
> > + [nmi_act_kdb] = "kdb",
> > + [nmi_act_kgdb] = "kgdb",
> > + [nmi_act_health] = "health",
> > +};
> > +
> > +static const char * const actions_desc[] = {
> > + [nmi_act_kdump] = "do kernel crash dump",
> > + [nmi_act_dump] = "dump process stack for each cpu",
> > + [nmi_act_ips] = "dump Inst Ptr info for each cpu",
> > + [nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
> > + [nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
> > + [nmi_act_health] = "check if CPUs respond to NMI",
> > +};
> > +
> > +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
> > +
> > +static enum action_t uv_nmi_action = nmi_act_dump;
> >
> > static int param_get_action(char *buffer, const struct kernel_param *kp)
> > {
> > - return sprintf(buffer, "%s\n", uv_nmi_action);
> > + return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
> > }
> >
> > static int param_set_action(const char *val, const struct kernel_param *kp)
> > {
> > - int i;
> > - int n = ARRAY_SIZE(valid_acts);
> > - char arg[ACTION_LEN];
> > + int i, n = ARRAY_SIZE(actions);
> >
> > - /* (remove possible '\n') */
> > - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
> > -
> > - for (i = 0; i < n; i++)
> > - if (!strcmp(arg, valid_acts[i].action))
> > - break;
> > -
> > - if (i < n) {
> > - strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
> > - pr_info("UV: New NMI action:%s\n", uv_nmi_action);
> > + i = sysfs_match_string(actions, val);
> > + if (i >= 0) {
> > + uv_nmi_action = i;
> > + pr_info("UV: New NMI action:%s\n", actions[i]);
> > return 0;
> > }
> >
> > - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> > + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
>
> This is a very minor nit in an otherwise fine patch:
>
> Testing by echoing to /sys/module/uv_nmi/parameters/action shows an
> invalid action in val has a trailing newline that appears just before
> the comma:
>
> # echo "invalid" >/sys/module/uv_nmi/parameters/action
> [ 1070.079303] UV: Invalid NMI action:invalid
> [ 1070.079303] , valid actions are:
> [ 1070.087485] UV: kdump - do kernel crash dump
> [ 1070.092558] UV: dump - dump process stack for each cpu
> [ 1070.098694] UV: ips - dump Inst Ptr info for each cpu
> [ 1070.098697] UV: kdb - enter KDB (needs kgdboc= assignment)
> [ 1070.098699] UV: kgdb - enter KGDB (needs gdb target remote)
> [ 1070.098702] UV: health - check if CPUs respond to NMI
> -bash: echo: write error: Invalid argument
> #
>
> There's no newline in val if it comes from the kernel command line, so
> you can't just assume it's there.
>
> It would be bad style to just overwrite the newline in place.
> Allocating space for and copying a string seems a waste. Maybe rework
> the message so a possible newline doesn't look so awkward, by removing
> the comma?
>
> > + pr_err("UV: Invalid NMI action:%s Valid actions are:\n", val);
>
> Frankly, I approve of this patch going in, regardless of what, if
> anything, is done about this.
>
> Thanks.
I forgot to mention that testing included setting all option values from
kernel command line or /sys file, and sending NMIs to see that they
triggered the action that was set.
--> Steve
> Reveiwed-by: Steve Wahl <[email protected]>
> Tested-by: Steve Wahl <[email protected]>
>
>
>
> > for (i = 0; i < n; i++)
> > - pr_err("UV: %-8s - %s\n",
> > - valid_acts[i].action, valid_acts[i].desc);
> > + pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
> > +
> > return -EINVAL;
> > }
> >
> > @@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
> > .get = param_get_action,
> > .set = param_set_action,
> > };
> > -#define param_check_action(name, p) __param_check(name, p, action_t)
> > +#define param_check_action(name, p) __param_check(name, p, enum action_t)
> >
> > module_param_named(action, uv_nmi_action, action, 0644);
> >
> > -static inline bool uv_nmi_action_is(const char *action)
> > -{
> > - return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
> > -}
> > -
> > /* Setup which NMI support is present in system */
> > static void uv_nmi_setup_mmrs(void)
> > {
> > @@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> > if (cpu == 0)
> > uv_nmi_dump_cpu_ip_hdr();
> >
> > - if (current->pid != 0 || !uv_nmi_action_is("ips"))
> > + if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
> > uv_nmi_dump_cpu_ip(cpu, regs);
> >
> > - if (uv_nmi_action_is("dump")) {
> > + if (uv_nmi_action == nmi_act_dump) {
> > pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
> > show_regs(regs);
> > }
> > @@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
> > int saved_console_loglevel = console_loglevel;
> >
> > pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
> > - uv_nmi_action_is("ips") ? "IPs" : "processes",
> > + uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
> > atomic_read(&uv_nmi_cpus_in_nmi), cpu);
> >
> > console_loglevel = uv_nmi_loglevel;
> > @@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
> > static inline int uv_nmi_kdb_reason(void)
> > {
> > /* Ensure user is expecting to attach gdb remote */
> > - if (uv_nmi_action_is("kgdb"))
> > + if (uv_nmi_action == nmi_act_kgdb)
> > return 0;
> >
> > pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
> > @@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
> > master = (atomic_read(&uv_nmi_cpu) == cpu);
> >
> > /* If NMI action is "kdump", then attempt to do it */
> > - if (uv_nmi_action_is("kdump")) {
> > + if (uv_nmi_action == nmi_act_kdump) {
> > uv_nmi_kdump(cpu, master, regs);
> >
> > /* Unexpected return, revert action to "dump" */
> > if (master)
> > - strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
> > + uv_nmi_action = nmi_act_dump;
> > }
> >
> > /* Pause as all CPU's enter the NMI handler */
> > uv_nmi_wait(master);
> >
> > /* Process actions other than "kdump": */
> > - if (uv_nmi_action_is("health")) {
> > + switch (uv_nmi_action) {
> > + case nmi_act_health:
> > uv_nmi_action_health(cpu, regs, master);
> > - } else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
> > + break;
> > + case nmi_act_ips:
> > + case nmi_act_dump:
> > uv_nmi_dump_state(cpu, regs, master);
> > - } else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
> > + break;
> > + case nmi_act_kdb:
> > + case nmi_act_kgdb:
> > uv_call_kgdb_kdb(cpu, regs, master);
> > - } else {
> > + break;
> > + default:
> > if (master)
> > - pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
> > + pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
> > uv_nmi_sync_exit(master);
> > + break;
> > }
> >
> > /* Clear per_cpu "in_nmi" flag */
> > --
> > 2.41.0
> >
>
> --
> Steve Wahl, Hewlett Packard Enterprise
--
Steve Wahl, Hewlett Packard Enterprise
On Wed, Sep 13, 2023 at 11:01 AM Hans de Goede <[email protected]> wrote:
>
> Rework NMI "action" modparam handling:
>
> 1. Replace the uv_nmi_action string with an enum; and
> 2. Use sysfs_match_string() for string parsing in param_set_action()
>
> Suggested-by: Steve Wahl <[email protected]>
> Cc: Justin Stitt <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
Reviewed-by: Justin Stitt <[email protected]>
> Changes in v2:
> - Also change uv_nmi_action to an enum to replace a bunch of
> strcmp() calls
> ---
> arch/x86/platform/uv/uv_nmi.c | 104 +++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 45d0c17ce77c..b92f1b4adeb0 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -178,49 +178,57 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
> } while (0)
>
> /* Valid NMI Actions */
> -#define ACTION_LEN 16
> -static struct nmi_action {
> - char *action;
> - char *desc;
> -} valid_acts[] = {
> - { "kdump", "do kernel crash dump" },
> - { "dump", "dump process stack for each cpu" },
> - { "ips", "dump Inst Ptr info for each cpu" },
> - { "kdb", "enter KDB (needs kgdboc= assignment)" },
> - { "kgdb", "enter KGDB (needs gdb target remote)" },
> - { "health", "check if CPUs respond to NMI" },
> +enum action_t {
> + nmi_act_kdump,
> + nmi_act_dump,
> + nmi_act_ips,
> + nmi_act_kdb,
> + nmi_act_kgdb,
> + nmi_act_health,
> };
> -typedef char action_t[ACTION_LEN];
> -static action_t uv_nmi_action = { "dump" };
> +
> +static const char * const actions[] = {
> + [nmi_act_kdump] = "kdump",
> + [nmi_act_dump] = "dump",
> + [nmi_act_ips] = "ips",
> + [nmi_act_kdb] = "kdb",
> + [nmi_act_kgdb] = "kgdb",
> + [nmi_act_health] = "health",
> +};
> +
> +static const char * const actions_desc[] = {
> + [nmi_act_kdump] = "do kernel crash dump",
> + [nmi_act_dump] = "dump process stack for each cpu",
> + [nmi_act_ips] = "dump Inst Ptr info for each cpu",
> + [nmi_act_kdb] = "enter KDB (needs kgdboc= assignment)",
> + [nmi_act_kgdb] = "enter KGDB (needs gdb target remote)",
> + [nmi_act_health] = "check if CPUs respond to NMI",
> +};
> +
> +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
> +
> +static enum action_t uv_nmi_action = nmi_act_dump;
>
> static int param_get_action(char *buffer, const struct kernel_param *kp)
> {
> - return sprintf(buffer, "%s\n", uv_nmi_action);
> + return sprintf(buffer, "%s\n", actions[uv_nmi_action]);
> }
>
> static int param_set_action(const char *val, const struct kernel_param *kp)
> {
> - int i;
> - int n = ARRAY_SIZE(valid_acts);
> - char arg[ACTION_LEN];
> + int i, n = ARRAY_SIZE(actions);
>
> - /* (remove possible '\n') */
> - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
> -
> - for (i = 0; i < n; i++)
> - if (!strcmp(arg, valid_acts[i].action))
> - break;
> -
> - if (i < n) {
> - strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action));
> - pr_info("UV: New NMI action:%s\n", uv_nmi_action);
> + i = sysfs_match_string(actions, val);
> + if (i >= 0) {
> + uv_nmi_action = i;
> + pr_info("UV: New NMI action:%s\n", actions[i]);
> return 0;
> }
>
> - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
> for (i = 0; i < n; i++)
> - pr_err("UV: %-8s - %s\n",
> - valid_acts[i].action, valid_acts[i].desc);
> + pr_err("UV: %-8s - %s\n", actions[i], actions_desc[i]);
> +
> return -EINVAL;
> }
>
> @@ -228,15 +236,10 @@ static const struct kernel_param_ops param_ops_action = {
> .get = param_get_action,
> .set = param_set_action,
> };
> -#define param_check_action(name, p) __param_check(name, p, action_t)
> +#define param_check_action(name, p) __param_check(name, p, enum action_t)
>
> module_param_named(action, uv_nmi_action, action, 0644);
>
> -static inline bool uv_nmi_action_is(const char *action)
> -{
> - return (strncmp(uv_nmi_action, action, strlen(action)) == 0);
> -}
> -
> /* Setup which NMI support is present in system */
> static void uv_nmi_setup_mmrs(void)
> {
> @@ -727,10 +730,10 @@ static void uv_nmi_dump_state_cpu(int cpu, struct pt_regs *regs)
> if (cpu == 0)
> uv_nmi_dump_cpu_ip_hdr();
>
> - if (current->pid != 0 || !uv_nmi_action_is("ips"))
> + if (current->pid != 0 || uv_nmi_action != nmi_act_ips)
> uv_nmi_dump_cpu_ip(cpu, regs);
>
> - if (uv_nmi_action_is("dump")) {
> + if (uv_nmi_action == nmi_act_dump) {
> pr_info("UV:%sNMI process trace for CPU %d\n", dots, cpu);
> show_regs(regs);
> }
> @@ -798,7 +801,7 @@ static void uv_nmi_dump_state(int cpu, struct pt_regs *regs, int master)
> int saved_console_loglevel = console_loglevel;
>
> pr_alert("UV: tracing %s for %d CPUs from CPU %d\n",
> - uv_nmi_action_is("ips") ? "IPs" : "processes",
> + uv_nmi_action == nmi_act_ips ? "IPs" : "processes",
> atomic_read(&uv_nmi_cpus_in_nmi), cpu);
>
> console_loglevel = uv_nmi_loglevel;
> @@ -874,7 +877,7 @@ static inline int uv_nmi_kdb_reason(void)
> static inline int uv_nmi_kdb_reason(void)
> {
> /* Ensure user is expecting to attach gdb remote */
> - if (uv_nmi_action_is("kgdb"))
> + if (uv_nmi_action == nmi_act_kgdb)
> return 0;
>
> pr_err("UV: NMI error: KDB is not enabled in this kernel\n");
> @@ -950,28 +953,35 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs)
> master = (atomic_read(&uv_nmi_cpu) == cpu);
>
> /* If NMI action is "kdump", then attempt to do it */
> - if (uv_nmi_action_is("kdump")) {
> + if (uv_nmi_action == nmi_act_kdump) {
> uv_nmi_kdump(cpu, master, regs);
>
> /* Unexpected return, revert action to "dump" */
> if (master)
> - strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action));
> + uv_nmi_action = nmi_act_dump;
> }
>
> /* Pause as all CPU's enter the NMI handler */
> uv_nmi_wait(master);
>
> /* Process actions other than "kdump": */
> - if (uv_nmi_action_is("health")) {
> + switch (uv_nmi_action) {
> + case nmi_act_health:
> uv_nmi_action_health(cpu, regs, master);
> - } else if (uv_nmi_action_is("ips") || uv_nmi_action_is("dump")) {
> + break;
> + case nmi_act_ips:
> + case nmi_act_dump:
> uv_nmi_dump_state(cpu, regs, master);
> - } else if (uv_nmi_action_is("kdb") || uv_nmi_action_is("kgdb")) {
> + break;
> + case nmi_act_kdb:
> + case nmi_act_kgdb:
> uv_call_kgdb_kdb(cpu, regs, master);
> - } else {
> + break;
> + default:
> if (master)
> - pr_alert("UV: unknown NMI action: %s\n", uv_nmi_action);
> + pr_alert("UV: unknown NMI action: %d\n", uv_nmi_action);
> uv_nmi_sync_exit(master);
> + break;
> }
>
> /* Clear per_cpu "in_nmi" flag */
> --
> 2.41.0
>
On Thu, Sep 14, 2023 at 11:42 AM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Sep 13, 2023 at 9:01 PM Hans de Goede <[email protected]> wrote:
...
> > - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> > + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
>
> As mentioned previously the val may or may not have a new line in it.
> I dunno about comma removal, but a new line presence can be easily checked.
>
> Either way it's not so critical, hence removing comma. or replacing it
> by '-' (dash) may be enough.
Or even
pr_err("UV: Invalid NMI action:%s --> valid actions are:\n", val);
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 13, 2023 at 9:01 PM Hans de Goede <[email protected]> wrote:
>
> Rework NMI "action" modparam handling:
>
> 1. Replace the uv_nmi_action string with an enum; and
> 2. Use sysfs_match_string() for string parsing in param_set_action()
...
Don't you need to include string.h?
...
> +enum action_t {
> + nmi_act_kdump,
> + nmi_act_dump,
> + nmi_act_ips,
> + nmi_act_kdb,
> + nmi_act_kgdb,
> + nmi_act_health,
> };
> +
> +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
I believe with enum in place you don't need this, just add a terminator to it
enum action_t {
...
nmi_act_max
};
and use as an array size both arrays.
...
> - int i;
> - int n = ARRAY_SIZE(valid_acts);
> + int i, n = ARRAY_SIZE(actions);
Since you are changing them, why not make them unsigned?
...
> - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
> + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
As mentioned previously the val may or may not have a new line in it.
I dunno about comma removal, but a new line presence can be easily checked.
Either way it's not so critical, hence removing comma. or replacing it
by '-' (dash) may be enough.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 13, 2023 at 04:06:51PM -0500, Steve Wahl wrote:
> On Wed, Sep 13, 2023 at 08:01:11PM +0200, Hans de Goede wrote:
> Maybe rework the message so a possible newline doesn't look so
> awkward, by removing the comma?
>
> > + pr_err("UV: Invalid NMI action:%s Valid actions are:\n", val);
It occurred to me overnight that leaving val out completely is an
option, as printing it adds very little value. So a reasonable option
would be:
+ pr_err("UV: Invalid NMI action. Valid actions are:\n");
--> Steve
--
Steve Wahl, Hewlett Packard Enterprise
Hi,
On 9/14/23 10:42, Andy Shevchenko wrote:
> On Wed, Sep 13, 2023 at 9:01 PM Hans de Goede <[email protected]> wrote:
>>
>> Rework NMI "action" modparam handling:
>>
>> 1. Replace the uv_nmi_action string with an enum; and
>> 2. Use sysfs_match_string() for string parsing in param_set_action()
>
> ...
>
> Don't you need to include string.h?
Yes (pre-existing problem really, but still). Will fix for v3.
>
> ...
>
>> +enum action_t {
>> + nmi_act_kdump,
>> + nmi_act_dump,
>> + nmi_act_ips,
>> + nmi_act_kdb,
>> + nmi_act_kgdb,
>> + nmi_act_health,
>> };
>
>> +
>> +static_assert(ARRAY_SIZE(actions) == ARRAY_SIZE(actions_desc));
>
> I believe with enum in place you don't need this, just add a terminator to it
>
> enum action_t {
> ...
> nmi_act_max
> };
>
> and use as an array size both arrays.
>
> ...
>
>> - int i;
>> - int n = ARRAY_SIZE(valid_acts);
>
>> + int i, n = ARRAY_SIZE(actions);
>
> Since you are changing them, why not make them unsigned?
i is used to store the result of sysfs_match_string() which
returns a negative errno.
>
> ...
>
>> - pr_err("UV: Invalid NMI action:%s, valid actions are:\n", arg);
>> + pr_err("UV: Invalid NMI action:%s, valid actions are:\n", val);
>
> As mentioned previously the val may or may not have a new line in it.
> I dunno about comma removal, but a new line presence can be easily checked.
>
> Either way it's not so critical, hence removing comma. or replacing it
> by '-' (dash) may be enough.
I'll fix this for v3 by not printing the wrong val at all as suggested
by Steve Wahl.
Regards,
Hans