| Message ID | 20230526171658.3886-5-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3,1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings | expand |
On Fri, 26 May 2023, Mark Pearson wrote: > Add mutex protection around cases where an operation needs multiple > WMI calls - e.g. setting password. So you need this feature already for Patch 1/5? If that's the case, you should reorder the patches and put it before 1/5. That "e.g. setting password" sounds vague enough that I'm left to wonder if there are other cases in the driver which need locking too. It would be useful to be precise with wording here. It will help immensely when somebody looks this changelog 5 years from now if you explain all cases that need locking up front. So, is this needed also for some existing code, then Fixes tag might be in order? (I'm looking e.g. that cert auth block in current_value_store() which also does more than one call).
Hi Ilpo, On Mon, May 29, 2023, at 8:23 AM, Ilpo Järvinen wrote: > On Fri, 26 May 2023, Mark Pearson wrote: > >> Add mutex protection around cases where an operation needs multiple >> WMI calls - e.g. setting password. > > So you need this feature already for Patch 1/5? If that's the case, you > should reorder the patches and put it before 1/5. You're right....I was being lazy and just adding it to the end of the series as it was easier. I can re-order. As a side note, the chances of two people changing things on a system at the same time is rather unlikely - it just doesn't make sense as it's something done by an administrator. But a fix is a fix. > > That "e.g. setting password" sounds vague enough that I'm left to wonder > if there are other cases in the driver which need locking too. It would be > useful to be precise with wording here. It will help immensely when > somebody looks this changelog 5 years from now if you explain all cases > that need locking up front. OK. There are two cases and I can list both cases explicitly. > > So, is this needed also for some existing code, then Fixes tag might be in > order? (I'm looking e.g. that cert auth block in current_value_store() > which also does more than one call). True - I can add that. Thanks for the review. I'll hold off a couple of days before making those changes in case there is any feedback Mark
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 64cd453d6e7d..86185358dba2 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -14,6 +14,7 @@ #include <linux/acpi.h> #include <linux/errno.h> #include <linux/fs.h> +#include <linux/mutex.h> #include <linux/string.h> #include <linux/types.h> #include <linux/dmi.h> @@ -195,6 +196,7 @@ static const char * const level_options[] = { }; static struct think_lmi tlmi_priv; static struct class *fw_attr_class; +static DEFINE_MUTEX(tlmi_mutex); /* ------ Utility functions ------------*/ /* Strip out CR if one is present */ @@ -437,6 +439,9 @@ static ssize_t new_password_store(struct kobject *kobj, /* Strip out CR if one is present, setting password won't work if it is present */ strip_cr(new_pwd); + /* Use lock in case multiple WMI operations needed */ + mutex_lock(&tlmi_mutex); + pwdlen = strlen(new_pwd); /* pwdlen == 0 is allowed to clear the password */ if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) { @@ -493,6 +498,7 @@ static ssize_t new_password_store(struct kobject *kobj, kfree(auth_str); } out: + mutex_unlock(&tlmi_mutex); kfree(new_pwd); return ret ?: count; } @@ -987,6 +993,9 @@ static ssize_t current_value_store(struct kobject *kobj, /* Strip out CR if one is present */ strip_cr(new_setting); + /* Use lock in case multiple WMI operations needed */ + mutex_lock(&tlmi_mutex); + /* 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) { @@ -1031,7 +1040,6 @@ static ssize_t current_value_store(struct kobject *kobj, 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]) { @@ -1071,6 +1079,7 @@ static ssize_t current_value_store(struct kobject *kobj, kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE); } out: + mutex_unlock(&tlmi_mutex); kfree(auth_str); kfree(set_str); kfree(new_setting);
Add mutex protection around cases where an operation needs multiple WMI calls - e.g. setting password. Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- Changes in v2: New commit added after review of other patches in series. Changes in v3: Simplified mutex handling as recommended. drivers/platform/x86/think-lmi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)