[PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
---
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

[PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
---
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

[PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
---
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

[PATCH v3 4/5] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
---
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

[PATCH v3 5/5] platform/x86: think-lmi: mutex protection around multiple WMI calls

[Mark Pearson]

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

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

Re: [PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

[Ilpo Järvinen]

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 <mpearson-lenovo@squebb.ca>

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");
> 

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

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 <mpearson-lenovo@squebb.ca>
> ---
> 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 <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

[Ilpo Järvinen]

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 <mpearson-lenovo@squebb.ca>
> ---
> 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 ||
> 

Re: [PATCH v3 4/5] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Ilpo Järvinen]

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 <mpearson-lenovo@squebb.ca>
> ---
> 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.

Re: [PATCH v3 5/5] platform/x86: think-lmi: mutex protection around multiple WMI calls

[Ilpo Järvinen]

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 <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(-)
> 
> 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);
> 

Re: [PATCH v3 5/5] platform/x86: think-lmi: mutex protection around multiple WMI calls

[Mark Pearson]

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

Re: [PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>
> 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

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>> ---
>> 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 <ilpo.jarvinen@linux.intel.com>
>
Thanks for the review!
Mark

Re: [PATCH v3 4/5] platform/x86: think-lmi: Don't display unnecessary authentication settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>> ---
>> 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

Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

[Mark Pearson]

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

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

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 <mpearson-lenovo@squebb.ca>
> >> ---
> >> 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.

Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

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.

Re: [PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

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 <mpearson-lenovo@squebb.ca>
> >
> > 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.

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>> >> ---
>> >> 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.

Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

[Mark Pearson]

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

Re: [PATCH v3 2/5] platform/x86: think-lmi: Correct System password interface

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>> >
>> > 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

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Hans de Goede]

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 <mpearson-lenovo@squebb.ca>

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,

Re: [PATCH v3 1/5] platform/x86: think-lmi: Enable opcode support on BIOS settings

[Mark Pearson]

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 <mpearson-lenovo@squebb.ca>
>
> 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