Re: [PATCH v2] x86/platform/uv: Rework NMI "action" modparam handling

[Justin Stitt]

On Wed, Sep 13, 2023 at 11:01 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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 <steve.wahl@xxxxxxx>
> Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
Reviewed-by: Justin Stitt <justinstitt@xxxxxxxxxx>

> 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
>