Re: [PATCH v2 1/3] platform/x86: think-lmi: Add certificate GUID structure

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

Thanks Ilpo,

On Fri, Aug 22, 2025, at 11:41 AM, Ilpo Järvinen wrote:
> On Fri, 22 Aug 2025, Mark Pearson wrote:
>
>> Add a certificate GUID structure to make it easier to add different
>> options for other platforms that need different GUIDs.
>> 
>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  drivers/platform/x86/lenovo/think-lmi.c | 41 ++++++++++++++++++++-----
>>  1 file changed, 33 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/lenovo/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
>> index 0992b41b6221..88bae5b33c57 100644
>> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -177,6 +177,28 @@ MODULE_PARM_DESC(debug_support, "Enable debug command support");
>>  #define TLMI_CERT_SVC BIT(7) /* Admin Certificate Based */
>>  #define TLMI_CERT_SMC BIT(8) /* System Certificate Based */
>>  
>> +struct tlmi_cert_guids {
>> +	char *thumbprint;
>> +	char *set_bios_setting;
>> +	char *save_bios_setting;
>> +	char *cert_to_password;
>> +	char *clear_bios_cert;
>> +	char *update_bios_cert;
>> +	char *set_bios_cert;
>> +};
>> +
>> +static struct tlmi_cert_guids thinkpad_cert_guid = {
>> +	LENOVO_CERT_THUMBPRINT_GUID,
>
> Don't use the anonymous initialization but name the members for better 
> readability.
>
Sure - I will fix that

>> +	LENOVO_SET_BIOS_SETTING_CERT_GUID,
>> +	LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>> +	LENOVO_CERT_TO_PASSWORD_GUID,
>> +	LENOVO_CLEAR_BIOS_CERT_GUID,
>> +	LENOVO_UPDATE_BIOS_CERT_GUID,
>> +	LENOVO_SET_BIOS_CERT_GUID
>
> Always remember to add comma to any non-terminator entry.
>
Yep - will do. Interesting that the checkpatch --strict didn't catch that one.

> ...And thanks for reworking these, they look so simple now to review. :-)
>
It was a good suggestion :)

>> +};
>> +
>> +static struct tlmi_cert_guids *cert_guid = &thinkpad_cert_guid;
>> +
>>  static const struct tlmi_err_codes tlmi_errs[] = {
>>  	{"Success", 0},
>>  	{"Not Supported", -EOPNOTSUPP},
>> @@ -668,7 +690,10 @@ static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>>  	const union acpi_object *obj;
>>  	acpi_status status;
>>  
>> -	status = wmi_evaluate_method(LENOVO_CERT_THUMBPRINT_GUID, 0, 0, &input, &output);
>> +	if (!cert_guid->thumbprint)
>> +		return -EOPNOTSUPP;
>
> Either mention this in the changelog or move it into the next patch
> that is the one needing the check. The latter of those seems more logical 
> as this is the only GUID you're NULL checking.
>
I did debate whether it was patch 1 or 2.
I think you're right...I'll move it to patch 2

> -- 
>  i.
>
>> +
>> +	status = wmi_evaluate_method(cert_guid->thumbprint, 0, 0, &input, &output);
>>  	if (ACPI_FAILURE(status)) {
>>  		kfree(output.pointer);
>>  		return -EIO;
>> @@ -751,7 +776,7 @@ static ssize_t cert_to_password_store(struct kobject *kobj,
>>  		kfree_sensitive(passwd);
>>  		return -ENOMEM;
>>  	}
>> -	ret = tlmi_simple_call(LENOVO_CERT_TO_PASSWORD_GUID, auth_str);
>> +	ret = tlmi_simple_call(cert_guid->cert_to_password, auth_str);
>>  	kfree(auth_str);
>>  	kfree_sensitive(passwd);
>>  
>> @@ -797,7 +822,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  		if (!auth_str)
>>  			return -ENOMEM;
>>  
>> -		ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>> +		ret = tlmi_simple_call(cert_guid->clear_bios_cert, auth_str);
>>  		kfree(auth_str);
>>  
>>  		return ret ?: count;
>> @@ -834,7 +859,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  			kfree(new_cert);
>>  			return -EACCES;
>>  		}
>> -		guid = LENOVO_UPDATE_BIOS_CERT_GUID;
>> +		guid = cert_guid->update_bios_cert;
>>  		/* Format: 'Certificate,Signature' */
>>  		auth_str = cert_command(setting, new_cert, signature);
>>  	} else {
>> @@ -845,7 +870,7 @@ static ssize_t certificate_store(struct kobject *kobj,
>>  			kfree(new_cert);
>>  			return -EACCES;
>>  		}
>> -		guid = LENOVO_SET_BIOS_CERT_GUID;
>> +		guid = cert_guid->set_bios_cert;
>>  		/* Format: 'Certificate, password' */
>>  		auth_str = cert_command(setting, new_cert, setting->password);
>>  	}
>> @@ -1071,13 +1096,13 @@ static ssize_t current_value_store(struct kobject *kobj,
>>  			goto out;
>>  		}
>>  
>> -		ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTING_CERT_GUID, set_str);
>> +		ret = tlmi_simple_call(cert_guid->set_bios_setting, set_str);
>>  		if (ret)
>>  			goto out;
>>  		if (tlmi_priv.save_mode == TLMI_SAVE_BULK)
>>  			tlmi_priv.save_required = true;
>>  		else
>> -			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>> +			ret = tlmi_simple_call(cert_guid->save_bios_setting,
>>  					       tlmi_priv.pwd_admin->save_signature);
>>  	} else if (tlmi_priv.opcode_support) {
>>  		/*
>> @@ -1282,7 +1307,7 @@ static ssize_t save_settings_store(struct kobject *kobj, struct kobj_attribute *
>>  				ret = -EINVAL;
>>  				goto out;
>>  			}
>> -			ret = tlmi_simple_call(LENOVO_SAVE_BIOS_SETTING_CERT_GUID,
>> +			ret = tlmi_simple_call(cert_guid->save_bios_setting,
>>  					       tlmi_priv.pwd_admin->save_signature);
>>  			if (ret)
>>  				goto out;
>>