Re: [PATCH] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/6/21 5:44 PM, Hans de Goede wrote:

<snip>

>> +static void attr_name_release(struct kobject *kobj)
>> +{
>> +	kfree(kobj);
>> +}
> 
> If we switch to embedding the kobjects then we no longer need this
> function, the memory will be free-ed when we free the encapsultating
> structures instead.

I just realized that I got this wrong, we still want the release,
but we want the release to free the tlmi_pwd_setting resp. the
tlmi_attr_setting struct embedding the kobj instead
(requiring 2 release functions and 2 separate ktypes).

These kfree-s in the release functions will then replace the kfree-s
done from tlmi_release_attr().

This will ensure that the kobject stays around if userspace has
a file-handle pointing to it when the driver is unbound (this
will delay the calling of release till userspace closes the handle).

Regards,

Hans


>> +
>> +static struct kobj_type attr_name_ktype = {
>> +	.release	= attr_name_release,
> 
> And then we can just leave .release NULL here (IOW drop this line).
> 
>> +	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
>> +};
>> +
>> +/* ---- Initialisation --------------------------------------------------------- */
>> +static void tlmi_release_attr(void)
>> +{
>> +	int i;
>> +
>> +	/* Attribute structures */
>> +	for (i = 0; i < tlmi_priv.settings_count; i++) {
>> +		if (tlmi_priv.setting[i]) {
>> +			kfree(tlmi_priv.setting[i]->possible_values);
>> +
>> +			if (tlmi_priv.setting[i]->attr_name_kobj)
>> +				sysfs_remove_group(tlmi_priv.setting[i]->attr_name_kobj,
>> +						&tlmi_attr_group);
>> +			kfree(tlmi_priv.setting[i]);
>> +		}
>> +	}
>> +	if (tlmi_priv.attribute_kset) {
>> +		struct kobject *pos, *next;
>> +
>> +		list_for_each_entry_safe(pos, next, &tlmi_priv.attribute_kset->list, entry)
>> +			kobject_put(pos);
>> +		kset_unregister(tlmi_priv.attribute_kset);
>> +	}
>> +
>> +	/* Authentication structures */
>> +	if (tlmi_priv.pwd_admin->attr_name_kobj)
>> +		sysfs_remove_group(tlmi_priv.pwd_admin->attr_name_kobj, &auth_attr_group);
>> +	kfree(tlmi_priv.pwd_admin);
>> +	if (tlmi_priv.pwd_power->attr_name_kobj)
>> +		sysfs_remove_group(tlmi_priv.pwd_power->attr_name_kobj, &auth_attr_group);
>> +	kfree(tlmi_priv.pwd_power);
>> +	if (tlmi_priv.authentication_kset) {
>> +		struct kobject *pos, *next;
>> +
>> +		list_for_each_entry_safe(pos, next, &tlmi_priv.authentication_kset->list, entry)
>> +			kobject_put(pos);
>> +		kset_unregister(tlmi_priv.authentication_kset);
>> +	}
>> +}
>> +
>> +static int tlmi_sysfs_init(void)
>> +{
>> +	int i, ret;
>> +
>> +	ret = class_register(&firmware_attributes_class);
>> +	if (ret)
>> +		return ret;
>> +
>> +	tlmi_priv.class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
>> +			NULL, "%s", "thinklmi-sysfs");
>> +	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);
>> +	if (!tlmi_priv.attribute_kset) {
>> +		ret = -ENOMEM;
>> +		goto fail_device_created;
>> +	}
>> +
>> +	for (i = 0; i < tlmi_priv.settings_count; ++i) {
>> +		/*Check if index is a valid setting - skip if it isn't */
>> +		if (!tlmi_priv.setting[i])
>> +			continue;
>> +
>> +		/* build attribute */
>> +		tlmi_priv.setting[i]->attr_name_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
>> +		if (!tlmi_priv.setting[i]->attr_name_kobj) {
>> +			ret = -ENOMEM;
>> +			goto fail_create_attr;
>> +		}
>> +		tlmi_priv.setting[i]->attr_name_kobj->kset = tlmi_priv.attribute_kset;
>> +
>> +		ret = kobject_init_and_add(tlmi_priv.setting[i]->attr_name_kobj, &attr_name_ktype,
>> +				NULL, "%s", tlmi_priv.setting[i]->display_name);
>> +		if (ret)
>> +			goto fail_create_attr;
>> +
>> +		ret = sysfs_create_group(tlmi_priv.setting[i]->attr_name_kobj, &tlmi_attr_group);
>> +		if (ret)
>> +			goto fail_create_attr;
>> +	}
>> +
>> +	/* Create authentication entries */
>> +	tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
>> +								&tlmi_priv.class_dev->kobj);
>> +	if (!tlmi_priv.authentication_kset) {
>> +		ret = -ENOMEM;
>> +		goto fail_create_attr;
>> +	}
>> +	tlmi_priv.pwd_admin->attr_name_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
>> +	if (!tlmi_priv.pwd_admin->attr_name_kobj) {
>> +		ret = -ENOMEM;
>> +		goto fail_create_attr;
>> +	}
>> +	tlmi_priv.pwd_admin->attr_name_kobj->kset = tlmi_priv.authentication_kset;
>> +	ret = kobject_init_and_add(tlmi_priv.pwd_admin->attr_name_kobj, &attr_name_ktype,
>> +			NULL, "%s", "Admin");
>> +	if (ret)
>> +		goto fail_create_attr;
>> +
>> +	ret = sysfs_create_group(tlmi_priv.pwd_admin->attr_name_kobj, &auth_attr_group);
>> +	if (ret)
>> +		goto fail_create_attr;
>> +
>> +	tlmi_priv.pwd_power->attr_name_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
>> +	if (!tlmi_priv.pwd_power->attr_name_kobj) {
>> +		ret = -ENOMEM;
>> +		goto fail_create_attr;
>> +	}
>> +	tlmi_priv.pwd_power->attr_name_kobj->kset = tlmi_priv.authentication_kset;
>> +	ret = kobject_init_and_add(tlmi_priv.pwd_power->attr_name_kobj, &attr_name_ktype,
>> +			NULL, "%s", "System");
>> +	if (ret)
>> +		goto fail_create_attr;
>> +
>> +	ret = sysfs_create_group(tlmi_priv.pwd_power->attr_name_kobj, &auth_attr_group);
>> +	if (ret)
>> +		goto fail_create_attr;
>> +
>> +	return ret;
>> +
>> +fail_create_attr:
>> +	tlmi_release_attr();
>> +fail_device_created:
>> +	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
>> +fail_class_created:
>> +	class_unregister(&firmware_attributes_class);
>> +	return ret;
>> +}
>> +
>> +static void tlmi_sysfs_exit(void)
>> +{
>> +	tlmi_release_attr();
>> +	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
>> +	class_unregister(&firmware_attributes_class);
>> +}
>> +
>> +/* ---- Base Driver -------------------------------------------------------- */
>> +static int tlmi_analyze(void)
>> +{
>> +	struct tlmi_pwdcfg pwdcfg;
>> +	acpi_status status;
>> +	int i = 0;
>> +	int ret;
>> +
>> +	if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) &&
>> +	    wmi_has_guid(LENOVO_SAVE_BIOS_SETTINGS_GUID))
>> +		tlmi_priv.can_set_bios_settings = true;
>> +
>> +	if (wmi_has_guid(LENOVO_GET_BIOS_SELECTIONS_GUID))
>> +		tlmi_priv.can_get_bios_selections = true;
>> +
>> +	if (wmi_has_guid(LENOVO_SET_BIOS_PASSWORD_GUID))
>> +		tlmi_priv.can_set_bios_password = true;
>> +
>> +	if (wmi_has_guid(LENOVO_BIOS_PASSWORD_SETTINGS_GUID))
>> +		tlmi_priv.can_get_password_settings = true;
>> +
>> +	/*
>> +	 * Try to find the number of valid settings of this machine
>> +	 * and use it to create sysfs attributes
>> +	 */
>> +	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
>> +		char *item = NULL;
>> +		int spleng = 0;
>> +		int num = 0;
>> +		char *p;
>> +		struct tlmi_attr_setting *setting;
>> +
>> +		tlmi_priv.setting[i] = NULL;
>> +		status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
>> +		if (ACPI_FAILURE(status))
>> +			break;
>> +		if (!item)
>> +			break;
>> +		if (!*item)
>> +			continue;
>> +
>> +		/* It is not allowed to have '/' for file name. Convert it into '\'. */
>> +		spleng = strlen(item);
>> +		for (num = 0; num < spleng; num++) {
>> +			if (item[num] == '/')
>> +				item[num] = '\\';
>> +		}
>> +
>> +		/* Remove the value part */
>> +		p = strchr(item, ',');
>> +		if (p)
>> +			*p = '\0';
>> +
>> +		/* Create a setting entry */
>> +		setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL);
>> +		if (!setting) {
>> +			ret = -ENOMEM;
>> +			goto fail_clear_attr;
>> +		}
>> +		strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN);
>> +
>> +		/* If BIOS selections supported, load those */
>> +		if (tlmi_priv.can_get_bios_selections) {
>> +			ret = tlmi_get_bios_selections(setting->display_name,
>> +					&setting->possible_values);
>> +			if (ret || !setting->possible_values)
>> +				pr_info("Error retrieving possible values for %d : %s\n",
>> +						i, setting->display_name);
>> +		}
>> +		tlmi_priv.setting[i] = setting;
>> +		tlmi_priv.settings_count++;
>> +		kfree(item);
>> +	}
>> +
>> +	/* Create password setting structure */
>> +	ret = tlmi_get_pwd_settings(&pwdcfg);
>> +	if (ret)
>> +		goto fail_clear_attr;
>> +
>> +	tlmi_priv.pwd_admin = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
>> +	if (!tlmi_priv.pwd_admin) {
>> +		ret = -ENOMEM;
>> +		goto fail_clear_attr;
>> +	}
>> +	sprintf(tlmi_priv.pwd_admin->display_name, "admin");
>> +	sprintf(tlmi_priv.pwd_admin->kbdlang, "us");
>> +	sprintf(tlmi_priv.pwd_admin->encoding, "ascii");
>> +	tlmi_priv.pwd_admin->minlen = pwdcfg.min_length;
>> +	tlmi_priv.pwd_admin->maxlen = pwdcfg.max_length;
>> +	if (pwdcfg.password_state & TLMI_PAP_PWD)
>> +		tlmi_priv.pwd_admin->valid = true;
>> +
>> +	tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
>> +	if (!tlmi_priv.pwd_power) {
>> +		ret = -ENOMEM;
>> +		goto fail_clear_attr;
>> +	}
>> +	sprintf(tlmi_priv.pwd_power->display_name, "power-on");
>> +	sprintf(tlmi_priv.pwd_power->kbdlang, "us");
>> +	sprintf(tlmi_priv.pwd_power->encoding, "ascii");
>> +	tlmi_priv.pwd_power->minlen = pwdcfg.min_length;
>> +	tlmi_priv.pwd_power->maxlen = pwdcfg.max_length;
>> +
>> +	if (pwdcfg.password_state & TLMI_POP_PWD)
>> +		tlmi_priv.pwd_power->valid = true;
>> +
>> +	return 0;
>> +
>> +fail_clear_attr:
>> +	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i)
>> +		kfree(tlmi_priv.setting[i]);
>> +	return ret;
>> +}
>> +
>> +static int tlmi_remove(struct wmi_device *wdev)
>> +{
>> +	tlmi_sysfs_exit();
>> +	return 0;
>> +}
>> +
>> +static int tlmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> +	tlmi_analyze();
>> +	return tlmi_sysfs_init();
>> +}
>> +
>> +static const struct wmi_device_id tlmi_id_table[] = {
>> +	{ .guid_string = LENOVO_BIOS_SETTING_GUID },
>> +	{ }
>> +};
>> +
>> +static struct wmi_driver tlmi_driver = {
>> +	.driver = {
>> +		.name = "think-lmi",
>> +	},
>> +	.id_table = tlmi_id_table,
>> +	.probe = tlmi_probe,
>> +	.remove = tlmi_remove,
>> +};
>> +
>> +static int __init tlmi_init(void)
>> +{
>> +	return wmi_driver_register(&tlmi_driver);
>> +}
>> +
>> +static void __exit tlmi_exit(void)
>> +{
>> +	wmi_driver_unregister(&tlmi_driver);
>> +}
> 
> You can replace the declaration of these functions + the
> module_init and module_exit calls below with a single
> 
> module_wmi_driver(tlmi_driver);
> 
> "call".
> 
> 
>> +
>> +MODULE_AUTHOR("Sugumaran L <slacshiminar@lenovo.com>");
>> +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
>> +MODULE_AUTHOR("Corentin Chary <corentin.chary@gmail.com>");
>> +MODULE_DESCRIPTION("ThinkLMI Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +module_init(tlmi_init);
>> +module_exit(tlmi_exit);
>> diff --git a/include/linux/think-lmi.h b/include/linux/think-lmi.h
>> new file mode 100644
>> index 000000000..9133c0567
>> --- /dev/null
>> +++ b/include/linux/think-lmi.h
>> @@ -0,0 +1,72 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#ifndef _THINK_LMI_H_
>> +#define _THINK_LMI_H_
>> +
>> +#define TLMI_SETTINGS_COUNT  256
>> +#define TLMI_SETTINGS_MAXLEN 512
>> +#define TLMI_PWD_MAXLEN       64
>> +#define TLMI_PWDTYPE_MAXLEN   64
>> +#define TLMI_ENC_MAXLEN       64
>> +#define TLMI_LANG_MAXLEN       4
>> +/*
>> + * Longest string should be in the set command: allow size of BIOS
>> + * option and choice
>> + */
>> +#define TLMI_GETSET_MAXLEN (TLMI_SETTINGS_MAXLEN + TLMI_SETTINGS_MAXLEN)
>> +
>> +#define THINKLMI_GET_SETTINGS _IOR('T', 1, int *)
>> +#define THINKLMI_GET_SETTINGS_STRING _IOWR('T', 2, char *)
>> +#define THINKLMI_SET_SETTING _IOW('T', 3, char *)
>> +#define THINKLMI_SHOW_SETTING _IOWR('T', 4, char *)
>> +#define THINKLMI_AUTHENTICATE _IOW('T', 5, char *)
>> +#define THINKLMI_CHANGE_PASSWORD _IOW('T', 6, char *)
> 
> These defines seem to be a left-over from when the think-lmi driver
> was providing an ioctl interface, please drop these.
> 
>> +
>> +/* password configuration details */
>> +struct tlmi_pwdcfg {
>> +	uint32_t password_mode;
>> +	uint32_t password_state;
>> +	uint32_t min_length;
>> +	uint32_t max_length;
>> +	uint32_t supported_encodings;
>> +	uint32_t supported_keyboard;
>> +};
>> +
>> +/* password setting details */
>> +struct tlmi_pwd_setting {
>> +	struct kobject *attr_name_kobj;
>> +	bool valid;
>> +	char display_name[TLMI_PWDTYPE_MAXLEN];
>> +	char password[TLMI_PWD_MAXLEN];
>> +	int minlen;
>> +	int maxlen;
>> +	char encoding[TLMI_ENC_MAXLEN];
>> +	char kbdlang[TLMI_LANG_MAXLEN];
>> +};
>> +
>> +/* Attribute setting details */
>> +struct tlmi_attr_setting {
>> +	struct kobject *attr_name_kobj;
>> +	char display_name[TLMI_SETTINGS_MAXLEN];
>> +	char *possible_values;
>> +};
>> +
>> +struct think_lmi {
>> +	struct wmi_device *wmi_device;
>> +
>> +	int settings_count;
>> +	bool can_set_bios_settings;
>> +	bool can_get_bios_selections;
>> +	bool can_set_bios_password;
>> +	bool can_get_password_settings;
>> +
>> +	struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT];
>> +	struct device *class_dev;
>> +	struct kset *attribute_kset;
>> +	struct kset *authentication_kset;
>> +	struct tlmi_pwd_setting *pwd_admin;
>> +	struct tlmi_pwd_setting *pwd_power;
>> +};
>> +
>> +#endif /* !_THINK_LMI_H_ */
>> +
>>
> 
> Regards,
> 
> Hans
>