| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230109230953.GA1788936@roeck-us.net>
Date: Mon, 9 Jan 2023 15:09:53 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Denis Pauk <pauk.denis@...il.com>
Cc: ahmad@...lifa.ws, chunkeey@...il.com, greg@...pto.org,
hubert.banas@...il.com, igor@...lig.com, jaap.dehaan@...enet.de,
jdelvare@...e.com, jeroen@...rstra.org, jonfarr87@...il.com,
jwp@...hat.com, kdudka@...hat.com, kernel@...in.net,
kpietrzak@...root.org, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org, me@...toor.com,
metalcaedes@...il.com, michael@...oddone.net,
mikhail.v.gavrilov@...il.com, mundanedefoliation@...il.com,
nephartyz@...il.com, oleksandr@...alenko.name, pehlm@...holm.org,
renedis@...mail.com, robert@...ecki.net,
sahan.h.fernando@...il.com, sebastian.arnhold@...teo.de,
sefoci9222@...unway.com, sst@...zta.fm, to.eivind@...il.com,
torvic9@...lbox.org
Subject: Re: [PATCH v2 1/2] hwmon: (nct6775) Directly call ASUS ACPI WMI
method
On Mon, Jan 09, 2023 at 11:15:07PM +0200, Denis Pauk 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@...il.com>
> Co-developed-by: Ahmad Khalifa <ahmad@...lifa.ws>
> Signed-off-by: Ahmad Khalifa <ahmad@...lifa.ws>
> ---
> Changes:
> v1:
> 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
Why ? More on that below.
> make asus_acpi_dev static
>
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/nct6775-platform.c | 97 ++++++++++++++++++++++----------
> 2 files changed, 69 insertions(+), 30 deletions(-)
>
> 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..1f7885af524e 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,44 @@ 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"
> +
> +static struct acpi_device *asus_acpi_dev;
>
> 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;
> -
> - kfree(obj);
> + *retval = (u32)result & 0xFFFFFFFF;
>
> - if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
> - return -ENODEV;
> return 0;
> #else
> return -EOPNOTSUPP;
> @@ -1099,6 +1102,50 @@ static const char * const asus_wmi_boards[] = {
> "TUF GAMING Z490-PLUS (WI-FI)",
> };
>
> +struct each_device_arg {
> + struct acpi_device *adev;
> + const char *match;
> +};
> +
> +/*
> + * Callback for acpi_bus_for_each_dev() to find the right device
> + * by _UID and _HID and store to each_device_arg.
> + */
> +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);
> + struct each_device_arg *arg = data;
> +
> + if (hid && !strcmp(hid, ASUSWMI_DEVICE_HID) &&
> + uid && !strcmp(uid, arg->match)) {
> + arg->adev = adev;
Why not return 1 for match here ? If there is a reason to look for
the last match instead of the first match, it needs to be explained.
> + }
> +
> + return 0;
> +}
> +
> +static enum sensor_access nct6775_determine_access(const char *device_uid)
> +{
> + struct each_device_arg arg;
> + u8 tmp;
> +
> + arg.match = device_uid;
> + acpi_bus_for_each_dev(nct6775_asuswmi_device_match, &arg);
> + if (!arg.adev)
> + return access_direct;
> +
> + asus_acpi_dev = arg.adev;
The use of the static variable made me look into this further. Why
all that complexity with struct each_device_arg and passing the structure
and adev to/from the match function instead of just passing the
match string as parameter and storing the matching adev directly in
asus_acpi_dev instead of passing it back and storing it here ?
Also, the use of a static variable makes me wonder: would asus_acpi_dev
be the same for both chips if there are two Super-IO chips in the system ?
Thanks,
Guenter
> + /* 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;
> + }
> +
> + return access_direct;
> +}
> +
> static int __init sensors_nct6775_platform_init(void)
> {
> int i, err;
> @@ -1109,7 +1156,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 +1168,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);
> }
>
> /*
>
> base-commit: b0587c87abc891e313d63946ff8c9f4939d1ea1a
> --
> 2.39.0
>
Powered by blists - more mailing lists