Whilst reviewing some documentation from the FW team on using WMI on
Lenovo system I noticed that we weren't using Opcode support when
changing BIOS settings in the thinkLMI driver.
We should be doing this to ensure we're future proof as the old
non-opcode mechanism has been deprecated.
Tested on X1 Carbon G10 and G11.
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in V2: Update comment for clearer explanation of what the driver
is doing
drivers/platform/x86/think-lmi.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 1138f770149d..2745224f62ab 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1001,7 +1001,33 @@ static ssize_t current_value_store(struct kobject *kobj,
tlmi_priv.pwd_admin->save_signature);
if (ret)
goto out;
- } else { /* Non certiifcate based authentication */
+ } else if (tlmi_priv.opcode_support) {
+ /*
+ * If opcode support is present use that interface.
+ * Note - this sets the variable and then the password as separate
+ * WMI calls. Function tlmi_save_bios_settings will error if the
+ * password is incorrect.
+ */
+ set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
+ new_setting);
+ if (!set_str) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
+ 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;
+ }
+
+ 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,
--
2.40.1
Add mutex protection around cases where an operation needs multiple
WMI calls - e.g. setting password.
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in V2: New commit added after review of other patches in series.
drivers/platform/x86/think-lmi.c | 46 ++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 64cd453d6e7d..f3e1e4dacba2 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 */
@@ -463,23 +465,32 @@ static ssize_t new_password_store(struct kobject *kobj,
sprintf(pwd_type, "%s", setting->pwd_type);
}
+ mutex_lock(&tlmi_mutex);
ret = tlmi_opcode_setting("WmiOpcodePasswordType", pwd_type);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
-
+ }
if (tlmi_priv.pwd_admin->valid) {
ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
tlmi_priv.pwd_admin->password);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
+ }
}
ret = tlmi_opcode_setting("WmiOpcodePasswordCurrent01", setting->password);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
+ }
ret = tlmi_opcode_setting("WmiOpcodePasswordNew01", new_pwd);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
+ }
ret = tlmi_simple_call(LENOVO_OPCODE_IF_GUID, "WmiOpcodePasswordSetUpdate;");
+ mutex_unlock(&tlmi_mutex);
} else {
/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
@@ -1000,11 +1011,16 @@ static ssize_t current_value_store(struct kobject *kobj,
goto out;
}
+ mutex_lock(&tlmi_mutex);
ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
+ }
ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
tlmi_priv.pwd_admin->save_signature);
+
+ mutex_unlock(&tlmi_mutex);
if (ret)
goto out;
} else if (tlmi_priv.opcode_support) {
@@ -1021,18 +1037,23 @@ static ssize_t current_value_store(struct kobject *kobj,
goto out;
}
+ mutex_lock(&tlmi_mutex);
ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
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)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
+ }
}
-
ret = tlmi_save_bios_settings("");
+ mutex_unlock(&tlmi_mutex);
} 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;",
@@ -1056,14 +1077,17 @@ static ssize_t current_value_store(struct kobject *kobj,
goto out;
}
+ mutex_lock(&tlmi_mutex);
ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
- if (ret)
+ if (ret) {
+ mutex_unlock(&tlmi_mutex);
goto out;
-
+ }
if (auth_str)
ret = tlmi_save_bios_settings(auth_str);
else
ret = tlmi_save_bios_settings("");
+ mutex_unlock(&tlmi_mutex);
}
if (!ret && !tlmi_priv.pending_changes) {
tlmi_priv.pending_changes = true;
--
2.40.1
If Opcode support is available (which is the standard for all platforms
going forward) then there is no need to have the encoding and kbdlang
attributes visible
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in V2: None. Version bumped in series
drivers/platform/x86/think-lmi.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 1c02958035ad..64cd453d6e7d 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -879,6 +879,12 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
return 0;
}
+ /* Don't display un-needed settings if opcode available */
+ if ((attr == &auth_encoding.attr ||
+ attr == &auth_kbdlang.attr) &&
+ tlmi_priv.opcode_support)
+ return 0;
+
return attr->mode;
}
--
2.40.1
Hi Mark,
On 5/25/23 21:31, Mark Pearson wrote:
> Add mutex protection around cases where an operation needs multiple
> WMI calls - e.g. setting password.
>
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> Changes in V2: New commit added after review of other patches in series.
>
> drivers/platform/x86/think-lmi.c | 46 ++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 64cd453d6e7d..f3e1e4dacba2 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 */
> @@ -463,23 +465,32 @@ static ssize_t new_password_store(struct kobject *kobj,
> sprintf(pwd_type, "%s", setting->pwd_type);
> }
>
> + mutex_lock(&tlmi_mutex);
> ret = tlmi_opcode_setting("WmiOpcodePasswordType", pwd_type);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> -
> + }
> if (tlmi_priv.pwd_admin->valid) {
> ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> tlmi_priv.pwd_admin->password);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> + }
> }
> ret = tlmi_opcode_setting("WmiOpcodePasswordCurrent01", setting->password);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> + }
> ret = tlmi_opcode_setting("WmiOpcodePasswordNew01", new_pwd);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> + }
> ret = tlmi_simple_call(LENOVO_OPCODE_IF_GUID, "WmiOpcodePasswordSetUpdate;");
> + mutex_unlock(&tlmi_mutex);
> } else {
> /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
I haven't take a really close / good look yet. But at a first glance
I think it would be cleaner to just take the mutex at the top
and unlock it after the out label to which all the existing goto-s
already go ?
> @@ -1000,11 +1011,16 @@ static ssize_t current_value_store(struct kobject *kobj,
> goto out;
> }
>
> + mutex_lock(&tlmi_mutex);
> ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> + }
> ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
> tlmi_priv.pwd_admin->save_signature);
> +
> + mutex_unlock(&tlmi_mutex);
> if (ret)
> goto out;
> } else if (tlmi_priv.opcode_support) {
> @@ -1021,18 +1037,23 @@ static ssize_t current_value_store(struct kobject *kobj,
> goto out;
> }
>
> + mutex_lock(&tlmi_mutex);
> ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> 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)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> + }
> }
> -
> ret = tlmi_save_bios_settings("");
> + mutex_unlock(&tlmi_mutex);
> } 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;",
> @@ -1056,14 +1077,17 @@ static ssize_t current_value_store(struct kobject *kobj,
> goto out;
> }
>
> + mutex_lock(&tlmi_mutex);
> ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&tlmi_mutex);
> goto out;
> -
> + }
> if (auth_str)
> ret = tlmi_save_bios_settings(auth_str);
> else
> ret = tlmi_save_bios_settings("");
> + mutex_unlock(&tlmi_mutex);
> }
> if (!ret && !tlmi_priv.pending_changes) {
> tlmi_priv.pending_changes = true;
And the same here.
Regards,
Hans
NVME passwords identifier have been standardised across the Lenovo
systems and now use udrp and adrp (user and admin level) instead of
unvp and mnvp.
This should apparently be backwards compatible.
Also cleaned up so the index is set to a default of 1 rather than 0
as this just makes more sense (there is no device 0).
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in V2: None. Version bumped in series
drivers/platform/x86/think-lmi.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c7e98fbe7c3d..1c02958035ad 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -456,9 +456,9 @@ static ssize_t new_password_store(struct kobject *kobj,
sprintf(pwd_type, "mhdp%d", setting->index);
} else if (setting == tlmi_priv.pwd_nvme) {
if (setting->level == TLMI_LEVEL_USER)
- sprintf(pwd_type, "unvp%d", setting->index);
+ sprintf(pwd_type, "udrp%d", setting->index);
else
- sprintf(pwd_type, "mnvp%d", setting->index);
+ sprintf(pwd_type, "adrp%d", setting->index);
} else {
sprintf(pwd_type, "%s", setting->pwd_type);
}
@@ -1524,6 +1524,10 @@ static int tlmi_analyze(void)
if (!tlmi_priv.pwd_nvme)
goto fail_clear_attr;
+ /* Set default hdd/nvme index to 1 as there is no device 0 */
+ tlmi_priv.pwd_hdd->index = 1;
+ tlmi_priv.pwd_nvme->index = 1;
+
if (tlmi_priv.pwdcfg.core.password_state & TLMI_HDD_PWD) {
/* Check if PWD is configured and set index to first drive found */
if (tlmi_priv.pwdcfg.ext.hdd_user_password ||
--
2.40.1
Hi,
On 5/25/23 21:39, Mark Pearson wrote:
> Hi Hans,
>
> My apologies - I completely forgot to add the v2 tag on this patch series...had a complete brain fart.
No worries I did not even notice the v2 was missing,
my brain automatically added it :)
> I assume I should resend them all - correctly named.
Well I have some review remarks on patch 5/5, it would be good if you can address those (if you agree with them) and then lets jump straight to v3 to avoid confusion ?
> I also have no idea why they showed up out of order...I'm blaming that one on the email server.
The out of order thing is normal, this sometimes happens with email.
> Sorry :(
Again no worries / no problem ...
Regards,
Hans
> On Thu, May 25, 2023, at 3:31 PM, Mark Pearson wrote:
>> NVME passwords identifier have been standardised across the Lenovo
>> systems and now use udrp and adrp (user and admin level) instead of
>> unvp and mnvp.
>>
>> This should apparently be backwards compatible.
>>
>> Also cleaned up so the index is set to a default of 1 rather than 0
>> as this just makes more sense (there is no device 0).
>>
>> Signed-off-by: Mark Pearson <[email protected]>
>> ---
>> Changes in V2: None. Version bumped in series
>>
>> drivers/platform/x86/think-lmi.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index c7e98fbe7c3d..1c02958035ad 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -456,9 +456,9 @@ static ssize_t new_password_store(struct kobject *kobj,
>> sprintf(pwd_type, "mhdp%d", setting->index);
>> } else if (setting == tlmi_priv.pwd_nvme) {
>> if (setting->level == TLMI_LEVEL_USER)
>> - sprintf(pwd_type, "unvp%d", setting->index);
>> + sprintf(pwd_type, "udrp%d", setting->index);
>> else
>> - sprintf(pwd_type, "mnvp%d", setting->index);
>> + sprintf(pwd_type, "adrp%d", setting->index);
>> } else {
>> sprintf(pwd_type, "%s", setting->pwd_type);
>> }
>> @@ -1524,6 +1524,10 @@ static int tlmi_analyze(void)
>> if (!tlmi_priv.pwd_nvme)
>> goto fail_clear_attr;
>>
>> + /* Set default hdd/nvme index to 1 as there is no device 0 */
>> + tlmi_priv.pwd_hdd->index = 1;
>> + tlmi_priv.pwd_nvme->index = 1;
>> +
>> if (tlmi_priv.pwdcfg.core.password_state & TLMI_HDD_PWD) {
>> /* Check if PWD is configured and set index to first drive found */
>> if (tlmi_priv.pwdcfg.ext.hdd_user_password ||
>> --
>> 2.40.1
>
The system password identification was incorrect. This means that if
the password was enabled it wouldn't be detected correctly; and setting
it would not work.
Also updated code to use TLMI_SMP_PWD instead of TLMI_SYS_PWD to be in
sync with Lenovo documentation.
Correct these mistakes.
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in V2:
- Updated define name to be SMP_PWD instead of SYS_PWD
- Clarified in comments what each password type is.
drivers/platform/x86/think-lmi.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 2745224f62ab..c7e98fbe7c3d 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -168,11 +168,11 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
*/
#define LENOVO_CERT_THUMBPRINT_GUID "C59119ED-1C0D-4806-A8E9-59AA318176C4"
-#define TLMI_POP_PWD (1 << 0)
-#define TLMI_PAP_PWD (1 << 1)
-#define TLMI_HDD_PWD (1 << 2)
-#define TLMI_SYS_PWD (1 << 3)
-#define TLMI_CERT (1 << 7)
+#define TLMI_POP_PWD (1 << 0) /* Supervisor */
+#define TLMI_PAP_PWD (1 << 1) /* Power-on */
+#define TLMI_HDD_PWD (1 << 2) /* HDD/NVME */
+#define TLMI_SMP_PWD (1 << 6) /* System Management */
+#define TLMI_CERT (1 << 7) /* Certificate Based */
#define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj)
#define to_tlmi_attr_setting(kobj) container_of(kobj, struct tlmi_attr_setting, kobj)
@@ -1509,11 +1509,11 @@ static int tlmi_analyze(void)
tlmi_priv.pwd_power->valid = true;
if (tlmi_priv.opcode_support) {
- tlmi_priv.pwd_system = tlmi_create_auth("sys", "system");
+ tlmi_priv.pwd_system = tlmi_create_auth("smp", "system");
if (!tlmi_priv.pwd_system)
goto fail_clear_attr;
- if (tlmi_priv.pwdcfg.core.password_state & TLMI_SYS_PWD)
+ if (tlmi_priv.pwdcfg.core.password_state & TLMI_SMP_PWD)
tlmi_priv.pwd_system->valid = true;
tlmi_priv.pwd_hdd = tlmi_create_auth("hdd", "hdd");
--
2.40.1
On Thu, May 25, 2023, at 3:41 PM, Hans de Goede wrote:
> Hi Mark,
>
> On 5/25/23 21:31, Mark Pearson wrote:
>> Add mutex protection around cases where an operation needs multiple
>> WMI calls - e.g. setting password.
>>
>> Signed-off-by: Mark Pearson <[email protected]>
>> ---
>> Changes in V2: New commit added after review of other patches in series.
>>
>> drivers/platform/x86/think-lmi.c | 46 ++++++++++++++++++++++++--------
>> 1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 64cd453d6e7d..f3e1e4dacba2 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 */
>> @@ -463,23 +465,32 @@ static ssize_t new_password_store(struct kobject *kobj,
>> sprintf(pwd_type, "%s", setting->pwd_type);
>> }
>>
>> + mutex_lock(&tlmi_mutex);
>> ret = tlmi_opcode_setting("WmiOpcodePasswordType", pwd_type);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&tlmi_mutex);
>> goto out;
>> -
>> + }
>> if (tlmi_priv.pwd_admin->valid) {
>> ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>> tlmi_priv.pwd_admin->password);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&tlmi_mutex);
>> goto out;
>> + }
>> }
>> ret = tlmi_opcode_setting("WmiOpcodePasswordCurrent01", setting->password);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&tlmi_mutex);
>> goto out;
>> + }
>> ret = tlmi_opcode_setting("WmiOpcodePasswordNew01", new_pwd);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&tlmi_mutex);
>> goto out;
>> + }
>> ret = tlmi_simple_call(LENOVO_OPCODE_IF_GUID, "WmiOpcodePasswordSetUpdate;");
>> + mutex_unlock(&tlmi_mutex);
>> } else {
>> /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
>
>
> I haven't take a really close / good look yet. But at a first glance
> I think it would be cleaner to just take the mutex at the top
> and unlock it after the out label to which all the existing goto-s
> already go ?
>
I did consider that - and it was in my first implementation; but then I got concerned
about if the mutex_unlock could potentially get called without mutex_lock having been
called beforehand. I couldn't find any good reference as to whether that was safe or not.
I ended up deciding that a few extra brackets and unlock calls wasn't that ugly and was 'safer'...and
so went that route.
Happy to change it - but do you happen to know if it's safe to call unlock without a lock? If it is then
that implementation is cleaner.
Mark
Hi Hans,
My apologies - I completely forgot to add the v2 tag on this patch series...had a complete brain fart.
I assume I should resend them all - correctly named.
I also have no idea why they showed up out of order...I'm blaming that one on the email server.
Sorry :(
Mark
On Thu, May 25, 2023, at 3:31 PM, Mark Pearson wrote:
> NVME passwords identifier have been standardised across the Lenovo
> systems and now use udrp and adrp (user and admin level) instead of
> unvp and mnvp.
>
> This should apparently be backwards compatible.
>
> Also cleaned up so the index is set to a default of 1 rather than 0
> as this just makes more sense (there is no device 0).
>
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> Changes in V2: None. Version bumped in series
>
> drivers/platform/x86/think-lmi.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index c7e98fbe7c3d..1c02958035ad 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -456,9 +456,9 @@ static ssize_t new_password_store(struct kobject *kobj,
> sprintf(pwd_type, "mhdp%d", setting->index);
> } else if (setting == tlmi_priv.pwd_nvme) {
> if (setting->level == TLMI_LEVEL_USER)
> - sprintf(pwd_type, "unvp%d", setting->index);
> + sprintf(pwd_type, "udrp%d", setting->index);
> else
> - sprintf(pwd_type, "mnvp%d", setting->index);
> + sprintf(pwd_type, "adrp%d", setting->index);
> } else {
> sprintf(pwd_type, "%s", setting->pwd_type);
> }
> @@ -1524,6 +1524,10 @@ static int tlmi_analyze(void)
> if (!tlmi_priv.pwd_nvme)
> goto fail_clear_attr;
>
> + /* Set default hdd/nvme index to 1 as there is no device 0 */
> + tlmi_priv.pwd_hdd->index = 1;
> + tlmi_priv.pwd_nvme->index = 1;
> +
> if (tlmi_priv.pwdcfg.core.password_state & TLMI_HDD_PWD) {
> /* Check if PWD is configured and set index to first drive found */
> if (tlmi_priv.pwdcfg.ext.hdd_user_password ||
> --
> 2.40.1
On Thu, May 25, 2023, at 3:43 PM, Hans de Goede wrote:
> Hi,
>
> On 5/25/23 21:39, Mark Pearson wrote:
>> Hi Hans,
>>
>> My apologies - I completely forgot to add the v2 tag on this patch series...had a complete brain fart.
>
> No worries I did not even notice the v2 was missing,
> my brain automatically added it :)
>
>> I assume I should resend them all - correctly named.
>
> Well I have some review remarks on patch 5/5, it would be good if you
> can address those (if you agree with them) and then lets jump straight
> to v3 to avoid confusion ?
>
Works for me :)
Mark
Hi,
On 5/25/23 21:50, Mark Pearson wrote:
>
>
> On Thu, May 25, 2023, at 3:41 PM, Hans de Goede wrote:
>> Hi Mark,
>>
>> On 5/25/23 21:31, Mark Pearson wrote:
>>> Add mutex protection around cases where an operation needs multiple
>>> WMI calls - e.g. setting password.
>>>
>>> Signed-off-by: Mark Pearson <[email protected]>
>>> ---
>>> Changes in V2: New commit added after review of other patches in series.
>>>
>>> drivers/platform/x86/think-lmi.c | 46 ++++++++++++++++++++++++--------
>>> 1 file changed, 35 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index 64cd453d6e7d..f3e1e4dacba2 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 */
>>> @@ -463,23 +465,32 @@ static ssize_t new_password_store(struct kobject *kobj,
>>> sprintf(pwd_type, "%s", setting->pwd_type);
>>> }
>>>
>>> + mutex_lock(&tlmi_mutex);
>>> ret = tlmi_opcode_setting("WmiOpcodePasswordType", pwd_type);
>>> - if (ret)
>>> + if (ret) {
>>> + mutex_unlock(&tlmi_mutex);
>>> goto out;
>>> -
>>> + }
>>> if (tlmi_priv.pwd_admin->valid) {
>>> ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>>> tlmi_priv.pwd_admin->password);
>>> - if (ret)
>>> + if (ret) {
>>> + mutex_unlock(&tlmi_mutex);
>>> goto out;
>>> + }
>>> }
>>> ret = tlmi_opcode_setting("WmiOpcodePasswordCurrent01", setting->password);
>>> - if (ret)
>>> + if (ret) {
>>> + mutex_unlock(&tlmi_mutex);
>>> goto out;
>>> + }
>>> ret = tlmi_opcode_setting("WmiOpcodePasswordNew01", new_pwd);
>>> - if (ret)
>>> + if (ret) {
>>> + mutex_unlock(&tlmi_mutex);
>>> goto out;
>>> + }
>>> ret = tlmi_simple_call(LENOVO_OPCODE_IF_GUID, "WmiOpcodePasswordSetUpdate;");
>>> + mutex_unlock(&tlmi_mutex);
>>> } else {
>>> /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
>>
>>
>> I haven't take a really close / good look yet. But at a first glance
>> I think it would be cleaner to just take the mutex at the top
>> and unlock it after the out label to which all the existing goto-s
>> already go ?
>>
> I did consider that - and it was in my first implementation; but then I got concerned
> about if the mutex_unlock could potentially get called without mutex_lock having been
> called beforehand. I couldn't find any good reference as to whether that was safe or not.
>
> I ended up deciding that a few extra brackets and unlock calls wasn't that ugly and was 'safer'...and
> so went that route.
>
> Happy to change it - but do you happen to know if it's safe to call unlock without a lock? If it is then
> that implementation is cleaner.
It is not allowed to unlock without a lock. But if you put the lock directly after the malloc for which the out: does the free then there should be no goto out paths which don't have the lock.
E.g. for new_password_store() put it here:
new_pwd = kstrdup(buf, GFP_KERNEL);
if (!new_pwd)
return -ENOMEM;
mutex_lock(&tlmi_mutex);
/* Strip out CR if one is present, setting password won't work if it is present */
...
This does mean also taking the lock in the case where the new password store is done with a single WMI call, but that is not an issue. It makes things a tiny bit slower but WMI calls already are not fast and it is not like we are going to change the password / settings 100-times per second.
And the same thing can be done in current_value_store():
new_setting = kstrdup(buf, GFP_KERNEL);
if (!new_setting)
return -ENOMEM;
mutex_lock(&tlmi_mutex);
/* Strip out CR if one is present */
...
Regards,
Hans
On Fri, May 26, 2023, at 4:12 AM, Hans de Goede wrote:
> Hi,
>
> On 5/25/23 21:50, Mark Pearson wrote:
>>
>>
>> On Thu, May 25, 2023, at 3:41 PM, Hans de Goede wrote:
>>> Hi Mark,
>>>
>>> On 5/25/23 21:31, Mark Pearson wrote:
>>>> Add mutex protection around cases where an operation needs multiple
>>>> WMI calls - e.g. setting password.
>>>>
>>>> Signed-off-by: Mark Pearson <[email protected]>
>>>> ---
>>>> Changes in V2: New commit added after review of other patches in series.
>>>>
>>>> drivers/platform/x86/think-lmi.c | 46 ++++++++++++++++++++++++--------
>>>> 1 file changed, 35 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>>> index 64cd453d6e7d..f3e1e4dacba2 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 */
>>>> @@ -463,23 +465,32 @@ static ssize_t new_password_store(struct kobject *kobj,
>>>> sprintf(pwd_type, "%s", setting->pwd_type);
>>>> }
>>>>
>>>> + mutex_lock(&tlmi_mutex);
>>>> ret = tlmi_opcode_setting("WmiOpcodePasswordType", pwd_type);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + mutex_unlock(&tlmi_mutex);
>>>> goto out;
>>>> -
>>>> + }
>>>> if (tlmi_priv.pwd_admin->valid) {
>>>> ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
>>>> tlmi_priv.pwd_admin->password);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + mutex_unlock(&tlmi_mutex);
>>>> goto out;
>>>> + }
>>>> }
>>>> ret = tlmi_opcode_setting("WmiOpcodePasswordCurrent01", setting->password);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + mutex_unlock(&tlmi_mutex);
>>>> goto out;
>>>> + }
>>>> ret = tlmi_opcode_setting("WmiOpcodePasswordNew01", new_pwd);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + mutex_unlock(&tlmi_mutex);
>>>> goto out;
>>>> + }
>>>> ret = tlmi_simple_call(LENOVO_OPCODE_IF_GUID, "WmiOpcodePasswordSetUpdate;");
>>>> + mutex_unlock(&tlmi_mutex);
>>>> } else {
>>>> /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>>>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
>>>
>>>
>>> I haven't take a really close / good look yet. But at a first glance
>>> I think it would be cleaner to just take the mutex at the top
>>> and unlock it after the out label to which all the existing goto-s
>>> already go ?
>>>
>> I did consider that - and it was in my first implementation; but then I got concerned
>> about if the mutex_unlock could potentially get called without mutex_lock having been
>> called beforehand. I couldn't find any good reference as to whether that was safe or not.
>>
>> I ended up deciding that a few extra brackets and unlock calls wasn't that ugly and was 'safer'...and
>> so went that route.
>>
>> Happy to change it - but do you happen to know if it's safe to call unlock without a lock? If it is then
>> that implementation is cleaner.
>
> It is not allowed to unlock without a lock. But if you put the lock
> directly after the malloc for which the out: does the free then there
> should be no goto out paths which don't have the lock.
>
> E.g. for new_password_store() put it here:
>
> new_pwd = kstrdup(buf, GFP_KERNEL);
> if (!new_pwd)
> return -ENOMEM;
>
> mutex_lock(&tlmi_mutex);
>
> /* Strip out CR if one is present, setting password won't work if it
> is present */
> ...
>
> This does mean also taking the lock in the case where the new password
> store is done with a single WMI call, but that is not an issue. It
> makes things a tiny bit slower but WMI calls already are not fast and
> it is not like we are going to change the password / settings 100-times
> per second.
>
> And the same thing can be done in current_value_store():
>
> new_setting = kstrdup(buf, GFP_KERNEL);
> if (!new_setting)
> return -ENOMEM;
>
> mutex_lock(&tlmi_mutex);
>
> /* Strip out CR if one is present */
> ...
>
Yeah - you're right.
For some reason I was trying to do the lock only in the block of code that needed locking...but it makes more sense to do it earlier. I'll update.
Thanks!
Mark