[PATCH 0/5] platform/x86: think-lmi: Clean up code style

[PATCH 0/5] platform/x86: think-lmi: Clean up code style

[Benjamin Philip]

This patchset cleans up the code style checks flagged by checkpatch in
think-lmi.c and think-lmi.h. All in all, it fixes 9 different unique checks
across 67 different instances.

Benjamin Philip (5):
  platform/x86: think-lmi: Clean up types in headers
  platform/x86: think-lmi: Remove unnecessary parens
  platform/x86: think-lmi: Clean up misc checks
  platform/x86: think-lmi: Handle 80 and 100 column limit overflows
  platform/x86: think-lmi: Clean up alignment

 drivers/platform/x86/lenovo/think-lmi.c | 160 +++++++++++++-----------
 drivers/platform/x86/lenovo/think-lmi.h |  20 +--
 2 files changed, 95 insertions(+), 85 deletions(-)

-- 
2.52.0

[PATCH 1/5] platform/x86: think-lmi: Clean up types in headers

[Benjamin Philip]

This commit replaces the uint32_t standard type with preferred u32
kernel type, fixing the following checkpatch check:

CHECK: Prefer kernel type 'u32' over 'uint32_t'

Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
---
 drivers/platform/x86/lenovo/think-lmi.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/lenovo/think-lmi.h
b/drivers/platform/x86/lenovo/think-lmi.h
index 017644323d46..6ea4bceafab2 100644
--- a/drivers/platform/x86/lenovo/think-lmi.h
+++ b/drivers/platform/x86/lenovo/think-lmi.h
@@ -58,19 +58,19 @@ struct tlmi_cert_guids {
 #define TLMI_PWDCFG_MODE_MULTICERT 3

 struct tlmi_pwdcfg_core {
-	uint32_t password_mode;
-	uint32_t password_state;
-	uint32_t min_length;
-	uint32_t max_length;
-	uint32_t supported_encodings;
-	uint32_t supported_keyboard;
+	u32 password_mode;
+	u32 password_state;
+	u32 min_length;
+	u32 max_length;
+	u32 supported_encodings;
+	u32 supported_keyboard;
 };

 struct tlmi_pwdcfg_ext {
-	uint32_t hdd_user_password;
-	uint32_t hdd_master_password;
-	uint32_t nvme_user_password;
-	uint32_t nvme_master_password;
+	u32 hdd_user_password;
+	u32 hdd_master_password;
+	u32 nvme_user_password;
+	u32 nvme_master_password;
 };

 struct tlmi_pwdcfg {
-- 
2.52.0

[PATCH 2/5] platform/x86: think-lmi: Remove unnecessary parens

[Benjamin Philip]

This commit removes any unnecessary parentheses as flagged by
checkpatch in the following check:

CHECK: Unnecessary parentheses around '...'

Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
---
 drivers/platform/x86/lenovo/think-lmi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/lenovo/think-lmi.c
b/drivers/platform/x86/lenovo/think-lmi.c
index 540b472b1bf3..1fac8986d077 100644
--- a/drivers/platform/x86/lenovo/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -440,7 +440,7 @@ static ssize_t current_password_store(struct kobject *kobj,

 	pwdlen = strlen(buf);
 	/* pwdlen == 0 is allowed to clear the password */
-	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
+	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen))
 		return -EINVAL;

 	strscpy(setting->password, buf, setting->maxlen);
@@ -476,7 +476,7 @@ static ssize_t new_password_store(struct kobject *kobj,

 	pwdlen = strlen(new_pwd);
 	/* pwdlen == 0 is allowed to clear the password */
-	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
+	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -859,7 +859,7 @@ static ssize_t certificate_store(struct kobject *kobj,
 			signature = setting->signature;
 	} else { /* Cert install */
 		/* Check if SMC and SVC already installed */
-		if ((setting == tlmi_priv.pwd_system) &&
tlmi_priv.pwd_admin->cert_installed) {
+		if (setting == tlmi_priv.pwd_system && tlmi_priv.pwd_admin->cert_installed) {
 			/* This gets treated as a cert update */
 			install_mode = TLMI_CERT_UPDATE;
 			signature = tlmi_priv.pwd_admin->signature;
@@ -881,7 +881,7 @@ static ssize_t certificate_store(struct kobject *kobj,
 	} else {
 		/* This is a fresh install */
 		/* To set admin cert, a password must be enabled */
-		if ((setting == tlmi_priv.pwd_admin) &&
+		if (setting == tlmi_priv.pwd_admin &&
 		    (!setting->pwd_enabled || !setting->password[0])) {
 			kfree(new_cert);
 			return -EACCES;
@@ -965,13 +965,13 @@ static ssize_t save_signature_store(struct kobject *kobj,
 static struct kobj_attribute auth_save_signature = __ATTR_WO(save_signature);

 static umode_t auth_attr_is_visible(struct kobject *kobj,
-					     struct attribute *attr, int n)
+				    struct attribute *attr, int n)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	/* We only want to display level and index settings on HDD/NVMe */
 	if (attr == &auth_index.attr || attr == &auth_level.attr) {
-		if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
+		if (setting == tlmi_priv.pwd_hdd || setting == tlmi_priv.pwd_nvme)
 			return attr->mode;
 		return 0;
 	}
@@ -985,8 +985,8 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
 		if (tlmi_priv.certificate_support) {
 			if (setting == tlmi_priv.pwd_admin)
 				return attr->mode;
-			if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
-			    (setting == tlmi_priv.pwd_system))
+			if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT &&
+			    setting == tlmi_priv.pwd_system)
 				return attr->mode;
 		}
 		return 0;
@@ -1216,13 +1216,13 @@ static struct kobj_attribute attr_current_val
= __ATTR_RW_MODE(current_value, 06

 static struct kobj_attribute attr_type = __ATTR_RO(type);

-static umode_t attr_is_visible(struct kobject *kobj,
-					     struct attribute *attr, int n)
+static umode_t attr_is_visible(struct kobject *kobj, struct attribute *attr,
+			       int n)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);

 	/* We don't want to display possible_values attributes if not available */
-	if ((attr == &attr_possible_values.attr) && (!setting->possible_values))
+	if (attr == &attr_possible_values.attr && !setting->possible_values)
 		return 0;

 	return attr->mode;
-- 
2.52.0

Re: [PATCH 2/5] platform/x86: think-lmi: Remove unnecessary parens

[Mark Pearson]

On Tue, Dec 23, 2025, at 2:23 PM, Benjamin Philip wrote:
> This commit removes any unnecessary parentheses as flagged by
> checkpatch in the following check:
>
> CHECK: Unnecessary parentheses around '...'
>

Shouldn't you mention that you also corrected pieces of code alignment here too?

> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> ---
>  drivers/platform/x86/lenovo/think-lmi.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> b/drivers/platform/x86/lenovo/think-lmi.c
> index 540b472b1bf3..1fac8986d077 100644
> --- a/drivers/platform/x86/lenovo/think-lmi.c
> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> @@ -440,7 +440,7 @@ static ssize_t current_password_store(struct kobject *kobj,
>
>  	pwdlen = strlen(buf);
>  	/* pwdlen == 0 is allowed to clear the password */
> -	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
> +	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen))
>  		return -EINVAL;
>
>  	strscpy(setting->password, buf, setting->maxlen);
> @@ -476,7 +476,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>
>  	pwdlen = strlen(new_pwd);
>  	/* pwdlen == 0 is allowed to clear the password */
> -	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
> +	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen)) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -859,7 +859,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>  			signature = setting->signature;
>  	} else { /* Cert install */
>  		/* Check if SMC and SVC already installed */
> -		if ((setting == tlmi_priv.pwd_system) &&
> tlmi_priv.pwd_admin->cert_installed) {
> +		if (setting == tlmi_priv.pwd_system && tlmi_priv.pwd_admin->cert_installed) {
>  			/* This gets treated as a cert update */
>  			install_mode = TLMI_CERT_UPDATE;
>  			signature = tlmi_priv.pwd_admin->signature;
> @@ -881,7 +881,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>  	} else {
>  		/* This is a fresh install */
>  		/* To set admin cert, a password must be enabled */
> -		if ((setting == tlmi_priv.pwd_admin) &&
> +		if (setting == tlmi_priv.pwd_admin &&
>  		    (!setting->pwd_enabled || !setting->password[0])) {
>  			kfree(new_cert);
>  			return -EACCES;
> @@ -965,13 +965,13 @@ static ssize_t save_signature_store(struct kobject *kobj,
>  static struct kobj_attribute auth_save_signature = __ATTR_WO(save_signature);
>
>  static umode_t auth_attr_is_visible(struct kobject *kobj,
> -					     struct attribute *attr, int n)
> +				    struct attribute *attr, int n)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	/* We only want to display level and index settings on HDD/NVMe */
>  	if (attr == &auth_index.attr || attr == &auth_level.attr) {
> -		if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
> +		if (setting == tlmi_priv.pwd_hdd || setting == tlmi_priv.pwd_nvme)
>  			return attr->mode;
>  		return 0;
>  	}
> @@ -985,8 +985,8 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
>  		if (tlmi_priv.certificate_support) {
>  			if (setting == tlmi_priv.pwd_admin)
>  				return attr->mode;
> -			if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
> -			    (setting == tlmi_priv.pwd_system))
> +			if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT &&
> +			    setting == tlmi_priv.pwd_system)
>  				return attr->mode;
>  		}
>  		return 0;
> @@ -1216,13 +1216,13 @@ static struct kobj_attribute attr_current_val
> = __ATTR_RW_MODE(current_value, 06
>
>  static struct kobj_attribute attr_type = __ATTR_RO(type);
>
> -static umode_t attr_is_visible(struct kobject *kobj,
> -					     struct attribute *attr, int n)
> +static umode_t attr_is_visible(struct kobject *kobj, struct attribute *attr,
> +			       int n)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>
>  	/* We don't want to display possible_values attributes if not available */
> -	if ((attr == &attr_possible_values.attr) && (!setting->possible_values))
> +	if (attr == &attr_possible_values.attr && !setting->possible_values)
>  		return 0;
>
>  	return attr->mode;
> -- 
> 2.52.0

I prefer the brackets as they were - for me it's clearer and removing them adds little value.
But the changes are fine, and I guess it's preferred overall by kernel community, so shrug.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Re: [PATCH 2/5] platform/x86: think-lmi: Remove unnecessary parens

[Benjamin Philip]

"Mark Pearson" <mpearson-lenovo@squebb.ca> writes:

> On Tue, Dec 23, 2025, at 2:23 PM, Benjamin Philip wrote:
>> This commit removes any unnecessary parentheses as flagged by
>> checkpatch in the following check:
>>
>> CHECK: Unnecessary parentheses around '...'
>>
>
> Shouldn't you mention that you also corrected pieces of code alignment here too?
>

I must have included them in by accident, since they share the same hunk
as a parens fix. Maybe splitting the hunk and then moving the alignment
changes to the last patch would be better?

>> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>> ---
>>  drivers/platform/x86/lenovo/think-lmi.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> b/drivers/platform/x86/lenovo/think-lmi.c
>> index 540b472b1bf3..1fac8986d077 100644
>> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -440,7 +440,7 @@ static ssize_t current_password_store(struct kobject *kobj,
>>
>>  	pwdlen = strlen(buf);
>>  	/* pwdlen == 0 is allowed to clear the password */
>> -	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
>> +	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen))
>>  		return -EINVAL;
>>
>>  	strscpy(setting->password, buf, setting->maxlen);
>> @@ -476,7 +476,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>>
>>  	pwdlen = strlen(new_pwd);
>>  	/* pwdlen == 0 is allowed to clear the password */
>> -	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
>> +	if (pwdlen && (pwdlen < setting->minlen || pwdlen > setting->maxlen)) {
>>  		ret = -EINVAL;
>>  		goto out;
>>  	}
>> @@ -859,7 +859,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  			signature = setting->signature;
>>  	} else { /* Cert install */
>>  		/* Check if SMC and SVC already installed */
>> -		if ((setting == tlmi_priv.pwd_system) &&
>> tlmi_priv.pwd_admin->cert_installed) {
>> +		if (setting == tlmi_priv.pwd_system && tlmi_priv.pwd_admin->cert_installed) {
>>  			/* This gets treated as a cert update */
>>  			install_mode = TLMI_CERT_UPDATE;
>>  			signature = tlmi_priv.pwd_admin->signature;
>> @@ -881,7 +881,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  	} else {
>>  		/* This is a fresh install */
>>  		/* To set admin cert, a password must be enabled */
>> -		if ((setting == tlmi_priv.pwd_admin) &&
>> +		if (setting == tlmi_priv.pwd_admin &&
>>  		    (!setting->pwd_enabled || !setting->password[0])) {
>>  			kfree(new_cert);
>>  			return -EACCES;
>> @@ -965,13 +965,13 @@ static ssize_t save_signature_store(struct kobject *kobj,
>>  static struct kobj_attribute auth_save_signature = __ATTR_WO(save_signature);
>>
>>  static umode_t auth_attr_is_visible(struct kobject *kobj,
>> -					     struct attribute *attr, int n)
>> +				    struct attribute *attr, int n)
>>  {
>>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>
>>  	/* We only want to display level and index settings on HDD/NVMe */
>>  	if (attr == &auth_index.attr || attr == &auth_level.attr) {
>> -		if ((setting == tlmi_priv.pwd_hdd) || (setting == tlmi_priv.pwd_nvme))
>> +		if (setting == tlmi_priv.pwd_hdd || setting == tlmi_priv.pwd_nvme)
>>  			return attr->mode;
>>  		return 0;
>>  	}
>> @@ -985,8 +985,8 @@ static umode_t auth_attr_is_visible(struct kobject *kobj,
>>  		if (tlmi_priv.certificate_support) {
>>  			if (setting == tlmi_priv.pwd_admin)
>>  				return attr->mode;
>> -			if ((tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT) &&
>> -			    (setting == tlmi_priv.pwd_system))
>> +			if (tlmi_priv.pwdcfg.core.password_mode >= TLMI_PWDCFG_MODE_MULTICERT &&
>> +			    setting == tlmi_priv.pwd_system)
>>  				return attr->mode;
>>  		}
>>  		return 0;
>> @@ -1216,13 +1216,13 @@ static struct kobj_attribute attr_current_val
>> = __ATTR_RW_MODE(current_value, 06
>>
>>  static struct kobj_attribute attr_type = __ATTR_RO(type);
>>
>> -static umode_t attr_is_visible(struct kobject *kobj,
>> -					     struct attribute *attr, int n)
>> +static umode_t attr_is_visible(struct kobject *kobj, struct attribute *attr,
>> +			       int n)
>>  {
>>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>
>>  	/* We don't want to display possible_values attributes if not available */
>> -	if ((attr == &attr_possible_values.attr) && (!setting->possible_values))
>> +	if (attr == &attr_possible_values.attr && !setting->possible_values)
>>  		return 0;
>>
>>  	return attr->mode;
>> --
>> 2.52.0
>
> I prefer the brackets as they were - for me it's clearer and removing them adds little value.
> But the changes are fine, and I guess it's preferred overall by kernel community, so shrug.
>

I find the previous style clearer too, and I agree that this change
comes down to pedantry and offer no real readability improvements.
Nevertheless, I think there's value in enforcing consistency and the
popular preference.

Note to Mark: Resent with the Cc: list included. Ignore the private
copy. I'm still ironing out my notmuch-emacs config :)

-- bp

> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

[PATCH 3/5] platform/x86: think-lmi: Clean up misc checks

[Benjamin Philip]

This commit cleans up the following checks:

- CHECK: braces {} should be used on all arms of this statement
- CHECK: Please use a blank line after function/struct/union/enum
  declarations
- CHECK: Prefer kzalloc(sizeof(*new_pwd)...) over
  kzalloc(sizeof(struct tlmi_pwd_setting)...)
- CHECK: spaces preferred around that '/' (ctx:VxV)
- CHECK: Unbalanced braces around else statement

Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
---
 drivers/platform/x86/lenovo/think-lmi.c | 32 +++++++++++++------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/lenovo/think-lmi.c
b/drivers/platform/x86/lenovo/think-lmi.c
index 1fac8986d077..1ada4d800383 100644
--- a/drivers/platform/x86/lenovo/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -223,14 +223,16 @@ static const struct tlmi_err_codes tlmi_errs[] = {
 	{"Set Certificate operation failed with status:Certificate too
large.", -EFBIG},
 };

-static const char * const encoding_options[] = {
+static const char *const encoding_options[] = {
 	[TLMI_ENCODING_ASCII] = "ascii",
 	[TLMI_ENCODING_SCANCODE] = "scancode",
 };
-static const char * const level_options[] = {
+
+static const char *const level_options[] = {
 	[TLMI_LEVEL_USER] = "user",
 	[TLMI_LEVEL_MASTER] = "master",
 };
+
 static struct think_lmi tlmi_priv;
 static DEFINE_MUTEX(tlmi_mutex);

@@ -249,7 +251,7 @@ static int tlmi_errstr_to_err(const char *errstr)
 {
 	int i;

-	for (i = 0; i < sizeof(tlmi_errs)/sizeof(struct tlmi_err_codes); i++) {
+	for (i = 0; i < sizeof(tlmi_errs) / sizeof(struct tlmi_err_codes); i++) {
 		if (!strcmp(tlmi_errs[i].err_str, errstr))
 			return tlmi_errs[i].err_code;
 	}
@@ -570,19 +572,19 @@ static ssize_t mechanism_show(struct kobject
*kobj, struct kobj_attribute *attr,
 		return sysfs_emit(buf, "certificate\n");
 	return sysfs_emit(buf, "password\n");
 }
+
 static struct kobj_attribute auth_mechanism = __ATTR_RO(mechanism);

 static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr,
-			 char *buf)
+			     char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	return sysfs_emit(buf, "%s\n", encoding_options[setting->encoding]);
 }

-static ssize_t encoding_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+static ssize_t encoding_store(struct kobject *kobj, struct
kobj_attribute *attr,
+			      const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	int i;
@@ -632,19 +634,19 @@ static ssize_t role_show(struct kobject *kobj,
struct kobj_attribute *attr,

 	return sysfs_emit(buf, "%s\n", setting->role);
 }
+
 static struct kobj_attribute auth_role = __ATTR_RO(role);

 static ssize_t index_show(struct kobject *kobj, struct kobj_attribute *attr,
-			 char *buf)
+			  char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	return sysfs_emit(buf, "%d\n", setting->index);
 }

-static ssize_t index_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+static ssize_t index_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	int err, val;
@@ -1047,9 +1049,9 @@ static ssize_t current_value_show(struct kobject
*kobj, struct kobj_attribute *a

 	/* validate and split from `item,value` -> `value` */
 	value = strpbrk(item, ",");
-	if (!value || value == item || !strlen(value + 1))
+	if (!value || value == item || !strlen(value + 1)) {
 		ret = -EINVAL;
-	else {
+	} else {
 		/* On Workstations remove the Options part after the value */
 		strreplace(value, ';', '\0');
 		ret = sysfs_emit(buf, "%s\n", value + 1);
@@ -1585,11 +1587,11 @@ static int tlmi_sysfs_init(void)

 /* ---- Base Driver -------------------------------------------------------- */
 static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
-			    const char *pwd_role)
+						 const char *pwd_role)
 {
 	struct tlmi_pwd_setting *new_pwd;

-	new_pwd = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
+	new_pwd = kzalloc(sizeof(*new_pwd), GFP_KERNEL);
 	if (!new_pwd)
 		return NULL;

-- 
2.52.0

Re: [PATCH 3/5] platform/x86: think-lmi: Clean up misc checks

[Mark Pearson]

On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
> This commit cleans up the following checks:
>
> - CHECK: braces {} should be used on all arms of this statement
> - CHECK: Please use a blank line after function/struct/union/enum
>   declarations
> - CHECK: Prefer kzalloc(sizeof(*new_pwd)...) over
>   kzalloc(sizeof(struct tlmi_pwd_setting)...)
> - CHECK: spaces preferred around that '/' (ctx:VxV)
> - CHECK: Unbalanced braces around else statement
>
> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> ---
>  drivers/platform/x86/lenovo/think-lmi.c | 32 +++++++++++++------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> b/drivers/platform/x86/lenovo/think-lmi.c
> index 1fac8986d077..1ada4d800383 100644
> --- a/drivers/platform/x86/lenovo/think-lmi.c
> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> @@ -223,14 +223,16 @@ static const struct tlmi_err_codes tlmi_errs[] = {
>  	{"Set Certificate operation failed with status:Certificate too
> large.", -EFBIG},
>  };
>
> -static const char * const encoding_options[] = {
> +static const char *const encoding_options[] = {
>  	[TLMI_ENCODING_ASCII] = "ascii",
>  	[TLMI_ENCODING_SCANCODE] = "scancode",
>  };
> -static const char * const level_options[] = {
> +
> +static const char *const level_options[] = {
>  	[TLMI_LEVEL_USER] = "user",
>  	[TLMI_LEVEL_MASTER] = "master",
>  };
> +
>  static struct think_lmi tlmi_priv;
>  static DEFINE_MUTEX(tlmi_mutex);
>
> @@ -249,7 +251,7 @@ static int tlmi_errstr_to_err(const char *errstr)
>  {
>  	int i;
>
> -	for (i = 0; i < sizeof(tlmi_errs)/sizeof(struct tlmi_err_codes); i++) {
> +	for (i = 0; i < sizeof(tlmi_errs) / sizeof(struct tlmi_err_codes); i++) {
>  		if (!strcmp(tlmi_errs[i].err_str, errstr))
>  			return tlmi_errs[i].err_code;
>  	}
> @@ -570,19 +572,19 @@ static ssize_t mechanism_show(struct kobject
> *kobj, struct kobj_attribute *attr,
>  		return sysfs_emit(buf, "certificate\n");
>  	return sysfs_emit(buf, "password\n");
>  }
> +
>  static struct kobj_attribute auth_mechanism = __ATTR_RO(mechanism);
>
>  static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr,
> -			 char *buf)
> +			     char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	return sysfs_emit(buf, "%s\n", encoding_options[setting->encoding]);
>  }
>
> -static ssize_t encoding_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +static ssize_t encoding_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> +			      const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	int i;
> @@ -632,19 +634,19 @@ static ssize_t role_show(struct kobject *kobj,
> struct kobj_attribute *attr,
>
>  	return sysfs_emit(buf, "%s\n", setting->role);
>  }
> +
>  static struct kobj_attribute auth_role = __ATTR_RO(role);
>
>  static ssize_t index_show(struct kobject *kobj, struct kobj_attribute *attr,
> -			 char *buf)
> +			  char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	return sysfs_emit(buf, "%d\n", setting->index);
>  }
>
> -static ssize_t index_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +static ssize_t index_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	int err, val;
> @@ -1047,9 +1049,9 @@ static ssize_t current_value_show(struct kobject
> *kobj, struct kobj_attribute *a
>
>  	/* validate and split from `item,value` -> `value` */
>  	value = strpbrk(item, ",");
> -	if (!value || value == item || !strlen(value + 1))
> +	if (!value || value == item || !strlen(value + 1)) {
>  		ret = -EINVAL;
> -	else {
> +	} else {
>  		/* On Workstations remove the Options part after the value */
>  		strreplace(value, ';', '\0');
>  		ret = sysfs_emit(buf, "%s\n", value + 1);
> @@ -1585,11 +1587,11 @@ static int tlmi_sysfs_init(void)
>
>  /* ---- Base Driver -------------------------------------------------------- */
>  static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
> -			    const char *pwd_role)
> +						 const char *pwd_role)
>  {
>  	struct tlmi_pwd_setting *new_pwd;
>
> -	new_pwd = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
> +	new_pwd = kzalloc(sizeof(*new_pwd), GFP_KERNEL);
>  	if (!new_pwd)
>  		return NULL;
>
> -- 
> 2.52.0

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Mark

[PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Benjamin Philip]

This commit handles some column limit overflows (that occur after fixing
their alignment), i.e. the following check:

CHECK: line length of ... exceeds 100 columns

by defining a constant opt, and replacing the offending
expression with opt.

Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
---
 drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/lenovo/think-lmi.c
b/drivers/platform/x86/lenovo/think-lmi.c
index 1ada4d800383..07ba0d84720a 100644
--- a/drivers/platform/x86/lenovo/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
struct kobj_attribute *attr,
 }

 static ssize_t current_value_store(struct kobject *kobj,
-		struct kobj_attribute *attr,
-		const char *buf, size_t count)
+				   struct kobj_attribute *attr, const char *buf,
+				   size_t count)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
 	char *set_str = NULL, *new_setting = NULL;
 	char *auth_str = NULL;
+	const char *opt;
 	int ret;

 	if (!tlmi_priv.can_set_bios_settings)
@@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
 			ret = tlmi_save_bios_settings("");
 	} else { /* old non-opcode based authentication method (deprecated) */
 		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
+			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
 			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
-					tlmi_priv.pwd_admin->password,
-					encoding_options[tlmi_priv.pwd_admin->encoding],
-					tlmi_priv.pwd_admin->kbdlang);
+					     tlmi_priv.pwd_admin->password,
+					     opt,
+					     tlmi_priv.pwd_admin->kbdlang);
 			if (!auth_str) {
 				ret = -ENOMEM;
 				goto out;
@@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
kobject *kobj, struct kobj_attribute *
 				   const char *buf, size_t count)
 {
 	char *auth_str = NULL;
+	const char *opt;
 	int ret = 0;
 	int cmd;

@@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
kobject *kobj, struct kobj_attribute *
 			ret = tlmi_save_bios_settings("");
 		} else { /* old non-opcode based authentication method (deprecated) */
 			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
+				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
 				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
 						     tlmi_priv.pwd_admin->password,
-						     encoding_options[tlmi_priv.pwd_admin->encoding],
+						     opt,
 						     tlmi_priv.pwd_admin->kbdlang);
 				if (!auth_str) {
 					ret = -ENOMEM;
@@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
kobject *kobj, struct kobj_attribute *
 static struct kobj_attribute save_settings = __ATTR_RW(save_settings);

 /* ---- Debug interface---------------------------------------------------------
*/
-static ssize_t debug_cmd_store(struct kobject *kobj, struct
kobj_attribute *attr,
-				const char *buf, size_t count)
+static ssize_t debug_cmd_store(struct kobject *kobj,
+			       struct kobj_attribute *attr, const char *buf,
+			       size_t count)
 {
 	char *set_str = NULL, *new_setting = NULL;
 	char *auth_str = NULL;
+	const char *opt;
 	int ret;

 	if (!tlmi_priv.can_debug_cmd)
@@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
*kobj, struct kobj_attribute *attr
 		return -ENOMEM;

 	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
+		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
 		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
-				tlmi_priv.pwd_admin->password,
-				encoding_options[tlmi_priv.pwd_admin->encoding],
-				tlmi_priv.pwd_admin->kbdlang);
+				     tlmi_priv.pwd_admin->password,
+				     opt,
+				     tlmi_priv.pwd_admin->kbdlang);
 		if (!auth_str) {
 			ret = -ENOMEM;
 			goto out;
@@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
 						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
 			}
 			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
-					tlmi_priv.pwdcfg.ext.nvme_master_password) {
+			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
 				tlmi_priv.pwd_nvme->pwd_enabled = true;
 				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
 					tlmi_priv.pwd_nvme->index =
-- 
2.52.0

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Mark Pearson]

On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
> This commit handles some column limit overflows (that occur after fixing
> their alignment), i.e. the following check:
>
> CHECK: line length of ... exceeds 100 columns
>
> by defining a constant opt, and replacing the offending
> expression with opt.
>
> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> ---
>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> b/drivers/platform/x86/lenovo/think-lmi.c
> index 1ada4d800383..07ba0d84720a 100644
> --- a/drivers/platform/x86/lenovo/think-lmi.c
> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
> struct kobj_attribute *attr,
>  }
>
>  static ssize_t current_value_store(struct kobject *kobj,
> -		struct kobj_attribute *attr,
> -		const char *buf, size_t count)
> +				   struct kobj_attribute *attr, const char *buf,
> +				   size_t count)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>  	char *set_str = NULL, *new_setting = NULL;
>  	char *auth_str = NULL;
> +	const char *opt;
>  	int ret;
>
>  	if (!tlmi_priv.can_set_bios_settings)
> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>  			ret = tlmi_save_bios_settings("");
>  	} else { /* old non-opcode based authentication method (deprecated) */
>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> -					tlmi_priv.pwd_admin->password,
> -					encoding_options[tlmi_priv.pwd_admin->encoding],
> -					tlmi_priv.pwd_admin->kbdlang);
> +					     tlmi_priv.pwd_admin->password,
> +					     opt,
> +					     tlmi_priv.pwd_admin->kbdlang);
>  			if (!auth_str) {
>  				ret = -ENOMEM;
>  				goto out;
> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
> kobject *kobj, struct kobj_attribute *
>  				   const char *buf, size_t count)
>  {
>  	char *auth_str = NULL;
> +	const char *opt;
>  	int ret = 0;
>  	int cmd;
>
> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
> kobject *kobj, struct kobj_attribute *
>  			ret = tlmi_save_bios_settings("");
>  		} else { /* old non-opcode based authentication method (deprecated) */
>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>  						     tlmi_priv.pwd_admin->password,
> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
> +						     opt,
>  						     tlmi_priv.pwd_admin->kbdlang);
>  				if (!auth_str) {
>  					ret = -ENOMEM;
> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
> kobject *kobj, struct kobj_attribute *
>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>
>  /* ---- Debug 
> interface---------------------------------------------------------
> */
> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> -				const char *buf, size_t count)
> +static ssize_t debug_cmd_store(struct kobject *kobj,
> +			       struct kobj_attribute *attr, const char *buf,
> +			       size_t count)
>  {
>  	char *set_str = NULL, *new_setting = NULL;
>  	char *auth_str = NULL;
> +	const char *opt;
>  	int ret;
>
>  	if (!tlmi_priv.can_debug_cmd)
> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
> *kobj, struct kobj_attribute *attr
>  		return -ENOMEM;
>
>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> -				tlmi_priv.pwd_admin->password,
> -				encoding_options[tlmi_priv.pwd_admin->encoding],
> -				tlmi_priv.pwd_admin->kbdlang);
> +				     tlmi_priv.pwd_admin->password,
> +				     opt,
> +				     tlmi_priv.pwd_admin->kbdlang);
>  		if (!auth_str) {
>  			ret = -ENOMEM;
>  			goto out;
> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>  			}
>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>  					tlmi_priv.pwd_nvme->index =
> -- 
> 2.52.0

I'll defer to the pdx86 reviewers for this set of changes.

This seems to me to make things more complicated than needed, purely to address a 100 column limit. I personally don't like the change.

Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid

Mark

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Benjamin Philip]

"Mark Pearson" <mpearson-lenovo@squebb.ca> writes:

> On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>> This commit handles some column limit overflows (that occur after fixing
>> their alignment), i.e. the following check:
>>
>> CHECK: line length of ... exceeds 100 columns
>>
>> by defining a constant opt, and replacing the offending
>> expression with opt.
>>
>> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>> ---
>>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>>  1 file changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> b/drivers/platform/x86/lenovo/think-lmi.c
>> index 1ada4d800383..07ba0d84720a 100644
>> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>> struct kobj_attribute *attr,
>>  }
>>
>>  static ssize_t current_value_store(struct kobject *kobj,
>> -		struct kobj_attribute *attr,
>> -		const char *buf, size_t count)
>> +				   struct kobj_attribute *attr, const char *buf,
>> +				   size_t count)
>>  {
>>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>  	char *set_str = NULL, *new_setting = NULL;
>>  	char *auth_str = NULL;
>> +	const char *opt;
>>  	int ret;
>>
>>  	if (!tlmi_priv.can_set_bios_settings)
>> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>>  			ret = tlmi_save_bios_settings("");
>>  	} else { /* old non-opcode based authentication method (deprecated) */
>>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> -					tlmi_priv.pwd_admin->password,
>> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>> -					tlmi_priv.pwd_admin->kbdlang);
>> +					     tlmi_priv.pwd_admin->password,
>> +					     opt,
>> +					     tlmi_priv.pwd_admin->kbdlang);
>>  			if (!auth_str) {
>>  				ret = -ENOMEM;
>>  				goto out;
>> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>>  				   const char *buf, size_t count)
>>  {
>>  	char *auth_str = NULL;
>> +	const char *opt;
>>  	int ret = 0;
>>  	int cmd;
>>
>> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>>  			ret = tlmi_save_bios_settings("");
>>  		} else { /* old non-opcode based authentication method (deprecated) */
>>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>  						     tlmi_priv.pwd_admin->password,
>> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>> +						     opt,
>>  						     tlmi_priv.pwd_admin->kbdlang);
>>  				if (!auth_str) {
>>  					ret = -ENOMEM;
>> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>
>>  /* ---- Debug
>> interface---------------------------------------------------------
>> */
>> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>> kobj_attribute *attr,
>> -				const char *buf, size_t count)
>> +static ssize_t debug_cmd_store(struct kobject *kobj,
>> +			       struct kobj_attribute *attr, const char *buf,
>> +			       size_t count)
>>  {
>>  	char *set_str = NULL, *new_setting = NULL;
>>  	char *auth_str = NULL;
>> +	const char *opt;
>>  	int ret;
>>
>>  	if (!tlmi_priv.can_debug_cmd)
>> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>> *kobj, struct kobj_attribute *attr
>>  		return -ENOMEM;
>>
>>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> -				tlmi_priv.pwd_admin->password,
>> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>> -				tlmi_priv.pwd_admin->kbdlang);
>> +				     tlmi_priv.pwd_admin->password,
>> +				     opt,
>> +				     tlmi_priv.pwd_admin->kbdlang);
>>  		if (!auth_str) {
>>  			ret = -ENOMEM;
>>  			goto out;
>> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>>  			}
>>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>>  					tlmi_priv.pwd_nvme->index =
>> --
>> 2.52.0
>
> I'll defer to the pdx86 reviewers for this set of changes.
>
> This seems to me to make things more complicated than needed, purely to address a 100 column limit. I personally don't like the change.
>
> Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>
> Mark

An alternative could be to set this a constant pwd_admin to
tlmi_priv.pwd_admin. There are 13 other references to
tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
meaningful improvement?

Then again, the question arises why we aren't following the same pattern
for all the other heavily used fields under tlmi_priv. Adding more
constants seems to be wrong way forward.

Another option would be to move repeated functionality into other
functions (all the column limit violations seem to be identical and
involve encoding_options and kasprintf in the same way), but a refactor
of this nature seemed *way* beyond the scope of a simple code syle clean
up.

Thus, assigning the required value to a small constant seemed to be the
best immediate solution. I can see why you feel it adds complexity.
Typically in a dynamic language (or even in a declare-as-you-need code
style in C) this is a trivial change, whereas in a top-of-block style
this does seem to add some cruft.

Perhaps we should drop this patch for now? Nevertheless, I think the
column limit violations, long functions, and repeated lines are a sign
that some refactoring is in order.

Thoughts?

-- bp

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Ilpo Järvinen]

On Wed, 24 Dec 2025, Benjamin Philip wrote:
> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
> >> This commit handles some column limit overflows (that occur after fixing
> >> their alignment), i.e. the following check:
> >>
> >> CHECK: line length of ... exceeds 100 columns
> >>
> >> by defining a constant opt, and replacing the offending
> >> expression with opt.
> >>
> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> >> ---
> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
> >>  1 file changed, 19 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> >> b/drivers/platform/x86/lenovo/think-lmi.c
> >> index 1ada4d800383..07ba0d84720a 100644
> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
> >> struct kobj_attribute *attr,
> >>  }
> >>
> >>  static ssize_t current_value_store(struct kobject *kobj,
> >> -		struct kobj_attribute *attr,
> >> -		const char *buf, size_t count)
> >> +				   struct kobj_attribute *attr, const char *buf,
> >> +				   size_t count)
> >>  {
> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> >>  	char *set_str = NULL, *new_setting = NULL;
> >>  	char *auth_str = NULL;
> >> +	const char *opt;
> >>  	int ret;
> >>
> >>  	if (!tlmi_priv.can_set_bios_settings)
> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
> >>  			ret = tlmi_save_bios_settings("");
> >>  	} else { /* old non-opcode based authentication method (deprecated) */
> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >> -					tlmi_priv.pwd_admin->password,
> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
> >> -					tlmi_priv.pwd_admin->kbdlang);
> >> +					     tlmi_priv.pwd_admin->password,
> >> +					     opt,
> >> +					     tlmi_priv.pwd_admin->kbdlang);
> >>  			if (!auth_str) {
> >>  				ret = -ENOMEM;
> >>  				goto out;
> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
> >> kobject *kobj, struct kobj_attribute *
> >>  				   const char *buf, size_t count)
> >>  {
> >>  	char *auth_str = NULL;
> >> +	const char *opt;
> >>  	int ret = 0;
> >>  	int cmd;
> >>
> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
> >> kobject *kobj, struct kobj_attribute *
> >>  			ret = tlmi_save_bios_settings("");
> >>  		} else { /* old non-opcode based authentication method (deprecated) */
> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >>  						     tlmi_priv.pwd_admin->password,
> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
> >> +						     opt,
> >>  						     tlmi_priv.pwd_admin->kbdlang);
> >>  				if (!auth_str) {
> >>  					ret = -ENOMEM;
> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
> >> kobject *kobj, struct kobj_attribute *
> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
> >>
> >>  /* ---- Debug
> >> interface---------------------------------------------------------
> >> */
> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
> >> kobj_attribute *attr,
> >> -				const char *buf, size_t count)
> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
> >> +			       struct kobj_attribute *attr, const char *buf,
> >> +			       size_t count)
> >>  {
> >>  	char *set_str = NULL, *new_setting = NULL;
> >>  	char *auth_str = NULL;
> >> +	const char *opt;
> >>  	int ret;
> >>
> >>  	if (!tlmi_priv.can_debug_cmd)
> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
> >> *kobj, struct kobj_attribute *attr
> >>  		return -ENOMEM;
> >>
> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >> -				tlmi_priv.pwd_admin->password,
> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
> >> -				tlmi_priv.pwd_admin->kbdlang);
> >> +				     tlmi_priv.pwd_admin->password,
> >> +				     opt,
> >> +				     tlmi_priv.pwd_admin->kbdlang);
> >>  		if (!auth_str) {
> >>  			ret = -ENOMEM;
> >>  			goto out;
> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
> >>  			}
> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
> >>  					tlmi_priv.pwd_nvme->index =
> >> --
> >> 2.52.0
> >
> > I'll defer to the pdx86 reviewers for this set of changes.
> >
> > This seems to me to make things more complicated than needed, purely 
> > to address a 100 column limit. I personally don't like the change. 
> >
> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
> >
> > Mark
> 
> An alternative could be to set this a constant pwd_admin to
> tlmi_priv.pwd_admin. There are 13 other references to
> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
> meaningful improvement?

Hi,

The general theme in this driver seems to be that tlmi_priv.pwd_admin 
usually causes long lines so introducing a local variable for it in such 
functions would certainly help.

This probably came to be when pwd_admin was migrated over into tlmi_priv 
which certainly was correct place for it, but case with the cost of adding 
into the line lengths.

> Then again, the question arises why we aren't following the same pattern
> for all the other heavily used fields under tlmi_priv. Adding more
> constants seems to be wrong way forward.

Locally it might make sense on case by case basis, to me it looks 
business as usual to store things into function local vars to control 
deref line lengths.

> Another option would be to move repeated functionality into other
> functions (all the column limit violations seem to be identical and
> involve encoding_options and kasprintf in the same way), but a refactor
> of this nature seemed *way* beyond the scope of a simple code syle clean
> up.
> 
> Thus, assigning the required value to a small constant seemed to be the
> best immediate solution. I can see why you feel it adds complexity.
> Typically in a dynamic language (or even in a declare-as-you-need code
> style in C) this is a trivial change, whereas in a top-of-block style
> this does seem to add some cruft.
> 
> Perhaps we should drop this patch for now? Nevertheless, I think the
> column limit violations, long functions, and repeated lines are a sign
> that some refactoring is in order.
> 
> Thoughts?

As a general note on these changes, I'm wondering if you're planning on 
doing these checkpatch cleanups for a large number of drivers beyond those 
you've already submitted (which you likely won't be able to test for 
real)?

I'm asking this because "fixing" in-tree checkpatch errors/warnings is 
generally not considered something one should be doing without other work 
on the particular drivers (which normally implies proper testing beyond 
compiling). Checkpatch has its uses, but this is not one of them.

-- 
 i.

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Benjamin Philip]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Wed, 24 Dec 2025, Benjamin Philip wrote:
>> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>> >> This commit handles some column limit overflows (that occur after fixing
>> >> their alignment), i.e. the following check:
>> >>
>> >> CHECK: line length of ... exceeds 100 columns
>> >>
>> >> by defining a constant opt, and replacing the offending
>> >> expression with opt.
>> >>
>> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>> >> ---
>> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> >> b/drivers/platform/x86/lenovo/think-lmi.c
>> >> index 1ada4d800383..07ba0d84720a 100644
>> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>> >> struct kobj_attribute *attr,
>> >>  }
>> >>
>> >>  static ssize_t current_value_store(struct kobject *kobj,
>> >> -		struct kobj_attribute *attr,
>> >> -		const char *buf, size_t count)
>> >> +				   struct kobj_attribute *attr, const char *buf,
>> >> +				   size_t count)
>> >>  {
>> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> >>  	char *set_str = NULL, *new_setting = NULL;
>> >>  	char *auth_str = NULL;
>> >> +	const char *opt;
>> >>  	int ret;
>> >>
>> >>  	if (!tlmi_priv.can_set_bios_settings)
>> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>> >>  			ret = tlmi_save_bios_settings("");
>> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >> -					tlmi_priv.pwd_admin->password,
>> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> -					tlmi_priv.pwd_admin->kbdlang);
>> >> +					     tlmi_priv.pwd_admin->password,
>> >> +					     opt,
>> >> +					     tlmi_priv.pwd_admin->kbdlang);
>> >>  			if (!auth_str) {
>> >>  				ret = -ENOMEM;
>> >>  				goto out;
>> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>> >> kobject *kobj, struct kobj_attribute *
>> >>  				   const char *buf, size_t count)
>> >>  {
>> >>  	char *auth_str = NULL;
>> >> +	const char *opt;
>> >>  	int ret = 0;
>> >>  	int cmd;
>> >>
>> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>> >> kobject *kobj, struct kobj_attribute *
>> >>  			ret = tlmi_save_bios_settings("");
>> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >>  						     tlmi_priv.pwd_admin->password,
>> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> +						     opt,
>> >>  						     tlmi_priv.pwd_admin->kbdlang);
>> >>  				if (!auth_str) {
>> >>  					ret = -ENOMEM;
>> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>> >> kobject *kobj, struct kobj_attribute *
>> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>> >>
>> >>  /* ---- Debug
>> >> interface---------------------------------------------------------
>> >> */
>> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>> >> kobj_attribute *attr,
>> >> -				const char *buf, size_t count)
>> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>> >> +			       struct kobj_attribute *attr, const char *buf,
>> >> +			       size_t count)
>> >>  {
>> >>  	char *set_str = NULL, *new_setting = NULL;
>> >>  	char *auth_str = NULL;
>> >> +	const char *opt;
>> >>  	int ret;
>> >>
>> >>  	if (!tlmi_priv.can_debug_cmd)
>> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>> >> *kobj, struct kobj_attribute *attr
>> >>  		return -ENOMEM;
>> >>
>> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >> -				tlmi_priv.pwd_admin->password,
>> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> -				tlmi_priv.pwd_admin->kbdlang);
>> >> +				     tlmi_priv.pwd_admin->password,
>> >> +				     opt,
>> >> +				     tlmi_priv.pwd_admin->kbdlang);
>> >>  		if (!auth_str) {
>> >>  			ret = -ENOMEM;
>> >>  			goto out;
>> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>> >>  			}
>> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>> >>  					tlmi_priv.pwd_nvme->index =
>> >> --
>> >> 2.52.0
>> >
>> > I'll defer to the pdx86 reviewers for this set of changes.
>> >
>> > This seems to me to make things more complicated than needed, purely 
>> > to address a 100 column limit. I personally don't like the change. 
>> >
>> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>> >
>> > Mark
>> 
>> An alternative could be to set this a constant pwd_admin to
>> tlmi_priv.pwd_admin. There are 13 other references to
>> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>> meaningful improvement?
>
> Hi,
>
> The general theme in this driver seems to be that tlmi_priv.pwd_admin 
> usually causes long lines so introducing a local variable for it in such 
> functions would certainly help.
>
> This probably came to be when pwd_admin was migrated over into tlmi_priv 
> which certainly was correct place for it, but case with the cost of adding 
> into the line lengths.
>

In this case, would you prefer that a local variable for pwd_admin be
introduced instead of variables for values in the long lines? 

>> Then again, the question arises why we aren't following the same pattern
>> for all the other heavily used fields under tlmi_priv. Adding more
>> constants seems to be wrong way forward.
>
> Locally it might make sense on case by case basis, to me it looks 
> business as usual to store things into function local vars to control 
> deref line lengths.
>
>> Another option would be to move repeated functionality into other
>> functions (all the column limit violations seem to be identical and
>> involve encoding_options and kasprintf in the same way), but a refactor
>> of this nature seemed *way* beyond the scope of a simple code syle clean
>> up.
>> 
>> Thus, assigning the required value to a small constant seemed to be the
>> best immediate solution. I can see why you feel it adds complexity.
>> Typically in a dynamic language (or even in a declare-as-you-need code
>> style in C) this is a trivial change, whereas in a top-of-block style
>> this does seem to add some cruft.
>> 
>> Perhaps we should drop this patch for now? Nevertheless, I think the
>> column limit violations, long functions, and repeated lines are a sign
>> that some refactoring is in order.
>> 
>> Thoughts?
>
> As a general note on these changes, I'm wondering if you're planning on 
> doing these checkpatch cleanups for a large number of drivers beyond those 
> you've already submitted (which you likely won't be able to test for 
> real)?
>
> I'm asking this because "fixing" in-tree checkpatch errors/warnings is 
> generally not considered something one should be doing without other work 
> on the particular drivers (which normally implies proper testing beyond 
> compiling). Checkpatch has its uses, but this is not one of them.
>
> -- 
>  i.

As of right now, I planned to send a checkpatch cleanup for
thinkpad-acpi at something point, but nothing more than that. I
understand why any fixes imply proper testing. In the case of this
patchset I can definitely test the changes, since the machine I created
these patches uses this driver.

I would have thought that checkpatch "fixes" would have some value alone
(although minimal). I'm guessing you do not encourage coccinelle, sparse
or kasan "fixes" without prior work as well?

I'm sorry if these cleanups have been just noise and a misuse of
checkpatch. My intention with these patches was to get a feel for the
development process, the code base and the tooling before moving
towards any "real" contributions.

-- bp

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Ilpo Järvinen]

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

On Mon, 29 Dec 2025, Benjamin Philip wrote:
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
> >> >> This commit handles some column limit overflows (that occur after fixing
> >> >> their alignment), i.e. the following check:
> >> >>
> >> >> CHECK: line length of ... exceeds 100 columns
> >> >>
> >> >> by defining a constant opt, and replacing the offending
> >> >> expression with opt.
> >> >>
> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> >> >> ---
> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
> >> >>
> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
> >> >> index 1ada4d800383..07ba0d84720a 100644
> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
> >> >> struct kobj_attribute *attr,
> >> >>  }
> >> >>
> >> >>  static ssize_t current_value_store(struct kobject *kobj,
> >> >> -		struct kobj_attribute *attr,
> >> >> -		const char *buf, size_t count)
> >> >> +				   struct kobj_attribute *attr, const char *buf,
> >> >> +				   size_t count)
> >> >>  {
> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> >> >>  	char *set_str = NULL, *new_setting = NULL;
> >> >>  	char *auth_str = NULL;
> >> >> +	const char *opt;
> >> >>  	int ret;
> >> >>
> >> >>  	if (!tlmi_priv.can_set_bios_settings)
> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
> >> >>  			ret = tlmi_save_bios_settings("");
> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >> >> -					tlmi_priv.pwd_admin->password,
> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
> >> >> -					tlmi_priv.pwd_admin->kbdlang);
> >> >> +					     tlmi_priv.pwd_admin->password,
> >> >> +					     opt,
> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
> >> >>  			if (!auth_str) {
> >> >>  				ret = -ENOMEM;
> >> >>  				goto out;
> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
> >> >> kobject *kobj, struct kobj_attribute *
> >> >>  				   const char *buf, size_t count)
> >> >>  {
> >> >>  	char *auth_str = NULL;
> >> >> +	const char *opt;
> >> >>  	int ret = 0;
> >> >>  	int cmd;
> >> >>
> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
> >> >> kobject *kobj, struct kobj_attribute *
> >> >>  			ret = tlmi_save_bios_settings("");
> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >> >>  						     tlmi_priv.pwd_admin->password,
> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
> >> >> +						     opt,
> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
> >> >>  				if (!auth_str) {
> >> >>  					ret = -ENOMEM;
> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
> >> >> kobject *kobj, struct kobj_attribute *
> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
> >> >>
> >> >>  /* ---- Debug
> >> >> interface---------------------------------------------------------
> >> >> */
> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
> >> >> kobj_attribute *attr,
> >> >> -				const char *buf, size_t count)
> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
> >> >> +			       struct kobj_attribute *attr, const char *buf,
> >> >> +			       size_t count)
> >> >>  {
> >> >>  	char *set_str = NULL, *new_setting = NULL;
> >> >>  	char *auth_str = NULL;
> >> >> +	const char *opt;
> >> >>  	int ret;
> >> >>
> >> >>  	if (!tlmi_priv.can_debug_cmd)
> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
> >> >> *kobj, struct kobj_attribute *attr
> >> >>  		return -ENOMEM;
> >> >>
> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
> >> >> -				tlmi_priv.pwd_admin->password,
> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
> >> >> -				tlmi_priv.pwd_admin->kbdlang);
> >> >> +				     tlmi_priv.pwd_admin->password,
> >> >> +				     opt,
> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
> >> >>  		if (!auth_str) {
> >> >>  			ret = -ENOMEM;
> >> >>  			goto out;
> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
> >> >>  			}
> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
> >> >>  					tlmi_priv.pwd_nvme->index =
> >> >> --
> >> >> 2.52.0
> >> >
> >> > I'll defer to the pdx86 reviewers for this set of changes.
> >> >
> >> > This seems to me to make things more complicated than needed, purely 
> >> > to address a 100 column limit. I personally don't like the change. 
> >> >
> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
> >> >
> >> > Mark
> >> 
> >> An alternative could be to set this a constant pwd_admin to
> >> tlmi_priv.pwd_admin. There are 13 other references to
> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
> >> meaningful improvement?
> >
> > Hi,
> >
> > The general theme in this driver seems to be that tlmi_priv.pwd_admin 
> > usually causes long lines so introducing a local variable for it in such 
> > functions would certainly help.
> >
> > This probably came to be when pwd_admin was migrated over into tlmi_priv 
> > which certainly was correct place for it, but case with the cost of adding 
> > into the line lengths.
> >
> 
> In this case, would you prefer that a local variable for pwd_admin be
> introduced instead of variables for values in the long lines? 

Mark who's the maintainer seems to be against it, so I don't want to step
too strongly on his lot in this. It's merely my opinion how we normally 
handle cases like this (I probably didn't express that clearly enough).

That being said, I think there are very good reasons for the 100 limit 
but it also varies how imporant the bit that is very right columnwise is 
(and function parameters that don't contain odd logic in them are quite 
low on that list, only little above container_of() boilerplate).

> >> Then again, the question arises why we aren't following the same pattern
> >> for all the other heavily used fields under tlmi_priv. Adding more
> >> constants seems to be wrong way forward.
> >
> > Locally it might make sense on case by case basis, to me it looks 
> > business as usual to store things into function local vars to control 
> > deref line lengths.
> >
> >> Another option would be to move repeated functionality into other
> >> functions (all the column limit violations seem to be identical and
> >> involve encoding_options and kasprintf in the same way), but a refactor
> >> of this nature seemed *way* beyond the scope of a simple code syle clean
> >> up.
> >> 
> >> Thus, assigning the required value to a small constant seemed to be the
> >> best immediate solution. I can see why you feel it adds complexity.
> >> Typically in a dynamic language (or even in a declare-as-you-need code
> >> style in C) this is a trivial change, whereas in a top-of-block style
> >> this does seem to add some cruft.
> >> 
> >> Perhaps we should drop this patch for now? Nevertheless, I think the
> >> column limit violations, long functions, and repeated lines are a sign
> >> that some refactoring is in order.
> >> 
> >> Thoughts?
> >
> > As a general note on these changes, I'm wondering if you're planning on 
> > doing these checkpatch cleanups for a large number of drivers beyond those 
> > you've already submitted (which you likely won't be able to test for 
> > real)?
> >
> > I'm asking this because "fixing" in-tree checkpatch errors/warnings is 
> > generally not considered something one should be doing without other work 
> > on the particular drivers (which normally implies proper testing beyond 
> > compiling). Checkpatch has its uses, but this is not one of them.
> >
> > -- 
> >  i.
> 
> As of right now, I planned to send a checkpatch cleanup for
> thinkpad-acpi at something point, but nothing more than that. I
> understand why any fixes imply proper testing. In the case of this
> patchset I can definitely test the changes, since the machine I created
> these patches uses this driver.
> 
> I would have thought that checkpatch "fixes" would have some value alone
> (although minimal). I'm guessing you do not encourage coccinelle, sparse
> or kasan "fixes" without prior work as well?

I do run sparse almost daily here and it's warnings are much more valuable 
over checkpatch's (excluding the recent addition of "error: bad constant 
expression" madness thanks to Kees (IIRC) which I now have to filter away 
~forever)!

I'm also under impression that kasan also points out problems which are 
not pure stylistics preferences.

However, when we start to remove extra parenthesis because checkpatch has 
some rule that doesn't like using parenthesis to group logic, and somebody 
has to spend cycles to review those patches, I think we've reached a point 
of negative returns. It's surprisingly hard to visually check such patch 
for correct nesting, so there's a real risk as well with it.

I'm not entirely convinced by all checkpatch rules to begin with, one 
needs to take its output with a grain of salt.

> I'm sorry if these cleanups have been just noise and a misuse of
> checkpatch. My intention with these patches was to get a feel for the
> development process, the code base and the tooling before moving
> towards any "real" contributions.

It's just finding a logic issue has way more value than pure syntax 
changes. Perhaps checkpatch doesn't teach much about tools needed for 
"real" contributions ;-) (I can see some value in learning to deal with 
the patch submission and feedback process though).

And if you really intend to work with these drivers beyond cleanup you're 
now submitting to, I certainly find it less disruptive than doing it on a 
large-scale to random things.

-- 
 i.

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Mark Pearson]

On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>> >> >> This commit handles some column limit overflows (that occur after fixing
>> >> >> their alignment), i.e. the following check:
>> >> >>
>> >> >> CHECK: line length of ... exceeds 100 columns
>> >> >>
>> >> >> by defining a constant opt, and replacing the offending
>> >> >> expression with opt.
>> >> >>
>> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>> >> >> ---
>> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
>> >> >> index 1ada4d800383..07ba0d84720a 100644
>> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>> >> >> struct kobj_attribute *attr,
>> >> >>  }
>> >> >>
>> >> >>  static ssize_t current_value_store(struct kobject *kobj,
>> >> >> -		struct kobj_attribute *attr,
>> >> >> -		const char *buf, size_t count)
>> >> >> +				   struct kobj_attribute *attr, const char *buf,
>> >> >> +				   size_t count)
>> >> >>  {
>> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>> >> >>  	char *auth_str = NULL;
>> >> >> +	const char *opt;
>> >> >>  	int ret;
>> >> >>
>> >> >>  	if (!tlmi_priv.can_set_bios_settings)
>> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>> >> >>  			ret = tlmi_save_bios_settings("");
>> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >> >> -					tlmi_priv.pwd_admin->password,
>> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> >> -					tlmi_priv.pwd_admin->kbdlang);
>> >> >> +					     tlmi_priv.pwd_admin->password,
>> >> >> +					     opt,
>> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
>> >> >>  			if (!auth_str) {
>> >> >>  				ret = -ENOMEM;
>> >> >>  				goto out;
>> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>> >> >> kobject *kobj, struct kobj_attribute *
>> >> >>  				   const char *buf, size_t count)
>> >> >>  {
>> >> >>  	char *auth_str = NULL;
>> >> >> +	const char *opt;
>> >> >>  	int ret = 0;
>> >> >>  	int cmd;
>> >> >>
>> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>> >> >> kobject *kobj, struct kobj_attribute *
>> >> >>  			ret = tlmi_save_bios_settings("");
>> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >> >>  						     tlmi_priv.pwd_admin->password,
>> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> >> +						     opt,
>> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
>> >> >>  				if (!auth_str) {
>> >> >>  					ret = -ENOMEM;
>> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>> >> >> kobject *kobj, struct kobj_attribute *
>> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>> >> >>
>> >> >>  /* ---- Debug
>> >> >> interface---------------------------------------------------------
>> >> >> */
>> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>> >> >> kobj_attribute *attr,
>> >> >> -				const char *buf, size_t count)
>> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>> >> >> +			       struct kobj_attribute *attr, const char *buf,
>> >> >> +			       size_t count)
>> >> >>  {
>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>> >> >>  	char *auth_str = NULL;
>> >> >> +	const char *opt;
>> >> >>  	int ret;
>> >> >>
>> >> >>  	if (!tlmi_priv.can_debug_cmd)
>> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>> >> >> *kobj, struct kobj_attribute *attr
>> >> >>  		return -ENOMEM;
>> >> >>
>> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> >> >> -				tlmi_priv.pwd_admin->password,
>> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>> >> >> -				tlmi_priv.pwd_admin->kbdlang);
>> >> >> +				     tlmi_priv.pwd_admin->password,
>> >> >> +				     opt,
>> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
>> >> >>  		if (!auth_str) {
>> >> >>  			ret = -ENOMEM;
>> >> >>  			goto out;
>> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>> >> >>  			}
>> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>> >> >>  					tlmi_priv.pwd_nvme->index =
>> >> >> --
>> >> >> 2.52.0
>> >> >
>> >> > I'll defer to the pdx86 reviewers for this set of changes.
>> >> >
>> >> > This seems to me to make things more complicated than needed, purely 
>> >> > to address a 100 column limit. I personally don't like the change. 
>> >> >
>> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>> >> >
>> >> > Mark
>> >> 
>> >> An alternative could be to set this a constant pwd_admin to
>> >> tlmi_priv.pwd_admin. There are 13 other references to
>> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>> >> meaningful improvement?
>> >
>> > Hi,
>> >
>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin 
>> > usually causes long lines so introducing a local variable for it in such 
>> > functions would certainly help.
>> >
>> > This probably came to be when pwd_admin was migrated over into tlmi_priv 
>> > which certainly was correct place for it, but case with the cost of adding 
>> > into the line lengths.
>> >
>> 
>> In this case, would you prefer that a local variable for pwd_admin be
>> introduced instead of variables for values in the long lines? 
>
> Mark who's the maintainer seems to be against it, so I don't want to step
> too strongly on his lot in this. It's merely my opinion how we normally 
> handle cases like this (I probably didn't express that clearly enough).
>

I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.

From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks. 
I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.

Mark

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Benjamin Philip]

"Mark Pearson" <mpearson-lenovo@squebb.ca> writes:

> On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
>> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>>> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>>> >> >> This commit handles some column limit overflows (that occur after fixing
>>> >> >> their alignment), i.e. the following check:
>>> >> >>
>>> >> >> CHECK: line length of ... exceeds 100 columns
>>> >> >>
>>> >> >> by defining a constant opt, and replacing the offending
>>> >> >> expression with opt.
>>> >> >>
>>> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>>> >> >> ---
>>> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>>> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>>> >> >>
>>> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>>> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
>>> >> >> index 1ada4d800383..07ba0d84720a 100644
>>> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>>> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>>> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>>> >> >> struct kobj_attribute *attr,
>>> >> >>  }
>>> >> >>
>>> >> >>  static ssize_t current_value_store(struct kobject *kobj,
>>> >> >> -		struct kobj_attribute *attr,
>>> >> >> -		const char *buf, size_t count)
>>> >> >> +				   struct kobj_attribute *attr, const char *buf,
>>> >> >> +				   size_t count)
>>> >> >>  {
>>> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>> >> >>  	char *auth_str = NULL;
>>> >> >> +	const char *opt;
>>> >> >>  	int ret;
>>> >> >>
>>> >> >>  	if (!tlmi_priv.can_set_bios_settings)
>>> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>>> >> >>  			ret = tlmi_save_bios_settings("");
>>> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>>> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>> >> >> -					tlmi_priv.pwd_admin->password,
>>> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>>> >> >> -					tlmi_priv.pwd_admin->kbdlang);
>>> >> >> +					     tlmi_priv.pwd_admin->password,
>>> >> >> +					     opt,
>>> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
>>> >> >>  			if (!auth_str) {
>>> >> >>  				ret = -ENOMEM;
>>> >> >>  				goto out;
>>> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>>> >> >> kobject *kobj, struct kobj_attribute *
>>> >> >>  				   const char *buf, size_t count)
>>> >> >>  {
>>> >> >>  	char *auth_str = NULL;
>>> >> >> +	const char *opt;
>>> >> >>  	int ret = 0;
>>> >> >>  	int cmd;
>>> >> >>
>>> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>>> >> >> kobject *kobj, struct kobj_attribute *
>>> >> >>  			ret = tlmi_save_bios_settings("");
>>> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>>> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>> >> >>  						     tlmi_priv.pwd_admin->password,
>>> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>> >> >> +						     opt,
>>> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
>>> >> >>  				if (!auth_str) {
>>> >> >>  					ret = -ENOMEM;
>>> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>>> >> >> kobject *kobj, struct kobj_attribute *
>>> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>> >> >>
>>> >> >>  /* ---- Debug
>>> >> >> interface---------------------------------------------------------
>>> >> >> */
>>> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>>> >> >> kobj_attribute *attr,
>>> >> >> -				const char *buf, size_t count)
>>> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>>> >> >> +			       struct kobj_attribute *attr, const char *buf,
>>> >> >> +			       size_t count)
>>> >> >>  {
>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>> >> >>  	char *auth_str = NULL;
>>> >> >> +	const char *opt;
>>> >> >>  	int ret;
>>> >> >>
>>> >> >>  	if (!tlmi_priv.can_debug_cmd)
>>> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>>> >> >> *kobj, struct kobj_attribute *attr
>>> >> >>  		return -ENOMEM;
>>> >> >>
>>> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>> >> >> -				tlmi_priv.pwd_admin->password,
>>> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>>> >> >> -				tlmi_priv.pwd_admin->kbdlang);
>>> >> >> +				     tlmi_priv.pwd_admin->password,
>>> >> >> +				     opt,
>>> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
>>> >> >>  		if (!auth_str) {
>>> >> >>  			ret = -ENOMEM;
>>> >> >>  			goto out;
>>> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>>> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>>> >> >>  			}
>>> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>>> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>>> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>>> >> >>  					tlmi_priv.pwd_nvme->index =
>>> >> >> --
>>> >> >> 2.52.0
>>> >> >
>>> >> > I'll defer to the pdx86 reviewers for this set of changes.
>>> >> >
>>> >> > This seems to me to make things more complicated than needed, purely 
>>> >> > to address a 100 column limit. I personally don't like the change. 
>>> >> >
>>> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>>> >> >
>>> >> > Mark
>>> >> 
>>> >> An alternative could be to set this a constant pwd_admin to
>>> >> tlmi_priv.pwd_admin. There are 13 other references to
>>> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>>> >> meaningful improvement?
>>> >
>>> > Hi,
>>> >
>>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin 
>>> > usually causes long lines so introducing a local variable for it in such 
>>> > functions would certainly help.
>>> >
>>> > This probably came to be when pwd_admin was migrated over into tlmi_priv 
>>> > which certainly was correct place for it, but case with the cost of adding 
>>> > into the line lengths.
>>> >
>>> 
>>> In this case, would you prefer that a local variable for pwd_admin be
>>> introduced instead of variables for values in the long lines? 
>>
>> Mark who's the maintainer seems to be against it, so I don't want to step
>> too strongly on his lot in this. It's merely my opinion how we normally 
>> handle cases like this (I probably didn't express that clearly enough).
>>
>
> I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.
>
> From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks. 
> I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.
>
> Mark

Before you do that I can send a v2 with the parentheses changes removed.
If you can point to the tests in particular you run, I can test on a
Thinkpad as well before sending.

-- bp

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Mark Pearson]

On Mon, Dec 29, 2025, at 10:24 PM, Benjamin Philip wrote:
> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>
>> On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
>>> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>>>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>>>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>>>> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>>> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>>>> >> >> This commit handles some column limit overflows (that occur after fixing
>>>> >> >> their alignment), i.e. the following check:
>>>> >> >>
>>>> >> >> CHECK: line length of ... exceeds 100 columns
>>>> >> >>
>>>> >> >> by defining a constant opt, and replacing the offending
>>>> >> >> expression with opt.
>>>> >> >>
>>>> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>>>> >> >> ---
>>>> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>>>> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>>>> >> >>
>>>> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>>>> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
>>>> >> >> index 1ada4d800383..07ba0d84720a 100644
>>>> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>>>> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>>>> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>>>> >> >> struct kobj_attribute *attr,
>>>> >> >>  }
>>>> >> >>
>>>> >> >>  static ssize_t current_value_store(struct kobject *kobj,
>>>> >> >> -		struct kobj_attribute *attr,
>>>> >> >> -		const char *buf, size_t count)
>>>> >> >> +				   struct kobj_attribute *attr, const char *buf,
>>>> >> >> +				   size_t count)
>>>> >> >>  {
>>>> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>> >> >>  	char *auth_str = NULL;
>>>> >> >> +	const char *opt;
>>>> >> >>  	int ret;
>>>> >> >>
>>>> >> >>  	if (!tlmi_priv.can_set_bios_settings)
>>>> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>>>> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>> >> >> -					tlmi_priv.pwd_admin->password,
>>>> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>>>> >> >> -					tlmi_priv.pwd_admin->kbdlang);
>>>> >> >> +					     tlmi_priv.pwd_admin->password,
>>>> >> >> +					     opt,
>>>> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
>>>> >> >>  			if (!auth_str) {
>>>> >> >>  				ret = -ENOMEM;
>>>> >> >>  				goto out;
>>>> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>> >> >>  				   const char *buf, size_t count)
>>>> >> >>  {
>>>> >> >>  	char *auth_str = NULL;
>>>> >> >> +	const char *opt;
>>>> >> >>  	int ret = 0;
>>>> >> >>  	int cmd;
>>>> >> >>
>>>> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>>>> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>> >> >>  						     tlmi_priv.pwd_admin->password,
>>>> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>>> >> >> +						     opt,
>>>> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
>>>> >> >>  				if (!auth_str) {
>>>> >> >>  					ret = -ENOMEM;
>>>> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>>> >> >>
>>>> >> >>  /* ---- Debug
>>>> >> >> interface---------------------------------------------------------
>>>> >> >> */
>>>> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>>>> >> >> kobj_attribute *attr,
>>>> >> >> -				const char *buf, size_t count)
>>>> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>>>> >> >> +			       struct kobj_attribute *attr, const char *buf,
>>>> >> >> +			       size_t count)
>>>> >> >>  {
>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>> >> >>  	char *auth_str = NULL;
>>>> >> >> +	const char *opt;
>>>> >> >>  	int ret;
>>>> >> >>
>>>> >> >>  	if (!tlmi_priv.can_debug_cmd)
>>>> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>>>> >> >> *kobj, struct kobj_attribute *attr
>>>> >> >>  		return -ENOMEM;
>>>> >> >>
>>>> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>> >> >> -				tlmi_priv.pwd_admin->password,
>>>> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>>>> >> >> -				tlmi_priv.pwd_admin->kbdlang);
>>>> >> >> +				     tlmi_priv.pwd_admin->password,
>>>> >> >> +				     opt,
>>>> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
>>>> >> >>  		if (!auth_str) {
>>>> >> >>  			ret = -ENOMEM;
>>>> >> >>  			goto out;
>>>> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>>>> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>>>> >> >>  			}
>>>> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>>>> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>>>> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>>>> >> >>  					tlmi_priv.pwd_nvme->index =
>>>> >> >> --
>>>> >> >> 2.52.0
>>>> >> >
>>>> >> > I'll defer to the pdx86 reviewers for this set of changes.
>>>> >> >
>>>> >> > This seems to me to make things more complicated than needed, purely 
>>>> >> > to address a 100 column limit. I personally don't like the change. 
>>>> >> >
>>>> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>>>> >> >
>>>> >> > Mark
>>>> >> 
>>>> >> An alternative could be to set this a constant pwd_admin to
>>>> >> tlmi_priv.pwd_admin. There are 13 other references to
>>>> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>>>> >> meaningful improvement?
>>>> >
>>>> > Hi,
>>>> >
>>>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin 
>>>> > usually causes long lines so introducing a local variable for it in such 
>>>> > functions would certainly help.
>>>> >
>>>> > This probably came to be when pwd_admin was migrated over into tlmi_priv 
>>>> > which certainly was correct place for it, but case with the cost of adding 
>>>> > into the line lengths.
>>>> >
>>>> 
>>>> In this case, would you prefer that a local variable for pwd_admin be
>>>> introduced instead of variables for values in the long lines? 
>>>
>>> Mark who's the maintainer seems to be against it, so I don't want to step
>>> too strongly on his lot in this. It's merely my opinion how we normally 
>>> handle cases like this (I probably didn't express that clearly enough).
>>>
>>
>> I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.
>>
>> From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks. 
>> I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.
>>
>> Mark
>
> Before you do that I can send a v2 with the parentheses changes removed.
> If you can point to the tests in particular you run, I can test on a
> Thinkpad as well before sending.
>
OK.
Testing - nothing too exciting, just check that everything shows up under /sys/class/firmware-attributes and check a setting can be changed back/forth.

Mark

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Benjamin Philip]

"Mark Pearson" <mpearson-lenovo@squebb.ca> writes:

> On Mon, Dec 29, 2025, at 10:24 PM, Benjamin Philip wrote:
>> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>
>>> On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
>>>> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>>>>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>>>>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>>>>> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>>>> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>>>>> >> >> This commit handles some column limit overflows (that occur after fixing
>>>>> >> >> their alignment), i.e. the following check:
>>>>> >> >>
>>>>> >> >> CHECK: line length of ... exceeds 100 columns
>>>>> >> >>
>>>>> >> >> by defining a constant opt, and replacing the offending
>>>>> >> >> expression with opt.
>>>>> >> >>
>>>>> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>>>>> >> >> ---
>>>>> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>>>>> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>>>>> >> >>
>>>>> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>>>>> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
>>>>> >> >> index 1ada4d800383..07ba0d84720a 100644
>>>>> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>>>>> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>>>>> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>>>>> >> >> struct kobj_attribute *attr,
>>>>> >> >>  }
>>>>> >> >>
>>>>> >> >>  static ssize_t current_value_store(struct kobject *kobj,
>>>>> >> >> -		struct kobj_attribute *attr,
>>>>> >> >> -		const char *buf, size_t count)
>>>>> >> >> +				   struct kobj_attribute *attr, const char *buf,
>>>>> >> >> +				   size_t count)
>>>>> >> >>  {
>>>>> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>>> >> >>  	char *auth_str = NULL;
>>>>> >> >> +	const char *opt;
>>>>> >> >>  	int ret;
>>>>> >> >>
>>>>> >> >>  	if (!tlmi_priv.can_set_bios_settings)
>>>>> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>>> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>>>>> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>> >> >> -					tlmi_priv.pwd_admin->password,
>>>>> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>> >> >> -					tlmi_priv.pwd_admin->kbdlang);
>>>>> >> >> +					     tlmi_priv.pwd_admin->password,
>>>>> >> >> +					     opt,
>>>>> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
>>>>> >> >>  			if (!auth_str) {
>>>>> >> >>  				ret = -ENOMEM;
>>>>> >> >>  				goto out;
>>>>> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>> >> >>  				   const char *buf, size_t count)
>>>>> >> >>  {
>>>>> >> >>  	char *auth_str = NULL;
>>>>> >> >> +	const char *opt;
>>>>> >> >>  	int ret = 0;
>>>>> >> >>  	int cmd;
>>>>> >> >>
>>>>> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>>> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>>>>> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>> >> >>  						     tlmi_priv.pwd_admin->password,
>>>>> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>> >> >> +						     opt,
>>>>> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
>>>>> >> >>  				if (!auth_str) {
>>>>> >> >>  					ret = -ENOMEM;
>>>>> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>>>> >> >>
>>>>> >> >>  /* ---- Debug
>>>>> >> >> interface---------------------------------------------------------
>>>>> >> >> */
>>>>> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>>>>> >> >> kobj_attribute *attr,
>>>>> >> >> -				const char *buf, size_t count)
>>>>> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>>>>> >> >> +			       struct kobj_attribute *attr, const char *buf,
>>>>> >> >> +			       size_t count)
>>>>> >> >>  {
>>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>>> >> >>  	char *auth_str = NULL;
>>>>> >> >> +	const char *opt;
>>>>> >> >>  	int ret;
>>>>> >> >>
>>>>> >> >>  	if (!tlmi_priv.can_debug_cmd)
>>>>> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>>>>> >> >> *kobj, struct kobj_attribute *attr
>>>>> >> >>  		return -ENOMEM;
>>>>> >> >>
>>>>> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>> >> >> -				tlmi_priv.pwd_admin->password,
>>>>> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>> >> >> -				tlmi_priv.pwd_admin->kbdlang);
>>>>> >> >> +				     tlmi_priv.pwd_admin->password,
>>>>> >> >> +				     opt,
>>>>> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
>>>>> >> >>  		if (!auth_str) {
>>>>> >> >>  			ret = -ENOMEM;
>>>>> >> >>  			goto out;
>>>>> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>>>>> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>>>>> >> >>  			}
>>>>> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>>>>> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>>> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>>> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>>>>> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>>>>> >> >>  					tlmi_priv.pwd_nvme->index =
>>>>> >> >> --
>>>>> >> >> 2.52.0
>>>>> >> >
>>>>> >> > I'll defer to the pdx86 reviewers for this set of changes.
>>>>> >> >
>>>>> >> > This seems to me to make things more complicated than needed, purely
>>>>> >> > to address a 100 column limit. I personally don't like the change.
>>>>> >> >
>>>>> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>>>>> >> >
>>>>> >> > Mark
>>>>> >>
>>>>> >> An alternative could be to set this a constant pwd_admin to
>>>>> >> tlmi_priv.pwd_admin. There are 13 other references to
>>>>> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>>>>> >> meaningful improvement?
>>>>> >
>>>>> > Hi,
>>>>> >
>>>>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin
>>>>> > usually causes long lines so introducing a local variable for it in such
>>>>> > functions would certainly help.
>>>>> >
>>>>> > This probably came to be when pwd_admin was migrated over into tlmi_priv
>>>>> > which certainly was correct place for it, but case with the cost of adding
>>>>> > into the line lengths.
>>>>> >
>>>>>
>>>>> In this case, would you prefer that a local variable for pwd_admin be
>>>>> introduced instead of variables for values in the long lines?
>>>>
>>>> Mark who's the maintainer seems to be against it, so I don't want to step
>>>> too strongly on his lot in this. It's merely my opinion how we normally
>>>> handle cases like this (I probably didn't express that clearly enough).
>>>>
>>>
>>> I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.
>>>
>>> From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks.
>>> I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.
>>>
>>> Mark
>>
>> Before you do that I can send a v2 with the parentheses changes removed.
>> If you can point to the tests in particular you run, I can test on a
>> Thinkpad as well before sending.
>>
> OK.
> Testing - nothing too exciting, just check that everything shows up under /sys/class/firmware-attributes and check a setting can be changed back/forth.
>
> Mark

If you're just testing the sysfs interface, why not write a kselftest?

On a side note, looking at tools/testing/selftests/drivers/platform/x86/
I noticed that there are only tests for some intel driver. Am I missing
something here? Have tests for all the other drivers been left out due
to time/resource constraints, or is there some other reason?

-- bp

Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow

[Mark Pearson]

On Tue, Jan 6, 2026, at 2:33 PM, Benjamin Philip wrote:
> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>
>> On Mon, Dec 29, 2025, at 10:24 PM, Benjamin Philip wrote:
>>> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>>
>>>> On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
>>>>> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>>>>>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>>>>>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>>>>>> >> "Mark Pearson" <mpearson-lenovo@squebb.ca> writes:
>>>>>> >> > On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>>>>>> >> >> This commit handles some column limit overflows (that occur after fixing
>>>>>> >> >> their alignment), i.e. the following check:
>>>>>> >> >>
>>>>>> >> >> CHECK: line length of ... exceeds 100 columns
>>>>>> >> >>
>>>>>> >> >> by defining a constant opt, and replacing the offending
>>>>>> >> >> expression with opt.
>>>>>> >> >>
>>>>>> >> >> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
>>>>>> >> >> ---
>>>>>> >> >>  drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>>>>>> >> >>  1 file changed, 19 insertions(+), 12 deletions(-)
>>>>>> >> >>
>>>>>> >> >> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>>>>>> >> >> b/drivers/platform/x86/lenovo/think-lmi.c
>>>>>> >> >> index 1ada4d800383..07ba0d84720a 100644
>>>>>> >> >> --- a/drivers/platform/x86/lenovo/think-lmi.c
>>>>>> >> >> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>>>>>> >> >> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>>>>>> >> >> struct kobj_attribute *attr,
>>>>>> >> >>  }
>>>>>> >> >>
>>>>>> >> >>  static ssize_t current_value_store(struct kobject *kobj,
>>>>>> >> >> -		struct kobj_attribute *attr,
>>>>>> >> >> -		const char *buf, size_t count)
>>>>>> >> >> +				   struct kobj_attribute *attr, const char *buf,
>>>>>> >> >> +				   size_t count)
>>>>>> >> >>  {
>>>>>> >> >>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>>>> >> >>  	char *auth_str = NULL;
>>>>>> >> >> +	const char *opt;
>>>>>> >> >>  	int ret;
>>>>>> >> >>
>>>>>> >> >>  	if (!tlmi_priv.can_set_bios_settings)
>>>>>> >> >> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>>>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>>>> >> >>  	} else { /* old non-opcode based authentication method (deprecated) */
>>>>>> >> >>  		if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>>> >> >> +			opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>>> >> >>  			auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>>> >> >> -					tlmi_priv.pwd_admin->password,
>>>>>> >> >> -					encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>>> >> >> -					tlmi_priv.pwd_admin->kbdlang);
>>>>>> >> >> +					     tlmi_priv.pwd_admin->password,
>>>>>> >> >> +					     opt,
>>>>>> >> >> +					     tlmi_priv.pwd_admin->kbdlang);
>>>>>> >> >>  			if (!auth_str) {
>>>>>> >> >>  				ret = -ENOMEM;
>>>>>> >> >>  				goto out;
>>>>>> >> >> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>>> >> >>  				   const char *buf, size_t count)
>>>>>> >> >>  {
>>>>>> >> >>  	char *auth_str = NULL;
>>>>>> >> >> +	const char *opt;
>>>>>> >> >>  	int ret = 0;
>>>>>> >> >>  	int cmd;
>>>>>> >> >>
>>>>>> >> >> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>>> >> >>  			ret = tlmi_save_bios_settings("");
>>>>>> >> >>  		} else { /* old non-opcode based authentication method (deprecated) */
>>>>>> >> >>  			if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>>> >> >> +				opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>>> >> >>  				auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>>> >> >>  						     tlmi_priv.pwd_admin->password,
>>>>>> >> >> -						     encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>>> >> >> +						     opt,
>>>>>> >> >>  						     tlmi_priv.pwd_admin->kbdlang);
>>>>>> >> >>  				if (!auth_str) {
>>>>>> >> >>  					ret = -ENOMEM;
>>>>>> >> >> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>>>>>> >> >> kobject *kobj, struct kobj_attribute *
>>>>>> >> >>  static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>>>>> >> >>
>>>>>> >> >>  /* ---- Debug
>>>>>> >> >> interface---------------------------------------------------------
>>>>>> >> >> */
>>>>>> >> >> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>>>>>> >> >> kobj_attribute *attr,
>>>>>> >> >> -				const char *buf, size_t count)
>>>>>> >> >> +static ssize_t debug_cmd_store(struct kobject *kobj,
>>>>>> >> >> +			       struct kobj_attribute *attr, const char *buf,
>>>>>> >> >> +			       size_t count)
>>>>>> >> >>  {
>>>>>> >> >>  	char *set_str = NULL, *new_setting = NULL;
>>>>>> >> >>  	char *auth_str = NULL;
>>>>>> >> >> +	const char *opt;
>>>>>> >> >>  	int ret;
>>>>>> >> >>
>>>>>> >> >>  	if (!tlmi_priv.can_debug_cmd)
>>>>>> >> >> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>>>>>> >> >> *kobj, struct kobj_attribute *attr
>>>>>> >> >>  		return -ENOMEM;
>>>>>> >> >>
>>>>>> >> >>  	if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>>>>>> >> >> +		opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>>>>>> >> >>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>>>>>> >> >> -				tlmi_priv.pwd_admin->password,
>>>>>> >> >> -				encoding_options[tlmi_priv.pwd_admin->encoding],
>>>>>> >> >> -				tlmi_priv.pwd_admin->kbdlang);
>>>>>> >> >> +				     tlmi_priv.pwd_admin->password,
>>>>>> >> >> +				     opt,
>>>>>> >> >> +				     tlmi_priv.pwd_admin->kbdlang);
>>>>>> >> >>  		if (!auth_str) {
>>>>>> >> >>  			ret = -ENOMEM;
>>>>>> >> >>  			goto out;
>>>>>> >> >> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>>>>>> >> >>  						ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>>>>>> >> >>  			}
>>>>>> >> >>  			if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>>>>>> >> >> -					tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>>>> >> >> +			    tlmi_priv.pwdcfg.ext.nvme_master_password) {
>>>>>> >> >>  				tlmi_priv.pwd_nvme->pwd_enabled = true;
>>>>>> >> >>  				if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>>>>>> >> >>  					tlmi_priv.pwd_nvme->index =
>>>>>> >> >> --
>>>>>> >> >> 2.52.0
>>>>>> >> >
>>>>>> >> > I'll defer to the pdx86 reviewers for this set of changes.
>>>>>> >> >
>>>>>> >> > This seems to me to make things more complicated than needed, purely
>>>>>> >> > to address a 100 column limit. I personally don't like the change.
>>>>>> >> >
>>>>>> >> > Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>>>>>> >> >
>>>>>> >> > Mark
>>>>>> >>
>>>>>> >> An alternative could be to set this a constant pwd_admin to
>>>>>> >> tlmi_priv.pwd_admin. There are 13 other references to
>>>>>> >> tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
>>>>>> >> meaningful improvement?
>>>>>> >
>>>>>> > Hi,
>>>>>> >
>>>>>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin
>>>>>> > usually causes long lines so introducing a local variable for it in such
>>>>>> > functions would certainly help.
>>>>>> >
>>>>>> > This probably came to be when pwd_admin was migrated over into tlmi_priv
>>>>>> > which certainly was correct place for it, but case with the cost of adding
>>>>>> > into the line lengths.
>>>>>> >
>>>>>>
>>>>>> In this case, would you prefer that a local variable for pwd_admin be
>>>>>> introduced instead of variables for values in the long lines?
>>>>>
>>>>> Mark who's the maintainer seems to be against it, so I don't want to step
>>>>> too strongly on his lot in this. It's merely my opinion how we normally
>>>>> handle cases like this (I probably didn't express that clearly enough).
>>>>>
>>>>
>>>> I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.
>>>>
>>>> From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks.
>>>> I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.
>>>>
>>>> Mark
>>>
>>> Before you do that I can send a v2 with the parentheses changes removed.
>>> If you can point to the tests in particular you run, I can test on a
>>> Thinkpad as well before sending.
>>>
>> OK.
>> Testing - nothing too exciting, just check that everything shows up under /sys/class/firmware-attributes and check a setting can be changed back/forth.
>>
>> Mark
>
> If you're just testing the sysfs interface, why not write a kselftest?
>
> On a side note, looking at tools/testing/selftests/drivers/platform/x86/
> I noticed that there are only tests for some intel driver. Am I missing
> something here? Have tests for all the other drivers been left out due
> to time/resource constraints, or is there some other reason?
>
Honestly, never really looked at it - time/resource constraints is probably the most accurate excuse.
I also find, for most of what I do, that testing on real HW is better as that's where the problems show up. It's also where we/Lenovo can add more value for the Linux community (we have the HW after all).
Specifically for this case, reboots are needed when changing attributes, which I'm guessing could be tricky.

But I should probably find some time to look at more testing....I shall throw it on the todo heap :) Thanks for the pointer

Mark

[PATCH 5/5] platform/x86: think-lmi: Clean up alignment

[Benjamin Philip]

This commit fixes multiple instances of the following checkpatch check:

CHECK: Alignment should match open parenthesis

Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
---
 drivers/platform/x86/lenovo/think-lmi.c | 75 +++++++++++++------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/lenovo/think-lmi.c
b/drivers/platform/x86/lenovo/think-lmi.c
index 07ba0d84720a..ccb767c3972e 100644
--- a/drivers/platform/x86/lenovo/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -424,7 +424,7 @@ static int tlmi_get_bios_selections(const char
*item, char **value)

 /* ---- Authentication sysfs
--------------------------------------------------------- */
 static ssize_t is_enabled_show(struct kobject *kobj, struct
kobj_attribute *attr,
-					  char *buf)
+			       char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

@@ -513,7 +513,7 @@ static ssize_t new_password_store(struct kobject *kobj,
 		 */
 		if (tlmi_priv.pwd_admin->pwd_enabled &&
strlen(tlmi_priv.pwd_admin->password)) {
 			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
-					tlmi_priv.pwd_admin->password);
+						  tlmi_priv.pwd_admin->password);
 			if (ret)
 				goto out;
 		}
@@ -527,8 +527,8 @@ static ssize_t new_password_store(struct kobject *kobj,
 	} else {
 		/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
 		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
-				setting->pwd_type, setting->password, new_pwd,
-				encoding_options[setting->encoding], setting->kbdlang);
+				     setting->pwd_type, setting->password, new_pwd,
+				     encoding_options[setting->encoding], setting->kbdlang);
 		if (!auth_str) {
 			ret = -ENOMEM;
 			goto out;
@@ -545,7 +545,7 @@ static ssize_t new_password_store(struct kobject *kobj,
 static struct kobj_attribute auth_new_password = __ATTR_WO(new_password);

 static ssize_t min_password_length_show(struct kobject *kobj, struct
kobj_attribute *attr,
-			 char *buf)
+					char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

@@ -555,16 +555,17 @@ static ssize_t min_password_length_show(struct
kobject *kobj, struct kobj_attrib
 static struct kobj_attribute auth_min_pass_length =
__ATTR_RO(min_password_length);

 static ssize_t max_password_length_show(struct kobject *kobj, struct
kobj_attribute *attr,
-			 char *buf)
+					char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	return sysfs_emit(buf, "%d\n", setting->maxlen);
 }
+
 static struct kobj_attribute auth_max_pass_length =
__ATTR_RO(max_password_length);

 static ssize_t mechanism_show(struct kobject *kobj, struct
kobj_attribute *attr,
-			 char *buf)
+			      char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

@@ -601,16 +602,15 @@ static ssize_t encoding_store(struct kobject
*kobj, struct kobj_attribute *attr,
 static struct kobj_attribute auth_encoding = __ATTR_RW(encoding);

 static ssize_t kbdlang_show(struct kobject *kobj, struct kobj_attribute *attr,
-			 char *buf)
+			    char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	return sysfs_emit(buf, "%s\n", setting->kbdlang);
 }

-static ssize_t kbdlang_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+static ssize_t kbdlang_store(struct kobject *kobj, struct kobj_attribute *attr,
+			     const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	int length;
@@ -665,16 +665,15 @@ static ssize_t index_store(struct kobject *kobj,
struct kobj_attribute *attr,
 static struct kobj_attribute auth_index = __ATTR_RW(index);

 static ssize_t level_show(struct kobject *kobj, struct kobj_attribute *attr,
-			 char *buf)
+			  char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);

 	return sysfs_emit(buf, "%s\n", level_options[setting->level]);
 }

-static ssize_t level_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+static ssize_t level_store(struct kobject *kobj, struct kobj_attribute *attr,
+			   const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	int i;
@@ -731,8 +730,9 @@ static ssize_t cert_thumbprint(char *buf, const
char *arg, int count)

 static char *thumbtypes[] = {"Md5", "Sha1", "Sha256"};

-static ssize_t certificate_thumbprint_show(struct kobject *kobj,
struct kobj_attribute *attr,
-			 char *buf)
+static ssize_t certificate_thumbprint_show(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   char *buf)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	unsigned int i;
@@ -764,8 +764,8 @@ static ssize_t certificate_thumbprint_show(struct
kobject *kobj, struct kobj_att
 static struct kobj_attribute auth_cert_thumb =
__ATTR_RO(certificate_thumbprint);

 static ssize_t cert_to_password_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+				      struct kobj_attribute *attr,
+				      const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	char *auth_str, *passwd;
@@ -809,8 +809,8 @@ enum cert_install_mode {
 };

 static ssize_t certificate_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+				 struct kobj_attribute *attr, const char *buf,
+				 size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	enum cert_install_mode install_mode = TLMI_CERT_INSTALL;
@@ -913,8 +913,8 @@ static ssize_t certificate_store(struct kobject *kobj,
 static struct kobj_attribute auth_certificate = __ATTR_WO(certificate);

 static ssize_t signature_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+			       struct kobj_attribute *attr, const char *buf,
+			       size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	char *new_signature;
@@ -940,8 +940,8 @@ static ssize_t signature_store(struct kobject *kobj,
 static struct kobj_attribute auth_signature = __ATTR_WO(signature);

 static ssize_t save_signature_store(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  const char *buf, size_t count)
+				    struct kobj_attribute *attr,
+				    const char *buf, size_t count)
 {
 	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
 	char *new_signature;
@@ -1029,8 +1029,8 @@ static const struct attribute_group auth_attr_group = {
 __ATTRIBUTE_GROUPS(auth_attr);

 /* ---- Attributes sysfs
--------------------------------------------------------- */
-static ssize_t display_name_show(struct kobject *kobj, struct
kobj_attribute *attr,
-		char *buf)
+static ssize_t display_name_show(struct kobject *kobj,
+				 struct kobj_attribute *attr, char *buf)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);

@@ -1069,7 +1069,7 @@ static ssize_t possible_values_show(struct
kobject *kobj, struct kobj_attribute
 }

 static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
-		char *buf)
+			 char *buf)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);

@@ -1491,15 +1491,15 @@ static int tlmi_sysfs_init(void)
 {
 	int i, ret;

-	tlmi_priv.class_dev = device_create(&firmware_attributes_class,
NULL, MKDEV(0, 0),
-			NULL, "%s", "thinklmi");
+	tlmi_priv.class_dev = device_create(&firmware_attributes_class, NULL,
+					    MKDEV(0, 0), NULL, "%s", "thinklmi");
 	if (IS_ERR(tlmi_priv.class_dev)) {
 		ret = PTR_ERR(tlmi_priv.class_dev);
 		goto fail_class_created;
 	}

 	tlmi_priv.attribute_kset = kset_create_and_add("attributes", NULL,
-			&tlmi_priv.class_dev->kobj);
+						       &tlmi_priv.class_dev->kobj);
 	if (!tlmi_priv.attribute_kset) {
 		ret = -ENOMEM;
 		goto fail_device_created;
@@ -1637,8 +1637,8 @@ static int tlmi_analyze(struct wmi_device *wdev)
 		tlmi_priv.opcode_support = true;

 	if (wmi_has_guid(LENOVO_SET_BIOS_CERT_GUID) &&
-		wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
-		wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
+	    wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
+	    wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
 		tlmi_priv.certificate_support = true;

 	/* ThinkCenter uses different GUIDs for certificate support */
@@ -1693,7 +1693,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
 						       &setting->possible_values);
 			if (ret || !setting->possible_values)
 				pr_info("Error retrieving possible values for %d : %s\n",
-						i, setting->display_name);
+					i, setting->display_name);
 		} else {
 			/*
 			 * Older Thinkstations don't support the bios_selections API.
@@ -1710,8 +1710,9 @@ static int tlmi_analyze(struct wmi_device *wdev)
 					optend = strstr(optstart, "]");
 					if (optend)
 						setting->possible_values =
-							kstrndup(optstart, optend - optstart,
-									GFP_KERNEL);
+							kstrndup(optstart,
+								 optend - optstart,
+								 GFP_KERNEL);
 				}
 				kfree(optitem);
 			}
@@ -1772,7 +1773,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
 		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 ||
-					tlmi_priv.pwdcfg.ext.hdd_master_password) {
+			    tlmi_priv.pwdcfg.ext.hdd_master_password) {
 				tlmi_priv.pwd_hdd->pwd_enabled = true;
 				if (tlmi_priv.pwdcfg.ext.hdd_master_password)
 					tlmi_priv.pwd_hdd->index =
-- 
2.52.0

Re: [PATCH 5/5] platform/x86: think-lmi: Clean up alignment

[Mark Pearson]

On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
> This commit fixes multiple instances of the following checkpatch check:
>
> CHECK: Alignment should match open parenthesis
>
> Signed-off-by: Benjamin Philip <benjamin.philip495@gmail.com>
> ---
>  drivers/platform/x86/lenovo/think-lmi.c | 75 +++++++++++++------------
>  1 file changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
> b/drivers/platform/x86/lenovo/think-lmi.c
> index 07ba0d84720a..ccb767c3972e 100644
> --- a/drivers/platform/x86/lenovo/think-lmi.c
> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> @@ -424,7 +424,7 @@ static int tlmi_get_bios_selections(const char
> *item, char **value)
>
>  /* ---- Authentication sysfs
> --------------------------------------------------------- */
>  static ssize_t is_enabled_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> -					  char *buf)
> +			       char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
> @@ -513,7 +513,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>  		 */
>  		if (tlmi_priv.pwd_admin->pwd_enabled &&
> strlen(tlmi_priv.pwd_admin->password)) {
>  			ret = tlmi_opcode_setting("WmiOpcodePasswordAdmin",
> -					tlmi_priv.pwd_admin->password);
> +						  tlmi_priv.pwd_admin->password);
>  			if (ret)
>  				goto out;
>  		}
> @@ -527,8 +527,8 @@ static ssize_t new_password_store(struct kobject *kobj,
>  	} else {
>  		/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>  		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
> -				setting->pwd_type, setting->password, new_pwd,
> -				encoding_options[setting->encoding], setting->kbdlang);
> +				     setting->pwd_type, setting->password, new_pwd,
> +				     encoding_options[setting->encoding], setting->kbdlang);
>  		if (!auth_str) {
>  			ret = -ENOMEM;
>  			goto out;
> @@ -545,7 +545,7 @@ static ssize_t new_password_store(struct kobject *kobj,
>  static struct kobj_attribute auth_new_password = __ATTR_WO(new_password);
>
>  static ssize_t min_password_length_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> -			 char *buf)
> +					char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
> @@ -555,16 +555,17 @@ static ssize_t min_password_length_show(struct
> kobject *kobj, struct kobj_attrib
>  static struct kobj_attribute auth_min_pass_length =
> __ATTR_RO(min_password_length);
>
>  static ssize_t max_password_length_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> -			 char *buf)
> +					char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	return sysfs_emit(buf, "%d\n", setting->maxlen);
>  }
> +
>  static struct kobj_attribute auth_max_pass_length =
> __ATTR_RO(max_password_length);
>
>  static ssize_t mechanism_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> -			 char *buf)
> +			      char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
> @@ -601,16 +602,15 @@ static ssize_t encoding_store(struct kobject
> *kobj, struct kobj_attribute *attr,
>  static struct kobj_attribute auth_encoding = __ATTR_RW(encoding);
>
>  static ssize_t kbdlang_show(struct kobject *kobj, struct kobj_attribute *attr,
> -			 char *buf)
> +			    char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	return sysfs_emit(buf, "%s\n", setting->kbdlang);
>  }
>
> -static ssize_t kbdlang_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +static ssize_t kbdlang_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			     const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	int length;
> @@ -665,16 +665,15 @@ static ssize_t index_store(struct kobject *kobj,
> struct kobj_attribute *attr,
>  static struct kobj_attribute auth_index = __ATTR_RW(index);
>
>  static ssize_t level_show(struct kobject *kobj, struct kobj_attribute *attr,
> -			 char *buf)
> +			  char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>
>  	return sysfs_emit(buf, "%s\n", level_options[setting->level]);
>  }
>
> -static ssize_t level_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +static ssize_t level_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	int i;
> @@ -731,8 +730,9 @@ static ssize_t cert_thumbprint(char *buf, const
> char *arg, int count)
>
>  static char *thumbtypes[] = {"Md5", "Sha1", "Sha256"};
>
> -static ssize_t certificate_thumbprint_show(struct kobject *kobj,
> struct kobj_attribute *attr,
> -			 char *buf)
> +static ssize_t certificate_thumbprint_show(struct kobject *kobj,
> +					   struct kobj_attribute *attr,
> +					   char *buf)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	unsigned int i;
> @@ -764,8 +764,8 @@ static ssize_t certificate_thumbprint_show(struct
> kobject *kobj, struct kobj_att
>  static struct kobj_attribute auth_cert_thumb =
> __ATTR_RO(certificate_thumbprint);
>
>  static ssize_t cert_to_password_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +				      struct kobj_attribute *attr,
> +				      const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	char *auth_str, *passwd;
> @@ -809,8 +809,8 @@ enum cert_install_mode {
>  };
>
>  static ssize_t certificate_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +				 struct kobj_attribute *attr, const char *buf,
> +				 size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	enum cert_install_mode install_mode = TLMI_CERT_INSTALL;
> @@ -913,8 +913,8 @@ static ssize_t certificate_store(struct kobject *kobj,
>  static struct kobj_attribute auth_certificate = __ATTR_WO(certificate);
>
>  static ssize_t signature_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +			       struct kobj_attribute *attr, const char *buf,
> +			       size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	char *new_signature;
> @@ -940,8 +940,8 @@ static ssize_t signature_store(struct kobject *kobj,
>  static struct kobj_attribute auth_signature = __ATTR_WO(signature);
>
>  static ssize_t save_signature_store(struct kobject *kobj,
> -				  struct kobj_attribute *attr,
> -				  const char *buf, size_t count)
> +				    struct kobj_attribute *attr,
> +				    const char *buf, size_t count)
>  {
>  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>  	char *new_signature;
> @@ -1029,8 +1029,8 @@ static const struct attribute_group auth_attr_group = {
>  __ATTRIBUTE_GROUPS(auth_attr);
>
>  /* ---- Attributes sysfs
> --------------------------------------------------------- */
> -static ssize_t display_name_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> -		char *buf)
> +static ssize_t display_name_show(struct kobject *kobj,
> +				 struct kobj_attribute *attr, char *buf)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>
> @@ -1069,7 +1069,7 @@ static ssize_t possible_values_show(struct
> kobject *kobj, struct kobj_attribute
>  }
>
>  static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> -		char *buf)
> +			 char *buf)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>
> @@ -1491,15 +1491,15 @@ static int tlmi_sysfs_init(void)
>  {
>  	int i, ret;
>
> -	tlmi_priv.class_dev = device_create(&firmware_attributes_class,
> NULL, MKDEV(0, 0),
> -			NULL, "%s", "thinklmi");
> +	tlmi_priv.class_dev = device_create(&firmware_attributes_class, NULL,
> +					    MKDEV(0, 0), NULL, "%s", "thinklmi");
>  	if (IS_ERR(tlmi_priv.class_dev)) {
>  		ret = PTR_ERR(tlmi_priv.class_dev);
>  		goto fail_class_created;
>  	}
>
>  	tlmi_priv.attribute_kset = kset_create_and_add("attributes", NULL,
> -			&tlmi_priv.class_dev->kobj);
> +						       &tlmi_priv.class_dev->kobj);
>  	if (!tlmi_priv.attribute_kset) {
>  		ret = -ENOMEM;
>  		goto fail_device_created;
> @@ -1637,8 +1637,8 @@ static int tlmi_analyze(struct wmi_device *wdev)
>  		tlmi_priv.opcode_support = true;
>
>  	if (wmi_has_guid(LENOVO_SET_BIOS_CERT_GUID) &&
> -		wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
> -		wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
> +	    wmi_has_guid(LENOVO_SET_BIOS_SETTING_CERT_GUID) &&
> +	    wmi_has_guid(LENOVO_SAVE_BIOS_SETTING_CERT_GUID))
>  		tlmi_priv.certificate_support = true;
>
>  	/* ThinkCenter uses different GUIDs for certificate support */
> @@ -1693,7 +1693,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>  						       &setting->possible_values);
>  			if (ret || !setting->possible_values)
>  				pr_info("Error retrieving possible values for %d : %s\n",
> -						i, setting->display_name);
> +					i, setting->display_name);
>  		} else {
>  			/*
>  			 * Older Thinkstations don't support the bios_selections API.
> @@ -1710,8 +1710,9 @@ static int tlmi_analyze(struct wmi_device *wdev)
>  					optend = strstr(optstart, "]");
>  					if (optend)
>  						setting->possible_values =
> -							kstrndup(optstart, optend - optstart,
> -									GFP_KERNEL);
> +							kstrndup(optstart,
> +								 optend - optstart,
> +								 GFP_KERNEL);
>  				}
>  				kfree(optitem);
>  			}
> @@ -1772,7 +1773,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>  		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 ||
> -					tlmi_priv.pwdcfg.ext.hdd_master_password) {
> +			    tlmi_priv.pwdcfg.ext.hdd_master_password) {
>  				tlmi_priv.pwd_hdd->pwd_enabled = true;
>  				if (tlmi_priv.pwdcfg.ext.hdd_master_password)
>  					tlmi_priv.pwd_hdd->index =
> -- 
> 2.52.0

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>