Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Mon, 18 Sep 2023 16:57:58 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Mark Pearson <mpearson-lenovo@squebb.ca>
Cc:     hdegoede@redhat.com, markgross@kernel.org,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86: think-lmi: Add bulk save feature
Message-ID: <ZQhXZkBHviGOEl-x@smile.fi.intel.com>
References: <mpearson-lenovo@squebb.ca>
 <20230906121328.50437-1-mpearson-lenovo@squebb.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230906121328.50437-1-mpearson-lenovo@squebb.ca>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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