* [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events
@ 2021-05-26 20:14 Mark Pearson
2021-05-26 20:14 ` [PATCH v4 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper Mark Pearson
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Mark Pearson @ 2021-05-26 20:14 UTC (permalink / raw)
To: markpearson
Cc: hdegoede, andy.shevchenko, prasanth.ksr, divya.bharathi,
Dell.Client.Kernel, platform-driver-x86
This will be used by the Dell and Lenovo WMI management drivers to
prevent both drivers being active.
Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
Changes in v2:
- This is a new file requested as part of the review of the proposed
think_lmi.c driver. Labeling as V2 to keep series consistent
Changes in v3:
- Set default in Kconfig, and removed help text
- Allow multiple modules to register with module. Change API names to
better reflect this.
Changes in v4:
- version bump for consistency in series
drivers/platform/x86/Kconfig | 4 ++
drivers/platform/x86/Makefile | 1 +
.../platform/x86/firmware_attributes_class.c | 53 +++++++++++++++++++
.../platform/x86/firmware_attributes_class.h | 13 +++++
4 files changed, 71 insertions(+)
create mode 100644 drivers/platform/x86/firmware_attributes_class.c
create mode 100644 drivers/platform/x86/firmware_attributes_class.h
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 2714f7c38..57da8352d 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1076,6 +1076,10 @@ config TOUCHSCREEN_DMI
the OS-image for the device. This option supplies the missing info.
Enable this for x86 tablets with Silead or Chipone touchscreens.
+config FW_ATTR_CLASS
+ tristate "Firmware attributes class helper module"
+ default n
+
config INTEL_IMR
bool "Intel Isolated Memory Region support"
depends on X86_INTEL_QUARK && IOSF_MBI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index dcc8cdb95..147573f69 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o
obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o
+obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o
# Intel uncore drivers
obj-$(CONFIG_INTEL_IPS) += intel_ips.o
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
new file mode 100644
index 000000000..31393ce4d
--- /dev/null
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Firmware attributes class helper module */
+
+#include <linux/mutex.h>
+#include <linux/device/class.h>
+#include <linux/module.h>
+
+static DEFINE_MUTEX(fw_attr_lock);
+int fw_attr_inuse;
+
+static struct class firmware_attributes_class = {
+ .name = "firmware-attributes",
+};
+
+int fw_attributes_class_get(struct class **fw_attr_class)
+{
+ int err;
+
+ mutex_lock(&fw_attr_lock);
+ if (!fw_attr_inuse) { /*first time class is being used*/
+ err = class_register(&firmware_attributes_class);
+ if (err) {
+ mutex_unlock(&fw_attr_lock);
+ return err;
+ }
+ }
+ fw_attr_inuse++;
+ *fw_attr_class = &firmware_attributes_class;
+ mutex_unlock(&fw_attr_lock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fw_attributes_class_get);
+
+int fw_attributes_class_put(void)
+{
+ mutex_lock(&fw_attr_lock);
+ if (!fw_attr_inuse) {
+ mutex_unlock(&fw_attr_lock);
+ return -EINVAL;
+ }
+ fw_attr_inuse--;
+ if (!fw_attr_inuse) /* No more consumers */
+ class_unregister(&firmware_attributes_class);
+ mutex_unlock(&fw_attr_lock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fw_attributes_class_put);
+
+MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_LICENSE("GPL");
+
+
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
new file mode 100644
index 000000000..802f12b45
--- /dev/null
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Firmware attributes class helper module */
+
+#ifndef FW_ATTR_CLASS_H
+#define FW_ATTR_CLASS_H
+
+int fw_attributes_class_get(struct class **fw_attr_class);
+int fw_attributes_class_put(void);
+
+#endif /* FW_ATTR_CLASS_H */
+
+
--
2.31.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper 2021-05-26 20:14 [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Mark Pearson @ 2021-05-26 20:14 ` Mark Pearson 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson 2021-05-27 9:12 ` [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Hans de Goede 2 siblings, 0 replies; 15+ messages in thread From: Mark Pearson @ 2021-05-26 20:14 UTC (permalink / raw) To: markpearson Cc: hdegoede, andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Update Dell WMI sysman driver to use newly implemented helper module. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- Changes in v2: - firmare_attributes_class.c added as reqested during review of the proposed think_lmi.c driver. Labeling as V2 to keep series consistent - Note, compile tested only due to lack of HW to test on. Matching changes were made in the think-lmi.c driver and worked. Please let me know if any problems! Changes in v3: - Updated to use new firmware attribute class API names Changes in v4: - version bump for consistency in series drivers/platform/x86/dell/Kconfig | 1 + .../platform/x86/dell/dell-wmi-sysman/sysman.c | 18 ++++++++---------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig index e0a55337f..931f616ff 100644 --- a/drivers/platform/x86/dell/Kconfig +++ b/drivers/platform/x86/dell/Kconfig @@ -197,6 +197,7 @@ config DELL_WMI_SYSMAN depends on ACPI_WMI depends on DMI select NLS + select FW_ATTR_CLASS help This driver allows changing BIOS settings on many Dell machines from 2018 and newer without the use of any additional software. diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c index c8d276d78..06b3fe7da 100644 --- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c +++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c @@ -13,14 +13,11 @@ #include <linux/kernel.h> #include <linux/wmi.h> #include "dell-wmi-sysman.h" +#include "../../firmware_attributes_class.h" #define MAX_TYPES 4 #include <linux/nls.h> -static struct class firmware_attributes_class = { - .name = "firmware-attributes", -}; - struct wmi_sysman_priv wmi_priv = { .mutex = __MUTEX_INITIALIZER(wmi_priv.mutex), }; @@ -28,6 +25,7 @@ struct wmi_sysman_priv wmi_priv = { /* reset bios to defaults */ static const char * const reset_types[] = {"builtinsafe", "lastknowngood", "factory", "custom"}; static int reset_option = -1; +struct class *fw_attr_class; /** @@ -541,11 +539,11 @@ static int __init sysman_init(void) goto err_exit_bios_attr_pass_interface; } - ret = class_register(&firmware_attributes_class); + ret = fw_attributes_class_get(&fw_attr_class); if (ret) goto err_exit_bios_attr_pass_interface; - wmi_priv.class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0), + wmi_priv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), NULL, "%s", DRIVER_NAME); if (IS_ERR(wmi_priv.class_dev)) { ret = PTR_ERR(wmi_priv.class_dev); @@ -602,10 +600,10 @@ static int __init sysman_init(void) release_attributes_data(); err_destroy_classdev: - device_destroy(&firmware_attributes_class, MKDEV(0, 0)); + device_destroy(fw_attr_class, MKDEV(0, 0)); err_unregister_class: - class_unregister(&firmware_attributes_class); + fw_attributes_class_put(); err_exit_bios_attr_pass_interface: exit_bios_attr_pass_interface(); @@ -619,8 +617,8 @@ static int __init sysman_init(void) static void __exit sysman_exit(void) { release_attributes_data(); - device_destroy(&firmware_attributes_class, MKDEV(0, 0)); - class_unregister(&firmware_attributes_class); + device_destroy(fw_attr_class, MKDEV(0, 0)); + fw_attributes_class_put(); exit_bios_attr_set_interface(); exit_bios_attr_pass_interface(); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-26 20:14 [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Mark Pearson 2021-05-26 20:14 ` [PATCH v4 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper Mark Pearson @ 2021-05-26 20:14 ` Mark Pearson 2021-05-27 9:17 ` Hans de Goede ` (2 more replies) 2021-05-27 9:12 ` [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Hans de Goede 2 siblings, 3 replies; 15+ messages in thread From: Mark Pearson @ 2021-05-26 20:14 UTC (permalink / raw) To: markpearson Cc: hdegoede, andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 For Lenovo platforms that support a WMI interface to the BIOS add support, using the firmware-attributes class, to allow users to access and modify various BIOS related settings. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- Changes in V2: - Improved error handling - updated to use kobj's better (thanks Hans for pointers). Makes code simpler and cleaner - Removed some dead code from previous implementation that I'd missed - Moved think-lmi.h into platform/x86 directory - Use firmware_attributes_class helper - Corrected some bad indexing when storing an attribute - improved encoding options to limit selection - Added details on encoding and kbdlang to sysfs-class-firmware-attributes documentation Changes in v3: - Clean up is_enabled display - Use new firmware-attributes class API names - Correct release of kobj objects - Change pwd_type to const char - Add role field to pwd_settings - Improved checking when setting new password Changes in v4: - Updated documentation with password update specifics - Implement tlmi_errs to make error string scanning cleaner - Clean up pointer check handling - Clean up pointer free handling - Clean up pwd string length handling as recommended - Use kasprintf where appropriate - use strreplace where appropriate - Cosmetic cleanups as recommended .../testing/sysfs-class-firmware-attributes | 18 +- drivers/platform/x86/Kconfig | 11 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/think-lmi.c | 884 ++++++++++++++++++ drivers/platform/x86/think-lmi.h | 80 ++ 5 files changed, 993 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/x86/think-lmi.c create mode 100644 drivers/platform/x86/think-lmi.h diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes index 8ea59fea4..3348bf80a 100644 --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes @@ -197,8 +197,24 @@ Description: Drivers may emit a CHANGE uevent when a password is set or unset userspace may check it again. - On Dell systems, if Admin password is set, then all BIOS attributes + On Dell and Lenovo systems, if Admin password is set, then all BIOS attributes require password validation. + On Lenovo systems if you change the Admin password the new password is not active until + the next boot. + + Lenovo specific class extensions + ------------------------------ + + On Lenovo systems the following additional settings are available: + + lenovo_encoding: + The encoding method that is used. This can be either "ascii" + or "scancode". Default is set to "ascii" + + lenovo_kbdlang: + The keyboard language method that is used. This is generally a + two char code (e.g. "us", "fr", "gr") and may vary per platform. + Default is set to "us" What: /sys/class/firmware-attributes/*/attributes/pending_reboot Date: February 2021 diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 57da8352d..c61aab19e 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -639,6 +639,17 @@ config THINKPAD_ACPI_HOTKEY_POLL If you are not sure, say Y here. The driver enables polling only if it is strictly necessary to do so. +config THINKPAD_LMI + tristate "Lenovo WMI-based systems management driver" + depends on ACPI_WMI + select FW_ATTR_CLASS + help + This driver allows changing BIOS settings on Lenovo machines whose + BIOS support the WMI interface. + + To compile this driver as a module, choose M here: the module will + be called think-lmi. + config INTEL_ATOMISP2_LED tristate "Intel AtomISP2 camera LED driver" depends on GPIOLIB && LEDS_GPIO diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 147573f69..cab19672e 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_IBM_RTL) += ibm_rtl.o obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o +obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o # Intel obj-$(CONFIG_INTEL_ATOMISP2_LED) += intel_atomisp2_led.o diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c new file mode 100644 index 000000000..3ccae8224 --- /dev/null +++ b/drivers/platform/x86/think-lmi.c @@ -0,0 +1,884 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * think-lmi.c - Think LMI BIOS configuration driver + * + * Copyright(C) 2019-2021 Lenovo + * + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi + * Copyright(C) 2017 Corentin Chary <corentin.chary@gmail.com> + * Distributed under the GPL-2.0 license + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/acpi.h> +#include <linux/fs.h> +#include <linux/wmi.h> +#include "firmware_attributes_class.h" +#include "think-lmi.h" + +/* + * Name: + * Lenovo_BiosSetting + * Description: + * Get item name and settings for current LMI instance. + * Type: + * Query + * Returns: + * "Item,Value" + * Example: + * "WakeOnLAN,Enable" + */ +#define LENOVO_BIOS_SETTING_GUID "51F5230E-9677-46CD-A1CF-C0B23EE34DB7" + +/* + * Name: + * Lenovo_SetBiosSetting + * Description: + * Change the BIOS setting to the desired value using the Lenovo_SetBiosSetting + * class. To save the settings, use the Lenovo_SaveBiosSetting class. + * BIOS settings and values are case sensitive. + * After making changes to the BIOS settings, you must reboot the computer + * before the changes will take effect. + * Type: + * Method + * Arguments: + * "Item,Value,Password,Encoding,KbdLang;" + * Example: + * "WakeOnLAN,Disable,pswd,ascii,us;" + */ +#define LENOVO_SET_BIOS_SETTINGS_GUID "98479A64-33F5-4E33-A707-8E251EBBC3A1" + +/* + * Name: + * Lenovo_SaveBiosSettings + * Description: + * Save any pending changes in settings. + * Type: + * Method + * Arguments: + * "Password,Encoding,KbdLang;" + * Example: + * "pswd,ascii,us;" + */ +#define LENOVO_SAVE_BIOS_SETTINGS_GUID "6A4B54EF-A5ED-4D33-9455-B0D9B48DF4B3" + +/* + * Name: + * Lenovo_BiosPasswordSettings + * Description: + * Return BIOS Password settings + * Type: + * Query + * Returns: + * PasswordMode, PasswordState, MinLength, MaxLength, + * SupportedEncoding, SupportedKeyboard + */ +#define LENOVO_BIOS_PASSWORD_SETTINGS_GUID "8ADB159E-1E32-455C-BC93-308A7ED98246" + +/* + * Name: + * Lenovo_SetBiosPassword + * Description: + * Change a specific password. + * - BIOS settings cannot be changed at the same boot as power-on + * passwords (POP) and hard disk passwords (HDP). If you want to change + * BIOS settings and POP or HDP, you must reboot the system after changing + * one of them. + * - A password cannot be set using this method when one does not already + * exist. Passwords can only be updated or cleared. + * Type: + * Method + * Arguments: + * "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;" + * Example: + * "pop,oldpop,newpop,ascii,us;” + */ +#define LENOVO_SET_BIOS_PASSWORD_GUID "2651D9FD-911C-4B69-B94E-D0DED5963BD7" + +/* + * Name: + * Lenovo_GetBiosSelections + * Description: + * Return a list of valid settings for a given item. + * Type: + * Method + * Arguments: + * "Item" + * Returns: + * "Value1,Value2,Value3,..." + * Example: + * -> "FlashOverLAN" + * <- "Enabled,Disabled" + */ +#define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B" + +#define TLMI_POP_PWD (1 << 0) +#define TLMI_PAP_PWD (1 << 1) + +static const struct tlmi_err_codes tlmi_errs[] = { + {"Success", 0}, + {"Not Supported", -EOPNOTSUPP}, + {"Invalid Parameter", -EINVAL}, + {"Access Denied", -EACCES}, + {"System Busy", -EBUSY}, +}; + +static const char * const encoding_options[] = { + [TLMI_ENCODING_ASCII] = "ascii", + [TLMI_ENCODING_SCANCODE] = "scancode", +}; +static struct think_lmi tlmi_priv; +struct class *fw_attr_class; + +/* ------ Utility functions ------------*/ +/* Convert BIOS WMI error string to suitable error code */ +static int tlmi_errstr_to_err(const char *errstr) +{ + int 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; + } + return -EPERM; +} + +/* Extract error string from WMI return buffer */ +static int tlmi_extract_error(const struct acpi_buffer *output) +{ + const union acpi_object *obj; + + obj = output->pointer; + if (!obj) + return -ENOMEM; + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { + kfree(obj); + return -EIO; + } + return tlmi_errstr_to_err(obj->string.pointer); +} + +/* Utility function to execute WMI call to BIOS */ +static int tlmi_simple_call(const char *guid, const char *arg) +{ + const struct acpi_buffer input = { strlen(arg), (char *)arg }; + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + int i, err; + + /* + * Duplicated call required to match bios workaround for behavior + * seen when WMI accessed via scripting on other OS. + */ + for (i = 0; i < 2; i++) { + /* (re)initialize output buffer to default state */ + output.length = ACPI_ALLOCATE_BUFFER; + output.pointer = NULL; + + status = wmi_evaluate_method(guid, 0, 0, &input, &output); + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + err = tlmi_extract_error(&output); + kfree(output.pointer); + if (err) + return err; + } + return 0; +} + +/* Extract output string from WMI return buffer */ +static int tlmi_extract_output_string(const struct acpi_buffer *output, + char **string) +{ + const union acpi_object *obj; + char *s; + + obj = output->pointer; + if (!obj) + return -ENOMEM; + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { + kfree(obj); + return -EIO; + } + + s = kstrdup(obj->string.pointer, GFP_KERNEL); + if (!s) + return -ENOMEM; + *string = s; + return 0; +} + +/* ------ Core interface functions ------------*/ + +/* Get password settings from BIOS */ +static int tlmi_get_pwd_settings(struct tlmi_pwdcfg *pwdcfg) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + const union acpi_object *obj; + acpi_status status; + + if (!tlmi_priv.can_get_password_settings) + return -EOPNOTSUPP; + + status = wmi_query_block(LENOVO_BIOS_PASSWORD_SETTINGS_GUID, 0, + &output); + if (ACPI_FAILURE(status)) + return -EIO; + + obj = output.pointer; + if (!obj) + return -ENOMEM; + if (obj->type != ACPI_TYPE_BUFFER || !obj->buffer.pointer) { + kfree(obj); + return -EIO; + } + /* + * The size of thinkpad_wmi_pcfg on ThinkStation is larger than ThinkPad. + * To make the driver compatible on different brands, we permit it to get + * the data in below case. + */ + if (obj->buffer.length < sizeof(struct tlmi_pwdcfg)) { + pr_warn("Unknown pwdcfg buffer length %d\n", obj->buffer.length); + kfree(obj); + return -EIO; + } + memcpy(pwdcfg, obj->buffer.pointer, sizeof(struct tlmi_pwdcfg)); + kfree(obj); + return 0; +} + +static int tlmi_save_bios_settings(const char *password) +{ + return tlmi_simple_call(LENOVO_SAVE_BIOS_SETTINGS_GUID, + password); +} + +static int tlmi_setting(int item, char **value, const char *guid_string) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + int ret; + + status = wmi_query_block(guid_string, item, &output); + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + + ret = tlmi_extract_output_string(&output, value); + kfree(output.pointer); + return ret; +} + +static int tlmi_get_bios_selections(const char *item, char **value) +{ + const struct acpi_buffer input = { strlen(item), (char *)item }; + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + int ret; + + status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID, + 0, 0, &input, &output); + + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + + ret = tlmi_extract_output_string(&output, value); + kfree(output.pointer); + return ret; +} + +/* ---- Authentication sysfs --------------------------------------------------------- */ +static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%d\n", setting->valid); +} + +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); + +static ssize_t current_password_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int pwdlen; + + pwdlen = strlen(buf); + if (buf[pwdlen-1] == '\n') + pwdlen--; + + /* pwdlen == 0 is allowed to clear the password */ + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) + return -EINVAL; + + memcpy(setting->password, buf, pwdlen); + setting->password[pwdlen] = '\0'; + return count; +} + +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); + +static ssize_t new_password_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int ret, pwdlen; + char *new_pwd; + char *auth_str; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (!tlmi_priv.can_set_bios_password) + return -EOPNOTSUPP; + + new_pwd = kstrdup(buf, GFP_KERNEL); + if (!new_pwd) + return -ENOMEM; + + /* Strip out CR if one is present, setting password won't work if it is present */ + strreplace(new_pwd, '\n', '\0'); + + pwdlen = strlen(new_pwd); + /* pwdlen == 0 is allowed to clear the password */ + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) { + ret = -EINVAL; + goto out; + } + + /* 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); + if (!auth_str) { + ret = -ENOMEM; + goto out; + } + ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); + kfree(auth_str); +out: + kfree(new_pwd); + return ret ?: count; +} + +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) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%d\n", setting->minlen); +} + +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) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct 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) +{ + 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) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct 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) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int i; + + /* Scan for a matching profile */ + i = sysfs_match_string(encoding_options, buf); + if (i < 0) + return -EINVAL; + + setting->encoding = i; + return count; +} + +static struct kobj_attribute auth_encoding = __ATTR_RW(encoding); + +static ssize_t kbdlang_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct 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) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int length; + + length = strlen(buf); + if (buf[length-1] == '\n') + length--; + + if (!length || (length >= TLMI_LANG_MAXLEN)) + return -EINVAL; + + memcpy(setting->kbdlang, buf, length); + setting->kbdlang[length] = '\0'; + return count; +} + +static struct kobj_attribute auth_kbdlang = __ATTR_RW(kbdlang); + +static ssize_t role_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%s\n", setting->role); +} +static struct kobj_attribute auth_role = __ATTR_RO(role); + +static struct attribute *auth_attrs[] = { + &auth_is_pass_set.attr, + &auth_min_pass_length.attr, + &auth_max_pass_length.attr, + &auth_current_password.attr, + &auth_new_password.attr, + &auth_role.attr, + &auth_mechanism.attr, + &auth_encoding.attr, + &auth_kbdlang.attr, + NULL +}; + +static const struct attribute_group auth_attr_group = { + .attrs = auth_attrs, +}; + +/* ---- Attributes sysfs --------------------------------------------------------- */ +static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + + return sysfs_emit(buf, "%s\n", setting->display_name); +} + +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + char *item; + int ret; + + ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID); + if (ret) + return ret; + + ret = sysfs_emit(buf, "%s\n", item); + kfree(item); + return ret; +} + +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + + if (!tlmi_priv.can_get_bios_selections) + return -EOPNOTSUPP; + + return sysfs_emit(buf, "%s\n", setting->possible_values); +} + +static ssize_t current_value_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + char *set_str = NULL, *new_setting = NULL; + char *auth_str = NULL; + int ret; + + if (!tlmi_priv.can_set_bios_settings) + return -EOPNOTSUPP; + + new_setting = kstrdup(buf, GFP_KERNEL); + if (!new_setting) + return -ENOMEM; + + /* Strip out CR if one is present */ + strreplace(new_setting, '\n', '\0'); + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { + 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); + if (!auth_str) { + ret = -ENOMEM; + goto out; + } + } + + if (auth_str) + set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name, + new_setting, auth_str); + else + set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name, + new_setting); + if (!set_str) { + ret = -ENOMEM; + goto out; + } + + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str); + if (ret) + goto out; + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) + ret = tlmi_save_bios_settings(auth_str); + else + ret = tlmi_save_bios_settings(""); + +out: + kfree(auth_str); + kfree(set_str); + kfree(new_setting); + return ret ?: count; +} + +static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name); + +static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); + +static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); + +static struct attribute *tlmi_attrs[] = { + &attr_displ_name.attr, + &attr_current_val.attr, + &attr_possible_values.attr, + NULL +}; + +static const struct attribute_group tlmi_attr_group = { + .attrs = tlmi_attrs, +}; + +static ssize_t tlmi_attr_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + struct kobj_attribute *kattr; + + kattr = container_of(attr, struct kobj_attribute, attr); + if (kattr->show) + return kattr->show(kobj, kattr, buf); + return -EIO; +} + +static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + struct kobj_attribute *kattr; + + kattr = container_of(attr, struct kobj_attribute, attr); + if (kattr->store) + return kattr->store(kobj, kattr, buf, count); + return -EIO; +} + +static const struct sysfs_ops tlmi_kobj_sysfs_ops = { + .show = tlmi_attr_show, + .store = tlmi_attr_store, +}; + +static void tlmi_attr_setting_release(struct kobject *kobj) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + + kfree(setting); +} + +static void tlmi_pwd_setting_release(struct kobject *kobj) +{ + struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj); + + kfree(pwd); +} + +static struct kobj_type tlmi_attr_setting_ktype = { + .release = &tlmi_attr_setting_release, + .sysfs_ops = &tlmi_kobj_sysfs_ops, +}; + +static struct kobj_type tlmi_pwd_setting_ktype = { + .release = &tlmi_pwd_setting_release, + .sysfs_ops = &tlmi_kobj_sysfs_ops, +}; + +/* ---- Initialisation --------------------------------------------------------- */ +static void tlmi_release_attr(void) +{ + int i; + + /* Attribute structures */ + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { + if (tlmi_priv.setting[i]) { + kfree(tlmi_priv.setting[i]->possible_values); + sysfs_remove_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); + kobject_put(&tlmi_priv.setting[i]->kobj); + } + } + kset_unregister(tlmi_priv.attribute_kset); + + /* Authentication structures */ + sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); + kobject_put(&tlmi_priv.pwd_admin->kobj); + sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); + kobject_put(&tlmi_priv.pwd_power->kobj); + kset_unregister(tlmi_priv.authentication_kset); +} + +static int tlmi_sysfs_init(void) +{ + int i, ret; + + ret = fw_attributes_class_get(&fw_attr_class); + if (ret) + return ret; + + tlmi_priv.class_dev = device_create(fw_attr_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); + if (!tlmi_priv.attribute_kset) { + ret = -ENOMEM; + goto fail_device_created; + } + + for (i = 0; i < TLMI_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]->kobj.kset = tlmi_priv.attribute_kset; + ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype, + NULL, "%s", tlmi_priv.setting[i]->display_name); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.setting[i]->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->kobj.kset = tlmi_priv.authentication_kset; + ret = kobject_init_and_add(&tlmi_priv.pwd_admin->kobj, &tlmi_pwd_setting_ktype, + NULL, "%s", "Admin"); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); + if (ret) + goto fail_create_attr; + + tlmi_priv.pwd_power->kobj.kset = tlmi_priv.authentication_kset; + ret = kobject_init_and_add(&tlmi_priv.pwd_power->kobj, &tlmi_pwd_setting_ktype, + NULL, "%s", "System"); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); + if (ret) + goto fail_create_attr; + + return ret; + +fail_create_attr: + tlmi_release_attr(); +fail_device_created: + device_destroy(fw_attr_class, MKDEV(0, 0)); +fail_class_created: + fw_attributes_class_put(); + return ret; +} + +/* ---- Base Driver -------------------------------------------------------- */ +static int tlmi_analyze(void) +{ + struct tlmi_pwdcfg pwdcfg; + acpi_status status; + int i, 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) { + struct tlmi_attr_setting *setting; + char *item = NULL; + + 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 '\'. */ + strreplace(item, '/', '\\'); + + /* Remove the value part */ + strreplace(item, ',', '\0'); + + /* Create a setting entry */ + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); + if (!setting) { + ret = -ENOMEM; + goto fail_clear_attr; + } + setting->index = i; + 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"); + tlmi_priv.pwd_admin->encoding = TLMI_ENCODING_ASCII; + tlmi_priv.pwd_admin->pwd_type = "pap"; + tlmi_priv.pwd_admin->role = "bios-admin"; + tlmi_priv.pwd_admin->minlen = pwdcfg.min_length; + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; + 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"); + tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII; + tlmi_priv.pwd_power->pwd_type = "pop"; + tlmi_priv.pwd_power->role = "power-on"; + 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 void tlmi_remove(struct wmi_device *wdev) +{ + tlmi_release_attr(); + device_destroy(fw_attr_class, MKDEV(0, 0)); + fw_attributes_class_put(); +} + +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, +}; + +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_wmi_driver(tlmi_driver); diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h new file mode 100644 index 000000000..05005576b --- /dev/null +++ b/drivers/platform/x86/think-lmi.h @@ -0,0 +1,80 @@ +/* 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_BUFSIZE 129 +#define TLMI_PWDTYPE_MAXLEN 64 +#define TLMI_ENC_MAXLEN 64 +#define TLMI_LANG_MAXLEN 4 +#define TLMI_PWDTYPE_LEN 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) + +/* Possible error values */ +struct tlmi_err_codes { + const char *err_str; + int err_code; +}; + +enum encoding_option { + TLMI_ENCODING_ASCII, + TLMI_ENCODING_SCANCODE, +}; + +/* 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 kobj; + bool valid; + char display_name[TLMI_PWDTYPE_MAXLEN]; + char password[TLMI_PWD_BUFSIZE]; + const char *pwd_type; + const char *role; + int minlen; + int maxlen; + enum encoding_option encoding; + char kbdlang[TLMI_LANG_MAXLEN]; +}; + +/* Attribute setting details */ +struct tlmi_attr_setting { + struct kobject kobj; + int index; + 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_ */ + -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson @ 2021-05-27 9:17 ` Hans de Goede 2021-05-27 12:20 ` [External] " Mark Pearson 2021-05-27 14:18 ` Andy Shevchenko 2021-05-27 9:19 ` Hans de Goede 2021-05-27 14:16 ` Andy Shevchenko 2 siblings, 2 replies; 15+ messages in thread From: Hans de Goede @ 2021-05-27 9:17 UTC (permalink / raw) To: Mark Pearson Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Hi Mark, Andy, So as mentioned in my reply to patch 1/3, overall this looks pretty good. There are a few very small issues remaining, but they are so small that I've decided to fix them up and merge this into my review-hans branch with the issues fixed up. I plan to let this sit in review-hans a bit longer then usual to give you (Mark) a chance to check out the changes and ack them and to give Andy the time to check if his review remarks were addressed to his liking. I've put remarks inline / below about the 2 things which I've fixed up in this patch. Andy, thank you for your review of this. Your suggestions have improved this driver, esp. the use of kasprintf has made some of the functions a lot better. On 5/26/21 10:14 PM, Mark Pearson wrote: <snip> > +/* Extract error string from WMI return buffer */ > +static int tlmi_extract_error(const struct acpi_buffer *output) > +{ > + const union acpi_object *obj; > + > + obj = output->pointer; > + if (!obj) > + return -ENOMEM; > + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { > + kfree(obj); This kfree() should not be here, all the callers of tlmi_extract_error() unconditionally call: kfree(output->pointer); after calling tlmi_extract_error() so if hit this path with the kfree(obj) in place we get a double free. I've dropped the kfree() in my review-hans branch, as well as the now no longer necessary {}. > + return -EIO; > + } > + return tlmi_errstr_to_err(obj->string.pointer); > +} > + > +/* Utility function to execute WMI call to BIOS */ > +static int tlmi_simple_call(const char *guid, const char *arg) > +{ > + const struct acpi_buffer input = { strlen(arg), (char *)arg }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_status status; > + int i, err; > + > + /* > + * Duplicated call required to match bios workaround for behavior > + * seen when WMI accessed via scripting on other OS. > + */ > + for (i = 0; i < 2; i++) { > + /* (re)initialize output buffer to default state */ > + output.length = ACPI_ALLOCATE_BUFFER; > + output.pointer = NULL; > + > + status = wmi_evaluate_method(guid, 0, 0, &input, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + err = tlmi_extract_error(&output); > + kfree(output.pointer); > + if (err) > + return err; > + } > + return 0; > +} > + > +/* Extract output string from WMI return buffer */ > +static int tlmi_extract_output_string(const struct acpi_buffer *output, > + char **string) > +{ > + const union acpi_object *obj; > + char *s; > + > + obj = output->pointer; > + if (!obj) > + return -ENOMEM; > + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { > + kfree(obj); Same as above, also fixed in the same way. > + return -EIO; > + } > + > + s = kstrdup(obj->string.pointer, GFP_KERNEL); > + if (!s) > + return -ENOMEM; > + *string = s; > + return 0; > +} > + > +/* ------ Core interface functions ------------*/ Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [External] Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 9:17 ` Hans de Goede @ 2021-05-27 12:20 ` Mark Pearson 2021-05-27 14:18 ` Andy Shevchenko 1 sibling, 0 replies; 15+ messages in thread From: Mark Pearson @ 2021-05-27 12:20 UTC (permalink / raw) To: Hans de Goede Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Thanks Hans On 2021-05-27 5:17 a.m., Hans de Goede wrote: > Hi Mark, Andy, > > So as mentioned in my reply to patch 1/3, overall this looks pretty good. > There are a few very small issues remaining, but they are so small that > I've decided to fix them up and merge this into my review-hans branch > with the issues fixed up. > > I plan to let this sit in review-hans a bit longer then usual to > give you (Mark) a chance to check out the changes and ack them > and to give Andy the time to check if his review remarks were > addressed to his liking. Sounds good to me. > > I've put remarks inline / below about the 2 things which > I've fixed up in this patch. > > Andy, thank you for your review of this. Your suggestions have > improved this driver, esp. the use of kasprintf has made some > of the functions a lot better. Seconded :) Thank you to both of you for the reviews - I learnt a lot with this particular patch set. > > On 5/26/21 10:14 PM, Mark Pearson wrote: > > <snip> > >> +/* Extract error string from WMI return buffer */ >> +static int tlmi_extract_error(const struct acpi_buffer *output) >> +{ >> + const union acpi_object *obj; >> + >> + obj = output->pointer; >> + if (!obj) >> + return -ENOMEM; >> + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { >> + kfree(obj); > > This kfree() should not be here, all the callers of > tlmi_extract_error() unconditionally call: > > kfree(output->pointer); > > after calling tlmi_extract_error() so if hit this path with > the kfree(obj) in place we get a double free. > > I've dropped the kfree() in my review-hans branch, as well as the > now no longer necessary {}. Agreed - I should have noticed that one. Thanks > >> + return -EIO; >> + } >> + return tlmi_errstr_to_err(obj->string.pointer); >> +} >> + >> +/* Utility function to execute WMI call to BIOS */ >> +static int tlmi_simple_call(const char *guid, const char *arg) >> +{ >> + const struct acpi_buffer input = { strlen(arg), (char *)arg }; >> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; >> + acpi_status status; >> + int i, err; >> + >> + /* >> + * Duplicated call required to match bios workaround for behavior >> + * seen when WMI accessed via scripting on other OS. >> + */ >> + for (i = 0; i < 2; i++) { >> + /* (re)initialize output buffer to default state */ >> + output.length = ACPI_ALLOCATE_BUFFER; >> + output.pointer = NULL; >> + >> + status = wmi_evaluate_method(guid, 0, 0, &input, &output); >> + if (ACPI_FAILURE(status)) { >> + kfree(output.pointer); >> + return -EIO; >> + } >> + err = tlmi_extract_error(&output); >> + kfree(output.pointer); >> + if (err) >> + return err; >> + } >> + return 0; >> +} >> + >> +/* Extract output string from WMI return buffer */ >> +static int tlmi_extract_output_string(const struct acpi_buffer *output, >> + char **string) >> +{ >> + const union acpi_object *obj; >> + char *s; >> + >> + obj = output->pointer; >> + if (!obj) >> + return -ENOMEM; >> + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { >> + kfree(obj); > > Same as above, also fixed in the same way. Thanks > >> + return -EIO; >> + } >> + >> + s = kstrdup(obj->string.pointer, GFP_KERNEL); >> + if (!s) >> + return -ENOMEM; >> + *string = s; >> + return 0; >> +} >> + >> +/* ------ Core interface functions ------------*/ > Mark ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 9:17 ` Hans de Goede 2021-05-27 12:20 ` [External] " Mark Pearson @ 2021-05-27 14:18 ` Andy Shevchenko 1 sibling, 0 replies; 15+ messages in thread From: Andy Shevchenko @ 2021-05-27 14:18 UTC (permalink / raw) To: Hans de Goede Cc: Mark Pearson, Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver On Thu, May 27, 2021 at 12:17 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Mark, Andy, > > So as mentioned in my reply to patch 1/3, overall this looks pretty good. > There are a few very small issues remaining, but they are so small that > I've decided to fix them up and merge this into my review-hans branch > with the issues fixed up. > > I plan to let this sit in review-hans a bit longer then usual to > give you (Mark) a chance to check out the changes and ack them > and to give Andy the time to check if his review remarks were > addressed to his liking. I looked into it again and commented on some stuff, but nothing serious noted, so I think the next version would be ready to go. > I've put remarks inline / below about the 2 things which > I've fixed up in this patch. > > Andy, thank you for your review of this. Your suggestions have > improved this driver, esp. the use of kasprintf has made some > of the functions a lot better. You are welcome! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson 2021-05-27 9:17 ` Hans de Goede @ 2021-05-27 9:19 ` Hans de Goede 2021-05-27 12:21 ` [External] " Mark Pearson 2021-05-27 14:16 ` Andy Shevchenko 2 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2021-05-27 9:19 UTC (permalink / raw) To: Mark Pearson Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Hi again, On 5/26/21 10:14 PM, Mark Pearson wrote: > For Lenovo platforms that support a WMI interface to the BIOS add > support, using the firmware-attributes class, to allow users to access > and modify various BIOS related settings. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> Mark, checkpatch.pl is complaining about this patch not adding a MAINTAINERS entry for the think-lmi driver, can you please do a follow-up patch adding a MAINTAINERS entry for this? Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [External] Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 9:19 ` Hans de Goede @ 2021-05-27 12:21 ` Mark Pearson 0 siblings, 0 replies; 15+ messages in thread From: Mark Pearson @ 2021-05-27 12:21 UTC (permalink / raw) To: Hans de Goede Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 On 2021-05-27 5:19 a.m., Hans de Goede wrote: > Hi again, > > On 5/26/21 10:14 PM, Mark Pearson wrote: >> For Lenovo platforms that support a WMI interface to the BIOS add >> support, using the firmware-attributes class, to allow users to access >> and modify various BIOS related settings. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> > > Mark, checkpatch.pl is complaining about this patch not adding a > MAINTAINERS entry for the think-lmi driver, can you please do a > follow-up patch adding a MAINTAINERS entry for this? > Will do. Mark ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson 2021-05-27 9:17 ` Hans de Goede 2021-05-27 9:19 ` Hans de Goede @ 2021-05-27 14:16 ` Andy Shevchenko 2021-05-27 14:36 ` Hans de Goede 2021-05-28 17:36 ` [External] " Mark Pearson 2 siblings, 2 replies; 15+ messages in thread From: Andy Shevchenko @ 2021-05-27 14:16 UTC (permalink / raw) To: Mark Pearson Cc: Hans de Goede, Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote: > > For Lenovo platforms that support a WMI interface to the BIOS add > support, using the firmware-attributes class, to allow users to access > and modify various BIOS related settings. Thanks for an update! My comments below. ... > +/* > + * think-lmi.c - Think LMI BIOS configuration driver It's not the best idea to include the file name into the file. If the file gets renamed (by whatever reason) often this either brings an additional burden or simply forgotten (as from my experience). I recommend to drop the file names from the source code. > + * Copyright(C) 2019-2021 Lenovo > + * > + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi > + * Copyright(C) 2017 Corentin Chary <corentin.chary@gmail.com> > + * Distributed under the GPL-2.0 license > + */ ... > +#include <linux/acpi.h> linux/errno.h ? > +#include <linux/fs.h> linux/string.h ? linux/types.h ? > +#include <linux/wmi.h> > +#include "firmware_attributes_class.h" > +#include "think-lmi.h" ... > + * Type: > + * Method > + * Arguments: > + * "Item,Value,Password,Encoding,KbdLang;" > + * Example: > + * "WakeOnLAN,Disable,pswd,ascii,us;" Is 'pswd' here an example of the password? Hacker's language can make it more visible, e.g. 'pa55w0rd'. Same for other examples. > + */ ... > + /* > + * Duplicated call required to match bios workaround for behavior bios -> BIOS > + * seen when WMI accessed via scripting on other OS. > + */ ... > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); Candidate to have something like #define to_tlmi_pwd_setting(obj) container_of(...) since it has appeared a few times. > + return sysfs_emit(buf, "%d\n", setting->valid); > +} > + Unneeded blank line. Same for other similar places. > +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); Hmm... We have already define_one_global_ro(). The problems with that are the name and location. But perhaps you can copy that macro here with the same name and at least we can see the common grounds to clean up in the future. Another possibility is to comment that it can be replaced with the above mentioned macro. Ideally would be to refactor right now, but it's not anyhow crucial or required for this series, so may be postponed. ... > + int pwdlen; Strictly speaking it should be size_t. > + pwdlen = strlen(buf); > + if (buf[pwdlen-1] == '\n') > + pwdlen--; But the question is what will happen with the string like 'pa55\nw0rd\n' (note all \n:s)? See also below. > + /* pwdlen == 0 is allowed to clear the password */ > + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) The ' != 0' part is redundant. > + return -EINVAL; > + memcpy(setting->password, buf, pwdlen); > + setting->password[pwdlen] = '\0'; I'm not sure why we can't use strscpy() like you did in the other function. > + return count; ... > + /* Strip out CR if one is present, setting password won't work if it is present */ > + strreplace(new_pwd, '\n', '\0'); Basically it will stop on the first one. See the strchrnul() trick below. > + pwdlen = strlen(new_pwd); > + /* pwdlen == 0 is allowed to clear the password */ > + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) { No need for ' != 0'. > + ret = -EINVAL; > + goto out; > + } > +} ... > + int length; > + > + length = strlen(buf); > + if (buf[length-1] == '\n') > + length--; > + > + if (!length || (length >= TLMI_LANG_MAXLEN)) > + return -EINVAL; > + > + memcpy(setting->kbdlang, buf, length); > + setting->kbdlang[length] = '\0'; > + return count; Similar comments as per above. ... > + char *set_str = NULL, *new_setting = NULL; > + char *auth_str = NULL; The rule of thumb is to avoid forward assignments on stack allocated variables. It may decrease readability, hide real issues, and simply be unneeded churn, like here. Please revisit all of them in entire series. ... > + /* Strip out CR if one is present */ > + strreplace(new_setting, '\n', '\0'); As per above. ... > + /* Remove the value part */ > + strreplace(item, ',', '\0'); This is kinda non-standard pattern. I would see rather something like char *p; p = strchrnul(item, ','); *p = '\0'; Yes, it's longer, but better to understand what's going on here. > + /* Create a setting entry */ > + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); sizeof(*setting) ? > + if (!setting) { > + ret = -ENOMEM; > + goto fail_clear_attr; > + } > + setting->index = i; > + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); ... > + sprintf(tlmi_priv.pwd_admin->display_name, "admin"); > + sprintf(tlmi_priv.pwd_admin->kbdlang, "us"); Not sure why you need printf() type of function here. strcpy() or strscpy() should be enough. ... > + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) > + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; Not sure if WARN_ON() is really what has to be called here. But I haven't checked the context deeply. ... > + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); > + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); As above. ... > +#ifndef _THINK_LMI_H_ > +#define _THINK_LMI_H_ + linux/types.h (At leas bool is from there) > +#endif /* !_THINK_LMI_H_ */ -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 14:16 ` Andy Shevchenko @ 2021-05-27 14:36 ` Hans de Goede 2021-05-27 14:54 ` Andy Shevchenko 2021-05-28 17:36 ` [External] " Mark Pearson 1 sibling, 1 reply; 15+ messages in thread From: Hans de Goede @ 2021-05-27 14:36 UTC (permalink / raw) To: Andy Shevchenko, Mark Pearson Cc: Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver Hi, On 5/27/21 4:16 PM, Andy Shevchenko wrote: > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote: >> >> For Lenovo platforms that support a WMI interface to the BIOS add >> support, using the firmware-attributes class, to allow users to access >> and modify various BIOS related settings. > > Thanks for an update! My comments below. Since you have more remarks then I anticipated I guess it is best if I drop v4 from my review-hans branch and Mark does a v5. Mark, you can then also just squash the addition of the MAINTAINERS entry into v5. <snip> >> + int pwdlen; > > Strictly speaking it should be size_t. > >> + pwdlen = strlen(buf); >> + if (buf[pwdlen-1] == '\n') >> + pwdlen--; > > But the question is what will happen with the string like > 'pa55\nw0rd\n' (note all \n:s)? > See also below. As I already explained in a previous reply the password cannot contain '\n' so this cannot happen. I did miss that this was still there, where as new_password_store() uses strreplace() which is not really consistent with each other... >> + char *set_str = NULL, *new_setting = NULL; >> + char *auth_str = NULL; > > The rule of thumb is to avoid forward assignments on stack allocated > variables. It may decrease readability, hide real issues, and simply > be unneeded churn, like here. Please revisit all of them in entire > series. I asked for all this to be set to NULL in my review of v2, since there are various "goto out"s in the function and out: calls kfree() on all 3, in v2 there was a path which would end up calling kfree() on an uninitialized char *. IMHO just initializing all of them up front is best here because that guarantees that they are either still NULL, or point to memory returned by kmalloc. >> + /* Remove the value part */ >> + strreplace(item, ',', '\0'); > > This is kinda non-standard pattern. > > I would see rather something like > > char *p; > > p = strchrnul(item, ','); > *p = '\0'; > > Yes, it's longer, but better to understand what's going on here. Erm, you actually suggested using strreplace() here in your previous review ... >> + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) >> + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; > > Not sure if WARN_ON() is really what has to be called here. But I > haven't checked the context deeply. There are 2 max_lengths, one hardcoded in the driver so we don't need to dynamically manage memory for the password storage and one which is actually queried from the BIOS, the BIOS max-length should always be less then the hardcoded one in the driver (and currently this is true for all known models). I suggested adding the WARN_ON so that if future BIOS-es ever bump max_length we will get a bug-report about this and then we can bump the driver's TLMI_PWD_BUFSIZE value. Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 14:36 ` Hans de Goede @ 2021-05-27 14:54 ` Andy Shevchenko 0 siblings, 0 replies; 15+ messages in thread From: Andy Shevchenko @ 2021-05-27 14:54 UTC (permalink / raw) To: Hans de Goede Cc: Mark Pearson, Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver On Thu, May 27, 2021 at 5:36 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/27/21 4:16 PM, Andy Shevchenko wrote: > > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote: ... > >> + char *set_str = NULL, *new_setting = NULL; > >> + char *auth_str = NULL; > > > > The rule of thumb is to avoid forward assignments on stack allocated > > variables. It may decrease readability, hide real issues, and simply > > be unneeded churn, like here. Please revisit all of them in entire > > series. > > I asked for all this to be set to NULL in my review of v2, > since there are various "goto out"s in the function and out: > calls kfree() on all 3, in v2 there was a path which would > end up calling kfree() on an uninitialized char *. IMHO just > initializing all of them up front is best here because that > guarantees that they are either still NULL, or point to > memory returned by kmalloc. I see your point, fine with me! ... > >> + /* Remove the value part */ > >> + strreplace(item, ',', '\0'); > > > > This is kinda non-standard pattern. > > > > I would see rather something like > > > > char *p; > > > > p = strchrnul(item, ','); > > *p = '\0'; > > > > Yes, it's longer, but better to understand what's going on here. > > Erm, you actually suggested using strreplace() here in your previous review ... There is strreplace() in use somewhere still which is what I suggested. It might be that I missed the fact that in some places the change happens to the ''\0' rather than another non-NUL character. So, strreplace() is basically for changing '$a' to '$b' where neither of them is NUL. Otherwise we have - strsep() - strchrnul() - strtrim() depending on the case. Sorry if I was not completely clear about something. ... > >> + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) > >> + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; > > > > Not sure if WARN_ON() is really what has to be called here. But I > > haven't checked the context deeply. > > There are 2 max_lengths, one hardcoded in the driver so we don't > need to dynamically manage memory for the password storage and one > which is actually queried from the BIOS, the BIOS max-length should > always be less then the hardcoded one in the driver (and currently > this is true for all known models). > > I suggested adding the WARN_ON so that if future BIOS-es ever bump > max_length we will get a bug-report about this and then we can > bump the driver's TLMI_PWD_BUFSIZE value. I see your point, thanks for elaboration. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [External] Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-27 14:16 ` Andy Shevchenko 2021-05-27 14:36 ` Hans de Goede @ 2021-05-28 17:36 ` Mark Pearson 2021-05-28 18:19 ` Andy Shevchenko 1 sibling, 1 reply; 15+ messages in thread From: Mark Pearson @ 2021-05-28 17:36 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver Hi Andy, On 2021-05-27 10:16 a.m., Andy Shevchenko wrote: > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote: >> >> For Lenovo platforms that support a WMI interface to the BIOS add >> support, using the firmware-attributes class, to allow users to access >> and modify various BIOS related settings. > > Thanks for an update! My comments below. > > ... > >> +/* >> + * think-lmi.c - Think LMI BIOS configuration driver > > It's not the best idea to include the file name into the file. If the > file gets renamed (by whatever reason) often this either brings an > additional burden or simply forgotten (as from my experience). I > recommend to drop the file names from the source code. Good advice - I'll update > >> + * Copyright(C) 2019-2021 Lenovo >> + * >> + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi >> + * Copyright(C) 2017 Corentin Chary <corentin.chary@gmail.com> >> + * Distributed under the GPL-2.0 license >> + */ > > ... > >> +#include <linux/acpi.h> > > linux/errno.h ? > >> +#include <linux/fs.h> > > linux/string.h ? > linux/types.h ? > >> +#include <linux/wmi.h> >> +#include "firmware_attributes_class.h" >> +#include "think-lmi.h" I hadn't included those as they call get pulled in by linux/acpi.h - so aren't needed here. I take it best practice is to add them. I'd deliberately stripped down the list to the bare minimu previously but happy to reverse that if it's preferred. > > ... > >> + * Type: >> + * Method >> + * Arguments: >> + * "Item,Value,Password,Encoding,KbdLang;" >> + * Example: >> + * "WakeOnLAN,Disable,pswd,ascii,us;" > > Is 'pswd' here an example of the password? Hacker's language can make > it more visible, e.g. 'pa55w0rd'. > Same for other examples. Yes it is. I can update that :) > >> + */ > > ... > >> + /* >> + * Duplicated call required to match bios workaround for behavior > > bios -> BIOS Ack > >> + * seen when WMI accessed via scripting on other OS. >> + */ > > ... > >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > > Candidate to have something like > > #define to_tlmi_pwd_setting(obj) container_of(...) > > since it has appeared a few times. Ack > >> + return sysfs_emit(buf, "%d\n", setting->valid); >> +} > >> + > > Unneeded blank line. Same for other similar places. Shoot - I thought I'd cleaned a bunch of these up. I'll do another pass. > >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); > > Hmm... We have already define_one_global_ro(). The problems with that > are the name and location. But perhaps you can copy that macro here > with the same name and at least we can see the common grounds to clean > up in the future. Another possibility is to comment that it can be > replaced with the above mentioned macro. Ideally would be to refactor > right now, but it's not anyhow crucial or required for this series, so > may be postponed. OK - I'll have a look. I'm not 100% sure I understand the issues, but hopefully it will become clearer once I play with it a bit > > ... > >> + int pwdlen; > > Strictly speaking it should be size_t. > >> + pwdlen = strlen(buf); >> + if (buf[pwdlen-1] == '\n') >> + pwdlen--; > > But the question is what will happen with the string like > 'pa55\nw0rd\n' (note all \n:s)? > See also below. I know there is discussion on this in future mails, so I'll leave this one for those > >> + /* pwdlen == 0 is allowed to clear the password */ >> + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) > > The ' != 0' part is redundant. Ack > >> + return -EINVAL; > >> + memcpy(setting->password, buf, pwdlen); >> + setting->password[pwdlen] = '\0'; > > I'm not sure why we can't use strscpy() like you did in the other function. I'm not sure either :) I'll update > >> + return count; > > ... > >> + /* Strip out CR if one is present, setting password won't work if it is present */ >> + strreplace(new_pwd, '\n', '\0'); > > Basically it will stop on the first one. See the strchrnul() trick below. Thanks for the strchrnul trick - happy to use that > >> + pwdlen = strlen(new_pwd); >> + /* pwdlen == 0 is allowed to clear the password */ >> + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) { > > No need for ' != 0'. Ack > >> + ret = -EINVAL; >> + goto out; >> + } > >> +} > > > ... > >> + int length; >> + >> + length = strlen(buf); >> + if (buf[length-1] == '\n') >> + length--; >> + >> + if (!length || (length >= TLMI_LANG_MAXLEN)) >> + return -EINVAL; >> + >> + memcpy(setting->kbdlang, buf, length); >> + setting->kbdlang[length] = '\0'; >> + return count; > > Similar comments as per above. Ack > > ... > >> + char *set_str = NULL, *new_setting = NULL; >> + char *auth_str = NULL; > > The rule of thumb is to avoid forward assignments on stack allocated > variables. It may decrease readability, hide real issues, and simply > be unneeded churn, like here. Please revisit all of them in entire > series. Covered in Hans reply. Leaving as is > > ... > >> + /* Strip out CR if one is present */ >> + strreplace(new_setting, '\n', '\0'); > > As per above. > > ... > >> + /* Remove the value part */ >> + strreplace(item, ',', '\0'); > > This is kinda non-standard pattern. > > I would see rather something like > > char *p; > > p = strchrnul(item, ','); > *p = '\0'; > > Yes, it's longer, but better to understand what's going on here. Fair enough. I'll change. I quite liked the fact I had a one-liner do the same thing - oh well :) > >> + /* Create a setting entry */ >> + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); > > sizeof(*setting) ? OK. > >> + if (!setting) { >> + ret = -ENOMEM; >> + goto fail_clear_attr; >> + } >> + setting->index = i; >> + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); > > ... > >> + sprintf(tlmi_priv.pwd_admin->display_name, "admin"); >> + sprintf(tlmi_priv.pwd_admin->kbdlang, "us"); > > Not sure why you need printf() type of function here. strcpy() or > strscpy() should be enough. OK - I can update. > > ... > >> + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) >> + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; > > Not sure if WARN_ON() is really what has to be called here. But I > haven't checked the context deeply. Covered in Hans' reply.. Leaving as is > > ... > >> + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); >> + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); > > As above. Ack > > ... > >> +#ifndef _THINK_LMI_H_ >> +#define _THINK_LMI_H_ > > + linux/types.h > > (At leas bool is from there) Ack > > >> +#endif /* !_THINK_LMI_H_ */ > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [External] Re: [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms 2021-05-28 17:36 ` [External] " Mark Pearson @ 2021-05-28 18:19 ` Andy Shevchenko 0 siblings, 0 replies; 15+ messages in thread From: Andy Shevchenko @ 2021-05-28 18:19 UTC (permalink / raw) To: Mark Pearson Cc: Hans de Goede, Prasanth KSR, Divya Bharathi, Dell.Client.Kernel, Platform Driver On Fri, May 28, 2021 at 8:36 PM Mark Pearson <markpearson@lenovo.com> wrote: > On 2021-05-27 10:16 a.m., Andy Shevchenko wrote: > > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote: ... > >> +#include <linux/acpi.h> > > > > linux/errno.h ? > > > >> +#include <linux/fs.h> > > > > linux/string.h ? > > linux/types.h ? > > > >> +#include <linux/wmi.h> > >> +#include "firmware_attributes_class.h" > >> +#include "think-lmi.h" > > I hadn't included those as they call get pulled in by linux/acpi.h - so > aren't needed here. I take it best practice is to add them. I'd > deliberately stripped down the list to the bare minimu previously but > happy to reverse that if it's preferred. Basic headers like that are kinda what almost any file uses, but acpi.h is not the one which guarantees their inclusion (you see, it's a layering violation: acpi is not low level generic library). ... > >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); > > > > Hmm... We have already define_one_global_ro(). The problems with that > > are the name and location. But perhaps you can copy that macro here > > with the same name and at least we can see the common grounds to clean > > up in the future. Another possibility is to comment that it can be > > replaced with the above mentioned macro. Ideally would be to refactor > > right now, but it's not anyhow crucial or required for this series, so > > may be postponed. > OK - I'll have a look. I'm not 100% sure I understand the issues, but > hopefully it will become clearer once I play with it a bit Including cpufreq.h in your code will seem weird. That's why I marked it as an issue. ... > >> + pwdlen = strlen(buf); > >> + if (buf[pwdlen-1] == '\n') > >> + pwdlen--; > > > > But the question is what will happen with the string like > > 'pa55\nw0rd\n' (note all \n:s)? > > See also below. > I know there is discussion on this in future mails, so I'll leave this > one for those OK. ... > >> + /* Remove the value part */ > >> + strreplace(item, ',', '\0'); > > > > This is kinda non-standard pattern. > > > > I would see rather something like > > > > char *p; > > > > p = strchrnul(item, ','); > > *p = '\0'; > > > > Yes, it's longer, but better to understand what's going on here. > Fair enough. I'll change. I quite liked the fact I had a one-liner do > the same thing - oh well :) Me too, but the problem with strreplace(..., '\0') is that it sounds like a half-baked strsep() solution. First of all, it will go and replace all \n to \0 while you stop anyway on the first one in the following code. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events 2021-05-26 20:14 [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Mark Pearson 2021-05-26 20:14 ` [PATCH v4 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper Mark Pearson 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson @ 2021-05-27 9:12 ` Hans de Goede 2021-05-27 12:24 ` [External] " Mark Pearson 2 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2021-05-27 9:12 UTC (permalink / raw) To: Mark Pearson Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Hi Mark, Andy, Overall this looks pretty good. There are a few very small issues remaining, but they are so small that I've decided to fix them up and merge this into my review-hans branch with the issues fixed up. I plan to let this sit in review-hans a bit longer then usual to give you (Mark) a chance to check out the changes and ack them and to give Andy the time to check if his review remarks were addressed to his liking. I've put remarks inline / below about the few things which I've fixed up in this patch. I'll also reply to patch 3/3 with the fixups which I've done there. On 5/26/21 10:14 PM, Mark Pearson wrote: > This will be used by the Dell and Lenovo WMI management drivers to > prevent both drivers being active. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > Changes in v2: > - This is a new file requested as part of the review of the proposed > think_lmi.c driver. Labeling as V2 to keep series consistent > > Changes in v3: > - Set default in Kconfig, and removed help text > - Allow multiple modules to register with module. Change API names to > better reflect this. > > Changes in v4: > - version bump for consistency in series > > drivers/platform/x86/Kconfig | 4 ++ > drivers/platform/x86/Makefile | 1 + > .../platform/x86/firmware_attributes_class.c | 53 +++++++++++++++++++ > .../platform/x86/firmware_attributes_class.h | 13 +++++ > 4 files changed, 71 insertions(+) > create mode 100644 drivers/platform/x86/firmware_attributes_class.c > create mode 100644 drivers/platform/x86/firmware_attributes_class.h > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 2714f7c38..57da8352d 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1076,6 +1076,10 @@ config TOUCHSCREEN_DMI > the OS-image for the device. This option supplies the missing info. > Enable this for x86 tablets with Silead or Chipone touchscreens. > > +config FW_ATTR_CLASS > + tristate "Firmware attributes class helper module" This should be just " tristate" adding a string after the "tristate" makes this user selectable, I've dropped the string. > + default n > + > config INTEL_IMR > bool "Intel Isolated Memory Region support" > depends on X86_INTEL_QUARK && IOSF_MBI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index dcc8cdb95..147573f69 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o > +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o This was using spaces instead of tabs for indent before the += and it did not apply because of the "platform/x86: Rename hp-wireless to wireless-hotkey" patch in review-hans. > # Intel uncore drivers > obj-$(CONFIG_INTEL_IPS) += intel_ips.o > diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c > new file mode 100644 > index 000000000..31393ce4d > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.c This file had a couple of trailing empty lines which I've stripped. > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_class.h Idem, and also the same for think-lmi.c from patch 3/3. Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [External] Re: [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events 2021-05-27 9:12 ` [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Hans de Goede @ 2021-05-27 12:24 ` Mark Pearson 0 siblings, 0 replies; 15+ messages in thread From: Mark Pearson @ 2021-05-27 12:24 UTC (permalink / raw) To: Hans de Goede Cc: andy.shevchenko, prasanth.ksr, divya.bharathi, Dell.Client.Kernel, platform-driver-x86 Thanks Hans On 2021-05-27 5:12 a.m., Hans de Goede wrote: > Hi Mark, Andy, > > Overall this looks pretty good. There are a few very small issues > remaining, but they are so small that I've decided to fix them up > and merge this into my review-hans branch with the issues fixed up. > > I plan to let this sit in review-hans a bit longer then usual to > give you (Mark) a chance to check out the changes and ack them > and to give Andy the time to check if his review remarks were > addressed to his liking. > > I've put remarks inline / below about the few things which > I've fixed up in this patch. I'll also reply to patch 3/3 > with the fixups which I've done there. > > On 5/26/21 10:14 PM, Mark Pearson wrote: >> This will be used by the Dell and Lenovo WMI management drivers to >> prevent both drivers being active. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> --- >> Changes in v2: >> - This is a new file requested as part of the review of the proposed >> think_lmi.c driver. Labeling as V2 to keep series consistent >> >> Changes in v3: >> - Set default in Kconfig, and removed help text >> - Allow multiple modules to register with module. Change API names to >> better reflect this. >> >> Changes in v4: >> - version bump for consistency in series >> >> drivers/platform/x86/Kconfig | 4 ++ >> drivers/platform/x86/Makefile | 1 + >> .../platform/x86/firmware_attributes_class.c | 53 +++++++++++++++++++ >> .../platform/x86/firmware_attributes_class.h | 13 +++++ >> 4 files changed, 71 insertions(+) >> create mode 100644 drivers/platform/x86/firmware_attributes_class.c >> create mode 100644 drivers/platform/x86/firmware_attributes_class.h >> >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index 2714f7c38..57da8352d 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -1076,6 +1076,10 @@ config TOUCHSCREEN_DMI >> the OS-image for the device. This option supplies the missing info. >> Enable this for x86 tablets with Silead or Chipone touchscreens. >> >> +config FW_ATTR_CLASS >> + tristate "Firmware attributes class helper module" > > This should be just " tristate" adding a string after the "tristate" > makes this user selectable, I've dropped the string. Ah - my bad. Thanks for fixing. > >> + default n >> + >> config INTEL_IMR >> bool "Intel Isolated Memory Region support" >> depends on X86_INTEL_QUARK && IOSF_MBI >> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >> index dcc8cdb95..147573f69 100644 >> --- a/drivers/platform/x86/Makefile >> +++ b/drivers/platform/x86/Makefile >> @@ -115,6 +115,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o >> obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o >> obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o >> obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o >> +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > > This was using spaces instead of tabs for indent before the += and > it did not apply because of the "platform/x86: Rename hp-wireless to wireless-hotkey" > patch in review-hans. Ack. > > >> # Intel uncore drivers >> obj-$(CONFIG_INTEL_IPS) += intel_ips.o >> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c >> new file mode 100644 >> index 000000000..31393ce4d >> --- /dev/null >> +++ b/drivers/platform/x86/firmware_attributes_class.c > > This file had a couple of trailing empty lines which I've stripped. Ack - sorry I missed those. > >> --- /dev/null >> +++ b/drivers/platform/x86/firmware_attributes_class.h > > Idem, and also the same for think-lmi.c from patch 3/3> > Regards, > > Hans > Mark ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-05-28 18:19 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-26 20:14 [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Mark Pearson 2021-05-26 20:14 ` [PATCH v4 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper Mark Pearson 2021-05-26 20:14 ` [PATCH v4 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms Mark Pearson 2021-05-27 9:17 ` Hans de Goede 2021-05-27 12:20 ` [External] " Mark Pearson 2021-05-27 14:18 ` Andy Shevchenko 2021-05-27 9:19 ` Hans de Goede 2021-05-27 12:21 ` [External] " Mark Pearson 2021-05-27 14:16 ` Andy Shevchenko 2021-05-27 14:36 ` Hans de Goede 2021-05-27 14:54 ` Andy Shevchenko 2021-05-28 17:36 ` [External] " Mark Pearson 2021-05-28 18:19 ` Andy Shevchenko 2021-05-27 9:12 ` [PATCH v4 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events Hans de Goede 2021-05-27 12:24 ` [External] " Mark Pearson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.