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

From: Hans de Goede
Date: Thu Sep 21 2023 - 18:27:43 EST

Next message: Hans de Goede: "Re: [PATCH 1/1] MAINTAINERS: Add x86 platform drivers patchwork"
Previous message: Suraj Jitindar Singh: "Re: [PATCH stable 6.1.y 2/2] KVM: arm64: Prevent unconditional donation of unmapped regions from the host"
In reply to: Mark Pearson: "[PATCH v2] platform/x86: think-lmi: Add bulk save feature"
Next in thread: Mark Pearson: "Re: [PATCH v2] platform/x86: think-lmi: Add bulk save feature"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> +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;

Next message: Hans de Goede: "Re: [PATCH 1/1] MAINTAINERS: Add x86 platform drivers patchwork"
Previous message: Suraj Jitindar Singh: "Re: [PATCH stable 6.1.y 2/2] KVM: arm64: Prevent unconditional donation of unmapped regions from the host"
In reply to: Mark Pearson: "[PATCH v2] platform/x86: think-lmi: Add bulk save feature"
Next in thread: Mark Pearson: "Re: [PATCH v2] platform/x86: think-lmi: Add bulk save feature"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]