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

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

[Mark Pearson]

On Lenovo platforms there is a limitation in the number of times an
attribute can be saved. This is an architectural limitation and it limits
the number of attributes that can be modified to 48.
A solution for this is instead of the attribute being saved after every
modification allow a user to bulk set the attributes and then trigger a
final save. This allows unlimited attributes.

This patch introduces a save_settings attribute that can be configured to
either single or bulk mode by the user.
Single mode is the default but customers who want to avoid the 48
attribute limit can enable bulk mode.

Displaying the save_settings attribute will display the enabled mode.

When in bulk mode writing 'save' to the save_settings attribute will
trigger a save. Once this has been done a reboot is required before more
attributes can be modified.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2: Improve string handling in store and show functions. Use
sysfs_match_string for cleaner implementation.

 .../testing/sysfs-class-firmware-attributes   |  30 ++++
 drivers/platform/x86/think-lmi.c              | 152 ++++++++++++++++--
 drivers/platform/x86/think-lmi.h              |  15 ++
 3 files changed, 182 insertions(+), 15 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index f205d39409a3..c2f1a044475e 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -383,6 +383,36 @@ Description:
 		Note that any changes to this attribute requires a reboot
 		for changes to take effect.
 
+What:		/sys/class/firmware-attributes/*/attributes/save_settings
+Date:		August 2023
+KernelVersion:	6.5
+Contact:	Mark Pearson <mpearson-lenovo@squebb.ca>
+Description:
+		On Lenovo platforms there is a limitation in the number of times an attribute can be
+		saved. This is an architectural limitation and it limits the number of attributes
+		that can be modified to 48.
+		A solution for this is instead of the attribute being saved after every modification,
+		to allow a user to bulk set the attributes, and then trigger a final save. This allows
+		unlimited attributes.
+
+		Read the attribute to check what save mode is enabled (single or bulk).
+		E.g:
+		# cat /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+		single
+
+		Write the attribute with 'bulk' to enable bulk save mode.
+		Write the attribute with 'single' to enable saving, after every attribute set.
+		The default setting is single mode.
+		E.g:
+		# echo bulk > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+
+		When in bulk mode write 'save' to trigger a save of all currently modified attributes.
+		Note, once a save has been triggered, in bulk mode, attributes can no longer be set and
+		will return a permissions error. This is to prevent users hitting the 48+ save limitation
+		(which requires entering the BIOS to clear the error condition)
+		E.g:
+		# echo save > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+
 What:		/sys/class/firmware-attributes/*/attributes/debug_cmd
 Date:		July 2021
 KernelVersion:	5.14
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 52d1ce8dfe44..a319a358ddcf 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -985,6 +985,13 @@ static ssize_t current_value_store(struct kobject *kobj,
 	if (!tlmi_priv.can_set_bios_settings)
 		return -EOPNOTSUPP;
 
+	/*
+	 * If we are using bulk saves a reboot should be done once save has
+	 * been called
+	 */
+	if (tlmi_priv.save_mode == TLMI_SAVE_BULK && tlmi_priv.reboot_required)
+		return -EPERM;
+
 	new_setting = kstrdup(buf, GFP_KERNEL);
 	if (!new_setting)
 		return -ENOMEM;
@@ -1011,10 +1018,11 @@ static ssize_t current_value_store(struct kobject *kobj,
 		ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
 		if (ret)
 			goto out;
-		ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
-				tlmi_priv.pwd_admin->save_signature);
-		if (ret)
-			goto out;
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
+			tlmi_priv.save_required = true;
+		else
+			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
+					       tlmi_priv.pwd_admin->save_signature);
 	} else if (tlmi_priv.opcode_support) {
 		/*
 		 * If opcode support is present use that interface.
@@ -1033,14 +1041,17 @@ static ssize_t current_value_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
-			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
-						  tlmi_priv.pwd_admin->password);
-			if (ret)
-				goto out;
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
+			tlmi_priv.save_required = true;
+		} else {
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
+							  tlmi_priv.pwd_admin->password);
+				if (ret)
+					goto out;
+			}
+			ret = tlmi_save_bios_settings("");
 		}
-
-		ret = tlmi_save_bios_settings("");
 	} else { /* old non-opcode based authentication method (deprecated) */
 		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
 			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
@@ -1068,10 +1079,14 @@ static ssize_t current_value_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (auth_str)
-			ret = tlmi_save_bios_settings(auth_str);
-		else
-			ret = tlmi_save_bios_settings("");
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
+			tlmi_priv.save_required = true;
+		} else {
+			if (auth_str)
+				ret = tlmi_save_bios_settings(auth_str);
+			else
+				ret = tlmi_save_bios_settings("");
+		}
 	}
 	if (!ret && !tlmi_priv.pending_changes) {
 		tlmi_priv.pending_changes = true;
@@ -1152,6 +1167,107 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
 
 static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
 
+static const char * const save_mode_strings[] = {
+	[TLMI_SAVE_SINGLE] = "single",
+	[TLMI_SAVE_BULK] = "bulk",
+	[TLMI_SAVE_SAVE] = "save"
+};
+
+static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
+				  char *buf)
+{
+	/* Check that setting is valid */
+	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
+		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
+		return -EIO;
+	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
+}
+
+static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
+				   const char *buf, size_t count)
+{
+	char *auth_str = NULL;
+	int ret = 0;
+	int cmd;
+
+	cmd = sysfs_match_string(save_mode_strings, buf);
+
+	/* Use lock in case multiple WMI operations needed */
+	mutex_lock(&tlmi_mutex);
+
+	switch (cmd) {
+	case TLMI_SAVE_SINGLE:
+	case TLMI_SAVE_BULK:
+		tlmi_priv.save_mode = cmd;
+		goto out;
+	case TLMI_SAVE_SAVE:
+		/* Check if supported*/
+		if ((!tlmi_priv.can_set_bios_settings) ||
+		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
+			ret = -EOPNOTSUPP;
+			goto out;
+		}
+		/* Check there is actually something to save */
+		if (!tlmi_priv.save_required) {
+			ret = -ENOENT;
+			goto out;
+		}
+		/* Check if certificate authentication is enabled and active */
+		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
+			if (!tlmi_priv.pwd_admin->signature ||
+			    !tlmi_priv.pwd_admin->save_signature) {
+				ret = -EINVAL;
+				goto out;
+			}
+			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
+					       tlmi_priv.pwd_admin->save_signature);
+			if (ret)
+				goto out;
+		} else if (tlmi_priv.opcode_support) {
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
+							  tlmi_priv.pwd_admin->password);
+				if (ret)
+					goto out;
+			}
+			ret = tlmi_save_bios_settings("");
+		} else { /* old non-opcode based authentication method (deprecated) */
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
+						     tlmi_priv.pwd_admin->password,
+						     encoding_options[tlmi_priv.pwd_admin->encoding],
+						     tlmi_priv.pwd_admin->kbdlang);
+				if (!auth_str) {
+					ret = -ENOMEM;
+					goto out;
+				}
+			}
+
+			if (auth_str)
+				ret = tlmi_save_bios_settings(auth_str);
+			else
+				ret = tlmi_save_bios_settings("");
+		}
+		tlmi_priv.save_required = false;
+		tlmi_priv.reboot_required = true;
+
+		if (!ret && !tlmi_priv.pending_changes) {
+			tlmi_priv.pending_changes = true;
+			/* let userland know it may need to check reboot pending again */
+			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
+		}
+		break;
+	default:
+		ret = -EINVAL;
+	}
+out:
+	mutex_unlock(&tlmi_mutex);
+	kfree(auth_str);
+	return ret ?: count;
+}
+
+static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
+
 /* ---- Debug interface--------------------------------------------------------- */
 static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
 				const char *buf, size_t count)
@@ -1221,6 +1337,8 @@ static void tlmi_release_attr(void)
 		}
 	}
 	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
+	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
+
 	if (tlmi_priv.can_debug_cmd && debug_support)
 		sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
 
@@ -1302,6 +1420,10 @@ static int tlmi_sysfs_init(void)
 	if (ret)
 		goto fail_create_attr;
 
+	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
+	if (ret)
+		goto fail_create_attr;
+
 	if (tlmi_priv.can_debug_cmd && debug_support) {
 		ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
 		if (ret)
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index 4daba6151cd6..b2e654bd8316 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -27,6 +27,18 @@ enum level_option {
 	TLMI_LEVEL_MASTER,
 };
 
+/* There are a limit on the number of WMI operations you can do if you use
+ * the default implementation of saving on every set. This is due to a
+ * limitation in EFI variable space used.
+ * Have a 'bulk save' mode where you can manually trigger the save, and can
+ * therefore set unlimited variables - for users that need it.
+ */
+enum save_mode {
+	TLMI_SAVE_SINGLE,
+	TLMI_SAVE_BULK,
+	TLMI_SAVE_SAVE,
+};
+
 /* password configuration details */
 struct tlmi_pwdcfg_core {
 	uint32_t password_mode;
@@ -86,6 +98,9 @@ struct think_lmi {
 	bool can_debug_cmd;
 	bool opcode_support;
 	bool certificate_support;
+	enum save_mode save_mode;
+	bool save_required;
+	bool reboot_required;
 
 	struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
 	struct device *class_dev;
-- 
2.41.0

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

[Hans de Goede]

Hi,

On 9/6/23 14:13, Mark Pearson wrote:
> On Lenovo platforms there is a limitation in the number of times an
> attribute can be saved. This is an architectural limitation and it limits
> the number of attributes that can be modified to 48.
> A solution for this is instead of the attribute being saved after every
> modification allow a user to bulk set the attributes and then trigger a
> final save. This allows unlimited attributes.
> 
> This patch introduces a save_settings attribute that can be configured to
> either single or bulk mode by the user.
> Single mode is the default but customers who want to avoid the 48
> attribute limit can enable bulk mode.
> 
> Displaying the save_settings attribute will display the enabled mode.
> 
> When in bulk mode writing 'save' to the save_settings attribute will
> trigger a save. Once this has been done a reboot is required before more
> attributes can be modified.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
> Changes in v2: Improve string handling in store and show functions. Use
> sysfs_match_string for cleaner implementation.
> 
>  .../testing/sysfs-class-firmware-attributes   |  30 ++++
>  drivers/platform/x86/think-lmi.c              | 152 ++++++++++++++++--
>  drivers/platform/x86/think-lmi.h              |  15 ++
>  3 files changed, 182 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index f205d39409a3..c2f1a044475e 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -383,6 +383,36 @@ Description:
>  		Note that any changes to this attribute requires a reboot
>  		for changes to take effect.
>  
> +What:		/sys/class/firmware-attributes/*/attributes/save_settings
> +Date:		August 2023
> +KernelVersion:	6.5
> +Contact:	Mark Pearson <mpearson-lenovo@squebb.ca>
> +Description:
> +		On Lenovo platforms there is a limitation in the number of times an attribute can be
> +		saved. This is an architectural limitation and it limits the number of attributes
> +		that can be modified to 48.
> +		A solution for this is instead of the attribute being saved after every modification,
> +		to allow a user to bulk set the attributes, and then trigger a final save. This allows
> +		unlimited attributes.
> +
> +		Read the attribute to check what save mode is enabled (single or bulk).
> +		E.g:
> +		# cat /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +		single
> +
> +		Write the attribute with 'bulk' to enable bulk save mode.
> +		Write the attribute with 'single' to enable saving, after every attribute set.
> +		The default setting is single mode.
> +		E.g:
> +		# echo bulk > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +
> +		When in bulk mode write 'save' to trigger a save of all currently modified attributes.
> +		Note, once a save has been triggered, in bulk mode, attributes can no longer be set and
> +		will return a permissions error. This is to prevent users hitting the 48+ save limitation
> +		(which requires entering the BIOS to clear the error condition)
> +		E.g:
> +		# echo save > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +
>  What:		/sys/class/firmware-attributes/*/attributes/debug_cmd
>  Date:		July 2021
>  KernelVersion:	5.14
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 52d1ce8dfe44..a319a358ddcf 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -985,6 +985,13 @@ static ssize_t current_value_store(struct kobject *kobj,
>  	if (!tlmi_priv.can_set_bios_settings)
>  		return -EOPNOTSUPP;
>  
> +	/*
> +	 * If we are using bulk saves a reboot should be done once save has
> +	 * been called
> +	 */
> +	if (tlmi_priv.save_mode == TLMI_SAVE_BULK && tlmi_priv.reboot_required)
> +		return -EPERM;
> +
>  	new_setting = kstrdup(buf, GFP_KERNEL);
>  	if (!new_setting)
>  		return -ENOMEM;
> @@ -1011,10 +1018,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
>  		if (ret)
>  			goto out;
> -		ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> -				tlmi_priv.pwd_admin->save_signature);
> -		if (ret)
> -			goto out;
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
> +			tlmi_priv.save_required = true;
> +		else
> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> +					       tlmi_priv.pwd_admin->save_signature);
>  	} else if (tlmi_priv.opcode_support) {
>  		/*
>  		 * If opcode support is present use that interface.
> @@ -1033,14 +1041,17 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		if (ret)
>  			goto out;
>  
> -		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> -			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> -						  tlmi_priv.pwd_admin->password);
> -			if (ret)
> -				goto out;
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> +			tlmi_priv.save_required = true;
> +		} else {
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> +							  tlmi_priv.pwd_admin->password);
> +				if (ret)
> +					goto out;
> +			}
> +			ret = tlmi_save_bios_settings("");
>  		}
> -
> -		ret = tlmi_save_bios_settings("");
>  	} else { /* old non-opcode based authentication method (deprecated) */
>  		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> @@ -1068,10 +1079,14 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		if (ret)
>  			goto out;
>  
> -		if (auth_str)
> -			ret = tlmi_save_bios_settings(auth_str);
> -		else
> -			ret = tlmi_save_bios_settings("");
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> +			tlmi_priv.save_required = true;
> +		} else {
> +			if (auth_str)
> +				ret = tlmi_save_bios_settings(auth_str);
> +			else
> +				ret = tlmi_save_bios_settings("");
> +		}
>  	}
>  	if (!ret && !tlmi_priv.pending_changes) {
>  		tlmi_priv.pending_changes = true;
> @@ -1152,6 +1167,107 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
>  
>  static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
>  
> +static const char * const save_mode_strings[] = {
> +	[TLMI_SAVE_SINGLE] = "single",
> +	[TLMI_SAVE_BULK] = "bulk",
> +	[TLMI_SAVE_SAVE] = "save"
> +};
> +
> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
> +				  char *buf)
> +{
> +	/* Check that setting is valid */
> +	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
> +		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
> +		return -EIO;
> +	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
> +}
> +
> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	char *auth_str = NULL;
> +	int ret = 0;
> +	int cmd;
> +
> +	cmd = sysfs_match_string(save_mode_strings, buf);
> +
> +	/* Use lock in case multiple WMI operations needed */
> +	mutex_lock(&tlmi_mutex);
> +
> +	switch (cmd) {
> +	case TLMI_SAVE_SINGLE:
> +	case TLMI_SAVE_BULK:
> +		tlmi_priv.save_mode = cmd;
> +		goto out;
> +	case TLMI_SAVE_SAVE:
> +		/* Check if supported*/
> +		if ((!tlmi_priv.can_set_bios_settings) ||
> +		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +		/* Check there is actually something to save */
> +		if (!tlmi_priv.save_required) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		/* Check if certificate authentication is enabled and active */
> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
> +			if (!tlmi_priv.pwd_admin->signature ||
> +			    !tlmi_priv.pwd_admin->save_signature) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> +					       tlmi_priv.pwd_admin->save_signature);
> +			if (ret)
> +				goto out;
> +		} else if (tlmi_priv.opcode_support) {
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> +							  tlmi_priv.pwd_admin->password);
> +				if (ret)
> +					goto out;
> +			}
> +			ret = tlmi_save_bios_settings("");
> +		} else { /* old non-opcode based authentication method (deprecated) */
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> +						     tlmi_priv.pwd_admin->password,
> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
> +						     tlmi_priv.pwd_admin->kbdlang);
> +				if (!auth_str) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +			}
> +
> +			if (auth_str)
> +				ret = tlmi_save_bios_settings(auth_str);
> +			else
> +				ret = tlmi_save_bios_settings("");
> +		}
> +		tlmi_priv.save_required = false;
> +		tlmi_priv.reboot_required = true;
> +
> +		if (!ret && !tlmi_priv.pending_changes) {
> +			tlmi_priv.pending_changes = true;
> +			/* let userland know it may need to check reboot pending again */
> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +out:
> +	mutex_unlock(&tlmi_mutex);
> +	kfree(auth_str);
> +	return ret ?: count;
> +}
> +
> +static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
> +
>  /* ---- Debug interface--------------------------------------------------------- */
>  static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
>  				const char *buf, size_t count)
> @@ -1221,6 +1337,8 @@ static void tlmi_release_attr(void)
>  		}
>  	}
>  	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> +	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
> +
>  	if (tlmi_priv.can_debug_cmd && debug_support)
>  		sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
>  
> @@ -1302,6 +1420,10 @@ static int tlmi_sysfs_init(void)
>  	if (ret)
>  		goto fail_create_attr;
>  
> +	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
> +	if (ret)
> +		goto fail_create_attr;
> +
>  	if (tlmi_priv.can_debug_cmd && debug_support) {
>  		ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
>  		if (ret)
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index 4daba6151cd6..b2e654bd8316 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -27,6 +27,18 @@ enum level_option {
>  	TLMI_LEVEL_MASTER,
>  };
>  
> +/* There are a limit on the number of WMI operations you can do if you use
> + * the default implementation of saving on every set. This is due to a
> + * limitation in EFI variable space used.
> + * Have a 'bulk save' mode where you can manually trigger the save, and can
> + * therefore set unlimited variables - for users that need it.
> + */
> +enum save_mode {
> +	TLMI_SAVE_SINGLE,
> +	TLMI_SAVE_BULK,
> +	TLMI_SAVE_SAVE,
> +};
> +
>  /* password configuration details */
>  struct tlmi_pwdcfg_core {
>  	uint32_t password_mode;
> @@ -86,6 +98,9 @@ struct think_lmi {
>  	bool can_debug_cmd;
>  	bool opcode_support;
>  	bool certificate_support;
> +	enum save_mode save_mode;
> +	bool save_required;
> +	bool reboot_required;
>  
>  	struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
>  	struct device *class_dev;

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

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
> On Lenovo platforms there is a limitation in the number of times an
> attribute can be saved. This is an architectural limitation and it limits
> the number of attributes that can be modified to 48.
> A solution for this is instead of the attribute being saved after every
> modification allow a user to bulk set the attributes and then trigger a
> final save. This allows unlimited attributes.
> 
> This patch introduces a save_settings attribute that can be configured to
> either single or bulk mode by the user.
> Single mode is the default but customers who want to avoid the 48
> attribute limit can enable bulk mode.
> 
> Displaying the save_settings attribute will display the enabled mode.
> 
> When in bulk mode writing 'save' to the save_settings attribute will
> trigger a save. Once this has been done a reboot is required before more
> attributes can be modified.

...

> +Date:		August 2023
> +KernelVersion:	6.5

This is obviously incorrect (outdated) information.

...

> +static const char * const save_mode_strings[] = {
> +	[TLMI_SAVE_SINGLE] = "single",
> +	[TLMI_SAVE_BULK] = "bulk",
> +	[TLMI_SAVE_SAVE] = "save"

Missing comma.

> +};

...

> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
> +				  char *buf)
> +{
> +	/* Check that setting is valid */
> +	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
> +		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
> +		return -EIO;
> +	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);

According to the documentation it must be sysfs_emit() if I'm not missing
anything here.

> +}

...

> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	char *auth_str = NULL;
> +	int ret = 0;
> +	int cmd;
> +
> +	cmd = sysfs_match_string(save_mode_strings, buf);
> +
> +	/* Use lock in case multiple WMI operations needed */
> +	mutex_lock(&tlmi_mutex);
> +
> +	switch (cmd) {
> +	case TLMI_SAVE_SINGLE:
> +	case TLMI_SAVE_BULK:
> +		tlmi_priv.save_mode = cmd;
> +		goto out;
> +	case TLMI_SAVE_SAVE:
> +		/* Check if supported*/
> +		if ((!tlmi_priv.can_set_bios_settings) ||
> +		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +		/* Check there is actually something to save */
> +		if (!tlmi_priv.save_required) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		/* Check if certificate authentication is enabled and active */
> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
> +			if (!tlmi_priv.pwd_admin->signature ||
> +			    !tlmi_priv.pwd_admin->save_signature) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> +					       tlmi_priv.pwd_admin->save_signature);
> +			if (ret)
> +				goto out;
> +		} else if (tlmi_priv.opcode_support) {
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> +							  tlmi_priv.pwd_admin->password);
> +				if (ret)
> +					goto out;
> +			}
> +			ret = tlmi_save_bios_settings("");
> +		} else { /* old non-opcode based authentication method (deprecated) */
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> +						     tlmi_priv.pwd_admin->password,
> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
> +						     tlmi_priv.pwd_admin->kbdlang);
> +				if (!auth_str) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +			}
> +
> +			if (auth_str)
> +				ret = tlmi_save_bios_settings(auth_str);
> +			else
> +				ret = tlmi_save_bios_settings("");
> +		}
> +		tlmi_priv.save_required = false;
> +		tlmi_priv.reboot_required = true;
> +
> +		if (!ret && !tlmi_priv.pending_changes) {
> +			tlmi_priv.pending_changes = true;
> +			/* let userland know it may need to check reboot pending again */
> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
> +		}
> +		break;

> +	default:
> +		ret = -EINVAL;
> +	}

Missing break; and actually no need to do this part under the lock, besides
that it shadows an error code, that said this should be

	cmd = sysfs_match_string(...);
	if (cmd < 0)
		return cmd;


> +out:
> +	mutex_unlock(&tlmi_mutex);
> +	kfree(auth_str);
> +	return ret ?: count;

You can switch the driver to use cleanup.h at some point.

> +}

...

> +/* There are a limit on the number of WMI operations you can do if you use
> + * the default implementation of saving on every set. This is due to a
> + * limitation in EFI variable space used.
> + * Have a 'bulk save' mode where you can manually trigger the save, and can
> + * therefore set unlimited variables - for users that need it.
> + */

/*
 * This is wrong multi-line comment style. This one
 * is used solely in net subsystem.
 */

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Sep 18, 2023 at 04:57:58PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:

...

> > +Date:		August 2023
> > +KernelVersion:	6.5
> 
> This is obviously incorrect (outdated) information.

Joe, does checkpatch have a hook to test that (using phb-crystal-ball data)?

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Mon, 2023-09-18 at 17:00 +0300, Andy Shevchenko wrote:
> On Mon, Sep 18, 2023 at 04:57:58PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
> 
> ...
> 
> > > +Date:		August 2023
> > > +KernelVersion:	6.5
> > 
> > This is obviously incorrect (outdated) information.
> 
> Joe, does checkpatch have a hook to test that (using phb-crystal-ball data)?

No and I rather doubt it should.

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

[Hans de Goede]

Hi,

On 9/18/23 15:57, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
>> On Lenovo platforms there is a limitation in the number of times an
>> attribute can be saved. This is an architectural limitation and it limits
>> the number of attributes that can be modified to 48.
>> A solution for this is instead of the attribute being saved after every
>> modification allow a user to bulk set the attributes and then trigger a
>> final save. This allows unlimited attributes.
>>
>> This patch introduces a save_settings attribute that can be configured to
>> either single or bulk mode by the user.
>> Single mode is the default but customers who want to avoid the 48
>> attribute limit can enable bulk mode.
>>
>> Displaying the save_settings attribute will display the enabled mode.
>>
>> When in bulk mode writing 'save' to the save_settings attribute will
>> trigger a save. Once this has been done a reboot is required before more
>> attributes can be modified.
> 
> ...
> 
>> +Date:		August 2023
>> +KernelVersion:	6.5
> 
> This is obviously incorrect (outdated) information.

Mark can you please submit a follow up patch fixing this.

> 
> ...
> 
>> +static const char * const save_mode_strings[] = {
>> +	[TLMI_SAVE_SINGLE] = "single",
>> +	[TLMI_SAVE_BULK] = "bulk",
>> +	[TLMI_SAVE_SAVE] = "save"
> 
> Missing comma.

Fixing this retro-actively is not really useful, if we
ever need an extra entry we can deal with the churn then.

> 
>> +};
> 
> ...
> 
>> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
>> +				  char *buf)
>> +{
>> +	/* Check that setting is valid */
>> +	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
>> +		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
>> +		return -EIO;
>> +	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
> 
> According to the documentation it must be sysfs_emit() if I'm not missing
> anything here.

Yes switching to sysfs_emit() here in the followup patch would be good.

> 
>> +}
> 
> ...
> 
>> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
>> +				   const char *buf, size_t count)
>> +{
>> +	char *auth_str = NULL;
>> +	int ret = 0;
>> +	int cmd;
>> +
>> +	cmd = sysfs_match_string(save_mode_strings, buf);
>> +
>> +	/* Use lock in case multiple WMI operations needed */
>> +	mutex_lock(&tlmi_mutex);
>> +
>> +	switch (cmd) {
>> +	case TLMI_SAVE_SINGLE:
>> +	case TLMI_SAVE_BULK:
>> +		tlmi_priv.save_mode = cmd;
>> +		goto out;
>> +	case TLMI_SAVE_SAVE:
>> +		/* Check if supported*/
>> +		if ((!tlmi_priv.can_set_bios_settings) ||
>> +		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
>> +			ret = -EOPNOTSUPP;
>> +			goto out;
>> +		}
>> +		/* Check there is actually something to save */
>> +		if (!tlmi_priv.save_required) {
>> +			ret = -ENOENT;
>> +			goto out;
>> +		}
>> +		/* Check if certificate authentication is enabled and active */
>> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
>> +			if (!tlmi_priv.pwd_admin->signature ||
>> +			    !tlmi_priv.pwd_admin->save_signature) {
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>> +					       tlmi_priv.pwd_admin->save_signature);
>> +			if (ret)
>> +				goto out;
>> +		} else if (tlmi_priv.opcode_support) {
>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>> +							  tlmi_priv.pwd_admin->password);
>> +				if (ret)
>> +					goto out;
>> +			}
>> +			ret = tlmi_save_bios_settings("");
>> +		} else { /* old non-opcode based authentication method (deprecated) */
>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> +						     tlmi_priv.pwd_admin->password,
>> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
>> +						     tlmi_priv.pwd_admin->kbdlang);
>> +				if (!auth_str) {
>> +					ret = -ENOMEM;
>> +					goto out;
>> +				}
>> +			}
>> +
>> +			if (auth_str)
>> +				ret = tlmi_save_bios_settings(auth_str);
>> +			else
>> +				ret = tlmi_save_bios_settings("");
>> +		}
>> +		tlmi_priv.save_required = false;
>> +		tlmi_priv.reboot_required = true;
>> +
>> +		if (!ret && !tlmi_priv.pending_changes) {
>> +			tlmi_priv.pending_changes = true;
>> +			/* let userland know it may need to check reboot pending again */
>> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
>> +		}
>> +		break;
> 
>> +	default:
>> +		ret = -EINVAL;
>> +	}
> 
> Missing break; and actually no need to do this part under the lock, besides
> that it shadows an error code, that said this should be
> 
> 	cmd = sysfs_match_string(...);
> 	if (cmd < 0)
> 		return cmd;
> 
> 
>> +out:
>> +	mutex_unlock(&tlmi_mutex);
>> +	kfree(auth_str);
>> +	return ret ?: count;
> 
> You can switch the driver to use cleanup.h at some point.
> 
>> +}
> 
> ...
> 
>> +/* There are a limit on the number of WMI operations you can do if you use
>> + * the default implementation of saving on every set. This is due to a
>> + * limitation in EFI variable space used.
>> + * Have a 'bulk save' mode where you can manually trigger the save, and can
>> + * therefore set unlimited variables - for users that need it.
>> + */
> 
> /*
>  * This is wrong multi-line comment style. This one
>  * is used solely in net subsystem.
>  */
> 

Good catch, Mark can you fix this one too please ?

Also I thought that checkpatch.pl used to catch this ?

Regards,

hans

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

[Andy Shevchenko]

On Mon, Sep 18, 2023 at 04:07:53PM +0200, Hans de Goede wrote:
> On 9/18/23 15:57, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:

...

> >> +/* There are a limit on the number of WMI operations you can do if you use
> >> + * the default implementation of saving on every set. This is due to a
> >> + * limitation in EFI variable space used.
> >> + * Have a 'bulk save' mode where you can manually trigger the save, and can
> >> + * therefore set unlimited variables - for users that need it.
> >> + */
> > 
> > /*
> >  * This is wrong multi-line comment style. This one
> >  * is used solely in net subsystem.
> >  */
> 
> Good catch, Mark can you fix this one too please ?
> 
> Also I thought that checkpatch.pl used to catch this ?

I don't think it recognizes "net style is only for net related files".

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Mon, 2023-09-18 at 17:38 +0300, Andy Shevchenko wrote:
> On Mon, Sep 18, 2023 at 04:07:53PM +0200, Hans de Goede wrote:
> > On 9/18/23 15:57, Andy Shevchenko wrote:
> > > On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
> 
> ...
> 
> > > > +/* There are a limit on the number of WMI operations you can do if you use
> > > > + * the default implementation of saving on every set. This is due to a
> > > > + * limitation in EFI variable space used.
> > > > + * Have a 'bulk save' mode where you can manually trigger the save, and can
> > > > + * therefore set unlimited variables - for users that need it.
> > > > + */
> > > 
> > > /*
> > >  * This is wrong multi-line comment style. This one
> > >  * is used solely in net subsystem.
> > >  */
> > 
> > Good catch, Mark can you fix this one too please ?
> > 
> > Also I thought that checkpatch.pl used to catch this ?
> 
> I don't think it recognizes "net style is only for net related files".

It doesn't as there are just too many of them.

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

[Andy Shevchenko]

On Mon, Sep 18, 2023 at 04:07:53PM +0200, Hans de Goede wrote:
> On 9/18/23 15:57, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:

...

> >> +static const char * const save_mode_strings[] = {
> >> +	[TLMI_SAVE_SINGLE] = "single",
> >> +	[TLMI_SAVE_BULK] = "bulk",
> >> +	[TLMI_SAVE_SAVE] = "save"
> > 
> > Missing comma.
> 
> Fixing this retro-actively is not really useful, if we
> ever need an extra entry we can deal with the churn then.

I agree, but this is to give a comprehensive review as long as I went through
the entire change.

> >> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Thanks Andy for the review

On Mon, Sep 18, 2023, at 10:07 AM, Hans de Goede wrote:
> Hi,
>
> On 9/18/23 15:57, Andy Shevchenko wrote:
>> On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
>>> On Lenovo platforms there is a limitation in the number of times an
>>> attribute can be saved. This is an architectural limitation and it limits
>>> the number of attributes that can be modified to 48.
>>> A solution for this is instead of the attribute being saved after every
>>> modification allow a user to bulk set the attributes and then trigger a
>>> final save. This allows unlimited attributes.
>>>
>>> This patch introduces a save_settings attribute that can be configured to
>>> either single or bulk mode by the user.
>>> Single mode is the default but customers who want to avoid the 48
>>> attribute limit can enable bulk mode.
>>>
>>> Displaying the save_settings attribute will display the enabled mode.
>>>
>>> When in bulk mode writing 'save' to the save_settings attribute will
>>> trigger a save. Once this has been done a reboot is required before more
>>> attributes can be modified.
>> 
>> ...
>> 
>>> +Date:		August 2023
>>> +KernelVersion:	6.5
>> 
>> This is obviously incorrect (outdated) information.
>
> Mark can you please submit a follow up patch fixing this.

So I assume I put 6.6 in here? Or will it be 6.7?

>
>> 
>> ...
>> 
>>> +static const char * const save_mode_strings[] = {
>>> +	[TLMI_SAVE_SINGLE] = "single",
>>> +	[TLMI_SAVE_BULK] = "bulk",
>>> +	[TLMI_SAVE_SAVE] = "save"
>> 
>> Missing comma.
>
> Fixing this retro-actively is not really useful, if we
> ever need an extra entry we can deal with the churn then.

As I'm making other changes I assume I address this one too.

>
>> 
>>> +};
>> 
>> ...
>> 
>>> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> +				  char *buf)
>>> +{
>>> +	/* Check that setting is valid */
>>> +	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
>>> +		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
>>> +		return -EIO;
>>> +	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
>> 
>> According to the documentation it must be sysfs_emit() if I'm not missing
>> anything here.
>
> Yes switching to sysfs_emit() here in the followup patch would be good.

Ack

>
>> 
>>> +}
>> 
>> ...
>> 
>>> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
>>> +				   const char *buf, size_t count)
>>> +{
>>> +	char *auth_str = NULL;
>>> +	int ret = 0;
>>> +	int cmd;
>>> +
>>> +	cmd = sysfs_match_string(save_mode_strings, buf);
>>> +
>>> +	/* Use lock in case multiple WMI operations needed */
>>> +	mutex_lock(&tlmi_mutex);
>>> +
>>> +	switch (cmd) {
>>> +	case TLMI_SAVE_SINGLE:
>>> +	case TLMI_SAVE_BULK:
>>> +		tlmi_priv.save_mode = cmd;
>>> +		goto out;
>>> +	case TLMI_SAVE_SAVE:
>>> +		/* Check if supported*/
>>> +		if ((!tlmi_priv.can_set_bios_settings) ||
>>> +		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
>>> +			ret = -EOPNOTSUPP;
>>> +			goto out;
>>> +		}
>>> +		/* Check there is actually something to save */
>>> +		if (!tlmi_priv.save_required) {
>>> +			ret = -ENOENT;
>>> +			goto out;
>>> +		}
>>> +		/* Check if certificate authentication is enabled and active */
>>> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
>>> +			if (!tlmi_priv.pwd_admin->signature ||
>>> +			    !tlmi_priv.pwd_admin->save_signature) {
>>> +				ret = -EINVAL;
>>> +				goto out;
>>> +			}
>>> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>>> +					       tlmi_priv.pwd_admin->save_signature);
>>> +			if (ret)
>>> +				goto out;
>>> +		} else if (tlmi_priv.opcode_support) {
>>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>>> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>>> +							  tlmi_priv.pwd_admin->password);
>>> +				if (ret)
>>> +					goto out;
>>> +			}
>>> +			ret = tlmi_save_bios_settings("");
>>> +		} else { /* old non-opcode based authentication method (deprecated) */
>>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>>> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>> +						     tlmi_priv.pwd_admin->password,
>>> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>> +						     tlmi_priv.pwd_admin->kbdlang);
>>> +				if (!auth_str) {
>>> +					ret = -ENOMEM;
>>> +					goto out;
>>> +				}
>>> +			}
>>> +
>>> +			if (auth_str)
>>> +				ret = tlmi_save_bios_settings(auth_str);
>>> +			else
>>> +				ret = tlmi_save_bios_settings("");
>>> +		}
>>> +		tlmi_priv.save_required = false;
>>> +		tlmi_priv.reboot_required = true;
>>> +
>>> +		if (!ret && !tlmi_priv.pending_changes) {
>>> +			tlmi_priv.pending_changes = true;
>>> +			/* let userland know it may need to check reboot pending again */
>>> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
>>> +		}
>>> +		break;
>> 
>>> +	default:
>>> +		ret = -EINVAL;
>>> +	}
>> 
>> Missing break; and actually no need to do this part under the lock, besides
>> that it shadows an error code, that said this should be
>> 
>> 	cmd = sysfs_match_string(...);
>> 	if (cmd < 0)
>> 		return cmd;
>> 
>> 
>>> +out:
>>> +	mutex_unlock(&tlmi_mutex);
>>> +	kfree(auth_str);
>>> +	return ret ?: count;
>> 
>> You can switch the driver to use cleanup.h at some point.
>> 
>>> +}
>> 

Ack - thanks for the notes.

>> ...
>> 
>>> +/* There are a limit on the number of WMI operations you can do if you use
>>> + * the default implementation of saving on every set. This is due to a
>>> + * limitation in EFI variable space used.
>>> + * Have a 'bulk save' mode where you can manually trigger the save, and can
>>> + * therefore set unlimited variables - for users that need it.
>>> + */
>> 
>> /*
>>  * This is wrong multi-line comment style. This one
>>  * is used solely in net subsystem.
>>  */
>> 
>
> Good catch, Mark can you fix this one too please ?
>
Will do - not sure why I did that in the first place (it's a habit from a looong time ago that came back. Sigh).

Thanks!
Mark

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

[Hans de Goede]

Hi Mark,

On 9/18/23 17:23, Mark Pearson wrote:
> Thanks Andy for the review
> 
> On Mon, Sep 18, 2023, at 10:07 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/18/23 15:57, Andy Shevchenko wrote:
>>> On Wed, Sep 06, 2023 at 08:13:14AM -0400, Mark Pearson wrote:
>>>> On Lenovo platforms there is a limitation in the number of times an
>>>> attribute can be saved. This is an architectural limitation and it limits
>>>> the number of attributes that can be modified to 48.
>>>> A solution for this is instead of the attribute being saved after every
>>>> modification allow a user to bulk set the attributes and then trigger a
>>>> final save. This allows unlimited attributes.
>>>>
>>>> This patch introduces a save_settings attribute that can be configured to
>>>> either single or bulk mode by the user.
>>>> Single mode is the default but customers who want to avoid the 48
>>>> attribute limit can enable bulk mode.
>>>>
>>>> Displaying the save_settings attribute will display the enabled mode.
>>>>
>>>> When in bulk mode writing 'save' to the save_settings attribute will
>>>> trigger a save. Once this has been done a reboot is required before more
>>>> attributes can be modified.
>>>
>>> ...
>>>
>>>> +Date:		August 2023
>>>> +KernelVersion:	6.5
>>>
>>> This is obviously incorrect (outdated) information.
>>
>> Mark can you please submit a follow up patch fixing this.
> 
> So I assume I put 6.6 in here? Or will it be 6.7?

Yes this should be 6.6 .


>>> ...
>>>
>>>> +static const char * const save_mode_strings[] = {
>>>> +	[TLMI_SAVE_SINGLE] = "single",
>>>> +	[TLMI_SAVE_BULK] = "bulk",
>>>> +	[TLMI_SAVE_SAVE] = "save"
>>>
>>> Missing comma.
>>
>> Fixing this retro-actively is not really useful, if we
>> ever need an extra entry we can deal with the churn then.
> 
> As I'm making other changes I assume I address this one too.

Please skip fixing / do not fix this one. The goal of having
a "," after the last (non terminating) entry of an array
is to avoid getting the last line listed as modified in the diff
when adding further entries just to add the ','.

So fixing this after the fact is not really useful. If this array
ever gets an extra entry we can just add the ',' when that happens.


>>>
>>>> +};
>>>
>>> ...
>>>
>>>> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
>>>> +				  char *buf)
>>>> +{
>>>> +	/* Check that setting is valid */
>>>> +	if (WARN_ON((tlmi_priv.save_mode < TLMI_SAVE_SINGLE) ||
>>>> +		    (tlmi_priv.save_mode > TLMI_SAVE_BULK)))
>>>> +		return -EIO;
>>>> +	return sprintf(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
>>>
>>> According to the documentation it must be sysfs_emit() if I'm not missing
>>> anything here.
>>
>> Yes switching to sysfs_emit() here in the followup patch would be good.
> 
> Ack
> 
>>
>>>
>>>> +}
>>>
>>> ...
>>>
>>>> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
>>>> +				   const char *buf, size_t count)
>>>> +{
>>>> +	char *auth_str = NULL;
>>>> +	int ret = 0;
>>>> +	int cmd;
>>>> +
>>>> +	cmd = sysfs_match_string(save_mode_strings, buf);
>>>> +
>>>> +	/* Use lock in case multiple WMI operations needed */
>>>> +	mutex_lock(&tlmi_mutex);
>>>> +
>>>> +	switch (cmd) {
>>>> +	case TLMI_SAVE_SINGLE:
>>>> +	case TLMI_SAVE_BULK:
>>>> +		tlmi_priv.save_mode = cmd;
>>>> +		goto out;
>>>> +	case TLMI_SAVE_SAVE:
>>>> +		/* Check if supported*/
>>>> +		if ((!tlmi_priv.can_set_bios_settings) ||
>>>> +		    (tlmi_priv.save_mode == TLMI_SAVE_SINGLE)) {
>>>> +			ret = -EOPNOTSUPP;
>>>> +			goto out;
>>>> +		}
>>>> +		/* Check there is actually something to save */
>>>> +		if (!tlmi_priv.save_required) {
>>>> +			ret = -ENOENT;
>>>> +			goto out;
>>>> +		}
>>>> +		/* Check if certificate authentication is enabled and active */
>>>> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
>>>> +			if (!tlmi_priv.pwd_admin->signature ||
>>>> +			    !tlmi_priv.pwd_admin->save_signature) {
>>>> +				ret = -EINVAL;
>>>> +				goto out;
>>>> +			}
>>>> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>>>> +					       tlmi_priv.pwd_admin->save_signature);
>>>> +			if (ret)
>>>> +				goto out;
>>>> +		} else if (tlmi_priv.opcode_support) {
>>>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>>>> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>>>> +							  tlmi_priv.pwd_admin->password);
>>>> +				if (ret)
>>>> +					goto out;
>>>> +			}
>>>> +			ret = tlmi_save_bios_settings("");
>>>> +		} else { /* old non-opcode based authentication method (deprecated) */
>>>> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>>>> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>> +						     tlmi_priv.pwd_admin->password,
>>>> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>>> +						     tlmi_priv.pwd_admin->kbdlang);
>>>> +				if (!auth_str) {
>>>> +					ret = -ENOMEM;
>>>> +					goto out;
>>>> +				}
>>>> +			}
>>>> +
>>>> +			if (auth_str)
>>>> +				ret = tlmi_save_bios_settings(auth_str);
>>>> +			else
>>>> +				ret = tlmi_save_bios_settings("");
>>>> +		}
>>>> +		tlmi_priv.save_required = false;
>>>> +		tlmi_priv.reboot_required = true;
>>>> +
>>>> +		if (!ret && !tlmi_priv.pending_changes) {
>>>> +			tlmi_priv.pending_changes = true;
>>>> +			/* let userland know it may need to check reboot pending again */
>>>> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
>>>> +		}
>>>> +		break;
>>>
>>>> +	default:
>>>> +		ret = -EINVAL;
>>>> +	}
>>>
>>> Missing break; and actually no need to do this part under the lock, besides
>>> that it shadows an error code, that said this should be
>>>
>>> 	cmd = sysfs_match_string(...);
>>> 	if (cmd < 0)
>>> 		return cmd;
>>>
>>>
>>>> +out:
>>>> +	mutex_unlock(&tlmi_mutex);
>>>> +	kfree(auth_str);
>>>> +	return ret ?: count;
>>>
>>> You can switch the driver to use cleanup.h at some point.
>>>
>>>> +}
>>>
> 
> Ack - thanks for the notes.
> 
>>> ...
>>>
>>>> +/* There are a limit on the number of WMI operations you can do if you use
>>>> + * the default implementation of saving on every set. This is due to a
>>>> + * limitation in EFI variable space used.
>>>> + * Have a 'bulk save' mode where you can manually trigger the save, and can
>>>> + * therefore set unlimited variables - for users that need it.
>>>> + */
>>>
>>> /*
>>>  * This is wrong multi-line comment style. This one
>>>  * is used solely in net subsystem.
>>>  */
>>>
>>
>> Good catch, Mark can you fix this one too please ?
>>
> Will do - not sure why I did that in the first place (it's a habit from a looong time ago that came back. Sigh).

Regards,

Hans

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

[Mark Pearson]

On Lenovo platforms there is a limitation in the number of times an
attribute can be saved. This is an architectural limitation and it limits
the number of attributes that can be modified to 48.
A solution for this is instead of the attribute being saved after every
modification allow a user to bulk set the attributes and then trigger a
final save. This allows unlimited attributes.

This patch introduces a save_settings attribute that can be configured to
either single or bulk mode by the user.
Single mode is the default but customers who want to avoid the 48
attribute limit can enable bulk mode.

Displaying the save_settings attribute will display the enabled mode.

When in bulk mode writing 'save' to the save_settings attribute will
trigger a save. Once this has been done a reboot is required before more
attributes can be modified.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v2:
 - Correct kernel version in documentation
 - Updated to sysfs_emit
 - Clean up code in save_settings_store as recommended
 - Use correct comment formatting

 .../testing/sysfs-class-firmware-attributes   |  30 ++++
 drivers/platform/x86/think-lmi.c              | 152 ++++++++++++++++--
 drivers/platform/x86/think-lmi.h              |  16 ++
 3 files changed, 183 insertions(+), 15 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index f205d39409a3..9c82c7b42ff8 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -383,6 +383,36 @@ Description:
 		Note that any changes to this attribute requires a reboot
 		for changes to take effect.
 
+What:		/sys/class/firmware-attributes/*/attributes/save_settings
+Date:		August 2023
+KernelVersion:	6.6
+Contact:	Mark Pearson <mpearson-lenovo@squebb.ca>
+Description:
+		On Lenovo platforms there is a limitation in the number of times an attribute can be
+		saved. This is an architectural limitation and it limits the number of attributes
+		that can be modified to 48.
+		A solution for this is instead of the attribute being saved after every modification,
+		to allow a user to bulk set the attributes, and then trigger a final save. This allows
+		unlimited attributes.
+
+		Read the attribute to check what save mode is enabled (single or bulk).
+		E.g:
+		# cat /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+		single
+
+		Write the attribute with 'bulk' to enable bulk save mode.
+		Write the attribute with 'single' to enable saving, after every attribute set.
+		The default setting is single mode.
+		E.g:
+		# echo bulk > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+
+		When in bulk mode write 'save' to trigger a save of all currently modified attributes.
+		Note, once a save has been triggered, in bulk mode, attributes can no longer be set and
+		will return a permissions error. This is to prevent users hitting the 48+ save limitation
+		(which requires entering the BIOS to clear the error condition)
+		E.g:
+		# echo save > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
+
 What:		/sys/class/firmware-attributes/*/attributes/debug_cmd
 Date:		July 2021
 KernelVersion:	5.14
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 52d1ce8dfe44..6f9fa80b19fc 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -985,6 +985,13 @@ static ssize_t current_value_store(struct kobject *kobj,
 	if (!tlmi_priv.can_set_bios_settings)
 		return -EOPNOTSUPP;
 
+	/*
+	 * If we are using bulk saves a reboot should be done once save has
+	 * been called
+	 */
+	if (tlmi_priv.save_mode == TLMI_SAVE_BULK && tlmi_priv.reboot_required)
+		return -EPERM;
+
 	new_setting = kstrdup(buf, GFP_KERNEL);
 	if (!new_setting)
 		return -ENOMEM;
@@ -1011,10 +1018,11 @@ static ssize_t current_value_store(struct kobject *kobj,
 		ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
 		if (ret)
 			goto out;
-		ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
-				tlmi_priv.pwd_admin->save_signature);
-		if (ret)
-			goto out;
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
+			tlmi_priv.save_required = true;
+		else
+			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
+					       tlmi_priv.pwd_admin->save_signature);
 	} else if (tlmi_priv.opcode_support) {
 		/*
 		 * If opcode support is present use that interface.
@@ -1033,14 +1041,17 @@ static ssize_t current_value_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
-			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
-						  tlmi_priv.pwd_admin->password);
-			if (ret)
-				goto out;
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
+			tlmi_priv.save_required = true;
+		} else {
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
+							  tlmi_priv.pwd_admin->password);
+				if (ret)
+					goto out;
+			}
+			ret = tlmi_save_bios_settings("");
 		}
-
-		ret = tlmi_save_bios_settings("");
 	} else { /* old non-opcode based authentication method (deprecated) */
 		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
 			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
@@ -1068,10 +1079,14 @@ static ssize_t current_value_store(struct kobject *kobj,
 		if (ret)
 			goto out;
 
-		if (auth_str)
-			ret = tlmi_save_bios_settings(auth_str);
-		else
-			ret = tlmi_save_bios_settings("");
+		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
+			tlmi_priv.save_required = true;
+		} else {
+			if (auth_str)
+				ret = tlmi_save_bios_settings(auth_str);
+			else
+				ret = tlmi_save_bios_settings("");
+		}
 	}
 	if (!ret && !tlmi_priv.pending_changes) {
 		tlmi_priv.pending_changes = true;
@@ -1152,6 +1167,107 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
 
 static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
 
+static const char * const save_mode_strings[] = {
+	[TLMI_SAVE_SINGLE] = "single",
+	[TLMI_SAVE_BULK] = "bulk",
+	[TLMI_SAVE_SAVE] = "save"
+};
+
+static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
+				  char *buf)
+{
+	/* Check that setting is valid */
+	if (WARN_ON(tlmi_priv.save_mode < TLMI_SAVE_SINGLE ||
+		    tlmi_priv.save_mode > TLMI_SAVE_BULK))
+		return -EIO;
+	return sysfs_emit(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
+}
+
+static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
+				   const char *buf, size_t count)
+{
+	char *auth_str = NULL;
+	int ret = 0;
+	int cmd;
+
+	cmd = sysfs_match_string(save_mode_strings, buf);
+	if (cmd < 0)
+		return cmd;
+
+	/* Use lock in case multiple WMI operations needed */
+	mutex_lock(&tlmi_mutex);
+
+	switch (cmd) {
+	case TLMI_SAVE_SINGLE:
+	case TLMI_SAVE_BULK:
+		tlmi_priv.save_mode = cmd;
+		goto out;
+	case TLMI_SAVE_SAVE:
+		/* Check if supported*/
+		if (!tlmi_priv.can_set_bios_settings ||
+		    tlmi_priv.save_mode == TLMI_SAVE_SINGLE) {
+			ret = -EOPNOTSUPP;
+			goto out;
+		}
+		/* Check there is actually something to save */
+		if (!tlmi_priv.save_required) {
+			ret = -ENOENT;
+			goto out;
+		}
+		/* Check if certificate authentication is enabled and active */
+		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
+			if (!tlmi_priv.pwd_admin->signature ||
+			    !tlmi_priv.pwd_admin->save_signature) {
+				ret = -EINVAL;
+				goto out;
+			}
+			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
+					       tlmi_priv.pwd_admin->save_signature);
+			if (ret)
+				goto out;
+		} else if (tlmi_priv.opcode_support) {
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
+							  tlmi_priv.pwd_admin->password);
+				if (ret)
+					goto out;
+			}
+			ret = tlmi_save_bios_settings("");
+		} else { /* old non-opcode based authentication method (deprecated) */
+			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
+				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
+						     tlmi_priv.pwd_admin->password,
+						     encoding_options[tlmi_priv.pwd_admin->encoding],
+						     tlmi_priv.pwd_admin->kbdlang);
+				if (!auth_str) {
+					ret = -ENOMEM;
+					goto out;
+				}
+			}
+
+			if (auth_str)
+				ret = tlmi_save_bios_settings(auth_str);
+			else
+				ret = tlmi_save_bios_settings("");
+		}
+		tlmi_priv.save_required = false;
+		tlmi_priv.reboot_required = true;
+
+		if (!ret && !tlmi_priv.pending_changes) {
+			tlmi_priv.pending_changes = true;
+			/* let userland know it may need to check reboot pending again */
+			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
+		}
+		break;
+	}
+out:
+	mutex_unlock(&tlmi_mutex);
+	kfree(auth_str);
+	return ret ?: count;
+}
+
+static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
+
 /* ---- Debug interface--------------------------------------------------------- */
 static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
 				const char *buf, size_t count)
@@ -1221,6 +1337,8 @@ static void tlmi_release_attr(void)
 		}
 	}
 	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
+	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
+
 	if (tlmi_priv.can_debug_cmd && debug_support)
 		sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
 
@@ -1302,6 +1420,10 @@ static int tlmi_sysfs_init(void)
 	if (ret)
 		goto fail_create_attr;
 
+	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
+	if (ret)
+		goto fail_create_attr;
+
 	if (tlmi_priv.can_debug_cmd && debug_support) {
 		ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
 		if (ret)
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index 4daba6151cd6..e1975ffebeb4 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -27,6 +27,19 @@ enum level_option {
 	TLMI_LEVEL_MASTER,
 };
 
+/*
+ * There are a limit on the number of WMI operations you can do if you use
+ * the default implementation of saving on every set. This is due to a
+ * limitation in EFI variable space used.
+ * Have a 'bulk save' mode where you can manually trigger the save, and can
+ * therefore set unlimited variables - for users that need it.
+ */
+enum save_mode {
+	TLMI_SAVE_SINGLE,
+	TLMI_SAVE_BULK,
+	TLMI_SAVE_SAVE,
+};
+
 /* password configuration details */
 struct tlmi_pwdcfg_core {
 	uint32_t password_mode;
@@ -86,6 +99,9 @@ struct think_lmi {
 	bool can_debug_cmd;
 	bool opcode_support;
 	bool certificate_support;
+	enum save_mode save_mode;
+	bool save_required;
+	bool reboot_required;
 
 	struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
 	struct device *class_dev;
-- 
2.41.0

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

[Hans de Goede]

Hi Mark,

On 9/19/23 16:15, Mark Pearson wrote:
> On Lenovo platforms there is a limitation in the number of times an
> attribute can be saved. This is an architectural limitation and it limits
> the number of attributes that can be modified to 48.
> A solution for this is instead of the attribute being saved after every
> modification allow a user to bulk set the attributes and then trigger a
> final save. This allows unlimited attributes.
> 
> This patch introduces a save_settings attribute that can be configured to
> either single or bulk mode by the user.
> Single mode is the default but customers who want to avoid the 48
> attribute limit can enable bulk mode.
> 
> Displaying the save_settings attribute will display the enabled mode.
> 
> When in bulk mode writing 'save' to the save_settings attribute will
> trigger a save. Once this has been done a reboot is required before more
> attributes can be modified.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
> Changes in v2:
>  - Correct kernel version in documentation
>  - Updated to sysfs_emit
>  - Clean up code in save_settings_store as recommended
>  - Use correct comment formatting

Thank you for the new version, but next time when a patch has
already been merged please send any fixes as a follow-up /
separate patch with just the fixes.

I've replaced the original patch with this one now doing
a forced push to both my review-hans and the for-next
branch.

Regards,

Hans





> 
>  .../testing/sysfs-class-firmware-attributes   |  30 ++++
>  drivers/platform/x86/think-lmi.c              | 152 ++++++++++++++++--
>  drivers/platform/x86/think-lmi.h              |  16 ++
>  3 files changed, 183 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index f205d39409a3..9c82c7b42ff8 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -383,6 +383,36 @@ Description:
>  		Note that any changes to this attribute requires a reboot
>  		for changes to take effect.
>  
> +What:		/sys/class/firmware-attributes/*/attributes/save_settings
> +Date:		August 2023
> +KernelVersion:	6.6
> +Contact:	Mark Pearson <mpearson-lenovo@squebb.ca>
> +Description:
> +		On Lenovo platforms there is a limitation in the number of times an attribute can be
> +		saved. This is an architectural limitation and it limits the number of attributes
> +		that can be modified to 48.
> +		A solution for this is instead of the attribute being saved after every modification,
> +		to allow a user to bulk set the attributes, and then trigger a final save. This allows
> +		unlimited attributes.
> +
> +		Read the attribute to check what save mode is enabled (single or bulk).
> +		E.g:
> +		# cat /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +		single
> +
> +		Write the attribute with 'bulk' to enable bulk save mode.
> +		Write the attribute with 'single' to enable saving, after every attribute set.
> +		The default setting is single mode.
> +		E.g:
> +		# echo bulk > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +
> +		When in bulk mode write 'save' to trigger a save of all currently modified attributes.
> +		Note, once a save has been triggered, in bulk mode, attributes can no longer be set and
> +		will return a permissions error. This is to prevent users hitting the 48+ save limitation
> +		(which requires entering the BIOS to clear the error condition)
> +		E.g:
> +		# echo save > /sys/class/firmware-attributes/thinklmi/attributes/save_settings
> +
>  What:		/sys/class/firmware-attributes/*/attributes/debug_cmd
>  Date:		July 2021
>  KernelVersion:	5.14
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 52d1ce8dfe44..6f9fa80b19fc 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -985,6 +985,13 @@ static ssize_t current_value_store(struct kobject *kobj,
>  	if (!tlmi_priv.can_set_bios_settings)
>  		return -EOPNOTSUPP;
>  
> +	/*
> +	 * If we are using bulk saves a reboot should be done once save has
> +	 * been called
> +	 */
> +	if (tlmi_priv.save_mode == TLMI_SAVE_BULK && tlmi_priv.reboot_required)
> +		return -EPERM;
> +
>  	new_setting = kstrdup(buf, GFP_KERNEL);
>  	if (!new_setting)
>  		return -ENOMEM;
> @@ -1011,10 +1018,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
>  		if (ret)
>  			goto out;
> -		ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> -				tlmi_priv.pwd_admin->save_signature);
> -		if (ret)
> -			goto out;
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
> +			tlmi_priv.save_required = true;
> +		else
> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> +					       tlmi_priv.pwd_admin->save_signature);
>  	} else if (tlmi_priv.opcode_support) {
>  		/*
>  		 * If opcode support is present use that interface.
> @@ -1033,14 +1041,17 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		if (ret)
>  			goto out;
>  
> -		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> -			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> -						  tlmi_priv.pwd_admin->password);
> -			if (ret)
> -				goto out;
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> +			tlmi_priv.save_required = true;
> +		} else {
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> +							  tlmi_priv.pwd_admin->password);
> +				if (ret)
> +					goto out;
> +			}
> +			ret = tlmi_save_bios_settings("");
>  		}
> -
> -		ret = tlmi_save_bios_settings("");
>  	} else { /* old non-opcode based authentication method (deprecated) */
>  		if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> @@ -1068,10 +1079,14 @@ static ssize_t current_value_store(struct kobject *kobj,
>  		if (ret)
>  			goto out;
>  
> -		if (auth_str)
> -			ret = tlmi_save_bios_settings(auth_str);
> -		else
> -			ret = tlmi_save_bios_settings("");
> +		if (tlmi_priv.save_mode == TLMI_SAVE_BULK) {
> +			tlmi_priv.save_required = true;
> +		} else {
> +			if (auth_str)
> +				ret = tlmi_save_bios_settings(auth_str);
> +			else
> +				ret = tlmi_save_bios_settings("");
> +		}
>  	}
>  	if (!ret && !tlmi_priv.pending_changes) {
>  		tlmi_priv.pending_changes = true;
> @@ -1152,6 +1167,107 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *
>  
>  static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
>  
> +static const char * const save_mode_strings[] = {
> +	[TLMI_SAVE_SINGLE] = "single",
> +	[TLMI_SAVE_BULK] = "bulk",
> +	[TLMI_SAVE_SAVE] = "save"
> +};
> +
> +static ssize_t save_settings_show(struct kobject *kobj, struct kobj_attribute *attr,
> +				  char *buf)
> +{
> +	/* Check that setting is valid */
> +	if (WARN_ON(tlmi_priv.save_mode < TLMI_SAVE_SINGLE ||
> +		    tlmi_priv.save_mode > TLMI_SAVE_BULK))
> +		return -EIO;
> +	return sysfs_emit(buf, "%s\n", save_mode_strings[tlmi_priv.save_mode]);
> +}
> +
> +static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	char *auth_str = NULL;
> +	int ret = 0;
> +	int cmd;
> +
> +	cmd = sysfs_match_string(save_mode_strings, buf);
> +	if (cmd < 0)
> +		return cmd;
> +
> +	/* Use lock in case multiple WMI operations needed */
> +	mutex_lock(&tlmi_mutex);
> +
> +	switch (cmd) {
> +	case TLMI_SAVE_SINGLE:
> +	case TLMI_SAVE_BULK:
> +		tlmi_priv.save_mode = cmd;
> +		goto out;
> +	case TLMI_SAVE_SAVE:
> +		/* Check if supported*/
> +		if (!tlmi_priv.can_set_bios_settings ||
> +		    tlmi_priv.save_mode == TLMI_SAVE_SINGLE) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +		/* Check there is actually something to save */
> +		if (!tlmi_priv.save_required) {
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +		/* Check if certificate authentication is enabled and active */
> +		if (tlmi_priv.certificate_support && tlmi_priv.pwd_admin->cert_installed) {
> +			if (!tlmi_priv.pwd_admin->signature ||
> +			    !tlmi_priv.pwd_admin->save_signature) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> +					       tlmi_priv.pwd_admin->save_signature);
> +			if (ret)
> +				goto out;
> +		} else if (tlmi_priv.opcode_support) {
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> +							  tlmi_priv.pwd_admin->password);
> +				if (ret)
> +					goto out;
> +			}
> +			ret = tlmi_save_bios_settings("");
> +		} else { /* old non-opcode based authentication method (deprecated) */
> +			if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) {
> +				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> +						     tlmi_priv.pwd_admin->password,
> +						     encoding_options[tlmi_priv.pwd_admin->encoding],
> +						     tlmi_priv.pwd_admin->kbdlang);
> +				if (!auth_str) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +			}
> +
> +			if (auth_str)
> +				ret = tlmi_save_bios_settings(auth_str);
> +			else
> +				ret = tlmi_save_bios_settings("");
> +		}
> +		tlmi_priv.save_required = false;
> +		tlmi_priv.reboot_required = true;
> +
> +		if (!ret && !tlmi_priv.pending_changes) {
> +			tlmi_priv.pending_changes = true;
> +			/* let userland know it may need to check reboot pending again */
> +			kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE);
> +		}
> +		break;
> +	}
> +out:
> +	mutex_unlock(&tlmi_mutex);
> +	kfree(auth_str);
> +	return ret ?: count;
> +}
> +
> +static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
> +
>  /* ---- Debug interface--------------------------------------------------------- */
>  static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr,
>  				const char *buf, size_t count)
> @@ -1221,6 +1337,8 @@ static void tlmi_release_attr(void)
>  		}
>  	}
>  	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> +	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
> +
>  	if (tlmi_priv.can_debug_cmd && debug_support)
>  		sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
>  
> @@ -1302,6 +1420,10 @@ static int tlmi_sysfs_init(void)
>  	if (ret)
>  		goto fail_create_attr;
>  
> +	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &save_settings.attr);
> +	if (ret)
> +		goto fail_create_attr;
> +
>  	if (tlmi_priv.can_debug_cmd && debug_support) {
>  		ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
>  		if (ret)
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index 4daba6151cd6..e1975ffebeb4 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -27,6 +27,19 @@ enum level_option {
>  	TLMI_LEVEL_MASTER,
>  };
>  
> +/*
> + * There are a limit on the number of WMI operations you can do if you use
> + * the default implementation of saving on every set. This is due to a
> + * limitation in EFI variable space used.
> + * Have a 'bulk save' mode where you can manually trigger the save, and can
> + * therefore set unlimited variables - for users that need it.
> + */
> +enum save_mode {
> +	TLMI_SAVE_SINGLE,
> +	TLMI_SAVE_BULK,
> +	TLMI_SAVE_SAVE,
> +};
> +
>  /* password configuration details */
>  struct tlmi_pwdcfg_core {
>  	uint32_t password_mode;
> @@ -86,6 +99,9 @@ struct think_lmi {
>  	bool can_debug_cmd;
>  	bool opcode_support;
>  	bool certificate_support;
> +	enum save_mode save_mode;
> +	bool save_required;
> +	bool reboot_required;
>  
>  	struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
>  	struct device *class_dev;

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

[Mark Pearson]

On Thu, Sep 21, 2023, at 12:38 PM, Hans de Goede wrote:
> Hi Mark,
>
> On 9/19/23 16:15, Mark Pearson wrote:
>> On Lenovo platforms there is a limitation in the number of times an
>> attribute can be saved. This is an architectural limitation and it limits
>> the number of attributes that can be modified to 48.
>> A solution for this is instead of the attribute being saved after every
>> modification allow a user to bulk set the attributes and then trigger a
>> final save. This allows unlimited attributes.
>> 
>> This patch introduces a save_settings attribute that can be configured to
>> either single or bulk mode by the user.
>> Single mode is the default but customers who want to avoid the 48
>> attribute limit can enable bulk mode.
>> 
>> Displaying the save_settings attribute will display the enabled mode.
>> 
>> When in bulk mode writing 'save' to the save_settings attribute will
>> trigger a save. Once this has been done a reboot is required before more
>> attributes can be modified.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>> Changes in v2:
>>  - Correct kernel version in documentation
>>  - Updated to sysfs_emit
>>  - Clean up code in save_settings_store as recommended
>>  - Use correct comment formatting
>
> Thank you for the new version, but next time when a patch has
> already been merged please send any fixes as a follow-up /
> separate patch with just the fixes.
>
> I've replaced the original patch with this one now doing
> a forced push to both my review-hans and the for-next
> branch.
>
Oh - my apologies, I didn't think of that. Thanks for cleaning up for me.
Mark