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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

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.

> +	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.

...And thanks for reworking these, they look so simple now to review. :-)

> +};
> +
> +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.

> +
> +	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;
>