Re: [PATCH] platform/x86: think-lmi: Add bulk save feature

[Mark Pearson]

Thanks Hans,

On Tue, Sep 5, 2023, at 7:17 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 8/29/23 15:15, Mark Pearson wrote:
<snip>
>> +
>> +	/* Check if user is trying to change the save mode */
>> +	if (!strncmp(buf, "bulk", 4) || !strncmp(buf, "single", 6)) {
>> +		tlmi_priv.save_mode = strncmp(buf, "bulk", 4) ? TLMI_SAVE_SINGLE : TLMI_SAVE_BULK;
>> +		return count;
>> +	}
>> +	if (strncmp(buf, "save", 4))
>> +		return -EINVAL;
>
> Things look good up to this point, but I'm not happy
> with the string parsing here. Using strncmp to avoid
> a possible '\n' means that writing
> "bulk extra special with onions" will also match "bulk".
>
> Instead I suggest the following (better names
> for the enum are welcome):
>
> 	enum { save_single, save_bulk, save_save };
>
> 	const char * const save_strings[] = {
> 		[save_single] = "single",
> 		[save_bulk] = "bulk",
> 		[save_save] = "save",
> 	};
>
> 	int ret = 0;
> 	int cmd;
>
> 	cmd = sysfs_match_string(save_strings, buf);
> 	if (cmd < 0)
> 		return cmd;
>
> 	mutex_lock(&tlmi_mutex);
>
> 	switch (cmd) {
> 	case save_single:
> 		tlmi_priv.save_mode = TLMI_SAVE_SINGLE;
> 		goto out;
> 	case save_bulk:
> 		tlmi_priv.save_mode = TLMI_SAVE_BULK;
> 		goto out;
> 	case save_save:
> 		break; /* Continue with saving settings */
> 	}
>
> 	/* The user is triggering a save - if supported */
> 	if (!tlmi_priv.can_set_bios_settings ||
> 	    tlmi_priv.save_mode == TLMI_SAVE_SINGLE)
> 		return -EOPNOTSUPP;
>
> 	...
>
> This lets sysfs_match_string() do the string parsing work
> for us, getting rid of having to do this ourselves.
>
Agreed - this is much better. Thanks for the suggestion and I'll make the change.

> Notice I have also moved the mutex_lock() up, so that
> it is also done for updating the save_mode since we
> don't want that the change halfway through a possibly
> racing current_value_store() call.
>
Ack

Mark