diff mbox series

[v4,1/2] hwmon: (nct6775) Directly call ASUS ACPI WMI method

Message ID 20230111212241.7456-1-pauk.denis@gmail.com (mailing list archive)
State Accepted
Headers show
Series [v4,1/2] hwmon: (nct6775) Directly call ASUS ACPI WMI method | expand

Commit Message

Denis Pauk Jan. 11, 2023, 9:22 p.m. UTC
New ASUS B650/B660/X670 boards firmware have not exposed WMI monitoring
GUID  and entrypoint method WMBD could be implemented for different device
UID.

Implement the direct call to entrypoint method for monitoring the device
UID of B550/X570 boards.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
Co-developed-by: Ahmad Khalifa <ahmad@khalifa.ws>
Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>
---
Changes:
v2:
  Rename each_port_arg to each_device_arg
  Rename nct6775_find_asus_acpi to nct6775_asuswmi_device_match
  Remove unrequired return -EEXIST, and iterate whole list of devices
  Make asus_acpi_dev static
v3:
  Restore break iteration logic in nct6775_asuswmi_device_match
  Add config check if ACPI disabled and acpi_device_* are undefined
  Pass device_id as data parameter of acpi_bus_for_each_dev
v4:
  Hide asus_acpi_dev definition inside ifdef ACPI
  
@all
 Could someone please test updated patch with B550/X570 boards?
 
@Guenter Roeck
> 0-day reported various errors. I suspect those are seen with CONFIG_ACPI=n.
> Have you tried the build reported as problematic ?
>
> I get
>
> drivers/hwmon/nct6775-platform.c:122:28: error: ‘asus_acpi_dev’ defined 
>  but not used [-Werror=unused-variable]
>
>   122 | static struct acpi_device *asus_acpi_dev;
>
> if I try to build nct6775-platform.o with W=1 and CONFIG_ACPI=n.
>
> Overall the #ifdefs in the driver get a bit out of hand. I think it
> may be time to consolidate that. Not necessarily now, but sometime soon.

Thank you, I have missed it, I have rechecked with:
make W=1 CONFIG_ACPI=n CONFIG_SENSORS_NCT6775=m drivers/hwmon/ 

 drivers/hwmon/Kconfig            |  2 +-
 drivers/hwmon/nct6775-platform.c | 99 ++++++++++++++++++++++----------
 2 files changed, 71 insertions(+), 30 deletions(-)


base-commit: b0587c87abc891e313d63946ff8c9f4939d1ea1a

Comments

Geert Uytterhoeven June 6, 2023, 10:29 a.m. UTC | #1
Hi Denis,

On Wed, Jan 11, 2023 at 10:24 PM Denis Pauk <pauk.denis@gmail.com> wrote:
> New ASUS B650/B660/X670 boards firmware have not exposed WMI monitoring
> GUID  and entrypoint method WMBD could be implemented for different device
> UID.
>
> Implement the direct call to entrypoint method for monitoring the device
> UID of B550/X570 boards.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
> Co-developed-by: Ahmad Khalifa <ahmad@khalifa.ws>
> Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>

Thanks for your patch, which is now commit c3b3747d02f571da ("hwmon:
(nct6775) Directly call ASUS ACPI WMI method") in  v6.3-rc1.

> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1516,7 +1516,7 @@ config SENSORS_NCT6775_CORE
>  config SENSORS_NCT6775
>         tristate "Platform driver for Nuvoton NCT6775F and compatibles"
>         depends on !PPC
> -       depends on ACPI_WMI || ACPI_WMI=n
> +       depends on ACPI || ACPI=n
>         select HWMON_VID
>         select SENSORS_NCT6775_CORE
>         help

The recent patches to add support for ACPI on RISC-V caused me to
see a question about this driver again when running "make oldconfig",
and I had a closer look at the driver...
Unless I am missing something, this is a really dangerous driver which
just bangs blindly into I/O space without doing any platform checks,
which could cause a crash or system lock-up?

Does the SENSORS_NCT6775 symbol need a better platform dependenc
than !PPC?

Gr{oetje,eeting}s,

                        Geert
Guenter Roeck June 6, 2023, 1:51 p.m. UTC | #2
On 6/6/23 03:29, Geert Uytterhoeven wrote:
> Hi Denis,
> 
> On Wed, Jan 11, 2023 at 10:24 PM Denis Pauk <pauk.denis@gmail.com> wrote:
>> New ASUS B650/B660/X670 boards firmware have not exposed WMI monitoring
>> GUID  and entrypoint method WMBD could be implemented for different device
>> UID.
>>
>> Implement the direct call to entrypoint method for monitoring the device
>> UID of B550/X570 boards.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
>> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
>> Co-developed-by: Ahmad Khalifa <ahmad@khalifa.ws>
>> Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>
> 
> Thanks for your patch, which is now commit c3b3747d02f571da ("hwmon:
> (nct6775) Directly call ASUS ACPI WMI method") in  v6.3-rc1.
> 
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1516,7 +1516,7 @@ config SENSORS_NCT6775_CORE
>>   config SENSORS_NCT6775
>>          tristate "Platform driver for Nuvoton NCT6775F and compatibles"
>>          depends on !PPC
>> -       depends on ACPI_WMI || ACPI_WMI=n
>> +       depends on ACPI || ACPI=n
>>          select HWMON_VID
>>          select SENSORS_NCT6775_CORE
>>          help
> 
> The recent patches to add support for ACPI on RISC-V caused me to
> see a question about this driver again when running "make oldconfig",
> and I had a closer look at the driver...
> Unless I am missing something, this is a really dangerous driver which
> just bangs blindly into I/O space without doing any platform checks,
> which could cause a crash or system lock-up?
> 
> Does the SENSORS_NCT6775 symbol need a better platform dependenc
> than !PPC?
> 

This is no different than all the other SuperIO drivers.

Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 3176c33af6c6..300ce8115ce4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1516,7 +1516,7 @@  config SENSORS_NCT6775_CORE
 config SENSORS_NCT6775
 	tristate "Platform driver for Nuvoton NCT6775F and compatibles"
 	depends on !PPC
-	depends on ACPI_WMI || ACPI_WMI=n
+	depends on ACPI || ACPI=n
 	select HWMON_VID
 	select SENSORS_NCT6775_CORE
 	help
diff --git a/drivers/hwmon/nct6775-platform.c b/drivers/hwmon/nct6775-platform.c
index bf43f73dc835..e3c3b35c8138 100644
--- a/drivers/hwmon/nct6775-platform.c
+++ b/drivers/hwmon/nct6775-platform.c
@@ -17,7 +17,6 @@ 
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
-#include <linux/wmi.h>
 
 #include "nct6775.h"
 
@@ -107,40 +106,50 @@  struct nct6775_sio_data {
 	void (*sio_exit)(struct nct6775_sio_data *sio_data);
 };
 
-#define ASUSWMI_MONITORING_GUID		"466747A0-70EC-11DE-8A39-0800200C9A66"
+#define ASUSWMI_METHOD			"WMBD"
 #define ASUSWMI_METHODID_RSIO		0x5253494F
 #define ASUSWMI_METHODID_WSIO		0x5753494F
 #define ASUSWMI_METHODID_RHWM		0x5248574D
 #define ASUSWMI_METHODID_WHWM		0x5748574D
 #define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
+#define ASUSWMI_DEVICE_HID		"PNP0C14"
+#define ASUSWMI_DEVICE_UID		"ASUSWMI"
+
+#if IS_ENABLED(CONFIG_ACPI)
+/*
+ * ASUS boards have only one device with WMI "WMBD" method and have provided
+ * access to only one SuperIO chip at 0x0290.
+ */
+static struct acpi_device *asus_acpi_dev;
+#endif
 
 static int nct6775_asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
 {
-#if IS_ENABLED(CONFIG_ACPI_WMI)
+#if IS_ENABLED(CONFIG_ACPI)
+	acpi_handle handle = acpi_device_handle(asus_acpi_dev);
 	u32 args = bank | (reg << 8) | (val << 16);
-	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
-	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_object_list input;
+	union acpi_object params[3];
+	unsigned long long result;
 	acpi_status status;
-	union acpi_object *obj;
-	u32 tmp = ASUSWMI_UNSUPPORTED_METHOD;
-
-	status = wmi_evaluate_method(ASUSWMI_MONITORING_GUID, 0,
-				     method_id, &input, &output);
 
+	params[0].type = ACPI_TYPE_INTEGER;
+	params[0].integer.value = 0;
+	params[1].type = ACPI_TYPE_INTEGER;
+	params[1].integer.value = method_id;
+	params[2].type = ACPI_TYPE_BUFFER;
+	params[2].buffer.length = sizeof(args);
+	params[2].buffer.pointer = (void *)&args;
+	input.count = 3;
+	input.pointer = params;
+
+	status = acpi_evaluate_integer(handle, ASUSWMI_METHOD, &input, &result);
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
-	obj = output.pointer;
-	if (obj && obj->type == ACPI_TYPE_INTEGER)
-		tmp = obj->integer.value;
-
 	if (retval)
-		*retval = tmp;
+		*retval = (u32)result & 0xFFFFFFFF;
 
-	kfree(obj);
-
-	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
-		return -ENODEV;
 	return 0;
 #else
 	return -EOPNOTSUPP;
@@ -1099,6 +1108,46 @@  static const char * const asus_wmi_boards[] = {
 	"TUF GAMING Z490-PLUS (WI-FI)",
 };
 
+#if IS_ENABLED(CONFIG_ACPI)
+/*
+ * Callback for acpi_bus_for_each_dev() to find the right device
+ * by _UID and _HID and return 1 to stop iteration.
+ */
+static int nct6775_asuswmi_device_match(struct device *dev, void *data)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+	const char *uid = acpi_device_uid(adev);
+	const char *hid = acpi_device_hid(adev);
+
+	if (hid && !strcmp(hid, ASUSWMI_DEVICE_HID) &&
+		uid && !strcmp(uid, data)) {
+		asus_acpi_dev = adev;
+		return 1;
+	}
+
+	return 0;
+}
+#endif
+
+static enum sensor_access nct6775_determine_access(const char *device_uid)
+{
+#if IS_ENABLED(CONFIG_ACPI)
+	u8 tmp;
+
+	acpi_bus_for_each_dev(nct6775_asuswmi_device_match, (void *)device_uid);
+	if (!asus_acpi_dev)
+		return access_direct;
+
+	/* if reading chip id via ACPI succeeds, use WMI "WMBD" method for access */
+	if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp) && tmp) {
+		pr_debug("Using Asus WMBD method of %s to access %#x chip.\n", device_uid, tmp);
+		return access_asuswmi;
+	}
+#endif
+
+	return access_direct;
+}
+
 static int __init sensors_nct6775_platform_init(void)
 {
 	int i, err;
@@ -1109,7 +1158,6 @@  static int __init sensors_nct6775_platform_init(void)
 	int sioaddr[2] = { 0x2e, 0x4e };
 	enum sensor_access access = access_direct;
 	const char *board_vendor, *board_name;
-	u8 tmp;
 
 	err = platform_driver_register(&nct6775_driver);
 	if (err)
@@ -1122,15 +1170,8 @@  static int __init sensors_nct6775_platform_init(void)
 	    !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
 		err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
 				   board_name);
-		if (err >= 0) {
-			/* if reading chip id via WMI succeeds, use WMI */
-			if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp) && tmp) {
-				pr_info("Using Asus WMI to access %#x chip.\n", tmp);
-				access = access_asuswmi;
-			} else {
-				pr_err("Can't read ChipID by Asus WMI.\n");
-			}
-		}
+		if (err >= 0)
+			access = nct6775_determine_access(ASUSWMI_DEVICE_UID);
 	}
 
 	/*