Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Feedback-ID: ibe194615:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-761-gece9e40c48-fm-20230913.001-gece9e40c
MIME-Version: 1.0
Message-Id: <bf511687-8b29-42e2-92df-fcbf763cd3a2@app.fastmail.com>
In-Reply-To: <878da778-e183-6ef5-61ce-3a3d059f1a1e@redhat.com>
References: <mpearson-lenovo@squebb.ca>
 <20230906121328.50437-1-mpearson-lenovo@squebb.ca>
 <ZQhXZkBHviGOEl-x@smile.fi.intel.com>
 <878da778-e183-6ef5-61ce-3a3d059f1a1e@redhat.com>
Date:   Mon, 18 Sep 2023 11:23:16 -0400
From:   "Mark Pearson" <mpearson-lenovo@squebb.ca>
To:     "Hans de Goede" <hdegoede@redhat.com>,
        "Andy Shevchenko" <andriy.shevchenko@intel.com>
Cc:     "markgross@kernel.org" <markgross@kernel.org>,
        "platform-driver-x86@vger.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
Content-Type: text/plain
Precedence: bulk

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