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
Changes in v3: None. Version bump with rest of series
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
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.
Changes in v3: None. Version bump with rest of series
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
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 & v3: 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
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 & v3: 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
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.
Changes in v3: Simplified mutex handling as recommended.
drivers/platform/x86/think-lmi.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
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);
--
2.40.1
On Fri, 26 May 2023, Mark Pearson wrote:
> 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]>
Missing Fixes tag?
> ---
> Changes in v2:
> - Updated define name to be SMP_PWD instead of SYS_PWD
> - Clarified in comments what each password type is.
> Changes in v3: None. Version bump with rest of series
>
> 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 */
Whe you're adding Fixes tag, please make this change minimal by just
adding TLMI_SMP_PWD.
The rest of these define changes are a good too but it's unrelated to the
actual fix so they should be in a separate patch. And once you move it
into own change, convert to BIT() while at it.
--
i.
> #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");
>
On Fri, 26 May 2023, 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).
These two sound entirely separate changes. If that's the case, please
make own patch from the send change.
Hmm, index_store() still allows 0, is that also related here? Please check
also ABI documentation as index default seems to be mentioned there as
well.
--
i.
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> Changes in v2 & v3: 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 ||
>
On Fri, 26 May 2023, Mark Pearson wrote:
> 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
> Changes in v3: None. Version bump with rest of series
>
> 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);
Alignment.
> + 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);
Align.
> + if (ret)
> + goto out;
> + }
> +
> + ret = tlmi_save_bios_settings("");
> + } else { /* old non opcode based authentication method (deprecated)*/
non missing hyphen.
Missing space at the comment closing.
> 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,
>
Except for those style issues, it look okay to me:
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Fri, 26 May 2023, Mark Pearson wrote:
> 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 & v3: 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) &&
Indentation issue here, attr must be at the same column although putting
the || on a single line might make this easier to read.
> + tlmi_priv.opcode_support)
> + return 0;
> +
> return attr->mode;
> }
>
>
--
i.
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).
--
i.
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> 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(-)
>
> 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);
>
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
On Mon, May 29, 2023, at 7:51 AM, Ilpo Järvinen wrote:
> On Fri, 26 May 2023, Mark Pearson wrote:
>
>> 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
>> Changes in v3: None. Version bump with rest of series
>>
>> 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);
>
> Alignment.
OK - I assume you want the new_setting lined up under the bracket.
I've not seen that called out as a requirement (https://www.kernel.org/doc/html/v4.10/process/coding-style.html) but I don't mind fixing....but if you can point me at the specifics it's appreciated
>
>> + 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);
>
> Align.
Ack.
>
>> + if (ret)
>> + goto out;
>> + }
>> +
>> + ret = tlmi_save_bios_settings("");
>> + } else { /* old non opcode based authentication method (deprecated)*/
>
> non missing hyphen.
non-opcode I assume?
>
> Missing space at the comment closing.
Will fix.
>
>> 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,
>>
>
> Except for those style issues, it look okay to me:
>
> Reviewed-by: Ilpo Järvinen <[email protected]>
>
Thanks for the review!
Mark
On Mon, May 29, 2023, at 8:05 AM, Ilpo Järvinen wrote:
> On Fri, 26 May 2023, Mark Pearson wrote:
>
>> 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 & v3: 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) &&
>
> Indentation issue here, attr must be at the same column although putting
> the || on a single line might make this easier to read.
OK - will fix.
>
>> + tlmi_priv.opcode_support)
>> + return 0;
>> +
>> return attr->mode;
>> }
>>
>>
>
> --
> i.
Thanks for the review
Mark
Thanks Ilpo
On Mon, May 29, 2023, at 7:36 AM, Ilpo Järvinen wrote:
> On Fri, 26 May 2023, Mark Pearson wrote:
>
>> 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]>
>
> Missing Fixes tag?
Yes - will add.
>
>> ---
>> Changes in v2:
>> - Updated define name to be SMP_PWD instead of SYS_PWD
>> - Clarified in comments what each password type is.
>> Changes in v3: None. Version bump with rest of series
>>
>> 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 */
>
> Whe you're adding Fixes tag, please make this change minimal by just
> adding TLMI_SMP_PWD.
>
> The rest of these define changes are a good too but it's unrelated to the
> actual fix so they should be in a separate patch. And once you move it
> into own change, convert to BIT() while at it.
>
I was asked previously to clarify what SMP stood for so added the comment and it seemed odd to only clarify one and not the others.
Can I push back on this request. Doing two separate patches for just that doesn't make sense to me.
Thanks for the review
Mark
Thanks Ilpo
On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
> On Fri, 26 May 2023, 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).
>
> These two sound entirely separate changes. If that's the case, please
> make own patch from the send change.
Ack. It was all related to the index setting and seemed trivial so I lumped together but I can split.
This patch series is turning into a good learning exercise for my git skills :) (which are limited)
>
> Hmm, index_store() still allows 0, is that also related here? Please check
> also ABI documentation as index default seems to be mentioned there as
> well.
>
I'd rather not limit it so 0 isn't allowed in case our BIOS team does something weird in the future; but right now 1 is the default so it makes more sense.
Well spotted on the ABI documentation - I had completely missed that. I will address that as well.
> --
> i.
Thanks for the review
Mark
On Mon, 29 May 2023, Mark Pearson wrote:
> On Mon, May 29, 2023, at 7:51 AM, Ilpo Järvinen wrote:
> > On Fri, 26 May 2023, Mark Pearson wrote:
> >
> >> 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
> >> Changes in v3: None. Version bump with rest of series
> >>
> >> 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);
> >
> > Alignment.
>
> OK - I assume you want the new_setting lined up under the bracket.
> I've not seen that called out as a requirement (https://www.kernel.org/doc/html/v4.10/process/coding-style.html) but I don't mind fixing....but if you can point me at the specifics it's appreciated
Yes, I meant aligning to the column following the opening parenthesis.
I guess it's not a hard requirement, however, there's a benefit from
certain things aligning because it helps in the brains in the process of
converting text into structure with less effort (when not specifically not
focusing on that particular line).
> >> + 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);
> >
> > Align.
>
> Ack.
>
> >
> >> + if (ret)
> >> + goto out;
> >> + }
> >> +
> >> + ret = tlmi_save_bios_settings("");
> >> + } else { /* old non opcode based authentication method (deprecated)*/
> >
> > non missing hyphen.
>
> non-opcode I assume?
I think the most proper English would be non-opcode-based since "opcode
based" belong together (but I'm not a native speaker here).
--
i.
On Mon, 29 May 2023, Mark Pearson wrote:
> On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
> > On Fri, 26 May 2023, 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).
> >
> > These two sound entirely separate changes. If that's the case, please
> > make own patch from the send change.
>
> Ack. It was all related to the index setting and seemed trivial so I
> lumped together but I can split. This patch series is turning into a
> good learning exercise for my git skills :) (which are limited)
>
> > Hmm, index_store() still allows 0, is that also related here? Please check
> > also ABI documentation as index default seems to be mentioned there as
> > well.
> >
>
> I'd rather not limit it so 0 isn't allowed in case our BIOS team does
> something weird in the future; but right now 1 is the default so it
> makes more sense.
Sure, do what you feel makes sense here. I was just pointing out the
perceived inconsistency in case it wasn't intentional.
It might be useful to add one sentence into changelog about the reasoning
so it can be found easier later on (effectively the paragraph you wrote
above with small tweaks is enough I think).
--
i.
On Mon, 29 May 2023, Mark Pearson wrote:
> Thanks Ilpo
>
> On Mon, May 29, 2023, at 7:36 AM, Ilpo J?rvinen wrote:
> > On Fri, 26 May 2023, Mark Pearson wrote:
> >
> >> 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]>
> >
> > Missing Fixes tag?
>
> Yes - will add.
>
> >
> >> ---
> >> Changes in v2:
> >> - Updated define name to be SMP_PWD instead of SYS_PWD
> >> - Clarified in comments what each password type is.
> >> Changes in v3: None. Version bump with rest of series
> >>
> >> 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 */
> >
> > Whe you're adding Fixes tag, please make this change minimal by just
> > adding TLMI_SMP_PWD.
> >
> > The rest of these define changes are a good too but it's unrelated to the
> > actual fix so they should be in a separate patch. And once you move it
> > into own change, convert to BIT() while at it.
>
> I was asked previously to clarify what SMP stood for so added the
> comment and it seemed odd to only clarify one and not the others.
> Can I push back on this request. Doing two separate patches for just
> that doesn't make sense to me.
I did not mean removing TLMI_SMP_PWD's comment from this patch just to add
it in the another but the comments to the other bits which should go into
their own patch. The thing here is that fixes should be made minimal to
comply with stable rules.
--
i.
On Mon, May 29, 2023, at 11:36 AM, Ilpo Järvinen wrote:
> On Mon, 29 May 2023, Mark Pearson wrote:
>> On Mon, May 29, 2023, at 7:51 AM, Ilpo Järvinen wrote:
>> > On Fri, 26 May 2023, Mark Pearson wrote:
>> >
>> >> 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
>> >> Changes in v3: None. Version bump with rest of series
>> >>
>> >> 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);
>> >
>> > Alignment.
>>
>> OK - I assume you want the new_setting lined up under the bracket.
>> I've not seen that called out as a requirement (https://www.kernel.org/doc/html/v4.10/process/coding-style.html) but I don't mind fixing....but if you can point me at the specifics it's appreciated
>
> Yes, I meant aligning to the column following the opening parenthesis.
>
> I guess it's not a hard requirement, however, there's a benefit from
> certain things aligning because it helps in the brains in the process of
> converting text into structure with less effort (when not specifically not
> focusing on that particular line).
Not a problem. Happy to make this change along with the others. Was just curious :)
>
>> >> + 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);
>> >
>> > Align.
>>
>> Ack.
>>
>> >
>> >> + if (ret)
>> >> + goto out;
>> >> + }
>> >> +
>> >> + ret = tlmi_save_bios_settings("");
>> >> + } else { /* old non opcode based authentication method (deprecated)*/
>> >
>> > non missing hyphen.
>>
>> non-opcode I assume?
>
> I think the most proper English would be non-opcode-based since "opcode
> based" belong together (but I'm not a native speaker here).
I am a native speaker....and I don't know :) (English is weird...)
Let's go with non-opcode; adding the based on there feels wrong to me (somewhat arbitrarily).
>
> --
> i.
On Mon, May 29, 2023, at 11:50 AM, Ilpo Järvinen wrote:
> On Mon, 29 May 2023, Mark Pearson wrote:
>
>> Thanks Ilpo
>>
>> On Mon, May 29, 2023, at 7:36 AM, Ilpo Järvinen wrote:
>> > On Fri, 26 May 2023, Mark Pearson wrote:
>> >
>> >> 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]>
>> >
>> > Missing Fixes tag?
>>
>> Yes - will add.
>>
>> >
>> >> ---
>> >> Changes in v2:
>> >> - Updated define name to be SMP_PWD instead of SYS_PWD
>> >> - Clarified in comments what each password type is.
>> >> Changes in v3: None. Version bump with rest of series
>> >>
>> >> 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 */
>> >
>> > Whe you're adding Fixes tag, please make this change minimal by just
>> > adding TLMI_SMP_PWD.
>> >
>> > The rest of these define changes are a good too but it's unrelated to the
>> > actual fix so they should be in a separate patch. And once you move it
>> > into own change, convert to BIT() while at it.
>>
>> I was asked previously to clarify what SMP stood for so added the
>> comment and it seemed odd to only clarify one and not the others.
>> Can I push back on this request. Doing two separate patches for just
>> that doesn't make sense to me.
>
> I did not mean removing TLMI_SMP_PWD's comment from this patch just to add
> it in the another but the comments to the other bits which should go into
> their own patch. The thing here is that fixes should be made minimal to
> comply with stable rules.
>
OK....seems odd to me to be honest, but not something I'd lose sleep over.
I'll do that in amongst all the other changes.
Thanks
Mark
On Mon, May 29, 2023, at 11:41 AM, Ilpo Järvinen wrote:
> On Mon, 29 May 2023, Mark Pearson wrote:
>> On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
>> > On Fri, 26 May 2023, 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).
>> >
>> > These two sound entirely separate changes. If that's the case, please
>> > make own patch from the send change.
>>
>> Ack. It was all related to the index setting and seemed trivial so I
>> lumped together but I can split. This patch series is turning into a
>> good learning exercise for my git skills :) (which are limited)
>>
>> > Hmm, index_store() still allows 0, is that also related here? Please check
>> > also ABI documentation as index default seems to be mentioned there as
>> > well.
>> >
>>
>> I'd rather not limit it so 0 isn't allowed in case our BIOS team does
>> something weird in the future; but right now 1 is the default so it
>> makes more sense.
>
> Sure, do what you feel makes sense here. I was just pointing out the
> perceived inconsistency in case it wasn't intentional.
>
> It might be useful to add one sentence into changelog about the reasoning
> so it can be found easier later on (effectively the paragraph you wrote
> above with small tweaks is enough I think).
Ack - will do. Thanks
Mark
Hi Mark,
On 5/26/23 19:16, Mark Pearson wrote:
> 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]>
Thank you for this new version. Please prepare a v4 addressing Ilpo's
review remarks.
About the aligning function arguments on the next line to the '('
of the function call start at the previous line, checkpatch also
checks for this.
It is always a good idea to run checkpatch before submitting patches.
E.g.:
git format-patch -v3 HEAD~5
scripts/checkpatch.pl v3-00*.patch
<check output is clean>
git send-email v3-00*.patch
Regards,
Hans
> ---
> Changes in v2: Update comment for clearer explanation of what the driver
> is doing
> Changes in v3: None. Version bump with rest of series
>
> 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,
Hi Hans
On Tue, May 30, 2023, at 6:54 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 5/26/23 19:16, Mark Pearson wrote:
>> 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]>
>
> Thank you for this new version. Please prepare a v4 addressing Ilpo's
> review remarks.
Will do
>
> About the aligning function arguments on the next line to the '('
> of the function call start at the previous line, checkpatch also
> checks for this.
>
> It is always a good idea to run checkpatch before submitting patches.
I always do - and checkpatch isn't complaining about the alignment here.
Mark