[PATCH v2 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events

[PATCH v2 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events

[Mark Pearson]

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 (hope
that's correct).

 drivers/platform/x86/Kconfig                  |  6 +++
 drivers/platform/x86/Makefile                 |  1 +
 .../platform/x86/firmware_attributes_class.c  | 51 +++++++++++++++++++
 .../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..b0e1e5f65 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1076,6 +1076,12 @@ 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"
+	help
+	  This option should be enabled by any modules using the firmware
+	  attributes class.
+
 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..4ed959d6c
--- /dev/null
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -0,0 +1,51 @@
+// 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);
+bool fw_attr_inuse;
+
+static struct class firmware_attributes_class = {
+	.name = "firmware-attributes",
+};
+
+int fw_attributes_class_register(struct class **fw_attr_class)
+{
+	int err;
+
+	mutex_lock(&fw_attr_lock);
+	/* We can only have one active FW attribute class */
+	if (fw_attr_inuse) {
+		mutex_unlock(&fw_attr_lock);
+		return -EEXIST;
+	}
+
+	err = class_register(&firmware_attributes_class);
+	if (err) {
+		mutex_unlock(&fw_attr_lock);
+		return err;
+	}
+	fw_attr_inuse = true;
+	*fw_attr_class = &firmware_attributes_class;
+	mutex_unlock(&fw_attr_lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fw_attributes_class_register);
+
+void fw_attributes_class_remove(void)
+{
+	mutex_lock(&fw_attr_lock);
+	fw_attr_inuse = false;
+	class_unregister(&firmware_attributes_class);
+	mutex_unlock(&fw_attr_lock);
+}
+EXPORT_SYMBOL_GPL(fw_attributes_class_remove);
+
+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..e479a5720
--- /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_register(struct class **fw_attr_class);
+void fw_attributes_class_remove(void);
+
+#endif /* FW_ATTR_CLASS_H */
+
+
-- 
2.31.1

[PATCH v2 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper

[Mark Pearson]

Update Dell WMI sysman driver to use newly implemented helper module.

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 (hope that's correct).
 - 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!

 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..91ecf49ab 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_register(&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_remove();
 
 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_remove();
 	exit_bios_attr_set_interface();
 	exit_bios_attr_pass_interface();
 }
-- 
2.31.1

Re: [PATCH v2 2/3] platform/x86: dell-wmi-sysman: Use firmware_attributes_class helper

[Hans de Goede]

Hi,

On 5/9/21 3:57 AM, Mark Pearson wrote:
> Update Dell WMI sysman driver to use newly implemented helper module.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

(note this will need to be adjusted for the function renames in patch 1/3
 but that is trivial, so feel free to keep my Reviewed-by after those changes).

Regards,

Hans

> ---
> 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 (hope that's correct).
>  - 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!
> 
>  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..91ecf49ab 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_register(&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_remove();
>  
>  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_remove();
>  	exit_bios_attr_set_interface();
>  	exit_bios_attr_pass_interface();
>  }
> 

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

[Mark Pearson]

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

 .../testing/sysfs-class-firmware-attributes   |  13 +-
 drivers/platform/x86/Kconfig                  |  12 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/think-lmi.c              | 890 ++++++++++++++++++
 drivers/platform/x86/think-lmi.h              |  73 ++
 5 files changed, 988 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..31d1becfa 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -185,6 +185,17 @@ Description:
 					A write only value that when used in tandem with
 					current_password will reset a system or admin password.
 
+		encoding:
+					For platforms that require it (currently Lenovo) the
+					encoding method that is used. This can be either "ascii"
+					or "scancode". Default is set to "ascii"
+
+		kbdlang:
+					For platforms that require it (currently Lenovo only) 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"
+
 		Note, password management is session specific. If Admin password is set,
 		same password must be written into current_password file (required for
 		password-validation) and must be cleared once the session is over.
@@ -197,7 +208,7 @@ 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.
 
 What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b0e1e5f65..0511c54fc 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -639,6 +639,18 @@ 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"
+	default m
+	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..2fa989e98
--- /dev/null
+++ b/drivers/platform/x86/think-lmi.c
@@ -0,0 +1,890 @@
+// 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 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)
+{
+	if (!strcmp(errstr, "Success"))
+		/* Operation completed successfully */
+		return 0;
+	if (!strcmp(errstr, "Not Supported"))
+		/* The feature is not supported on this system */
+		return -EOPNOTSUPP;
+	if (!strcmp(errstr, "Invalid"))
+		/* The item or value provided is not valid parameter */
+		return -EINVAL;
+	if (!strcmp(errstr, "Access Denied"))
+		/*
+		 * The change could not be made due to an authentication problem.
+		 * If a supervisor password exists, the correct supervisor password
+		 * must be provided.
+		 */
+		return -EPERM;
+	if (!strcmp(errstr, "System Busy"))
+		/*
+		 * BIOS changes have already been made that need to be committed.
+		 238G* Reboot the system and try again.
+		 */
+		return -EBUSY;
+
+	pr_debug("Unknown error string: '%s'", errstr);
+	return -EINVAL;
+}
+
+/* Extract error string from WMI return buffer */
+static int tlmi_extract_error(const struct acpi_buffer *output)
+{
+	const union acpi_object *obj;
+	int ret;
+
+	obj = output->pointer;
+	if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
+		kfree(obj);
+		return -EIO;
+	}
+
+	ret = tlmi_errstr_to_err(obj->string.pointer);
+	kfree(obj);
+	return ret;
+}
+
+/* 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);
+		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;
+
+	obj = output->pointer;
+	if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
+		kfree(obj);
+		return -EIO;
+	}
+
+	*string = kstrdup(obj->string.pointer, GFP_KERNEL);
+	kfree(obj);
+	return *string ? 0 : -ENOMEM;
+}
+
+/* ------ 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 || 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;
+
+	status = wmi_query_block(guid_string, item, &output);
+	if (ACPI_FAILURE(status)) {
+		kfree(output.pointer);
+		return -EIO;
+	}
+
+	return tlmi_extract_output_string(&output, value);
+}
+
+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;
+
+	status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID,
+				     0, 0, &input, &output);
+
+	if (ACPI_FAILURE(status)) {
+		kfree(output.pointer);
+		return -EIO;
+	}
+
+	return tlmi_extract_output_string(&output, value);
+}
+
+/* ---- 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);
+
+	if (setting->valid)
+		return sysfs_emit(buf, "1\n");
+	else
+		return sysfs_emit(buf, "0\n");
+}
+
+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 length;
+
+	length = strlen(buf);
+	if (buf[length-1] == '\n')
+		length--;
+
+	if (length >= TLMI_PWD_MAXLEN)
+		return -EINVAL;
+
+	memcpy(setting->password, buf, length);
+	setting->password[length] = '\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);
+	char *p, *new_pwd;
+	char *auth_str;
+	int ret = 0, len;
+
+	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;
+
+	p = strchr(new_pwd, '\n');
+	if (p)
+		*p = '\0';
+
+	if (strlen(new_pwd) > setting->maxlen) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
+	len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
+		+ strlen(setting->kbdlang) + 3 /* type */
+		+ strlen(new_pwd) + 6 /* punctuation and terminator*/;
+	auth_str = kzalloc(len, GFP_KERNEL);
+	snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
+		 setting->pwd_type, setting->password, new_pwd,
+		 encoding_options[setting->encoding], setting->kbdlang);
+	ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);
+	kfree(auth_str);
+out:
+	kfree(new_pwd);
+	return ret ? 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)
+{
+	if (strcmp(kobj->name, "Admin") == 0)
+		return sprintf(buf, "bios-admin\n");
+	else if (strcmp(kobj->name, "System") == 0)
+		return sprintf(buf, "power-on\n");
+	return -EIO;
+}
+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);
+	int alloc_len, auth_len = 0;
+	int str_ix = 0;
+	char *auth_str = NULL;
+	char *set_str, *new_setting, *p;
+	int ret;
+
+	if (!tlmi_priv.can_set_bios_settings)
+		return -EOPNOTSUPP;
+
+	new_setting = kstrdup(buf, GFP_KERNEL);
+	if (!new_setting)
+		return -ENOMEM;
+	p = strchr(new_setting, '\n');
+	if (p)
+		*p = '\0';
+
+	alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting);
+
+	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
+		auth_len += strlen(tlmi_priv.pwd_admin->password) + 1
+			+ strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1
+			+ strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1;
+		auth_str = kmalloc(auth_len, GFP_KERNEL);
+		if (!auth_str) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		sprintf(auth_str, "%s,%s,%s;",
+				tlmi_priv.pwd_admin->password,
+				encoding_options[tlmi_priv.pwd_admin->encoding],
+				tlmi_priv.pwd_admin->kbdlang);
+		alloc_len += auth_len;
+	}
+
+	set_str = kmalloc(alloc_len, GFP_KERNEL);
+	if (!set_str) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting);
+
+	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password)
+		sprintf(set_str + str_ix, ",%s", auth_str);
+	else
+		sprintf(set_str + str_ix, ";");
+
+	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("");
+	if (ret)
+		goto out;
+
+out:
+	kfree(auth_str);
+	kfree(set_str);
+	kfree(new_setting);
+	return ret ? 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;
+	ssize_t ret = -EIO;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->show)
+		ret = kattr->show(kobj, kattr, buf);
+	return ret;
+}
+
+static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kobj_attribute *kattr;
+	ssize_t ret = -EIO;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->store)
+		ret = kattr->store(kobj, kattr, buf, count);
+	return ret;
+}
+
+static const struct sysfs_ops tlmi_kobj_sysfs_ops = {
+	.show	= tlmi_attr_show,
+	.store	= tlmi_attr_store,
+};
+
+static struct kobj_type attr_name_ktype = {
+	.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_register(&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, &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]->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, &attr_name_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, &attr_name_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_remove();
+	return ret;
+}
+
+/* ---- 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;
+		}
+		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;
+	sprintf(tlmi_priv.pwd_admin->pwd_type, "pap");
+	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");
+	tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII;
+	sprintf(tlmi_priv.pwd_admin->pwd_type, "pop");
+	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_remove();
+}
+
+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..2c1484304
--- /dev/null
+++ b/drivers/platform/x86/think-lmi.h
@@ -0,0 +1,73 @@
+/* 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      128
+#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)
+
+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_MAXLEN];
+	char pwd_type[TLMI_PWDTYPE_LEN];
+	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

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

[Hans de Goede]

Hi,

On 5/9/21 3:57 AM, 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>
> ---
> 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

Thank you, overall this looks pretty good, a small review remarks inline.

> 
>  .../testing/sysfs-class-firmware-attributes   |  13 +-
>  drivers/platform/x86/Kconfig                  |  12 +
>  drivers/platform/x86/Makefile                 |   1 +
>  drivers/platform/x86/think-lmi.c              | 890 ++++++++++++++++++
>  drivers/platform/x86/think-lmi.h              |  73 ++
>  5 files changed, 988 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..31d1becfa 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -185,6 +185,17 @@ Description:
>  					A write only value that when used in tandem with
>  					current_password will reset a system or admin password.
>  
> +		encoding:
> +					For platforms that require it (currently Lenovo) the
> +					encoding method that is used. This can be either "ascii"
> +					or "scancode". Default is set to "ascii"
> +
> +		kbdlang:
> +					For platforms that require it (currently Lenovo only) 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"
> +

I should have caught this during my review of v1, these are Lenovo specific, so please prefix
them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them
to a separate "Lenovo specific class extensions" part of the doc.

>  		Note, password management is session specific. If Admin password is set,
>  		same password must be written into current_password file (required for
>  		password-validation) and must be cleared once the session is over.
> @@ -197,7 +208,7 @@ 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.
>  
>  What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index b0e1e5f65..0511c54fc 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -639,6 +639,18 @@ 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"
> +	default m

default n (or no default at all) please.

> +	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..2fa989e98
> --- /dev/null
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -0,0 +1,890 @@
> +// 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 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)
> +{
> +	if (!strcmp(errstr, "Success"))
> +		/* Operation completed successfully */
> +		return 0;
> +	if (!strcmp(errstr, "Not Supported"))
> +		/* The feature is not supported on this system */
> +		return -EOPNOTSUPP;
> +	if (!strcmp(errstr, "Invalid"))
> +		/* The item or value provided is not valid parameter */
> +		return -EINVAL;
> +	if (!strcmp(errstr, "Access Denied"))
> +		/*
> +		 * The change could not be made due to an authentication problem.
> +		 * If a supervisor password exists, the correct supervisor password
> +		 * must be provided.
> +		 */
> +		return -EPERM;
> +	if (!strcmp(errstr, "System Busy"))
> +		/*
> +		 * BIOS changes have already been made that need to be committed.
> +		 238G* Reboot the system and try again.
> +		 */
> +		return -EBUSY;
> +
> +	pr_debug("Unknown error string: '%s'", errstr);
> +	return -EINVAL;
> +}
> +
> +/* Extract error string from WMI return buffer */
> +static int tlmi_extract_error(const struct acpi_buffer *output)
> +{
> +	const union acpi_object *obj;
> +	int ret;
> +
> +	obj = output->pointer;
> +	if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> +		kfree(obj);
> +		return -EIO;
> +	}
> +
> +	ret = tlmi_errstr_to_err(obj->string.pointer);
> +	kfree(obj);
> +	return ret;
> +}
> +
> +/* 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);
> +		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;
> +
> +	obj = output->pointer;
> +	if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> +		kfree(obj);
> +		return -EIO;
> +	}
> +
> +	*string = kstrdup(obj->string.pointer, GFP_KERNEL);
> +	kfree(obj);
> +	return *string ? 0 : -ENOMEM;
> +}
> +
> +/* ------ 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 || 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;
> +
> +	status = wmi_query_block(guid_string, item, &output);
> +	if (ACPI_FAILURE(status)) {
> +		kfree(output.pointer);
> +		return -EIO;
> +	}
> +
> +	return tlmi_extract_output_string(&output, value);
> +}
> +
> +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;
> +
> +	status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID,
> +				     0, 0, &input, &output);
> +
> +	if (ACPI_FAILURE(status)) {
> +		kfree(output.pointer);
> +		return -EIO;
> +	}
> +
> +	return tlmi_extract_output_string(&output, value);
> +}
> +
> +/* ---- 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);
> +
> +	if (setting->valid)
> +		return sysfs_emit(buf, "1\n");
> +	else
> +		return sysfs_emit(buf, "0\n");

Maybe use:

	sysfs_emit(buf, "%d\n", settings->valid);

instead?


> +}
> +
> +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 length;
> +
> +	length = strlen(buf);
> +	if (buf[length-1] == '\n')
> +		length--;
> +
> +	if (length >= TLMI_PWD_MAXLEN)
> +		return -EINVAL;
> +
> +	memcpy(setting->password, buf, length);
> +	setting->password[length] = '\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);
> +	char *p, *new_pwd;
> +	char *auth_str;
> +	int ret = 0, len;
> +
> +	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;
> +
> +	p = strchr(new_pwd, '\n');
> +	if (p)
> +		*p = '\0';
> +
> +	if (strlen(new_pwd) > setting->maxlen) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
> +	len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
> +		+ strlen(setting->kbdlang) + 3 /* type */
> +		+ strlen(new_pwd) + 6 /* punctuation and terminator*/;
> +	auth_str = kzalloc(len, GFP_KERNEL);
> +	snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
> +		 setting->pwd_type, setting->password, new_pwd,
> +		 encoding_options[setting->encoding], setting->kbdlang);
> +	ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);

So I guess on success any subsequent calls need to use the new password,
so the user is expected to write the new password to the current_password
file after changing the password this way?

I just checked the dell-wmi-sysman code and that does this:

        /* clear current_password here and use user input from wmi_priv.current_password */
        if (!ret)
                memset(current_password, 0, MAX_BUFF);

Where current_password points to either the user or admin cached password,
depending on which one is being changed.

So that seems to work the same way as what you are doing here (the user needs to
write the new password to current_password after changing it through the
new_password sysfs attribute). Can you add a patch to the patch-set documenting
this in Documentation/ABI/testing/sysfs-class-firmware-attributes ?

Also you may want to consider clearing out the old cached password on success
like the Dell code is doing.


> +	kfree(auth_str);
> +out:
> +	kfree(new_pwd);
> +	return ret ? 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);
> +
> +{
> +	if (strcmp(kobj->name, "Admin") == 0)
> +		return sprintf(buf, "bios-admin\n");
> +	else if (strcmp(kobj->name, "System") == 0)
> +		return sprintf(buf, "power-on\n");
> +	return -EIO;
> +}

IMHO it would be nice to add a "const char *role" to struct tlmi_pwd_setting and
then change this to:

> +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", setting->role);
> +}

That would make this function consistent with the other password related functions.



> +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);
> +	int alloc_len, auth_len = 0;
> +	int str_ix = 0;
> +	char *auth_str = NULL;
> +	char *set_str, *new_setting, *p;
> +	int ret;
> +
> +	if (!tlmi_priv.can_set_bios_settings)
> +		return -EOPNOTSUPP;
> +
> +	new_setting = kstrdup(buf, GFP_KERNEL);
> +	if (!new_setting)
> +		return -ENOMEM;
> +	p = strchr(new_setting, '\n');
> +	if (p)
> +		*p = '\0';
> +
> +	alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting);
> +
> +	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
> +		auth_len += strlen(tlmi_priv.pwd_admin->password) + 1
> +			+ strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1
> +			+ strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1;
> +		auth_str = kmalloc(auth_len, GFP_KERNEL);
> +		if (!auth_str) {
> +			ret = -ENOMEM;
> +			goto out;

You end-up kfree-ing set_str here without it being initialized (compiler warning?)
Please just initialize all 3 strings which you kfree at the end to NULL, I know
this is not necessary for at least the new_setting string but it makes it easier
for someone reading the code to verify that the kfree() does not happens on
an uninitialized pointer.

> +		}
> +
> +		sprintf(auth_str, "%s,%s,%s;",
> +				tlmi_priv.pwd_admin->password,
> +				encoding_options[tlmi_priv.pwd_admin->encoding],
> +				tlmi_priv.pwd_admin->kbdlang);
> +		alloc_len += auth_len;
> +	}
> +
> +	set_str = kmalloc(alloc_len, GFP_KERNEL);
> +	if (!set_str) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting);
> +
> +	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password)

Maybe use:

	if (auth_str) 

here? This way if the condition of the above if block ever changes, you can't
end up passing a NULL auth_Str to the sprintf.

> +		sprintf(set_str + str_ix, ",%s", auth_str);
> +	else
> +		sprintf(set_str + str_ix, ";");
> +
> +	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("");
> +	if (ret)
> +		goto out;
> +
> +out:
> +	kfree(auth_str);
> +	kfree(set_str);
> +	kfree(new_setting);
> +	return ret ? 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;
> +	ssize_t ret = -EIO;
> +
> +	kattr = container_of(attr, struct kobj_attribute, attr);
> +	if (kattr->show)
> +		ret = kattr->show(kobj, kattr, buf);
> +	return ret;
> +}
> +
> +static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct kobj_attribute *kattr;
> +	ssize_t ret = -EIO;
> +
> +	kattr = container_of(attr, struct kobj_attribute, attr);
> +	if (kattr->store)
> +		ret = kattr->store(kobj, kattr, buf, count);
> +	return ret;
> +}
> +
> +static const struct sysfs_ops tlmi_kobj_sysfs_ops = {
> +	.show	= tlmi_attr_show,
> +	.store	= tlmi_attr_store,
> +};
> +
> +static struct kobj_type attr_name_ktype = {
> +	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
> +};

You need to re-add a release function here, sorry I was not clear
about that in my review of v1. Since you do a kobject_put and not a
direct kfree() on cleanup, you need to define a release function to
do the actual free once the refcount drops to 0.

And since you have 2 kind of structs embedding the kobjects you will
need 2 release functions:

static void tmli_attr_setting_release(struct kobject *kobj)
{
	struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj);

	kfree(setting);
}

static void tmli_pwd_setting_release(struct kobject *kobj)
{
	struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj);

	kfree(pwd);
}

And then have 2 kobject-types:

static struct kobj_type tmli_attr_setting_ktype = {
	.release	= tmli_attr_setting_release,
	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
};

static struct kobj_type tmli_pwd_setting_ktype = {
	.release	= tmli_pwd_setting_release,
	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
};

And use the right type when creating the objects.



> +
> +/* ---- 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_register(&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, &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]->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, &attr_name_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, &attr_name_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_remove();
> +	return ret;
> +}
> +
> +/* ---- 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;
> +		}
> +		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;
> +	sprintf(tlmi_priv.pwd_admin->pwd_type, "pap");
> +	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");
> +	tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII;
> +	sprintf(tlmi_priv.pwd_admin->pwd_type, "pop");
> +	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_remove();
> +}
> +
> +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..2c1484304
> --- /dev/null
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -0,0 +1,73 @@
> +/* 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      128
> +#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)
> +
> +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_MAXLEN];
> +	char pwd_type[TLMI_PWDTYPE_LEN];

Note you can just make this a "const char *" and init it like this:

	pwd_type = "pop";

The "pop" string will be part of the const data section so its
address will still be valid after the function has exited.


> +	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_ */
> +
> 



Regards,

Hans

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

[Mark Pearson]

Thanks for the review Hans,

On 2021-05-19 1:06 p.m., Hans de Goede wrote:
> Hi,
> 
> On 5/9/21 3:57 AM, Mark Pearson wrote:
<snip>
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> index 8ea59fea4..31d1becfa 100644
>> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>> @@ -185,6 +185,17 @@ Description:
>>  					A write only value that when used in tandem with
>>  					current_password will reset a system or admin password.
>>  
>> +		encoding:
>> +					For platforms that require it (currently Lenovo) the
>> +					encoding method that is used. This can be either "ascii"
>> +					or "scancode". Default is set to "ascii"
>> +
>> +		kbdlang:
>> +					For platforms that require it (currently Lenovo only) 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"
>> +
> 
> I should have caught this during my review of v1, these are Lenovo specific, so please prefix
> them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them
> to a separate "Lenovo specific class extensions" part of the doc.
> 
No problem

<snip>

>>  
>>  What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index b0e1e5f65..0511c54fc 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -639,6 +639,18 @@ 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"
>> +	default m
> 
> default n (or no default at all) please.
Ack

<snip>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> new file mode 100644
>> index 000000000..2fa989e98
>> --- /dev/null
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -0,0 +1,890 @@
<snip>
>> +
>> +/* ---- 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);
>> +
>> +	if (setting->valid)
>> +		return sysfs_emit(buf, "1\n");
>> +	else
>> +		return sysfs_emit(buf, "0\n");
> 
> Maybe use:
> 
> 	sysfs_emit(buf, "%d\n", settings->valid);
> 
> instead?
Definitely :)

> 
> 
>> +}
>> +
>> +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 length;
>> +
>> +	length = strlen(buf);
>> +	if (buf[length-1] == '\n')
>> +		length--;
>> +
>> +	if (length >= TLMI_PWD_MAXLEN)
>> +		return -EINVAL;
>> +
>> +	memcpy(setting->password, buf, length);
>> +	setting->password[length] = '\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);
>> +	char *p, *new_pwd;
>> +	char *auth_str;
>> +	int ret = 0, len;
>> +
>> +	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;
>> +
>> +	p = strchr(new_pwd, '\n');
>> +	if (p)
>> +		*p = '\0';
>> +
>> +	if (strlen(new_pwd) > setting->maxlen) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>> +	len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
>> +		+ strlen(setting->kbdlang) + 3 /* type */
>> +		+ strlen(new_pwd) + 6 /* punctuation and terminator*/;
>> +	auth_str = kzalloc(len, GFP_KERNEL);
>> +	snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
>> +		 setting->pwd_type, setting->password, new_pwd,
>> +		 encoding_options[setting->encoding], setting->kbdlang);
>> +	ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);
> 
> So I guess on success any subsequent calls need to use the new password,
> so the user is expected to write the new password to the current_password
> file after changing the password this way?
> 
> I just checked the dell-wmi-sysman code and that does this:
> 
>         /* clear current_password here and use user input from wmi_priv.current_password */
>         if (!ret)
>                 memset(current_password, 0, MAX_BUFF);
> 
> Where current_password points to either the user or admin cached password,
> depending on which one is being changed.
> 
> So that seems to work the same way as what you are doing here (the user needs to
> write the new password to current_password after changing it through the
> new_password sysfs attribute). Can you add a patch to the patch-set documenting
> this in Documentation/ABI/testing/sysfs-class-firmware-attributes ?
> 
> Also you may want to consider clearing out the old cached password on success
> like the Dell code is doing.
> 
Would you have any objections/concerns if, on a successful password
update, this function just updates the stored password setting to the
new password.
Seems nicer from a user point of view then having to go and feed the
current_password field again.
e.g: strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN)

I know it would make Dell and Lenovo operate differently (I can add that
detail to the documentation) but it just feels like a nicer design.

<snip>

>> +
>> +static struct kobj_attribute auth_kbdlang = __ATTR_RW(kbdlang);
>> +
>> +{
>> +	if (strcmp(kobj->name, "Admin") == 0)
>> +		return sprintf(buf, "bios-admin\n");
>> +	else if (strcmp(kobj->name, "System") == 0)
>> +		return sprintf(buf, "power-on\n");
>> +	return -EIO;
>> +}
> 
> IMHO it would be nice to add a "const char *role" to struct tlmi_pwd_setting and
> then change this to:
> 
>> +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", setting->role);
>> +}
> 
> That would make this function consistent with the other password related functions.
Agreed - I'll make that change

<snip>

>> +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);
>> +	int alloc_len, auth_len = 0;
>> +	int str_ix = 0;
>> +	char *auth_str = NULL;
>> +	char *set_str, *new_setting, *p;
>> +	int ret;
>> +
>> +	if (!tlmi_priv.can_set_bios_settings)
>> +		return -EOPNOTSUPP;
>> +
>> +	new_setting = kstrdup(buf, GFP_KERNEL);
>> +	if (!new_setting)
>> +		return -ENOMEM;
>> +	p = strchr(new_setting, '\n');
>> +	if (p)
>> +		*p = '\0';
>> +
>> +	alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting);
>> +
>> +	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
>> +		auth_len += strlen(tlmi_priv.pwd_admin->password) + 1
>> +			+ strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1
>> +			+ strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1;
>> +		auth_str = kmalloc(auth_len, GFP_KERNEL);
>> +		if (!auth_str) {
>> +			ret = -ENOMEM;
>> +			goto out;
> 
> You end-up kfree-ing set_str here without it being initialized (compiler warning?)
> Please just initialize all 3 strings which you kfree at the end to NULL, I know
> this is not necessary for at least the new_setting string but it makes it easier
> for someone reading the code to verify that the kfree() does not happens on
> an uninitialized pointer.

Agreed. Thanks.
> 
>> +		}
>> +
>> +		sprintf(auth_str, "%s,%s,%s;",
>> +				tlmi_priv.pwd_admin->password,
>> +				encoding_options[tlmi_priv.pwd_admin->encoding],
>> +				tlmi_priv.pwd_admin->kbdlang);
>> +		alloc_len += auth_len;
>> +	}
>> +
>> +	set_str = kmalloc(alloc_len, GFP_KERNEL);
>> +	if (!set_str) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting);
>> +
>> +	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password)
> 
> Maybe use:
> 
> 	if (auth_str) 
> 
> here? This way if the condition of the above if block ever changes, you can't
> end up passing a NULL auth_Str to the sprintf.

Agreed - makes sense
<snip>

>> +
>> +static const struct sysfs_ops tlmi_kobj_sysfs_ops = {
>> +	.show	= tlmi_attr_show,
>> +	.store	= tlmi_attr_store,
>> +};
>> +
>> +static struct kobj_type attr_name_ktype = {
>> +	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
>> +};
> 
> You need to re-add a release function here, sorry I was not clear
> about that in my review of v1. Since you do a kobject_put and not a
> direct kfree() on cleanup, you need to define a release function to
> do the actual free once the refcount drops to 0.
> 
> And since you have 2 kind of structs embedding the kobjects you will
> need 2 release functions:
> 
> static void tmli_attr_setting_release(struct kobject *kobj)
> {
> 	struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj);
> 
> 	kfree(setting);
> }
> 
> static void tmli_pwd_setting_release(struct kobject *kobj)
> {
> 	struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj);
> 
> 	kfree(pwd);
> }
> 
> And then have 2 kobject-types:
> 
> static struct kobj_type tmli_attr_setting_ktype = {
> 	.release	= tmli_attr_setting_release,
> 	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
> };
> 
> static struct kobj_type tmli_pwd_setting_ktype = {
> 	.release	= tmli_pwd_setting_release,
> 	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
> };
> 
> And use the right type when creating the objects.
> 
Ah - got it. I had a feeling I was missing something here. Thanks!

<snip>

>> +
>> +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..2c1484304
>> --- /dev/null
>> +++ b/drivers/platform/x86/think-lmi.h
>> @@ -0,0 +1,73 @@
<snip>>> +
>> +/* password setting details */
>> +struct tlmi_pwd_setting {
>> +	struct kobject kobj;
>> +	bool valid;
>> +	char display_name[TLMI_PWDTYPE_MAXLEN];
>> +	char password[TLMI_PWD_MAXLEN];
>> +	char pwd_type[TLMI_PWDTYPE_LEN];
> 
> Note you can just make this a "const char *" and init it like this:
> 
> 	pwd_type = "pop";
> 
> The "pop" string will be part of the const data section so its
> address will still be valid after the function has exited.
> 
Ack

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

[Hans de Goede]

Hi Mark,

On 5/20/21 7:18 PM, Mark Pearson wrote:
> Thanks for the review Hans,

You're welcome.

> On 2021-05-19 1:06 p.m., Hans de Goede wrote:
>> Hi,
>>
>> On 5/9/21 3:57 AM, Mark Pearson wrote:
> <snip>
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>>> index 8ea59fea4..31d1becfa 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
>>> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
>>> @@ -185,6 +185,17 @@ Description:
>>>  					A write only value that when used in tandem with
>>>  					current_password will reset a system or admin password.
>>>  
>>> +		encoding:
>>> +					For platforms that require it (currently Lenovo) the
>>> +					encoding method that is used. This can be either "ascii"
>>> +					or "scancode". Default is set to "ascii"
>>> +
>>> +		kbdlang:
>>> +					For platforms that require it (currently Lenovo only) 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"
>>> +
>>
>> I should have caught this during my review of v1, these are Lenovo specific, so please prefix
>> them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them
>> to a separate "Lenovo specific class extensions" part of the doc.
>>
> No problem
> 
> <snip>
> 
>>>  
>>>  What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index b0e1e5f65..0511c54fc 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -639,6 +639,18 @@ 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"
>>> +	default m
>>
>> default n (or no default at all) please.
> Ack
> 
> <snip>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> new file mode 100644
>>> index 000000000..2fa989e98
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -0,0 +1,890 @@
> <snip>
>>> +
>>> +/* ---- 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);
>>> +
>>> +	if (setting->valid)
>>> +		return sysfs_emit(buf, "1\n");
>>> +	else
>>> +		return sysfs_emit(buf, "0\n");
>>
>> Maybe use:
>>
>> 	sysfs_emit(buf, "%d\n", settings->valid);
>>
>> instead?
> Definitely :)
> 
>>
>>
>>> +}
>>> +
>>> +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 length;
>>> +
>>> +	length = strlen(buf);
>>> +	if (buf[length-1] == '\n')
>>> +		length--;
>>> +
>>> +	if (length >= TLMI_PWD_MAXLEN)
>>> +		return -EINVAL;
>>> +
>>> +	memcpy(setting->password, buf, length);
>>> +	setting->password[length] = '\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);
>>> +	char *p, *new_pwd;
>>> +	char *auth_str;
>>> +	int ret = 0, len;
>>> +
>>> +	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;
>>> +
>>> +	p = strchr(new_pwd, '\n');
>>> +	if (p)
>>> +		*p = '\0';
>>> +
>>> +	if (strlen(new_pwd) > setting->maxlen) {
>>> +		ret = -EINVAL;
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>>> +	len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
>>> +		+ strlen(setting->kbdlang) + 3 /* type */
>>> +		+ strlen(new_pwd) + 6 /* punctuation and terminator*/;
>>> +	auth_str = kzalloc(len, GFP_KERNEL);
>>> +	snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
>>> +		 setting->pwd_type, setting->password, new_pwd,
>>> +		 encoding_options[setting->encoding], setting->kbdlang);
>>> +	ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);
>>
>> So I guess on success any subsequent calls need to use the new password,
>> so the user is expected to write the new password to the current_password
>> file after changing the password this way?
>>
>> I just checked the dell-wmi-sysman code and that does this:
>>
>>         /* clear current_password here and use user input from wmi_priv.current_password */
>>         if (!ret)
>>                 memset(current_password, 0, MAX_BUFF);
>>
>> Where current_password points to either the user or admin cached password,
>> depending on which one is being changed.
>>
>> So that seems to work the same way as what you are doing here (the user needs to
>> write the new password to current_password after changing it through the
>> new_password sysfs attribute). Can you add a patch to the patch-set documenting
>> this in Documentation/ABI/testing/sysfs-class-firmware-attributes ?
>>
>> Also you may want to consider clearing out the old cached password on success
>> like the Dell code is doing.
>>
> Would you have any objections/concerns if, on a successful password
> update, this function just updates the stored password setting to the
> new password.
> Seems nicer from a user point of view then having to go and feed the
> current_password field again.
> e.g: strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN)

Please use strscpy, strncpy has horrible syntax and using it is easy
to get wrong.

e.g. the above example is wrong if strlen(new_pwd) >= TLMI_PWD_MAXLEN
the resulting setting->password will not be 0 terminated (and you
seem to have swapped src and dst, but that is unrelated to strncpy
being a sucky function).

This also make me realize that the code has 2 max-pwd-lengths:

setting->maxlen
TLMI_PWD_MAXLEN

I think you should put a:

	if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN))
		pwdcfg.max_length = TLMI_PWD_MAXLEN;

in tlmi_analyze() so that we get a kernel-backtrace (and thus bugreports
if this condition ever becomes true.

And then use pwdcfg.max_length everywhere (e.g. also for the check in
current_password_store()).

Also the length checks in current_password_store() and new_password_store() 
should probably also check against settings->minlen ?


> I know it would make Dell and Lenovo operate differently (I can add that
> detail to the documentation) but it just feels like a nicer design.

That works for me. Perhaps you can also do a (compile tested only) RFC
patch for the Dell code to do the same thing (replace the memset 0 with
the strscpy) to see if the Dell folks are ok with also doing things this
way ?

Regards,

Hans

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

[Hans de Goede]

Hi,

On 5/21/21 10:10 AM, Hans de Goede wrote:

<snip>

Some minor corrections to my last email:

> This also make me realize that the code has 2 max-pwd-lengths:
> 
> setting->maxlen
> TLMI_PWD_MAXLEN
> 
> I think you should put a:
> 
> 	if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN))
> 		pwdcfg.max_length = TLMI_PWD_MAXLEN;

This needs to be:

 	if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_MAXEN))
 		pwdcfg.max_length = TLMI_PWD_MAXLEN - 1;

To account for the terminating 0 (maybe bump TLMI_PWD_MAXEN with 1
and rename it to TLMI_PWD_BUFSIZE?)

> in tlmi_analyze() so that we get a kernel-backtrace (and thus bugreports
> if this condition ever becomes true.
> 
> And then use pwdcfg.max_length everywhere (e.g. also for the check in
> current_password_store()).
> 
> Also the length checks in current_password_store() and new_password_store() 
> should probably also check against settings->minlen ?

I just realized that current_password_store() should also allow a length
of 0 to clear the password when the user is done making changes, so the
check in current_password_store() should be something like this:

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

Regards,

Gans

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

[Mark Pearson]

Thanks Hans

On 2021-05-21 4:10 a.m., Hans de Goede wrote:
> Hi Mark,
> 
> On 5/20/21 7:18 PM, Mark Pearson wrote:
<snip>

>>>> + +	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;'
>>>> */ +	len = strlen(setting->password) +
>>>> strlen(encoding_options[setting->encoding]) +		+
>>>> strlen(setting->kbdlang) + 3 /* type */ +		+ strlen(new_pwd) +
>>>> 6 /* punctuation and terminator*/; +	auth_str = kzalloc(len,
>>>> GFP_KERNEL); +	snprintf(auth_str, len, "%s,%s,%s,%s,%s;", +
>>>> setting->pwd_type, setting->password, new_pwd, +
>>>> encoding_options[setting->encoding], setting->kbdlang); +	ret =
>>>> tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);
>>> 
>>> So I guess on success any subsequent calls need to use the new
>>> password, so the user is expected to write the new password to
>>> the current_password file after changing the password this way?
>>> 
>>> I just checked the dell-wmi-sysman code and that does this:
>>> 
>>> /* clear current_password here and use user input from
>>> wmi_priv.current_password */ if (!ret) memset(current_password,
>>> 0, MAX_BUFF);
>>> 
>>> Where current_password points to either the user or admin cached
>>> password, depending on which one is being changed.
>>> 
>>> So that seems to work the same way as what you are doing here
>>> (the user needs to write the new password to current_password
>>> after changing it through the new_password sysfs attribute). Can
>>> you add a patch to the patch-set documenting this in
>>> Documentation/ABI/testing/sysfs-class-firmware-attributes ?
>>> 
>>> Also you may want to consider clearing out the old cached
>>> password on success like the Dell code is doing.
>>> 
>> Would you have any objections/concerns if, on a successful
>> password update, this function just updates the stored password
>> setting to the new password. Seems nicer from a user point of view
>> then having to go and feed the current_password field again. e.g:
>> strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN)
> 
> Please use strscpy, strncpy has horrible syntax and using it is easy 
> to get wrong.
> 
> e.g. the above example is wrong if strlen(new_pwd) >=
> TLMI_PWD_MAXLEN the resulting setting->password will not be 0
> terminated (and you seem to have swapped src and dst, but that is
> unrelated to strncpy being a sucky function).
Yeah, sorry, typed as an example only, and will use strscpy

> 
> This also make me realize that the code has 2 max-pwd-lengths:
> 
> setting->maxlen TLMI_PWD_MAXLEN
> 
> I think you should put a:
> 
> if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN)) pwdcfg.max_length =
> TLMI_PWD_MAXLEN;
> 
> in tlmi_analyze() so that we get a kernel-backtrace (and thus
> bugreports if this condition ever becomes true.
Sounds good
> 
> And then use pwdcfg.max_length everywhere (e.g. also for the check
> in current_password_store()).
> 
> Also the length checks in current_password_store() and
> new_password_store() should probably also check against
> settings->minlen ?
Agreed - will add
> 
> 
>> I know it would make Dell and Lenovo operate differently (I can add
>> that detail to the documentation) but it just feels like a nicer
>> design.
> 
> That works for me. Perhaps you can also do a (compile tested only)
> RFC patch for the Dell code to do the same thing (replace the memset
> 0 with the strscpy) to see if the Dell folks are ok with also doing
> things this way ?
> 
I'm not hugely comfortable with that. If for some reason it broke things
for Dell customers I wouldn't want to be responsible :) I'd rather they
made the changes and were able to test it - I know that's what I'd
prefer if it was the other way around. Apologies if I'm being over cautious!
I've added the new Dell kernel group email to the thread so they're aware :)

Mark

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

[Hans de Goede]

Hi,

On 5/21/21 5:55 PM, Mark Pearson wrote:

<snip>

>>> I know it would make Dell and Lenovo operate differently (I can add
>>> that detail to the documentation) but it just feels like a nicer
>>> design.
>>
>> That works for me. Perhaps you can also do a (compile tested only)
>> RFC patch for the Dell code to do the same thing (replace the memset
>> 0 with the strscpy) to see if the Dell folks are ok with also doing
>> things this way ?
>>
> I'm not hugely comfortable with that. If for some reason it broke things
> for Dell customers I wouldn't want to be responsible :)

Right, that is why I suggested making it a RFC patch and I would
certainly not apply that patch without it being tested by Dell first.

The idea behind the patch is for it to be a way to get a discussion
about this started. In my experience patches tend to get more of
a reaction then hypothetical discussions about changes :)

> I'd rather they
> made the changes and were able to test it - I know that's what I'd
> prefer if it was the other way around. Apologies if I'm being over cautious!

If you don't feel comfortable doing this, that is fine, lets wait what
the Dell folks have to say; and if they don't respond I might do a RFC
myself.

> I've added the new Dell kernel group email to the thread so they're aware :)

Thanks.

Regards,

Hans

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

[Mark Pearson]

On 2021-05-21 12:55 p.m., Hans de Goede wrote:
> Hi,
> 
> On 5/21/21 5:55 PM, Mark Pearson wrote:
> 
> <snip>
> 
>>>> I know it would make Dell and Lenovo operate differently (I can add
>>>> that detail to the documentation) but it just feels like a nicer
>>>> design.
>>>
>>> That works for me. Perhaps you can also do a (compile tested only)
>>> RFC patch for the Dell code to do the same thing (replace the memset
>>> 0 with the strscpy) to see if the Dell folks are ok with also doing
>>> things this way ?
>>>
>> I'm not hugely comfortable with that. If for some reason it broke things
>> for Dell customers I wouldn't want to be responsible :)
> 
> Right, that is why I suggested making it a RFC patch and I would
> certainly not apply that patch without it being tested by Dell first.
> 
> The idea behind the patch is for it to be a way to get a discussion
> about this started. In my experience patches tend to get more of
> a reaction then hypothetical discussions about changes :)
> 
>> I'd rather they
>> made the changes and were able to test it - I know that's what I'd
>> prefer if it was the other way around. Apologies if I'm being over cautious!
> 
> If you don't feel comfortable doing this, that is fine, lets wait what
> the Dell folks have to say; and if they don't respond I might do a RFC
> myself.
> 

Ah - I'd misunderstood that point. I have no issues with that :)

I won't update the document and publish a RFC instead as the last patch
in the series

Mark

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

[Ksr, Prasanth]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Friday, May 21, 2021 10:25 PM
> To: Mark Pearson
> Cc: mgross@linux.intel.com; platform-driver-x86@vger.kernel.org; Bharathi,
> Divya; Ksr, Prasanth; Dell Client Kernel
> Subject: Re: [External] Re: [PATCH v2 3/3] platform/x86: think-lmi: Add WMI
> interface support on Lenovo platforms
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 5/21/21 5:55 PM, Mark Pearson wrote:
> 
> <snip>
> 
> >>> I know it would make Dell and Lenovo operate differently (I can add
> >>> that detail to the documentation) but it just feels like a nicer
> >>> design.
> >>
> >> That works for me. Perhaps you can also do a (compile tested only)
> >> RFC patch for the Dell code to do the same thing (replace the memset
> >> 0 with the strscpy) to see if the Dell folks are ok with also doing
> >> things this way ?
> >>
> > I'm not hugely comfortable with that. If for some reason it broke
> > things for Dell customers I wouldn't want to be responsible :)
> 
> Right, that is why I suggested making it a RFC patch and I would certainly not
> apply that patch without it being tested by Dell first.
> 
> The idea behind the patch is for it to be a way to get a discussion about this
> started. In my experience patches tend to get more of a reaction then
> hypothetical discussions about changes :)
> 
> > I'd rather they
> > made the changes and were able to test it - I know that's what I'd
> > prefer if it was the other way around. Apologies if I'm being over cautious!
> 
> If you don't feel comfortable doing this, that is fine, lets wait what the Dell
> folks have to say; and if they don't respond I might do a RFC myself.
> 

Ack. We will implement the same from Dell side as well to have uniformity and
seems nicer from a user point of view rather than populating the 
current_password field again in case of password change scenario. 

> > I've added the new Dell kernel group email to the thread so they're
> > aware :)
> 
> Thanks.
> 
> Regards,
> 
> Hans

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

[Hans de Goede]

Hi,

On 5/24/21 12:19 PM, Ksr, Prasanth wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Friday, May 21, 2021 10:25 PM
>> To: Mark Pearson
>> Cc: mgross@linux.intel.com; platform-driver-x86@vger.kernel.org; Bharathi,
>> Divya; Ksr, Prasanth; Dell Client Kernel
>> Subject: Re: [External] Re: [PATCH v2 3/3] platform/x86: think-lmi: Add WMI
>> interface support on Lenovo platforms
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 5/21/21 5:55 PM, Mark Pearson wrote:
>>
>> <snip>
>>
>>>>> I know it would make Dell and Lenovo operate differently (I can add
>>>>> that detail to the documentation) but it just feels like a nicer
>>>>> design.
>>>>
>>>> That works for me. Perhaps you can also do a (compile tested only)
>>>> RFC patch for the Dell code to do the same thing (replace the memset
>>>> 0 with the strscpy) to see if the Dell folks are ok with also doing
>>>> things this way ?
>>>>
>>> I'm not hugely comfortable with that. If for some reason it broke
>>> things for Dell customers I wouldn't want to be responsible :)
>>
>> Right, that is why I suggested making it a RFC patch and I would certainly not
>> apply that patch without it being tested by Dell first.
>>
>> The idea behind the patch is for it to be a way to get a discussion about this
>> started. In my experience patches tend to get more of a reaction then
>> hypothetical discussions about changes :)
>>
>>> I'd rather they
>>> made the changes and were able to test it - I know that's what I'd
>>> prefer if it was the other way around. Apologies if I'm being over cautious!
>>
>> If you don't feel comfortable doing this, that is fine, lets wait what the Dell
>> folks have to say; and if they don't respond I might do a RFC myself.
>>
> 
> Ack. We will implement the same from Dell side as well to have uniformity and
> seems nicer from a user point of view rather than populating the 
> current_password field again in case of password change scenario. 

Great, thank you.

Regards,

Hans

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

[Mark Pearson]

Hi

On 2021-05-24 11:27 a.m., Hans de Goede wrote:
> Hi,
> 
> On 5/24/21 12:19 PM, Ksr, Prasanth wrote:
>> Hi,
>> 
>>> -----Original Message----- From: Hans de Goede
>>> <hdegoede@redhat.com> Sent: Friday, May 21, 2021 10:25 PM To:
>>> Mark Pearson Cc: mgross@linux.intel.com;
>>> platform-driver-x86@vger.kernel.org; Bharathi, Divya; Ksr,
>>> Prasanth; Dell Client Kernel Subject: Re: [External] Re: [PATCH
>>> v2 3/3] platform/x86: think-lmi: Add WMI interface support on
>>> Lenovo platforms
>>> 
>>> 
>>> [EXTERNAL EMAIL]
>>> 
>>> Hi,
>>> 
>>> On 5/21/21 5:55 PM, Mark Pearson wrote:
>>> 
>>> <snip>
>>> 
>>>>>> I know it would make Dell and Lenovo operate differently (I
>>>>>> can add that detail to the documentation) but it just feels
>>>>>> like a nicer design.
>>>>> 
>>>>> That works for me. Perhaps you can also do a (compile tested
>>>>> only) RFC patch for the Dell code to do the same thing
>>>>> (replace the memset 0 with the strscpy) to see if the Dell
>>>>> folks are ok with also doing things this way ?
>>>>> 
>>>> I'm not hugely comfortable with that. If for some reason it
>>>> broke things for Dell customers I wouldn't want to be
>>>> responsible :)
>>> 
>>> Right, that is why I suggested making it a RFC patch and I would
>>> certainly not apply that patch without it being tested by Dell
>>> first.
>>> 
>>> The idea behind the patch is for it to be a way to get a
>>> discussion about this started. In my experience patches tend to
>>> get more of a reaction then hypothetical discussions about
>>> changes :)
>>> 
>>>> I'd rather they made the changes and were able to test it - I
>>>> know that's what I'd prefer if it was the other way around.
>>>> Apologies if I'm being over cautious!
>>> 
>>> If you don't feel comfortable doing this, that is fine, lets wait
>>> what the Dell folks have to say; and if they don't respond I
>>> might do a RFC myself.
>>> 
>> 
>> Ack. We will implement the same from Dell side as well to have
>> uniformity and seems nicer from a user point of view rather than
>> populating the current_password field again in case of password
>> change scenario.
> 
Just as a note, testing this over the weekend, I found that the password
change doesn't become active until after a reboot on our systems.

So, at least on our systems, copying over strings is pointless.
I'll just document the behaviour in the document for our systems, and
see if we can improve it in our firmware going forward.

Thanks
Mark

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

[Andy Shevchenko]

On Sun, May 9, 2021 at 5:02 AM 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.

Few comments below.

...

> +/* Convert BIOS WMI error string to suitable error code */

Can it be rather the mapping structure? In that case you move string
literals, error codes and comments to its entries.
Here will be something like for-loop.

...

> +static int tlmi_extract_error(const struct acpi_buffer *output)
> +{
> +       const union acpi_object *obj;
> +       int ret;
> +
> +       obj = output->pointer;
> +       if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {

I would split !obj check, but I don't know if it is even a useful
check. Can it be the case?

> +               kfree(obj);

Basically you double check for NULL (see above)

Same for other similar places in the code.

> +               return -EIO;
> +       }
> +
> +       ret = tlmi_errstr_to_err(obj->string.pointer);
> +       kfree(obj);
> +       return ret;

What I really do not like here is that you are freeing something out
of the scope. Please, free it where it was acquired.

> +}

...

> +       /*
> +        * duplicated call required to match bios workaround for behavior
> +        * seen when WMI accessed via scripting on other OS

/*
 * Multi-line comments
 * should have this kind of
 * style. Pay attention to the details.
 */

> +        */

...

> +       *string = kstrdup(obj->string.pointer, GFP_KERNEL);
> +       kfree(obj);
> +       return *string ? 0 : -ENOMEM;

This breaks the principle "don't touch the output in error case".

...

> +       if (setting->valid)
> +               return sysfs_emit(buf, "1\n");
> +       else
> +               return sysfs_emit(buf, "0\n");

sysfs_emit(buf, "%d\n", !!setting->valid);
?

...

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

> +       length = strlen(buf);
> +       if (buf[length-1] == '\n')
> +               length--;

This will prevent you from using \n in the password. Why?

> +       if (length >= TLMI_PWD_MAXLEN)
> +               return -EINVAL;

I guess password *string* length is wrong per se. count seems the
proper one which one should use.

> +
> +       memcpy(setting->password, buf, length);

> +       setting->password[length] = '\0';

Why is the password a *string*? From where that assumption comes from?

> +       return count;
> +}
> +
> +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password);

...

> +       p = strchr(new_pwd, '\n');
> +       if (p)
> +               *p = '\0';

strtrim(). But see above.

> +       if (strlen(new_pwd) > setting->maxlen) {
> +               ret = -EINVAL;
> +               goto out;
> +       }

...

> +       /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
> +       len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
> +               + strlen(setting->kbdlang) + 3 /* type */
> +               + strlen(new_pwd) + 6 /* punctuation and terminator*/;
> +       auth_str = kzalloc(len, GFP_KERNEL);
> +       snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
> +                setting->pwd_type, setting->password, new_pwd,
> +                encoding_options[setting->encoding], setting->kbdlang);

NIH of kasprintf()

...

> +       return ret ? ret : count;

      return ret ?: count;

is shorter.

...

> +       if (strcmp(kobj->name, "Admin") == 0)
> +               return sprintf(buf, "bios-admin\n");
> +       else if (strcmp(kobj->name, "System") == 0)

Redundant 'else'

> +               return sprintf(buf, "power-on\n");
> +       return -EIO;

...

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

The terminator line doesn't need a comma.

> +};

...

> +       new_setting = kstrdup(buf, GFP_KERNEL);

strtrim(buf) ?

> +       if (!new_setting)
> +               return -ENOMEM;

> +       p = strchr(new_setting, '\n');
> +       if (p)
> +               *p = '\0';

See above.

> +       alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting);
> +
> +       if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
> +               auth_len += strlen(tlmi_priv.pwd_admin->password) + 1
> +                       + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1
> +                       + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1;
> +               auth_str = kmalloc(auth_len, GFP_KERNEL);
> +               if (!auth_str) {
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +
> +               sprintf(auth_str, "%s,%s,%s;",
> +                               tlmi_priv.pwd_admin->password,
> +                               encoding_options[tlmi_priv.pwd_admin->encoding],
> +                               tlmi_priv.pwd_admin->kbdlang);
> +               alloc_len += auth_len;

HIN of kasprintf()

> +       }
> +
> +       set_str = kmalloc(alloc_len, GFP_KERNEL);
> +       if (!set_str) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting);

Ditto,

> +       if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password)
> +               sprintf(set_str + str_ix, ",%s", auth_str);
> +       else
> +               sprintf(set_str + str_ix, ";");
> +
> +       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("");

> +       if (ret)
> +               goto out;

Useless.

> +out:
> +       kfree(auth_str);
> +       kfree(set_str);
> +       kfree(new_setting);
> +       return ret ? ret : count;

Can be shorter.

> +}

> +static struct kobj_attribute attr_displ_name =
> +               __ATTR_RO(display_name);

One line (and so on for all of them).

...

> +static struct attribute *tlmi_attrs[] = {
> +       &attr_displ_name.attr,
> +       &attr_current_val.attr,
> +       &attr_possible_values.attr,
> +       NULL,

No comma.

> +};

...

> +       struct kobj_attribute *kattr;

> +       ssize_t ret = -EIO;

Useless. Use direct return.

> +
> +       kattr = container_of(attr, struct kobj_attribute, attr);
> +       if (kattr->show)
> +               ret = kattr->show(kobj, kattr, buf);
> +       return ret;

...

> +       struct kobj_attribute *kattr;
> +       ssize_t ret = -EIO;
> +
> +       kattr = container_of(attr, struct kobj_attribute, attr);
> +       if (kattr->store)
> +               ret = kattr->store(kobj, kattr, buf, count);
> +       return ret;

As above.

...

> +       for (i = 0; i < TLMI_SETTINGS_COUNT; i++) {
> +               /*Check if index is a valid setting - skip if it isn't */

Missed space.

> +               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, &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]->kobj, &tlmi_attr_group);
> +               if (ret)
> +                       goto fail_create_attr;
> +       }

...

> +       int i = 0;

Why assignment?

> +       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] = '\\';
> +               }

NIH of strreplace(), but please check its name.

> +
> +               /* 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;
> +               }
> +               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);
> +       }

I stopped here because it's full of such small issues.

Basically the summary
 - try to find the existing APIs for routines that quite often in use
(usually string operations)
 - try to make your code compact with the same level of readability


-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Hi Andy,

Many thanks for the review

On 2021-05-22 7:04 a.m., Andy Shevchenko wrote:
> On Sun, May 9, 2021 at 5:02 AM 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.
> 
> Few comments below.
> 
> ...
> 
>> +/* Convert BIOS WMI error string to suitable error code */
> 
> Can it be rather the mapping structure? In that case you move string
> literals, error codes and comments to its entries.
> Here will be something like for-loop.
> 
Yes, I can do this
> ...
> 
>> +static int tlmi_extract_error(const struct acpi_buffer *output)
>> +{
>> +       const union acpi_object *obj;
>> +       int ret;
>> +
>> +       obj = output->pointer;
>> +       if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> 
> I would split !obj check, but I don't know if it is even a useful
> check. Can it be the case?
I wasn't certain on if obj could ever be null, I suspect not but as I
wasn't 100% sure I put the check in just in case.
> 
>> +               kfree(obj);
> 
> Basically you double check for NULL (see above)
> 
> Same for other similar places in the code.
> 
OK - understood. Thanks.

>> +               return -EIO;
>> +       }
>> +
>> +       ret = tlmi_errstr_to_err(obj->string.pointer);
>> +       kfree(obj);
>> +       return ret;
> 
> What I really do not like here is that you are freeing something out
> of the scope. Please, free it where it was acquired.
Ack
> 
>> +}
> 
> ...
> 
>> +       /*
>> +        * duplicated call required to match bios workaround for behavior
>> +        * seen when WMI accessed via scripting on other OS
> 
> /*
>  * Multi-line comments
>  * should have this kind of
>  * style. Pay attention to the details.
>  */
Ack (and I'm assuming here the concern is the 'D' and missing '.', let
me know if there's anything else that is important)

> 
>> +        */
> 
> ...
> 
>> +       *string = kstrdup(obj->string.pointer, GFP_KERNEL);
>> +       kfree(obj);
>> +       return *string ? 0 : -ENOMEM;
> 
> This breaks the principle "don't touch the output in error case".

But I'm not changing *string in an error case here - I'm not
understanding the issue here.
Happy to rewrite it to make it clearer though if that would help.

> 
> ...
> 
>> +       if (setting->valid)
>> +               return sysfs_emit(buf, "1\n");
>> +       else
>> +               return sysfs_emit(buf, "0\n");
> 
> sysfs_emit(buf, "%d\n", !!setting->valid);
> ?
Yeah - this was picked up on in an earlier review. Apologies.
> 
> ...
> 
>> +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 length;
> 
>> +       length = strlen(buf);
>> +       if (buf[length-1] == '\n')
>> +               length--;
> 
> This will prevent you from using \n in the password. Why?
The BIOS doesn't like it - so we strip it out :)

> 
>> +       if (length >= TLMI_PWD_MAXLEN)
>> +               return -EINVAL;
> 
> I guess password *string* length is wrong per se. count seems the
> proper one which one should use.
Yes, this is being cleaned up after review from Hans.

> 
>> +
>> +       memcpy(setting->password, buf, length);
> 
>> +       setting->password[length] = '\0';
> 
> Why is the password a *string*? From where that assumption comes from?
Sorry, I'm not understanding the question here. It's what our BIOS is
expecting. I'm missing something here
> 
>> +       return count;
>> +}
>> +
>> +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password);
> 
> ...
> 
>> +       p = strchr(new_pwd, '\n');
>> +       if (p)
>> +               *p = '\0';
> 
> strtrim(). But see above.
ack.
> 
>> +       if (strlen(new_pwd) > setting->maxlen) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
> 
> ...
> 
>> +       /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>> +       len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
>> +               + strlen(setting->kbdlang) + 3 /* type */
>> +               + strlen(new_pwd) + 6 /* punctuation and terminator*/;
>> +       auth_str = kzalloc(len, GFP_KERNEL);
>> +       snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
>> +                setting->pwd_type, setting->password, new_pwd,
>> +                encoding_options[setting->encoding], setting->kbdlang);
> 
> NIH of kasprintf()
Not sure what NIH is - but I'm assuming I should be using kasprintf
instead of snprinf :)
I wasn't aware of it - thank you.
> 
> ...
> 
>> +       return ret ? ret : count;
> 
>       return ret ?: count;
> 
> is shorter.
Ack
> 
> ...
> 
>> +       if (strcmp(kobj->name, "Admin") == 0)
>> +               return sprintf(buf, "bios-admin\n");
>> +       else if (strcmp(kobj->name, "System") == 0)
> 
> Redundant 'else'
Ack
> 
>> +               return sprintf(buf, "power-on\n");
>> +       return -EIO;
> 
> ...
> 
>> +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,
> 
> The terminator line doesn't need a comma.
Argh. I always get this wrong as to when it is required and when it isn't.
I'll fix
> 
>> +};
> 
> ...
> 
>> +       new_setting = kstrdup(buf, GFP_KERNEL);
> 
> strtrim(buf) ?
ack
> 
>> +       if (!new_setting)
>> +               return -ENOMEM;
> 
>> +       p = strchr(new_setting, '\n');
>> +       if (p)
>> +               *p = '\0';
> 
> See above.
Thanks
> 
>> +       alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting);
>> +
>> +       if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
>> +               auth_len += strlen(tlmi_priv.pwd_admin->password) + 1
>> +                       + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1
>> +                       + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1;
>> +               auth_str = kmalloc(auth_len, GFP_KERNEL);
>> +               if (!auth_str) {
>> +                       ret = -ENOMEM;
>> +                       goto out;
>> +               }
>> +
>> +               sprintf(auth_str, "%s,%s,%s;",
>> +                               tlmi_priv.pwd_admin->password,
>> +                               encoding_options[tlmi_priv.pwd_admin->encoding],
>> +                               tlmi_priv.pwd_admin->kbdlang);
>> +               alloc_len += auth_len;
> 
> HIN of kasprintf()
ack
> 
>> +       }
>> +
>> +       set_str = kmalloc(alloc_len, GFP_KERNEL);
>> +       if (!set_str) {
>> +               ret = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting);
> 
> Ditto,
ack
> 
>> +       if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password)
>> +               sprintf(set_str + str_ix, ",%s", auth_str);
>> +       else
>> +               sprintf(set_str + str_ix, ";");
>> +
>> +       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("");
> 
>> +       if (ret)
>> +               goto out;
> 
> Useless.
Ack (and oops)
> 
>> +out:
>> +       kfree(auth_str);
>> +       kfree(set_str);
>> +       kfree(new_setting);
>> +       return ret ? ret : count;
> 
> Can be shorter.
Ack
> 
>> +}
> 
>> +static struct kobj_attribute attr_displ_name =
>> +               __ATTR_RO(display_name);
> 
> One line (and so on for all of them).
Ack
> 
> ...
> 
>> +static struct attribute *tlmi_attrs[] = {
>> +       &attr_displ_name.attr,
>> +       &attr_current_val.attr,
>> +       &attr_possible_values.attr,
>> +       NULL,
> 
> No comma.
Ack
> 
>> +};
> 
> ...
> 
>> +       struct kobj_attribute *kattr;
> 
>> +       ssize_t ret = -EIO;
> 
> Useless. Use direct return.
Ack
> 
>> +
>> +       kattr = container_of(attr, struct kobj_attribute, attr);
>> +       if (kattr->show)
>> +               ret = kattr->show(kobj, kattr, buf);
>> +       return ret;
> 
> ...
> 
>> +       struct kobj_attribute *kattr;
>> +       ssize_t ret = -EIO;
>> +
>> +       kattr = container_of(attr, struct kobj_attribute, attr);
>> +       if (kattr->store)
>> +               ret = kattr->store(kobj, kattr, buf, count);
>> +       return ret;
> 
> As above.
Ack
> 
> ...
> 
>> +       for (i = 0; i < TLMI_SETTINGS_COUNT; i++) {
>> +               /*Check if index is a valid setting - skip if it isn't */
> 
> Missed space.
Ack
> 
>> +               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, &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]->kobj, &tlmi_attr_group);
>> +               if (ret)
>> +                       goto fail_create_attr;
>> +       }
> 
> ...
> 
>> +       int i = 0;
> 
> Why assignment?
Left over from earlier iteration. Should be removed.

> 
>> +       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] = '\\';
>> +               }
> 
> NIH of strreplace(), but please check its name.
Will do. Thanks!

> 
>> +
>> +               /* 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;
>> +               }
>> +               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);
>> +       }
> 
> I stopped here because it's full of such small issues.
Ouch.
Many thanks for the review. I'll go through the rest and see if I can
clean it up more to make it less painful.
> 
> Basically the summary
>  - try to find the existing APIs for routines that quite often in use
> (usually string operations)
>  - try to make your code compact with the same level of readability
> 
> 
I really had tried to do that - many thanks for your patience and review
details.

Mark

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

[Andy Shevchenko]

On Tue, May 25, 2021 at 6:14 PM Mark Pearson <markpearson@lenovo.com> wrote:
> On 2021-05-22 7:04 a.m., Andy Shevchenko wrote:
> > On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote:

...

> >> +       *string = kstrdup(obj->string.pointer, GFP_KERNEL);
> >> +       kfree(obj);
> >> +       return *string ? 0 : -ENOMEM;
> >
> > This breaks the principle "don't touch the output in error case".
>
> But I'm not changing *string in an error case here - I'm not
> understanding the issue here.
> Happy to rewrite it to make it clearer though if that would help.

*string may be not NULL when you do assign it.
You need to assign it iff you are about to return 0.

...

> >> +       length = strlen(buf);
> >> +       if (buf[length-1] == '\n')
> >> +               length--;
> >
> > This will prevent you from using \n in the password. Why?
> The BIOS doesn't like it - so we strip it out :)

I haven't checked, but if there is no description of this in the
documentation/commit message, should be added.

...

> >> +       memcpy(setting->password, buf, length);
> >
> >> +       setting->password[length] = '\0';
> >
> > Why is the password a *string*? From where that assumption comes from?
> Sorry, I'm not understanding the question here. It's what our BIOS is
> expecting. I'm missing something here

So, BIOS restrictions should be documented if not yet.

...

> >> +       /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
> >> +       len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
> >> +               + strlen(setting->kbdlang) + 3 /* type */
> >> +               + strlen(new_pwd) + 6 /* punctuation and terminator*/;
> >> +       auth_str = kzalloc(len, GFP_KERNEL);
> >> +       snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
> >> +                setting->pwd_type, setting->password, new_pwd,
> >> +                encoding_options[setting->encoding], setting->kbdlang);
> >
> > NIH of kasprintf()
> Not sure what NIH is -

https://en.wikipedia.org/wiki/Not_invented_here

> but I'm assuming I should be using kasprintf
> instead of snprinf :)
> I wasn't aware of it - thank you.

strlen+kmalloc+sprintf == kasprintf

...

> > The terminator line doesn't need a comma.
> Argh. I always get this wrong as to when it is required and when it isn't.
> I'll fix

If it is supposed to be the last entry (i.o.w. terminator) --> no comma.

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Pearson]

Thanks Andy

On 2021-05-25 12:18 p.m., Andy Shevchenko wrote:
> On Tue, May 25, 2021 at 6:14 PM Mark Pearson <markpearson@lenovo.com> wrote:
>> On 2021-05-22 7:04 a.m., Andy Shevchenko wrote:
>>> On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote:
> 
> ...
> 
>>>> +       *string = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>> +       kfree(obj);
>>>> +       return *string ? 0 : -ENOMEM;
>>>
>>> This breaks the principle "don't touch the output in error case".
>>
>> But I'm not changing *string in an error case here - I'm not
>> understanding the issue here.
>> Happy to rewrite it to make it clearer though if that would help.
> 
> *string may be not NULL when you do assign it.
> You need to assign it iff you are about to return 0.
Ah - got it. I'll fix.

> 
> ...
> 
>>>> +       length = strlen(buf);
>>>> +       if (buf[length-1] == '\n')
>>>> +               length--;
>>>
>>> This will prevent you from using \n in the password. Why?
>> The BIOS doesn't like it - so we strip it out :)
> 
> I haven't checked, but if there is no description of this in the
> documentation/commit message, should be added.
Ack.
> 
> ...
> 
>>>> +       memcpy(setting->password, buf, length);
>>>
>>>> +       setting->password[length] = '\0';
>>>
>>> Why is the password a *string*? From where that assumption comes from?
>> Sorry, I'm not understanding the question here. It's what our BIOS is
>> expecting. I'm missing something here
> 
> So, BIOS restrictions should be documented if not yet.
Ack
> 
> ...
> 
>>>> +       /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
>>>> +       len = strlen(setting->password) + strlen(encoding_options[setting->encoding])
>>>> +               + strlen(setting->kbdlang) + 3 /* type */
>>>> +               + strlen(new_pwd) + 6 /* punctuation and terminator*/;
>>>> +       auth_str = kzalloc(len, GFP_KERNEL);
>>>> +       snprintf(auth_str, len, "%s,%s,%s,%s,%s;",
>>>> +                setting->pwd_type, setting->password, new_pwd,
>>>> +                encoding_options[setting->encoding], setting->kbdlang);
>>>
>>> NIH of kasprintf()
>> Not sure what NIH is -
> 
> https://en.wikipedia.org/wiki/Not_invented_here
Neat :)
> 
>> but I'm assuming I should be using kasprintf
>> instead of snprinf :)
>> I wasn't aware of it - thank you.
> 
> strlen+kmalloc+sprintf == kasprintf
> 
> ...
> 
>>> The terminator line doesn't need a comma.
>> Argh. I always get this wrong as to when it is required and when it isn't.
>> I'll fix
> 
> If it is supposed to be the last entry (i.o.w. terminator) --> no comma.
> 
Thanks

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

[Hans de Goede]

Hi,

On 5/25/21 5:14 PM, Mark Pearson wrote:
> Hi Andy,
> 
>>> +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 length;
>>
>>> +       length = strlen(buf);
>>> +       if (buf[length-1] == '\n')
>>> +               length--;
>>
>> This will prevent you from using \n in the password. Why?
> The BIOS doesn't like it - so we strip it out :)

Erm, I don't believe that that is the whole story, there are 2
separate things at play here:

1. When entering the BIOS password at system power-on pressing
enter means you're done and the BIOS should check what you've
just entered as password before pressing the enter key, so the
password can never contain '\n' since the enter key is the
terminator for entering the password at boot

2. People often use sysfs files by doing things like this:

echo mysecretpassword > /sys/.../current_password

And the "echo" shell command will then add an extra '\n' this
is why you will see code like this to strip the '\n' in functions
which use the input string as is (instead of doing strtol,
sysfs_match_string or something else which does not care about a
terminating '\n' already, note that functions like sysfs_str_equals
and sysfs_match_string are special helpers for not caring about
the '\n' without needing to strip it (because stripping it
requires a strdup).

So what is happening here is simply stripping the '\n' which may
have been added by echo (if it was added).

Regards,

Hans

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

[Mark Pearson]

On 2021-05-25 12:29 p.m., Hans de Goede wrote:
> Hi,
> 
> On 5/25/21 5:14 PM, Mark Pearson wrote:
>> Hi Andy,
>>
>>>> +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 length;
>>>
>>>> +       length = strlen(buf);
>>>> +       if (buf[length-1] == '\n')
>>>> +               length--;
>>>
>>> This will prevent you from using \n in the password. Why?
>> The BIOS doesn't like it - so we strip it out :)
> 
> Erm, I don't believe that that is the whole story, there are 2
> separate things at play here:
> 
> 1. When entering the BIOS password at system power-on pressing
> enter means you're done and the BIOS should check what you've
> just entered as password before pressing the enter key, so the
> password can never contain '\n' since the enter key is the
> terminator for entering the password at boot
> 
> 2. People often use sysfs files by doing things like this:
> 
> echo mysecretpassword > /sys/.../current_password
> 
> And the "echo" shell command will then add an extra '\n' this
> is why you will see code like this to strip the '\n' in functions
> which use the input string as is (instead of doing strtol,
> sysfs_match_string or something else which does not care about a
> terminating '\n' already, note that functions like sysfs_str_equals
> and sysfs_match_string are special helpers for not caring about
> the '\n' without needing to strip it (because stripping it
> requires a strdup).
> 
> So what is happening here is simply stripping the '\n' which may
> have been added by echo (if it was added).
> 
> Regards,
> 
> Hans
> 
Agreed on all, I guess I was giving the story summary :)
I've been using this method in testing and if the \n gets passed on then
you'll get an error returned - the BIOS doesn't like it.

I've no idea if this is Lenovo specific or not. I'll document it as a
Lenovo specific case unless there are objections.

Mark

Re: [PATCH v2 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events

[Hans de Goede]

Hi Mark,

On 5/9/21 3:57 AM, 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 (hope
> that's correct).

Yes that is correct; and thank you for taking care of making
the code for registering the firmware_attribute class shared.

> 
>  drivers/platform/x86/Kconfig                  |  6 +++
>  drivers/platform/x86/Makefile                 |  1 +
>  .../platform/x86/firmware_attributes_class.c  | 51 +++++++++++++++++++
>  .../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..b0e1e5f65 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1076,6 +1076,12 @@ 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"> +	help
> +	  This option should be enabled by any modules using the firmware
> +	  attributes class.

My idea here was to have this be a kernel-library which drivers which need it
select. In this case it should not be visible to end-users and does not need a
help text, so this should look like this:

config FW_ATTR_CLASS
	tristate
	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..4ed959d6c
> --- /dev/null
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -0,0 +1,51 @@
> +// 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);
> +bool fw_attr_inuse;
> +
> +static struct class firmware_attributes_class = {
> +	.name = "firmware-attributes",
> +};
> +
> +int fw_attributes_class_register(struct class **fw_attr_class)
> +{
> +	int err;
> +
> +	mutex_lock(&fw_attr_lock);
> +	/* We can only have one active FW attribute class */
> +	if (fw_attr_inuse) {
> +		mutex_unlock(&fw_attr_lock);
> +		return -EEXIST;
> +	}

I think it should be allowed to have multiple drivers
using the firmware_attributes class, e.g. besides the main system
BIOS an Ethermet or FiberChannel; card with an option ROM which supports
booting from iSCSI/FCoE or FiberChannel SCSI disks / LUNs could expose
settings to configure which disk/LUN to boot from this way.

And there is nothing wrong with multiple drivers calling device_create
with the same class.

So I suggest renaming fw_attributes_class_register to
fw_attributes_class_get (and unregister to put) and to make
fw_attr_inuse a counter and create the class when the counter
goes from 0 -> 1 and free it when it goes from 1 to 0 again.

Otherwise this looks good. I like that you already thought about
races and have protected fw_attr_inuse with a mutex.

Regards,

Hans






> +
> +	err = class_register(&firmware_attributes_class);
> +	if (err) {
> +		mutex_unlock(&fw_attr_lock);
> +		return err;
> +	}
> +	fw_attr_inuse = true;
> +	*fw_attr_class = &firmware_attributes_class;
> +	mutex_unlock(&fw_attr_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fw_attributes_class_register);
> +
> +void fw_attributes_class_remove(void)
> +{
> +	mutex_lock(&fw_attr_lock);
> +	fw_attr_inuse = false;
> +	class_unregister(&firmware_attributes_class);
> +	mutex_unlock(&fw_attr_lock);
> +}
> +EXPORT_SYMBOL_GPL(fw_attributes_class_remove);
> +
> +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..e479a5720
> --- /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_register(struct class **fw_attr_class);
> +void fw_attributes_class_remove(void);
> +
> +#endif /* FW_ATTR_CLASS_H */
> +
> +
> 

Re: [External] Re: [PATCH v2 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events

[Mark Pearson]

Thanks Hans

On 2021-05-19 12:15 p.m., Hans de Goede wrote:
> Hi Mark,
> 
> On 5/9/21 3:57 AM, 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 (hope
>> that's correct).
> 
> Yes that is correct; and thank you for taking care of making
> the code for registering the firmware_attribute class shared.
> 
>>
>>  drivers/platform/x86/Kconfig                  |  6 +++
>>  drivers/platform/x86/Makefile                 |  1 +
>>  .../platform/x86/firmware_attributes_class.c  | 51 +++++++++++++++++++
>>  .../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..b0e1e5f65 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1076,6 +1076,12 @@ 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"> +	help
>> +	  This option should be enabled by any modules using the firmware
>> +	  attributes class.
> 
> My idea here was to have this be a kernel-library which drivers which need it
> select. In this case it should not be visible to end-users and does not need a
> help text, so this should look like this:
> 
> config FW_ATTR_CLASS
> 	tristate
> 	default n
> 
Got it - I'll update

>>  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..4ed959d6c
>> --- /dev/null
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -0,0 +1,51 @@
>> +// 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);
>> +bool fw_attr_inuse;
>> +
>> +static struct class firmware_attributes_class = {
>> +	.name = "firmware-attributes",
>> +};
>> +
>> +int fw_attributes_class_register(struct class **fw_attr_class)
>> +{
>> +	int err;
>> +
>> +	mutex_lock(&fw_attr_lock);
>> +	/* We can only have one active FW attribute class */
>> +	if (fw_attr_inuse) {
>> +		mutex_unlock(&fw_attr_lock);
>> +		return -EEXIST;
>> +	}
> 
> I think it should be allowed to have multiple drivers
> using the firmware_attributes class, e.g. besides the main system
> BIOS an Ethermet or FiberChannel; card with an option ROM which supports
> booting from iSCSI/FCoE or FiberChannel SCSI disks / LUNs could expose
> settings to configure which disk/LUN to boot from this way.
> 
> And there is nothing wrong with multiple drivers calling device_create
> with the same class.
> 
> So I suggest renaming fw_attributes_class_register to
> fw_attributes_class_get (and unregister to put) and to make
> fw_attr_inuse a counter and create the class when the counter
> goes from 0 -> 1 and free it when it goes from 1 to 0 again.
> 
> Otherwise this looks good. I like that you already thought about
> races and have protected fw_attr_inuse with a mutex.
> 
Ah - I hadn't considered it as being used with devices other than the
BIOS. That all makes sense and I'll update.

Thanks for the review!
Mark

Re: [PATCH v2 1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events

[Hans de Goede]

p.s.

I'm getting bounces for: mario.limonciello@dell.com (Mario has a new (also Linux related) job elsewhere),
for the next version of the series please replace the Cc to Mario with a Cc to Dell.Client.Kernel@dell.com.





On 5/9/21 3:57 AM, 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 (hope
> that's correct).
> 
>  drivers/platform/x86/Kconfig                  |  6 +++
>  drivers/platform/x86/Makefile                 |  1 +
>  .../platform/x86/firmware_attributes_class.c  | 51 +++++++++++++++++++
>  .../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..b0e1e5f65 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1076,6 +1076,12 @@ 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"
> +	help
> +	  This option should be enabled by any modules using the firmware
> +	  attributes class.
> +
>  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..4ed959d6c
> --- /dev/null
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -0,0 +1,51 @@
> +// 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);
> +bool fw_attr_inuse;
> +
> +static struct class firmware_attributes_class = {
> +	.name = "firmware-attributes",
> +};
> +
> +int fw_attributes_class_register(struct class **fw_attr_class)
> +{
> +	int err;
> +
> +	mutex_lock(&fw_attr_lock);
> +	/* We can only have one active FW attribute class */
> +	if (fw_attr_inuse) {
> +		mutex_unlock(&fw_attr_lock);
> +		return -EEXIST;
> +	}
> +
> +	err = class_register(&firmware_attributes_class);
> +	if (err) {
> +		mutex_unlock(&fw_attr_lock);
> +		return err;
> +	}
> +	fw_attr_inuse = true;
> +	*fw_attr_class = &firmware_attributes_class;
> +	mutex_unlock(&fw_attr_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fw_attributes_class_register);
> +
> +void fw_attributes_class_remove(void)
> +{
> +	mutex_lock(&fw_attr_lock);
> +	fw_attr_inuse = false;
> +	class_unregister(&firmware_attributes_class);
> +	mutex_unlock(&fw_attr_lock);
> +}
> +EXPORT_SYMBOL_GPL(fw_attributes_class_remove);
> +
> +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..e479a5720
> --- /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_register(struct class **fw_attr_class);
> +void fw_attributes_class_remove(void);
> +
> +#endif /* FW_ATTR_CLASS_H */
> +
> +
>