[PATCH] x86/platform/uv: Use sysfs_match_string() for string parsing in param_set_action()

[PATCH] x86/platform/uv: Use sysfs_match_string() for string parsing in param_set_action()

[Hans de Goede]

Remove the custom, hard to read code to:

1. Make a copy of "val" with any potential '\n' at the end stripped
2. Compare the copy against an array of allowed string values

Linux has the sysfs_match_string() helper exactly for cases like this,
switch to this.

Cc: Justin Stitt <justinstitt@google.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/x86/platform/uv/uv_nmi.c | 48 +++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index 45d0c17ce77c..44bacb547c65 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -179,17 +179,27 @@ module_param_named(debug, uv_nmi_debug, int, 0644);
 
 /* 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"		},
+
+static const char * const valid_acts[] = {
+	"kdump",
+	"dump",
+	"ips",
+	"kdb",
+	"kgdb",
+	"health",
 };
+
+static const char * const valid_acts_desc[] = {
+	"do kernel crash dump",
+	"dump process stack for each cpu",
+	"dump Inst Ptr info for each cpu",
+	"enter KDB (needs kgdboc= assignment)",
+	"enter KGDB (needs gdb target remote)",
+	"check if CPUs respond to NMI",
+};
+
+static_assert(ARRAY_SIZE(valid_acts) == ARRAY_SIZE(valid_acts_desc));
+
 typedef char action_t[ACTION_LEN];
 static action_t uv_nmi_action = { "dump" };
 
@@ -202,25 +212,19 @@ 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];
 
-	/* (remove possible '\n') */
-	strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1);
+	i = sysfs_match_string(valid_acts, val);
 
-	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));
+	if (i >= 0) {
+		strscpy(uv_nmi_action, valid_acts[i], sizeof(uv_nmi_action));
 		pr_info("UV: New NMI action:%s\n", uv_nmi_action);
 		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", valid_acts[i], valid_acts_desc[i]);
+
 	return -EINVAL;
 }
 
-- 
2.41.0

Re: [PATCH] x86/platform/uv: Use sysfs_match_string() for string parsing in param_set_action()

[Steve Wahl]

On Wed, Sep 13, 2023 at 05:16:56PM +0200, Hans de Goede wrote:
> Remove the custom, hard to read code to:
> 
> 1. Make a copy of "val" with any potential '\n' at the end stripped
> 2. Compare the copy against an array of allowed string values
> 
> Linux has the sysfs_match_string() helper exactly for cases like this,
> switch to this.

Hans,

I like this patch, compiling and testing now.

I was wondering, as long as we're in the neighborhood, how you feel
about changing the stored variable uv_nmi_action to an int or enum
rather than a string, since it can only be one of 6 values, and the
string compare while processing an NMI strikes me as inefficent.

It could extend this patch, or be done as a follow on.  And I'm
willing to supply the effort if you want me to.

--> Steve Wahl

-- 
Steve Wahl, Hewlett Packard Enterprise

Re: [PATCH] x86/platform/uv: Use sysfs_match_string() for string parsing in param_set_action()

[Hans de Goede]

Hi Steve,

On 9/13/23 18:56, Steve Wahl wrote:
> On Wed, Sep 13, 2023 at 05:16:56PM +0200, Hans de Goede wrote:
>> Remove the custom, hard to read code to:
>>
>> 1. Make a copy of "val" with any potential '\n' at the end stripped
>> 2. Compare the copy against an array of allowed string values
>>
>> Linux has the sysfs_match_string() helper exactly for cases like this,
>> switch to this.
> 
> Hans,
> 
> I like this patch, compiling and testing now.
> 
> I was wondering, as long as we're in the neighborhood, how you feel
> about changing the stored variable uv_nmi_action to an int or enum
> rather than a string, since it can only be one of 6 values, and the
> string compare while processing an NMI strikes me as inefficent.
> 
> It could extend this patch, or be done as a follow on.  And I'm
> willing to supply the effort if you want me to.

I must admit I did not look at the code consuming uv_nmi_action
and I did wonder why this was not an enum from day 1.

I'll prepare a v2 of this patch which changes uv_nmi_action
to an enum.

Note I can compile test this only, so I gope you will be able to
test the v2 a bit more thoroughly :)

Regards,

Hans

Re: [PATCH] x86/platform/uv: Use sysfs_match_string() for string parsing in param_set_action()

[Steve Wahl]

On Wed, Sep 13, 2023 at 07:01:31PM +0200, Hans de Goede wrote:
> Hi Steve,
> 
> On 9/13/23 18:56, Steve Wahl wrote:
> > On Wed, Sep 13, 2023 at 05:16:56PM +0200, Hans de Goede wrote:
> >> Remove the custom, hard to read code to:
> >>
> >> 1. Make a copy of "val" with any potential '\n' at the end stripped
> >> 2. Compare the copy against an array of allowed string values
> >>
> >> Linux has the sysfs_match_string() helper exactly for cases like this,
> >> switch to this.
> > 
> > Hans,
> > 
> > I like this patch, compiling and testing now.
> > 
> > I was wondering, as long as we're in the neighborhood, how you feel
> > about changing the stored variable uv_nmi_action to an int or enum
> > rather than a string, since it can only be one of 6 values, and the
> > string compare while processing an NMI strikes me as inefficent.
> > 
> > It could extend this patch, or be done as a follow on.  And I'm
> > willing to supply the effort if you want me to.
> 
> I must admit I did not look at the code consuming uv_nmi_action
> and I did wonder why this was not an enum from day 1.
> 
> I'll prepare a v2 of this patch which changes uv_nmi_action
> to an enum.
> 
> Note I can compile test this only, so I gope you will be able to
> test the v2 a bit more thoroughly :)

I will!

--> Steve Wahl

-- 
Steve Wahl, Hewlett Packard Enterprise