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
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
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