LinuxLists.cc - [PATCH] platform/x86: add Gigabyte WMI temperature driver

[permalink] [raw]

Subject: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Changes since v1:
* Incorporate feedback from Barnabás Pőcze
* Use a WMI driver instead of a platform driver
* Let the kernel manage the driver lifecycle
* Fix errno/ACPI error confusion
* Fix resource cleanup
* Document reason for integer casting

Thank you Barnabás for your review, it is much appreciated.

-- >8 --

Tested with a X570 I Aorus Pro Wifi.
The mainboard contains an ITE IT8688E chip for management.
This chips is also handled by drivers/hwmon/i87.c but as it is also used
by the firmware itself it needs an ACPI driver.

Unfortunately not all sensor registers are handled by the firmware and even
less are exposed via WMI.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/platform/x86/Kconfig | 11 +++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++
3 files changed, 150 insertions(+)
create mode 100644 drivers/platform/x86/gigabyte-wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..96622a2106f7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,17 @@ config XIAOMI_WMI
To compile this driver as a module, choose M here: the module will
be called xiaomi-wmi.

+config GIGABYTE_WMI
+ tristate "Gigabyte WMI temperature driver"
+ depends on ACPI_WMI
+ depends on HWMON
+ help
+ Say Y here if you want to support WMI-based temperature reporting on
+ Gigabyte mainboards.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gigabyte-wmi.
+
config ACERHDF
tristate "Acer Aspire One temperature and fan driver"
depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..1621ebfd04fd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
+obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o

# Acer
obj-$(CONFIG_ACERHDF) += acerhdf.o
diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
new file mode 100644
index 000000000000..8618363e3ccf
--- /dev/null
+++ b/drivers/platform/x86/gigabyte-wmi.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
+
+enum gigabyte_wmi_commandtype {
+ GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
+ GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
+ GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
+ GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
+ GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
+};
+
+struct gigabyte_wmi_args {
+ u32 arg1;
+};
+
+static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, struct acpi_buffer *out)
+{
+ const struct acpi_buffer in = {
+ .length = sizeof(*args),
+ .pointer = args,
+ };
+
+ acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
+ if (ret == AE_OK) {
+ return 0;
+ } else {
+ return -EIO;
+ };
+}
+
+static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, u64 *res)
+{
+ union acpi_object *obj;
+ struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+ int ret;
+
+ ret = gigabyte_wmi_perform_query(command, args, &result);
+ if (ret) {
+ goto out;
+ }
+ obj = result.pointer;
+ if (obj && obj->type == ACPI_TYPE_INTEGER) {
+ *res = obj->integer.value;
+ ret = 0;
+ } else {
+ ret = -EIO;
+ }
+out:
+ kfree(result.pointer);
+ return ret;
+}
+
+static int gigabyte_wmi_temperature(u8 sensor, long *res)
+{
+ struct gigabyte_wmi_args args = {
+ .arg1 = sensor,
+ };
+ u64 temp;
+ acpi_status ret;
+
+ ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
+ if (ret == 0)
+ *res = (s8) temp * 1000; // value is a signed 8-bit integer
+ return ret;
+}
+
+static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ return gigabyte_wmi_temperature(channel, val);
+}
+
+static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return 0444;
+}
+
+static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT),
+ NULL,
+};
+
+static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
+ .read = gigabyte_wmi_hwmon_read,
+ .is_visible = gigabyte_wmi_hwmon_is_visible,
+};
+
+static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
+ .ops = &gigabyte_wmi_hwmon_ops,
+ .info = gigabyte_wmi_hwmon_info,
+};
+
+static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
+ "gigabyte_wmi", NULL,
+ &gigabyte_wmi_hwmon_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct wmi_device_id gigabyte_wmi_id_table[] = {
+ { GIGABYTE_WMI_GUID, NULL },
+ { },
+};
+
+static struct wmi_driver gigabyte_wmi_driver = {
+ .driver = {
+ .name = "gigabyte-wmi",
+ },
+ .id_table = gigabyte_wmi_id_table,
+ .probe = gigabyte_wmi_probe,
+};
+module_wmi_driver(gigabyte_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
+MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
+MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");
+MODULE_LICENSE("GPL");

base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
--
2.31.1

2021-04-07 21:28:35

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi Thomas,

Thank you for your new driver and thank you for the quick respin
addressing Barnabás' request to make it a WMI driver.

The code looks good, so merging this should be a no-brainer,
yet I'm not sure if I should merge this driver as-is, let me
explain.

The problem is that I assume that this is based on reverse-engineering?

We have some mixes experiences with reverse-engineered WMI drivers,
sometimes a feature is not supported yet the wmi_evaluate_method()
call happily succeeds. One example of this causing trouble is:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c

At a minimum I think your driver should check in its
probe function that

gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)

actually succeeds on the machine the driver is running on chances
are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
suggests that this is a pretty new API.

It would be good if you can see if you can find some DSDT-s for older
gigabyte motherboards attached to various distro's bug-tracking systems
or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.

Another open question to make sure this driver is suitable
as a generic driver (and does not misbehave) is how to figure out
how many temperature sensors there actually are.

Perhaps the WMI interface returns an error when you query an out-of-range
temperature channel?

One option here might be to add a DMI matching table and only load on
systems on that table for now. That table could then perhaps also provide
labels for each of the temperature channels, which is something which
would be nice to have regardless of my worries about how well this driver
will work on motherboards on which it has not been tested.

You could combine this DMI matching table with a "force" module option to
continue with probing on boards which are not on the table to allow users
to test and report their results to you.

And hopefully after a while, when we're confident that the code works
well on most gigabyte boards we can drop the DMI table, or at least
only use it for the channel labels.

Please don't take this the wrong way; I think it is great that you are
working on this. And the quick turnaround of the v2 of this drivers makes
me pretty certain that we can figure something out and get this merged.

Have you tried contacting Gigabyte about this? I don't think the WMI
interface is something which they need to keep secret for competitive
reasons, so maybe they can help? Note if they want you to sign a NDA
of sorts to view docs, then make sure that it contains some language
about them allowing you to release an opensource driver for their
hardware based on the "protected" information.

Regards,

Hans

On 4/5/21 10:48 PM, Thomas Weißschuh wrote:
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Thank you Barnabás for your review, it is much appreciated.
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 11 +++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++
> 3 files changed, 150 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..8618363e3ccf
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
> + if (ret == AE_OK) {
> + return 0;
> + } else {
> + return -EIO;
> + };
> +}
> +
> +static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(command, args, &result);
> + if (ret) {
> + goto out;
> + }
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> + *res = obj->integer.value;
> + ret = 0;
> + } else {
> + ret = -EIO;
> + }
> +out:
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0)
> + *res = (s8) temp * 1000; // value is a signed 8-bit integer
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + return gigabyte_wmi_temperature(channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL,
> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "gigabyte_wmi", NULL,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { },
> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>

2021-04-07 21:52:21

by Barnabás Pőcze

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi

2021. április 5., hétfő 22:48 keltezéssel, Thomas Weißschuh írta:

> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Thank you Barnabás for your review, it is much appreciated.
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.

I gather this means you're getting the

ACPI Warning: SystemIO range ... conflicts with ...
ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver

warning?

>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 11 +++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++
> 3 files changed, 150 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..8618363e3ccf
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);

Ideally, you'd use the WMI device that was passed to the probe method to do the query
using `wmidev_evaluate_method()`. You can pass the WMI device pointer
to `devm_hwmon_device_register_with_info()` in the `drvdata` argument,
then in the ->read() callback you can retrieve it:

static int gigabyte_wmi_hwmon_read(struct device *dev, ...)
{
struct wmi_device *wdev = dev_get_drvdata(dev);

and then you can pass that to the other functions.

> + if (ret == AE_OK) {
> + return 0;
> + } else {
> + return -EIO;
> + };

The `;` is not needed. And please use `ACPI_FAILURE()` or `ACPI_SUCCESS()`
to check the returned value. For example:

acpi_status ret = ...;
if (ACPI_FAILURE(ret))
return -EIO;

return 0;

> +}
> +
> +static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(command, args, &result);
> + if (ret) {
> + goto out;

I believe if this branch is taken, no buffer is allocated (due to the failure),
so you can just `return ret;` here and do away with the goto completely - if I'm not mistaken.

> + }
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> + *res = obj->integer.value;
> + ret = 0;
> + } else {
> + ret = -EIO;
> + }
> +out:
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0)
> + *res = (s8) temp * 1000; // value is a signed 8-bit integer
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + return gigabyte_wmi_temperature(channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL,
^
Minor thing: usually commas after sentinel values are omitted.

> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "gigabyte_wmi", NULL,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { },
^
Same here.

> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");

It's a very minor thing, but could you please
synchronize this description with the Kconfig?

> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1

Regards,
Barnabás Pőcze

2021-04-07 22:43:09

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi Hans,

On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
> Thank you for your new driver and thank you for the quick respin
> addressing Barnabás' request to make it a WMI driver.
>
> The code looks good, so merging this should be a no-brainer,
> yet I'm not sure if I should merge this driver as-is, let me
> explain.

thanks for the encouraging words.

> The problem is that I assume that this is based on reverse-engineering?

Yes, it is completely reverse-engineered.
Essentially I stumbled upon Matthews comment at
https://bugzilla.kernel.org/show_bug.cgi?id=204807#c37 and went from there.

> We have some mixes experiences with reverse-engineered WMI drivers,
> sometimes a feature is not supported yet the wmi_evaluate_method()
> call happily succeeds. One example of this causing trouble is:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c

There actually are reports of recent, similar mainboards with recent firmware and
similar sensor chips that do not support the temperature query.
(https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/3 and
https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)

Unfortunately for unknown WMI queries the firmware does not return any value.
This ends up as an ACPI integer with value 0 on the driver side.
(Which I could not yet find documentation for if that is expected)
In the current version of the driver EIO is returned for 0 values which
get translated to N/A by lm-sensors.

> At a minimum I think your driver should check in its
> probe function that
>
> gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)
>
> actually succeeds on the machine the driver is running on chances
> are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
> GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
> suggests that this is a pretty new API.

Would it be enough to probe all six sensors and check if all return 0?

> It would be good if you can see if you can find some DSDT-s for older
> gigabyte motherboards attached to various distro's bug-tracking systems
> or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.

Will do.

> Another open question to make sure this driver is suitable
> as a generic driver (and does not misbehave) is how to figure out
> how many temperature sensors there actually are.

So far I could not find out how to query this from the firmware.
The IT8688 chip can report the state of each sensor but that is not exposed by
the firmware.
But even the state information from the IT8688 is not accurate as is.
One of the sensors that is reported as being active (directly via it87) on my
machine always reports -55°C (yes, negative).

> Perhaps the WMI interface returns an error when you query an out-of-range
> temperature channel?

Also "0" as mentioned above.

> One option here might be to add a DMI matching table and only load on
> systems on that table for now. That table could then perhaps also provide
> labels for each of the temperature channels, which is something which
> would be nice to have regardless of my worries about how well this driver
> will work on motherboards on which it has not been tested.

I am collecting reports for working motherboards at
https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/1 .

> You could combine this DMI matching table with a "force" module option to
> continue with probing on boards which are not on the table to allow users
> to test and report their results to you.
>
> And hopefully after a while, when we're confident that the code works
> well on most gigabyte boards we can drop the DMI table, or at least
> only use it for the channel labels.

That sounds good.

> Please don't take this the wrong way; I think it is great that you are
> working on this. And the quick turnaround of the v2 of this drivers makes
> me pretty certain that we can figure something out and get this merged.

Thank you for the feedback!

> Have you tried contacting Gigabyte about this? I don't think the WMI
> interface is something which they need to keep secret for competitive
> reasons, so maybe they can help? Note if they want you to sign a NDA
> of sorts to view docs, then make sure that it contains some language
> about them allowing you to release an opensource driver for their
> hardware based on the "protected" information.

I have not contacted them yet, will do.

As mentioned in the initial patch submission there would be different ways to
access this information firmware:

* Directly call the underlying ACPI methods (these are present in all so far
observed firmwares, even if not exposed via WMI).
* Directly access the ACPI IndexField representing the it87 chip.
* Directly access the it87 registers while holding the relevant locks via ACPI.

I assume all of those mechanisms have no place in a proper kernel driver but
would like to get your opinion on it.

Thomas

2021-04-08 09:38:08

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi,

On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
> Hi Hans,
>
> On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
>> Thank you for your new driver and thank you for the quick respin
>> addressing Barnabás' request to make it a WMI driver.
>>
>> The code looks good, so merging this should be a no-brainer,
>> yet I'm not sure if I should merge this driver as-is, let me
>> explain.
>
> thanks for the encouraging words.
>
>> The problem is that I assume that this is based on reverse-engineering?
>
> Yes, it is completely reverse-engineered.
> Essentially I stumbled upon Matthews comment at
> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c37 and went from there.
>
>> We have some mixes experiences with reverse-engineered WMI drivers,
>> sometimes a feature is not supported yet the wmi_evaluate_method()
>> call happily succeeds. One example of this causing trouble is:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c
>
> There actually are reports of recent, similar mainboards with recent firmware and
> similar sensor chips that do not support the temperature query.
> (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/3 and
> https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
>
> Unfortunately for unknown WMI queries the firmware does not return any value.
> This ends up as an ACPI integer with value 0 on the driver side.
> (Which I could not yet find documentation for if that is expected)
> In the current version of the driver EIO is returned for 0 values which
> get translated to N/A by lm-sensors.
>
>> At a minimum I think your driver should check in its
>> probe function that
>>
>> gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)
>>
>> actually succeeds on the machine the driver is running on chances
>> are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
>> GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
>> suggests that this is a pretty new API.
>
> Would it be enough to probe all six sensors and check if all return 0?

I think that failing the probe with -ENODEV, with a dev_info() explaining why when
all six sensors return 0 would be good yes, that should also fix those 2
issues on https://github.com/t-8ch/linux-gigabyte-wmi-driver/.

>> It would be good if you can see if you can find some DSDT-s for older
>> gigabyte motherboards attached to various distro's bug-tracking systems
>> or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.
>
> Will do.

Since you alreayd have bugreports of boards where this does not work,
please don't spend too much time on this. I guess those older DSDT-s will
also just return an integer with value 0.

>> Another open question to make sure this driver is suitable
>> as a generic driver (and does not misbehave) is how to figure out
>> how many temperature sensors there actually are.
>
> So far I could not find out how to query this from the firmware.
> The IT8688 chip can report the state of each sensor but that is not exposed by
> the firmware.
> But even the state information from the IT8688 is not accurate as is.
> One of the sensors that is reported as being active (directly via it87) on my
> machine always reports -55°C (yes, negative).

Ok.

>> Perhaps the WMI interface returns an error when you query an out-of-range
>> temperature channel?
>
> Also "0" as mentioned above.

Hmm, so maybe this can be used to limit the amount of reported temperature
sensors, IOW if sensors 5 and 6 report 0, only register 4 sensors ?

>
>> One option here might be to add a DMI matching table and only load on
>> systems on that table for now. That table could then perhaps also provide
>> labels for each of the temperature channels, which is something which
>> would be nice to have regardless of my worries about how well this driver
>> will work on motherboards on which it has not been tested.
>
> I am collecting reports for working motherboards at
> https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/1 .

Good, you should probably ask reporters there to provide the output of:

grep . /sys/class/dmi/id/* 2> /dev/null

Ran as a normal user (so that the serial-numbers will be skipped) so that
you will have DMI strings to match on if you decide to go that route.

>
>> You could combine this DMI matching table with a "force" module option to
>> continue with probing on boards which are not on the table to allow users
>> to test and report their results to you.
>>
>> And hopefully after a while, when we're confident that the code works
>> well on most gigabyte boards we can drop the DMI table, or at least
>> only use it for the channel labels.
>
> That sounds good.
>
>> Please don't take this the wrong way; I think it is great that you are
>> working on this. And the quick turnaround of the v2 of this drivers makes
>> me pretty certain that we can figure something out and get this merged.
>
> Thank you for the feedback!
>
>> Have you tried contacting Gigabyte about this? I don't think the WMI
>> interface is something which they need to keep secret for competitive
>> reasons, so maybe they can help? Note if they want you to sign a NDA
>> of sorts to view docs, then make sure that it contains some language
>> about them allowing you to release an opensource driver for their
>> hardware based on the "protected" information.
>
> I have not contacted them yet, will do.
>
> As mentioned in the initial patch submission there would be different ways to
> access this information firmware:
>
> * Directly call the underlying ACPI methods (these are present in all so far
> observed firmwares, even if not exposed via WMI).
> * Directly access the ACPI IndexField representing the it87 chip.
> * Directly access the it87 registers while holding the relevant locks via ACPI.
>
> I assume all of those mechanisms have no place in a proper kernel driver but
> would like to get your opinion on it.

Actually the "Directly access the it87 registers" option is potentially interesting
since it will allow using the it87 driver which gives a lot more features.

I actually wrote a rough outline of how something like this could work here:

https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47

Note I'm not sure if that is the right approach, but it definitely is an
option. It seems that this one might also solve the X470-AORUS-ULTRA-GAMING
case (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)

Hopefully the direct-register ACPI/WMI access methods will also allow
reading the super-io vendor and product ids so that we can be reasonably
sure that we are not loading the wrong driver on a board.

OTOH the WMI-temp method approach may also work on boards where the sensors
(or some of the sensors) are not supported.

I'm afraid there is no obviously correct answer here. If you feel like it
experimenting with the "Directly access the it87 registers" option would certainly
be interesting IMHO.

It might be good to get hwmon subsystems maintainer's opinion on this
before sinking a lot of time into this though (added to the Cc).

Jean, Guenter,

Thomas has been working on a WMI driver to expose various motherboard
temperatures on a gigabyte board where the IO-addresses for the it87 chip
are reserved by ACPI. We are discussing how best to deal with this, there
are some ACPI methods to directly access the super-IO registers (with locking
to protect against other ACPI accesses). This reminded me of an idea I had
a while ago to solve a similar issue with an other superIO chip, abstract
the superIO register access-es using some reg_ops struct and allow an ACPI/WMI
driver to provide alternative reg_ops:
https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47

Do you think this is a good idea (or a bad one)? And would something like that
be acceptable to you ?

Regards,

Hans

2021-04-08 14:41:51

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi,

On Mi, 2021-04-07T18:27+0000, Barnabás Pőcze wrote:
> 2021. április 5., hétfő 22:48 keltezéssel, Thomas Weißschuh írta:
> > Tested with a X570 I Aorus Pro Wifi.
> > The mainboard contains an ITE IT8688E chip for management.
> > This chips is also handled by drivers/hwmon/i87.c but as it is also used
> > by the firmware itself it needs an ACPI driver.
>
> I gather this means you're getting the
>
> ACPI Warning: SystemIO range ... conflicts with ...
> ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
>
> warning?

Exactly.

> > +struct gigabyte_wmi_args {
> > + u32 arg1;
> > +};
> > +
> > +static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
> > + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> > +{
> > + const struct acpi_buffer in = {
> > + .length = sizeof(*args),
> > + .pointer = args,
> > + };
> > +
> > + acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
>
> Ideally, you'd use the WMI device that was passed to the probe method to do the query
> using `wmidev_evaluate_method()`. You can pass the WMI device pointer
> to `devm_hwmon_device_register_with_info()` in the `drvdata` argument,
> then in the ->read() callback you can retrieve it:
>
> static int gigabyte_wmi_hwmon_read(struct device *dev, ...)
> {
> struct wmi_device *wdev = dev_get_drvdata(dev);
>
> and then you can pass that to the other functions.

Done.

> > + if (ret == AE_OK) {
> > + return 0;
> > + } else {
> > + return -EIO;
> > + };
>
> The `;` is not needed. And please use `ACPI_FAILURE()` or `ACPI_SUCCESS()`
> to check the returned value. For example:
>
> acpi_status ret = ...;
> if (ACPI_FAILURE(ret))
> return -EIO;
>
> return 0;

Done.

> > +}
> > +
> > +static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
> > + struct gigabyte_wmi_args *args, u64 *res)
> > +{
> > + union acpi_object *obj;
> > + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> > + int ret;
> > +
> > + ret = gigabyte_wmi_perform_query(command, args, &result);
> > + if (ret) {
> > + goto out;
>
> I believe if this branch is taken, no buffer is allocated (due to the failure),
> so you can just `return ret;` here and do away with the goto completely - if I'm not mistaken.

Done.

> > +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> > + HWMON_CHANNEL_INFO(temp,
> > + HWMON_T_INPUT,
> > + HWMON_T_INPUT,
> > + HWMON_T_INPUT,
> > + HWMON_T_INPUT,
> > + HWMON_T_INPUT,
> > + HWMON_T_INPUT),
> > + NULL,
> ^
> Minor thing: usually commas after sentinel values are omitted.

Done.

> > +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> > + { GIGABYTE_WMI_GUID, NULL },
> > + { },
> ^
> Same here.

Done.

>
> > +};
> > +
> > +static struct wmi_driver gigabyte_wmi_driver = {
> > + .driver = {
> > + .name = "gigabyte-wmi",
> > + },
> > + .id_table = gigabyte_wmi_id_table,
> > + .probe = gigabyte_wmi_probe,
> > +};
> > +module_wmi_driver(gigabyte_wmi_driver);
> > +
> > +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> > +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> > +MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");
>
> It's a very minor thing, but could you please
> synchronize this description with the Kconfig?

Of course.

Thanks again for the review!

Thomas

2021-04-08 14:55:57

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi Hans,

On Do, 2021-04-08T11:36+0200, Hans de Goede wrote:
> On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
> > Hi Hans,
> >
> > On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
> >> Thank you for your new driver and thank you for the quick respin
> >> addressing Barnabás' request to make it a WMI driver.
> >>
> >> The code looks good, so merging this should be a no-brainer,
> >> yet I'm not sure if I should merge this driver as-is, let me
> >> explain.
> >
> > thanks for the encouraging words.
> >
> >> The problem is that I assume that this is based on reverse-engineering?
> >
> > Yes, it is completely reverse-engineered.
> > Essentially I stumbled upon Matthews comment at
> > https://bugzilla.kernel.org/show_bug.cgi?id=204807#c37 and went from there.
> >
> >> We have some mixes experiences with reverse-engineered WMI drivers,
> >> sometimes a feature is not supported yet the wmi_evaluate_method()
> >> call happily succeeds. One example of this causing trouble is:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c
> >
> > There actually are reports of recent, similar mainboards with recent firmware and
> > similar sensor chips that do not support the temperature query.
> > (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/3 and
> > https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
> >
> > Unfortunately for unknown WMI queries the firmware does not return any value.
> > This ends up as an ACPI integer with value 0 on the driver side.
> > (Which I could not yet find documentation for if that is expected)
> > In the current version of the driver EIO is returned for 0 values which
> > get translated to N/A by lm-sensors.
> >
> >> At a minimum I think your driver should check in its
> >> probe function that
> >>
> >> gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)
> >>
> >> actually succeeds on the machine the driver is running on chances
> >> are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
> >> GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
> >> suggests that this is a pretty new API.
> >
> > Would it be enough to probe all six sensors and check if all return 0?
>
> I think that failing the probe with -ENODEV, with a dev_info() explaining why when
> all six sensors return 0 would be good yes, that should also fix those 2
> issues on https://github.com/t-8ch/linux-gigabyte-wmi-driver/.

I added such a validation step.

> >> It would be good if you can see if you can find some DSDT-s for older
> >> gigabyte motherboards attached to various distro's bug-tracking systems
> >> or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.
> >
> > Will do.
>
> Since you alreayd have bugreports of boards where this does not work,
> please don't spend too much time on this. I guess those older DSDT-s will
> also just return an integer with value 0.

Ok.

> >> Another open question to make sure this driver is suitable
> >> as a generic driver (and does not misbehave) is how to figure out
> >> how many temperature sensors there actually are.
> >
> > So far I could not find out how to query this from the firmware.
> > The IT8688 chip can report the state of each sensor but that is not exposed by
> > the firmware.
> > But even the state information from the IT8688 is not accurate as is.
> > One of the sensors that is reported as being active (directly via it87) on my
> > machine always reports -55°C (yes, negative).
>
> Ok.
>
> >> Perhaps the WMI interface returns an error when you query an out-of-range
> >> temperature channel?
> >
> > Also "0" as mentioned above.
>
> Hmm, so maybe this can be used to limit the amount of reported temperature
> sensors, IOW if sensors 5 and 6 report 0, only register 4 sensors ?

So far the 0-returning sensors have not been at the end of the list but in the
middle. Is it worth building logic to properly probe a bitmask of useful
sensors?

> >> One option here might be to add a DMI matching table and only load on
> >> systems on that table for now. That table could then perhaps also provide
> >> labels for each of the temperature channels, which is something which
> >> would be nice to have regardless of my worries about how well this driver
> >> will work on motherboards on which it has not been tested.
> >
> > I am collecting reports for working motherboards at
> > https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/1 .
>
> Good, you should probably ask reporters there to provide the output of:
>
> grep . /sys/class/dmi/id/* 2> /dev/null

I added a DMI-based whitelist and asked users to submit their DMI information.

> Ran as a normal user (so that the serial-numbers will be skipped) so that
> you will have DMI strings to match on if you decide to go that route.

The serials seem not to be too critical on these boards:

/sys/class/dmi/id/board_serial:Default string
/sys/class/dmi/id/chassis_serial:Default string
/sys/class/dmi/id/product_serial:Default string

> > As mentioned in the initial patch submission there would be different ways to
> > access this information firmware:
> >
> > * Directly call the underlying ACPI methods (these are present in all so far
> > observed firmwares, even if not exposed via WMI).
> > * Directly access the ACPI IndexField representing the it87 chip.
> > * Directly access the it87 registers while holding the relevant locks via ACPI.
> >
> > I assume all of those mechanisms have no place in a proper kernel driver but
> > would like to get your opinion on it.
>
> Actually the "Directly access the it87 registers" option is potentially interesting
> since it will allow using the it87 driver which gives a lot more features.
>
> I actually wrote a rough outline of how something like this could work here:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47

I must have overread this one, but yes that's also what I had in mind.

> Note I'm not sure if that is the right approach, but it definitely is an
> option. It seems that this one might also solve the X470-AORUS-ULTRA-GAMING
> case (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
>
> Hopefully the direct-register ACPI/WMI access methods will also allow
> reading the super-io vendor and product ids so that we can be reasonably
> sure that we are not loading the wrong driver on a board.
>
> OTOH the WMI-temp method approach may also work on boards where the sensors
> (or some of the sensors) are not supported.
>
> I'm afraid there is no obviously correct answer here. If you feel like it
> experimenting with the "Directly access the it87 registers" option would certainly
> be interesting IMHO.
>
> It might be good to get hwmon subsystems maintainer's opinion on this
> before sinking a lot of time into this though (added to the Cc).

Sounds good.

2021-04-08 15:02:38

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

On 4/8/21 2:36 AM, Hans de Goede wrote:
> Hi,
>
> On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
>> Hi Hans,
>>
>> On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
>>> Thank you for your new driver and thank you for the quick respin
>>> addressing Barnabás' request to make it a WMI driver.
>>>
>>> The code looks good, so merging this should be a no-brainer,
>>> yet I'm not sure if I should merge this driver as-is, let me
>>> explain.
>>
>> thanks for the encouraging words.
>>
>>> The problem is that I assume that this is based on reverse-engineering?
>>
>> Yes, it is completely reverse-engineered.
>> Essentially I stumbled upon Matthews comment at
>> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c37 and went from there.
>>
>>> We have some mixes experiences with reverse-engineered WMI drivers,
>>> sometimes a feature is not supported yet the wmi_evaluate_method()
>>> call happily succeeds. One example of this causing trouble is:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1797d588af15174d4a4e7159dac8c800538e4f8c
>>
>> There actually are reports of recent, similar mainboards with recent firmware and
>> similar sensor chips that do not support the temperature query.
>> (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/3 and
>> https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
>>
>> Unfortunately for unknown WMI queries the firmware does not return any value.
>> This ends up as an ACPI integer with value 0 on the driver side.
>> (Which I could not yet find documentation for if that is expected)
>> In the current version of the driver EIO is returned for 0 values which
>> get translated to N/A by lm-sensors.
>>
>>> At a minimum I think your driver should check in its
>>> probe function that
>>>
>>> gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, ...)
>>>
>>> actually succeeds on the machine the driver is running on chances
>>> are that Gigabyte has been using the DEADBEEF-2001-0000-00A0-C90629100000
>>> GUID for ages, where as the 0x125 value for GIGABYTE_WMI_TEMPERATURE_QUERY
>>> suggests that this is a pretty new API.
>>
>> Would it be enough to probe all six sensors and check if all return 0?
>
> I think that failing the probe with -ENODEV, with a dev_info() explaining why when
> all six sensors return 0 would be good yes, that should also fix those 2
> issues on https://github.com/t-8ch/linux-gigabyte-wmi-driver/.
>
>>> It would be good if you can see if you can find some DSDT-s for older
>>> gigabyte motherboards attached to various distro's bug-tracking systems
>>> or forum posts and see how those respond to an unknown gigabyte_wmi_commandtype.
>>
>> Will do.
>
> Since you alreayd have bugreports of boards where this does not work,
> please don't spend too much time on this. I guess those older DSDT-s will
> also just return an integer with value 0.
>
>>> Another open question to make sure this driver is suitable
>>> as a generic driver (and does not misbehave) is how to figure out
>>> how many temperature sensors there actually are.
>>
>> So far I could not find out how to query this from the firmware.
>> The IT8688 chip can report the state of each sensor but that is not exposed by
>> the firmware.
>> But even the state information from the IT8688 is not accurate as is.
>> One of the sensors that is reported as being active (directly via it87) on my
>> machine always reports -55°C (yes, negative).
>
> Ok.
>
>>> Perhaps the WMI interface returns an error when you query an out-of-range
>>> temperature channel?
>>
>> Also "0" as mentioned above.
>
> Hmm, so maybe this can be used to limit the amount of reported temperature
> sensors, IOW if sensors 5 and 6 report 0, only register 4 sensors ?
>
>>
>>> One option here might be to add a DMI matching table and only load on
>>> systems on that table for now. That table could then perhaps also provide
>>> labels for each of the temperature channels, which is something which
>>> would be nice to have regardless of my worries about how well this driver
>>> will work on motherboards on which it has not been tested.
>>
>> I am collecting reports for working motherboards at
>> https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/1 .
>
> Good, you should probably ask reporters there to provide the output of:
>
> grep . /sys/class/dmi/id/* 2> /dev/null
>
> Ran as a normal user (so that the serial-numbers will be skipped) so that
> you will have DMI strings to match on if you decide to go that route.
>
>>
>>> You could combine this DMI matching table with a "force" module option to
>>> continue with probing on boards which are not on the table to allow users
>>> to test and report their results to you.
>>>
>>> And hopefully after a while, when we're confident that the code works
>>> well on most gigabyte boards we can drop the DMI table, or at least
>>> only use it for the channel labels.
>>
>> That sounds good.
>>
>>> Please don't take this the wrong way; I think it is great that you are
>>> working on this. And the quick turnaround of the v2 of this drivers makes
>>> me pretty certain that we can figure something out and get this merged.
>>
>> Thank you for the feedback!
>>
>>> Have you tried contacting Gigabyte about this? I don't think the WMI
>>> interface is something which they need to keep secret for competitive
>>> reasons, so maybe they can help? Note if they want you to sign a NDA
>>> of sorts to view docs, then make sure that it contains some language
>>> about them allowing you to release an opensource driver for their
>>> hardware based on the "protected" information.
>>
>> I have not contacted them yet, will do.
>>
>> As mentioned in the initial patch submission there would be different ways to
>> access this information firmware:
>>
>> * Directly call the underlying ACPI methods (these are present in all so far
>> observed firmwares, even if not exposed via WMI).
>> * Directly access the ACPI IndexField representing the it87 chip.
>> * Directly access the it87 registers while holding the relevant locks via ACPI.
>>
>> I assume all of those mechanisms have no place in a proper kernel driver but
>> would like to get your opinion on it.
>
> Actually the "Directly access the it87 registers" option is potentially interesting
> since it will allow using the it87 driver which gives a lot more features.
>
> I actually wrote a rough outline of how something like this could work here:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
>
> Note I'm not sure if that is the right approach, but it definitely is an
> option. It seems that this one might also solve the X470-AORUS-ULTRA-GAMING
> case (https://github.com/t-8ch/linux-gigabyte-wmi-driver/issues/2)
>
> Hopefully the direct-register ACPI/WMI access methods will also allow
> reading the super-io vendor and product ids so that we can be reasonably
> sure that we are not loading the wrong driver on a board.
>
> OTOH the WMI-temp method approach may also work on boards where the sensors
> (or some of the sensors) are not supported.
>
> I'm afraid there is no obviously correct answer here. If you feel like it
> experimenting with the "Directly access the it87 registers" option would certainly
> be interesting IMHO.
>
> It might be good to get hwmon subsystems maintainer's opinion on this
> before sinking a lot of time into this though (added to the Cc).
>
> Jean, Guenter,
>
> Thomas has been working on a WMI driver to expose various motherboard
> temperatures on a gigabyte board where the IO-addresses for the it87 chip
> are reserved by ACPI. We are discussing how best to deal with this, there
> are some ACPI methods to directly access the super-IO registers (with locking
> to protect against other ACPI accesses). This reminded me of an idea I had
> a while ago to solve a similar issue with an other superIO chip, abstract
> the superIO register access-es using some reg_ops struct and allow an ACPI/WMI
> driver to provide alternative reg_ops:
> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
>
> Do you think this is a good idea (or a bad one)? And would something like that
> be acceptable to you ?
>

The upstream it87 driver is severely out of date. I had an out-of-tree driver
with various improvements which I didn't upstream, first because no one was willing
to review changes and then because it had deviated too much. I pulled it from
public view because I got pounded for not upstreaming it, because people started
demanding support (not asking, demanding) for it, and because Gigabyte stopped
providing datasheets for the more recent ITE chips and it became effectively
unmaintainable.

Some ITE chips have issues which can cause system hangs if accessed directly.
I put some work to remedy that into the non-upstream driver, but that was all
just guesswork. Gigabyte knows about the problem (or so I was told from someone
who has an NDA with them), but I didn't get them or ITE to even acknowledge it
to me. I even had a support case open with Gigabyte for a while, but all I could
get out of them is that they don't support Linux and what I would have to reproduce
the problem with Windows for them to provide assistance (even though, again,
they knew about it).

As for using ACPI locks or WMI to ensure that ACPI leaves the chip alone while
the driver accesses chips directly: That is an option, but it has (at least)
two problems.

First, ACPI access methods are not well documented or standardized. I had tried
to find useful means to do that some time ago, but I gave up because each board
(even from the same vendor) handles locking and accesses differently. We would
end up with lots of board specific code. Coincidentally, that was for ASUS boards
and the nct6775 driver.

Second, access through ACPI is only one of the issues. Turns out there are two
ITE chips on many of the Gigabyte boards nowadays, and the two chips talk to each
other using I2C. My out-of-tree driver tried to remedy that by blocking those
accesses while the driver used the chip, but, again, without Gigabyte / ITE
support this was never a perfect solution, and there was always the risk that
the board ended up hanging because that access was blocked for too long.
Recent ITE chips solve that problem by providing memory mapped access to the
chip registers, but that is only useful if one has a datasheet.

Overall, I don't think it makes much sense trying to make significant changes
to the it87 driver without pulling in all the changes I had made, and without
finding a better fix for the cross-chip access problems. I for sure won't have
time for that (and getting hwmon patches reviewed is still very much an issue).

Having said that, I am of course open to adding WMI/ACPI drivers for the various
boards. Good luck getting support from Gigabyte, though. Or from ASUS, for that
matter.

Guenter

2021-04-08 15:11:01

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

On Mon, Apr 05, 2021 at 10:48:10PM +0200, Thomas Weißschuh wrote:
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Thank you Barnabás for your review, it is much appreciated.
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 11 +++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++

Originally drivers/platform was supposed to be used for platform specific
code. Not that I have control over it, but I really dislike that more and
more hwmon drivers end up there.

At least hwmon is in good company - I see drivers for various other subsystems
there as well. I just wonder if that is such a good idea. That entire directory
is bypassing subsystem maintainer reviews.

Guenter

> 3 files changed, 150 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..8618363e3ccf
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
> + if (ret == AE_OK) {
> + return 0;
> + } else {
> + return -EIO;
> + };
> +}
> +
> +static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(command, args, &result);
> + if (ret) {
> + goto out;
> + }
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> + *res = obj->integer.value;
> + ret = 0;
> + } else {
> + ret = -EIO;
> + }
> +out:
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0)
> + *res = (s8) temp * 1000; // value is a signed 8-bit integer
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + return gigabyte_wmi_temperature(channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL,
> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "gigabyte_wmi", NULL,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { },
> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1
>

2021-04-08 16:11:19

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi Guenter,

On 4/8/21 5:08 PM, Guenter Roeck wrote:
> On Mon, Apr 05, 2021 at 10:48:10PM +0200, Thomas Weißschuh wrote:
>> Changes since v1:
>> * Incorporate feedback from Barnabás Pőcze
>> * Use a WMI driver instead of a platform driver
>> * Let the kernel manage the driver lifecycle
>> * Fix errno/ACPI error confusion
>> * Fix resource cleanup
>> * Document reason for integer casting
>>
>> Thank you Barnabás for your review, it is much appreciated.
>>
>> -- >8 --
>>
>> Tested with a X570 I Aorus Pro Wifi.
>> The mainboard contains an ITE IT8688E chip for management.
>> This chips is also handled by drivers/hwmon/i87.c but as it is also used
>> by the firmware itself it needs an ACPI driver.
>>
>> Unfortunately not all sensor registers are handled by the firmware and even
>> less are exposed via WMI.
>>
>> Signed-off-by: Thomas Weißschuh <[email protected]>
>> ---
>> drivers/platform/x86/Kconfig | 11 +++
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++
>
> Originally drivers/platform was supposed to be used for platform specific
> code. Not that I have control over it, but I really dislike that more and
> more hwmon drivers end up there.
>
> At least hwmon is in good company - I see drivers for various other subsystems
> there as well. I just wonder if that is such a good idea. That entire directory
> is bypassing subsystem maintainer reviews.

In case you are not aware I've recent(ish) taken over the drivers/platform/x86
maintainership from Andy Shevchenko.

Yes it is a bit of an odd grab-bag it mostly deals with vendor specific
ACPI / WMI interfaces which often more or less require using a single
driver while offering multiple functionalities. These firmware interfaces
do not really lend themselves to demultiplexing through something like
MFD. These are mostly found on laptops where they deal with some or all of:

- Hotkeys for brightness adjust / wlan-on/off toggle, touchpad on/off toggle, etc.
(input subsystem stuff)
- Mic. / Speaker mute LEDS (and other special LEDs) found on some laptops
(LED subsystem stuff)
- Enabling/disabling radios
(rfkill stuff)
- Controlling the DPTF performance profile
(ACPI stuff)
- Various sensors, some hwmon, some IIO
- Backlight control (drm/kms subsys)
- Enabling/disabling of LCD-builtin privacy filters (requires KMS/DRM subsys integration, pending)
- Fan control (hwmon subsys)

And often all of this in a single driver. This is all "stuff" for which
there are no standard APIs shared between vendors, so it is a free for
all and often it is all stuffed behind a single WMI or ACPI object,
because that is how the vendor's drivers under Windows work.

It certainly is not my intention to bypass review by other subsystem
maintainers and when there are significant questions I do try to always
get other subsys maintainers involved. See e.g. this thread, but also the
"[PATCH 1/3] thinkpad_acpi: add support for force_discharge" thread
where I asked for input from sre for the power-supply aspects of that.

The WMI code was reworked a while back to make WMI be a bus and have
individual WMI objects be devices on that bus. version 2 of this
driver has been reworked to use this. Since this new driver is just a hwmon
driver and as this is for a desktop I expect it will stay that way,
I'm fine with moving this one over to drivers/hwmon if that has your
preference.

As for other cases then this driver, if you want to make sure you are at
least Cc-ed on all hwmon related changes I'm fine with adding you as a
reviewer to the pdx86 MAINTAINERS entry.

Regards,

Hans

>
> Guenter
>
>> 3 files changed, 150 insertions(+)
>> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index ad4e630e73e2..96622a2106f7 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -123,6 +123,17 @@ config XIAOMI_WMI
>> To compile this driver as a module, choose M here: the module will
>> be called xiaomi-wmi.
>>
>> +config GIGABYTE_WMI
>> + tristate "Gigabyte WMI temperature driver"
>> + depends on ACPI_WMI
>> + depends on HWMON
>> + help
>> + Say Y here if you want to support WMI-based temperature reporting on
>> + Gigabyte mainboards.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called gigabyte-wmi.
>> +
>> config ACERHDF
>> tristate "Acer Aspire One temperature and fan driver"
>> depends on ACPI && THERMAL
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 60d554073749..1621ebfd04fd 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
>> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
>> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
>> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
>> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>>
>> # Acer
>> obj-$(CONFIG_ACERHDF) += acerhdf.o
>> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
>> new file mode 100644
>> index 000000000000..8618363e3ccf
>> --- /dev/null
>> +++ b/drivers/platform/x86/gigabyte-wmi.c
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
>> + */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
>> +
>> +enum gigabyte_wmi_commandtype {
>> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
>> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
>> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
>> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
>> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
>> +};
>> +
>> +struct gigabyte_wmi_args {
>> + u32 arg1;
>> +};
>> +
>> +static int gigabyte_wmi_perform_query(enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
>> +{
>> + const struct acpi_buffer in = {
>> + .length = sizeof(*args),
>> + .pointer = args,
>> + };
>> +
>> + acpi_status ret = wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
>> + if (ret == AE_OK) {
>> + return 0;
>> + } else {
>> + return -EIO;
>> + };
>> +}
>> +
>> +static int gigabyte_wmi_query_integer(enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, u64 *res)
>> +{
>> + union acpi_object *obj;
>> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
>> + int ret;
>> +
>> + ret = gigabyte_wmi_perform_query(command, args, &result);
>> + if (ret) {
>> + goto out;
>> + }
>> + obj = result.pointer;
>> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
>> + *res = obj->integer.value;
>> + ret = 0;
>> + } else {
>> + ret = -EIO;
>> + }
>> +out:
>> + kfree(result.pointer);
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_temperature(u8 sensor, long *res)
>> +{
>> + struct gigabyte_wmi_args args = {
>> + .arg1 = sensor,
>> + };
>> + u64 temp;
>> + acpi_status ret;
>> +
>> + ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
>> + if (ret == 0)
>> + *res = (s8) temp * 1000; // value is a signed 8-bit integer
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + return gigabyte_wmi_temperature(channel, val);
>> +}
>> +
>> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + return 0444;
>> +}
>> +
>> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT),
>> + NULL,
>> +};
>> +
>> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
>> + .read = gigabyte_wmi_hwmon_read,
>> + .is_visible = gigabyte_wmi_hwmon_is_visible,
>> +};
>> +
>> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
>> + .ops = &gigabyte_wmi_hwmon_ops,
>> + .info = gigabyte_wmi_hwmon_info,
>> +};
>> +
>> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> + struct device *hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
>> + "gigabyte_wmi", NULL,
>> + &gigabyte_wmi_hwmon_chip_info, NULL);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
>> + { GIGABYTE_WMI_GUID, NULL },
>> + { },
>> +};
>> +
>> +static struct wmi_driver gigabyte_wmi_driver = {
>> + .driver = {
>> + .name = "gigabyte-wmi",
>> + },
>> + .id_table = gigabyte_wmi_id_table,
>> + .probe = gigabyte_wmi_probe,
>> +};
>> +module_wmi_driver(gigabyte_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
>> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
>> +MODULE_DESCRIPTION("Gigabyte Temperature WMI Driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>> --
>> 2.31.1
>>
>

2021-04-09 06:05:06

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

On Do, 2021-04-08T08:00-0700, Guenter Roeck wrote:
> On 4/8/21 2:36 AM, Hans de Goede wrote:
> > On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
> >> On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
> > Jean, Guenter,
> >
> > Thomas has been working on a WMI driver to expose various motherboard
> > temperatures on a gigabyte board where the IO-addresses for the it87 chip
> > are reserved by ACPI. We are discussing how best to deal with this, there
> > are some ACPI methods to directly access the super-IO registers (with locking
> > to protect against other ACPI accesses). This reminded me of an idea I had
> > a while ago to solve a similar issue with an other superIO chip, abstract
> > the superIO register access-es using some reg_ops struct and allow an ACPI/WMI
> > driver to provide alternative reg_ops:
> > https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
> >
> > Do you think this is a good idea (or a bad one)? And would something like that
> > be acceptable to you ?
> >
>
> The upstream it87 driver is severely out of date. I had an out-of-tree driver
> with various improvements which I didn't upstream, first because no one was willing
> to review changes and then because it had deviated too much. I pulled it from
> public view because I got pounded for not upstreaming it, because people started
> demanding support (not asking, demanding) for it, and because Gigabyte stopped
> providing datasheets for the more recent ITE chips and it became effectively
> unmaintainable.
>
> Some ITE chips have issues which can cause system hangs if accessed directly.
> I put some work to remedy that into the non-upstream driver, but that was all
> just guesswork. Gigabyte knows about the problem (or so I was told from someone
> who has an NDA with them), but I didn't get them or ITE to even acknowledge it
> to me. I even had a support case open with Gigabyte for a while, but all I could
> get out of them is that they don't support Linux and what I would have to reproduce
> the problem with Windows for them to provide assistance (even though, again,
> they knew about it).
>
> As for using ACPI locks or WMI to ensure that ACPI leaves the chip alone while
> the driver accesses chips directly: That is an option, but it has (at least)
> two problems.
>
> First, ACPI access methods are not well documented or standardized. I had tried
> to find useful means to do that some time ago, but I gave up because each board
> (even from the same vendor) handles locking and accesses differently. We would
> end up with lots of board specific code. Coincidentally, that was for ASUS boards
> and the nct6775 driver.

At least for all the Gigabyte ACPI tables I have looked at all access is done
via two-byte "OperationRegion" over the Index/Data addresses, a "Field" with
two entries for these and an "IndexField" that is actually used to perform the
accesses.
As the IndexField is synchronized via "Lock" it should take a lock on the
OperationRegion itself.

So I think we should be technically fine with validating these assumption and
then also taking locks on the OperationRegion.

If it is reasonable to do so is another question.

> Second, access through ACPI is only one of the issues. Turns out there are two
> ITE chips on many of the Gigabyte boards nowadays, and the two chips talk to each
> other using I2C. My out-of-tree driver tried to remedy that by blocking those
> accesses while the driver used the chip, but, again, without Gigabyte / ITE
> support this was never a perfect solution, and there was always the risk that
> the board ended up hanging because that access was blocked for too long.
> Recent ITE chips solve that problem by providing memory mapped access to the
> chip registers, but that is only useful if one has a datasheet.

Are both of these chips available at the two well-known registers 0x2e and
0x4e?

Would this too-long blocking also occur when only accessing single registers
for read-only access?
Any write access would probably have to be blocked anyways.

> Overall, I don't think it makes much sense trying to make significant changes
> to the it87 driver without pulling in all the changes I had made, and without
> finding a better fix for the cross-chip access problems. I for sure won't have
> time for that (and getting hwmon patches reviewed is still very much an issue).
>
> Having said that, I am of course open to adding WMI/ACPI drivers for the various
> boards. Good luck getting support from Gigabyte, though. Or from ASUS, for that
> matter.

Thomas

2021-04-10 06:41:16

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

On 4/8/21 9:07 AM, Hans de Goede wrote:
> Hi Guenter,
>
> On 4/8/21 5:08 PM, Guenter Roeck wrote:
>> On Mon, Apr 05, 2021 at 10:48:10PM +0200, Thomas Weißschuh wrote:
>>> Changes since v1:
>>> * Incorporate feedback from Barnabás Pőcze
>>> * Use a WMI driver instead of a platform driver
>>> * Let the kernel manage the driver lifecycle
>>> * Fix errno/ACPI error confusion
>>> * Fix resource cleanup
>>> * Document reason for integer casting
>>>
>>> Thank you Barnabás for your review, it is much appreciated.
>>>
>>> -- >8 --
>>>
>>> Tested with a X570 I Aorus Pro Wifi.
>>> The mainboard contains an ITE IT8688E chip for management.
>>> This chips is also handled by drivers/hwmon/i87.c but as it is also used
>>> by the firmware itself it needs an ACPI driver.
>>>
>>> Unfortunately not all sensor registers are handled by the firmware and even
>>> less are exposed via WMI.
>>>
>>> Signed-off-by: Thomas Weißschuh <[email protected]>
>>> ---
>>> drivers/platform/x86/Kconfig | 11 +++
>>> drivers/platform/x86/Makefile | 1 +
>>> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++
>>
>> Originally drivers/platform was supposed to be used for platform specific
>> code. Not that I have control over it, but I really dislike that more and
>> more hwmon drivers end up there.
>>
>> At least hwmon is in good company - I see drivers for various other subsystems
>> there as well. I just wonder if that is such a good idea. That entire directory
>> is bypassing subsystem maintainer reviews.
>
> In case you are not aware I've recent(ish) taken over the drivers/platform/x86
> maintainership from Andy Shevchenko.
>
> Yes it is a bit of an odd grab-bag it mostly deals with vendor specific
> ACPI / WMI interfaces which often more or less require using a single
> driver while offering multiple functionalities. These firmware interfaces
> do not really lend themselves to demultiplexing through something like
> MFD. These are mostly found on laptops where they deal with some or all of:
>
> - Hotkeys for brightness adjust / wlan-on/off toggle, touchpad on/off toggle, etc.
> (input subsystem stuff)
> - Mic. / Speaker mute LEDS (and other special LEDs) found on some laptops
> (LED subsystem stuff)
> - Enabling/disabling radios
> (rfkill stuff)
> - Controlling the DPTF performance profile
> (ACPI stuff)
> - Various sensors, some hwmon, some IIO
> - Backlight control (drm/kms subsys)
> - Enabling/disabling of LCD-builtin privacy filters (requires KMS/DRM subsys integration, pending)
> - Fan control (hwmon subsys)
>
> And often all of this in a single driver. This is all "stuff" for which
> there are no standard APIs shared between vendors, so it is a free for
> all and often it is all stuffed behind a single WMI or ACPI object,
> because that is how the vendor's drivers under Windows work.
>
> It certainly is not my intention to bypass review by other subsystem
> maintainers and when there are significant questions I do try to always
> get other subsys maintainers involved. See e.g. this thread, but also the
> "[PATCH 1/3] thinkpad_acpi: add support for force_discharge" thread
> where I asked for input from sre for the power-supply aspects of that.
>
> The WMI code was reworked a while back to make WMI be a bus and have
> individual WMI objects be devices on that bus. version 2 of this
> driver has been reworked to use this. Since this new driver is just a hwmon
> driver and as this is for a desktop I expect it will stay that way,
> I'm fine with moving this one over to drivers/hwmon if that has your
> preference.
>
I thought about it, but I don't think it makes much sense since all other
WMI drivers are in drivers/platform.

> As for other cases then this driver, if you want to make sure you are at
> least Cc-ed on all hwmon related changes I'm fine with adding you as a
> reviewer to the pdx86 MAINTAINERS entry.
>
I think I have a better idea: I'll add a regex pattern into the MAINTAINERS
entry for hardware monitoring drivers. This way we should get copied whenever
anyone adds a hardware monitoring driver into the tree.

Thanks,
Guenter

2021-04-10 06:57:19

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

On 4/8/21 11:02 PM, Thomas Weißschuh wrote:
> On Do, 2021-04-08T08:00-0700, Guenter Roeck wrote:
>> On 4/8/21 2:36 AM, Hans de Goede wrote:
>>> On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
>>>> On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
>>> Jean, Guenter,
>>>
>>> Thomas has been working on a WMI driver to expose various motherboard
>>> temperatures on a gigabyte board where the IO-addresses for the it87 chip
>>> are reserved by ACPI. We are discussing how best to deal with this, there
>>> are some ACPI methods to directly access the super-IO registers (with locking
>>> to protect against other ACPI accesses). This reminded me of an idea I had
>>> a while ago to solve a similar issue with an other superIO chip, abstract
>>> the superIO register access-es using some reg_ops struct and allow an ACPI/WMI
>>> driver to provide alternative reg_ops:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
>>>
>>> Do you think this is a good idea (or a bad one)? And would something like that
>>> be acceptable to you ?
>>>
>>
>> The upstream it87 driver is severely out of date. I had an out-of-tree driver
>> with various improvements which I didn't upstream, first because no one was willing
>> to review changes and then because it had deviated too much. I pulled it from
>> public view because I got pounded for not upstreaming it, because people started
>> demanding support (not asking, demanding) for it, and because Gigabyte stopped
>> providing datasheets for the more recent ITE chips and it became effectively
>> unmaintainable.
>>
>> Some ITE chips have issues which can cause system hangs if accessed directly.
>> I put some work to remedy that into the non-upstream driver, but that was all
>> just guesswork. Gigabyte knows about the problem (or so I was told from someone
>> who has an NDA with them), but I didn't get them or ITE to even acknowledge it
>> to me. I even had a support case open with Gigabyte for a while, but all I could
>> get out of them is that they don't support Linux and what I would have to reproduce
>> the problem with Windows for them to provide assistance (even though, again,
>> they knew about it).
>>
>> As for using ACPI locks or WMI to ensure that ACPI leaves the chip alone while
>> the driver accesses chips directly: That is an option, but it has (at least)
>> two problems.
>>
>> First, ACPI access methods are not well documented or standardized. I had tried
>> to find useful means to do that some time ago, but I gave up because each board
>> (even from the same vendor) handles locking and accesses differently. We would
>> end up with lots of board specific code. Coincidentally, that was for ASUS boards
>> and the nct6775 driver.
>
> At least for all the Gigabyte ACPI tables I have looked at all access is done
> via two-byte "OperationRegion" over the Index/Data addresses, a "Field" with
> two entries for these and an "IndexField" that is actually used to perform the
> accesses.
> As the IndexField is synchronized via "Lock" it should take a lock on the
> OperationRegion itself.
>
> So I think we should be technically fine with validating these assumption and
> then also taking locks on the OperationRegion.
>
> If it is reasonable to do so is another question.
>
You'd still have to validate this for each individual board unless you get
confirmation from Gigabyte that the mechanism is consistent on their boards.
Then you'd have to handle other vendors using it87 chips, and those are
just as close-lipped as Gigabyte. Ultimately it would require acpi match
tables to match the various boards and access methods. I had experimented
with this this a long time ago but gave up on it after concluding that it was
unmaintainable.

>> Second, access through ACPI is only one of the issues. Turns out there are two
>> ITE chips on many of the Gigabyte boards nowadays, and the two chips talk to each
>> other using I2C. My out-of-tree driver tried to remedy that by blocking those
>> accesses while the driver used the chip, but, again, without Gigabyte / ITE
>> support this was never a perfect solution, and there was always the risk that
>> the board ended up hanging because that access was blocked for too long.
>> Recent ITE chips solve that problem by providing memory mapped access to the
>> chip registers, but that is only useful if one has a datasheet.
>
> Are both of these chips available at the two well-known registers 0x2e and
> 0x4e?
>

The ones I know of are, yes.

Oh, that reminds me, there is another bug. Here are my comments about that:

/*
* On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
* (IT8792E) needs to be in configuration mode before accessing the first
* due to a bug in IT8792E which otherwise results in LPC bus access errors.
* This needs to be done before accessing the first Super-IO chip since
* the second chip may have been accessed prior to loading this driver.
*
* The problem is also reported to affect IT8795E, which is used on X299 boards
* and has the same chip ID as IT8792E (0x8733). It also appears to affect
* systems with IT8790E, which is used on some Z97X-Gaming boards as well as
* Z87X-OC.
* DMI entries for those systems will be added as they become available and
* as the problem is confirmed to affect those boards.
*/

> Would this too-long blocking also occur when only accessing single registers
> for read-only access?

I don't know. Remember, zero support from Gigabyte / ITE.

Guenter

2021-04-10 14:24:31

[permalink] [raw]

Subject: Re: [PATCH v2] platform/x86: add Gigabyte WMI temperature driver

Hi,

On 4/10/21 8:56 AM, Guenter Roeck wrote:
> On 4/8/21 11:02 PM, Thomas Weißschuh wrote:
>> On Do, 2021-04-08T08:00-0700, Guenter Roeck wrote:
>>> On 4/8/21 2:36 AM, Hans de Goede wrote:
>>>> On 4/7/21 9:43 PM, Thomas Weißschuh wrote:
>>>>> On Mi, 2021-04-07T17:54+0200, Hans de Goede wrote:
>>>> Jean, Guenter,
>>>>
>>>> Thomas has been working on a WMI driver to expose various motherboard
>>>> temperatures on a gigabyte board where the IO-addresses for the it87 chip
>>>> are reserved by ACPI. We are discussing how best to deal with this, there
>>>> are some ACPI methods to directly access the super-IO registers (with locking
>>>> to protect against other ACPI accesses). This reminded me of an idea I had
>>>> a while ago to solve a similar issue with an other superIO chip, abstract
>>>> the superIO register access-es using some reg_ops struct and allow an ACPI/WMI
>>>> driver to provide alternative reg_ops:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=204807#c47
>>>>
>>>> Do you think this is a good idea (or a bad one)? And would something like that
>>>> be acceptable to you ?
>>>>
>>>
>>> The upstream it87 driver is severely out of date. I had an out-of-tree driver
>>> with various improvements which I didn't upstream, first because no one was willing
>>> to review changes and then because it had deviated too much. I pulled it from
>>> public view because I got pounded for not upstreaming it, because people started
>>> demanding support (not asking, demanding) for it, and because Gigabyte stopped
>>> providing datasheets for the more recent ITE chips and it became effectively
>>> unmaintainable.
>>>
>>> Some ITE chips have issues which can cause system hangs if accessed directly.
>>> I put some work to remedy that into the non-upstream driver, but that was all
>>> just guesswork. Gigabyte knows about the problem (or so I was told from someone
>>> who has an NDA with them), but I didn't get them or ITE to even acknowledge it
>>> to me. I even had a support case open with Gigabyte for a while, but all I could
>>> get out of them is that they don't support Linux and what I would have to reproduce
>>> the problem with Windows for them to provide assistance (even though, again,
>>> they knew about it).
>>>
>>> As for using ACPI locks or WMI to ensure that ACPI leaves the chip alone while
>>> the driver accesses chips directly: That is an option, but it has (at least)
>>> two problems.
>>>
>>> First, ACPI access methods are not well documented or standardized. I had tried
>>> to find useful means to do that some time ago, but I gave up because each board
>>> (even from the same vendor) handles locking and accesses differently. We would
>>> end up with lots of board specific code. Coincidentally, that was for ASUS boards
>>> and the nct6775 driver.
>>
>> At least for all the Gigabyte ACPI tables I have looked at all access is done
>> via two-byte "OperationRegion" over the Index/Data addresses, a "Field" with
>> two entries for these and an "IndexField" that is actually used to perform the
>> accesses.
>> As the IndexField is synchronized via "Lock" it should take a lock on the
>> OperationRegion itself.
>>
>> So I think we should be technically fine with validating these assumption and
>> then also taking locks on the OperationRegion.
>>
>> If it is reasonable to do so is another question.
>>
> You'd still have to validate this for each individual board unless you get
> confirmation from Gigabyte that the mechanism is consistent on their boards.
> Then you'd have to handle other vendors using it87 chips, and those are
> just as close-lipped as Gigabyte. Ultimately it would require acpi match
> tables to match the various boards and access methods. I had experimented
> with this this a long time ago but gave up on it after concluding that it was
> unmaintainable.
>
>>> Second, access through ACPI is only one of the issues. Turns out there are two
>>> ITE chips on many of the Gigabyte boards nowadays, and the two chips talk to each
>>> other using I2C. My out-of-tree driver tried to remedy that by blocking those
>>> accesses while the driver used the chip, but, again, without Gigabyte / ITE
>>> support this was never a perfect solution, and there was always the risk that
>>> the board ended up hanging because that access was blocked for too long.
>>> Recent ITE chips solve that problem by providing memory mapped access to the
>>> chip registers, but that is only useful if one has a datasheet.
>>
>> Are both of these chips available at the two well-known registers 0x2e and
>> 0x4e?
>>
>
> The ones I know of are, yes.
>
> Oh, that reminds me, there is another bug. Here are my comments about that:
>
> /*
> * On various Gigabyte AM4 boards (AB350, AX370), the second Super-IO chip
> * (IT8792E) needs to be in configuration mode before accessing the first
> * due to a bug in IT8792E which otherwise results in LPC bus access errors.
> * This needs to be done before accessing the first Super-IO chip since
> * the second chip may have been accessed prior to loading this driver.
> *
> * The problem is also reported to affect IT8795E, which is used on X299 boards
> * and has the same chip ID as IT8792E (0x8733). It also appears to affect
> * systems with IT8790E, which is used on some Z97X-Gaming boards as well as
> * Z87X-OC.
> * DMI entries for those systems will be added as they become available and
> * as the problem is confirmed to affect those boards.
> */
>
>> Would this too-long blocking also occur when only accessing single registers
>> for read-only access?
>
> I don't know. Remember, zero support from Gigabyte / ITE.

So this all sounds like just using the WMI temperature functions as
v2 of this driver does, does sound best overall. Presumably those are also
used by Gigabyte's own Windows tool.

Although even there we have the issue of the interface possibly changing
from board to board. So even there I think we should start with a DMI
based allow-list approach for now; we can revisit this when we have a
better picture of things.

Regards,

Hans

2021-04-10 14:41:29

[permalink] [raw]

Subject: [PATCH v3] platform/x86: add Gigabyte WMI temperature driver

Changes since v1:
* Incorporate feedback from Barnabás Pőcze
* Use a WMI driver instead of a platform driver
* Let the kernel manage the driver lifecycle
* Fix errno/ACPI error confusion
* Fix resource cleanup
* Document reason for integer casting

Changes since v2:
* Style cleanups
* Test for usability during probing
* DMI-based whitelist
* CC hwmon maintainers

-- >8 --

Tested with a X570 I Aorus Pro Wifi.
The mainboard contains an ITE IT8688E chip for management.
This chips is also handled by drivers/hwmon/i87.c but as it is also used
by the firmware itself it needs an ACPI driver.

Unfortunately not all sensor registers are handled by the firmware and even
less are exposed via WMI.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/gigabyte-wmi.c | 194 ++++++++++++++++++++++++++++
4 files changed, 212 insertions(+)
create mode 100644 drivers/platform/x86/gigabyte-wmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..9c10cfc00fe8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
F: fs/gfs2/
F: include/uapi/linux/gfs2_ondisk.h

+GIGABYTE WMI DRIVER
+M: Thomas Weißschuh <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/platform/x86/gigabyte-wmi.c
+
GNSS SUBSYSTEM
M: Johan Hovold <[email protected]>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..96622a2106f7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,17 @@ config XIAOMI_WMI
To compile this driver as a module, choose M here: the module will
be called xiaomi-wmi.

+config GIGABYTE_WMI
+ tristate "Gigabyte WMI temperature driver"
+ depends on ACPI_WMI
+ depends on HWMON
+ help
+ Say Y here if you want to support WMI-based temperature reporting on
+ Gigabyte mainboards.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gigabyte-wmi.
+
config ACERHDF
tristate "Acer Aspire One temperature and fan driver"
depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..1621ebfd04fd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
+obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o

# Acer
obj-$(CONFIG_ACERHDF) += acerhdf.o
diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
new file mode 100644
index 000000000000..fb4e6d4c1823
--- /dev/null
+++ b/drivers/platform/x86/gigabyte-wmi.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
+#define NUM_TEMPERATURE_SENSORS 6
+
+static bool force_load;
+module_param(force_load, bool, 0);
+MODULE_PARM_DESC(force_load, "Force loading on non-whitelisted platform");
+
+enum gigabyte_wmi_commandtype {
+ GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
+ GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
+ GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
+ GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
+ GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
+};
+
+struct gigabyte_wmi_args {
+ u32 arg1;
+};
+
+static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, struct acpi_buffer *out)
+{
+ const struct acpi_buffer in = {
+ .length = sizeof(*args),
+ .pointer = args,
+ };
+
+ acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
+
+ if ACPI_FAILURE(ret)
+ return -EIO;
+
+ return 0;
+}
+
+static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, u64 *res)
+{
+ union acpi_object *obj;
+ struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+ int ret;
+
+ ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
+ if (ret)
+ return ret;
+ obj = result.pointer;
+ if (obj && obj->type == ACPI_TYPE_INTEGER)
+ *res = obj->integer.value;
+ else
+ ret = -EIO;
+ kfree(result.pointer);
+ return ret;
+}
+
+static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
+{
+ struct gigabyte_wmi_args args = {
+ .arg1 = sensor,
+ };
+ u64 temp;
+ acpi_status ret;
+
+ ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
+ if (ret == 0) {
+ if (temp == 0)
+ return -ENODEV;
+ *res = (s8)temp * 1000; // value is a signed 8-bit integer
+ }
+ return ret;
+}
+
+static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct wmi_device *wdev = dev_get_drvdata(dev);
+
+ return gigabyte_wmi_temperature(wdev, channel, val);
+}
+
+static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return 0444;
+}
+
+static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT),
+ NULL
+};
+
+static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
+ .read = gigabyte_wmi_hwmon_read,
+ .is_visible = gigabyte_wmi_hwmon_is_visible,
+};
+
+static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
+ .ops = &gigabyte_wmi_hwmon_ops,
+ .info = gigabyte_wmi_hwmon_info,
+};
+
+static int gigabyte_wmi_validate_sensor_presence(struct wmi_device *wdev)
+{
+ int working_sensors = 0, i;
+ long temp;
+
+ for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
+ if (!gigabyte_wmi_temperature(wdev, i, &temp))
+ working_sensors++;
+ }
+ return working_sensors ? 0 : -ENODEV;
+}
+
+static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
+ }},
+ { }
+};
+
+static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+ int ret;
+
+ if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
+ if (force_load)
+ dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");
+ else
+ return -ENODEV;
+ }
+
+ ret = gigabyte_wmi_validate_sensor_presence(wdev);
+ if (ret) {
+ dev_info(&wdev->dev, "No temperature sensors usable");
+ return ret;
+ }
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
+ &gigabyte_wmi_hwmon_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct wmi_device_id gigabyte_wmi_id_table[] = {
+ { GIGABYTE_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver gigabyte_wmi_driver = {
+ .driver = {
+ .name = "gigabyte-wmi",
+ },
+ .id_table = gigabyte_wmi_id_table,
+ .probe = gigabyte_wmi_probe,
+};
+module_wmi_driver(gigabyte_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
+MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
+MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
+MODULE_LICENSE("GPL");

base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
--
2.31.1

2021-04-10 15:00:53

[permalink] [raw]

Subject: Re: [PATCH v3] platform/x86: add Gigabyte WMI temperature driver

Hi,

On 4/10/21 4:40 PM, Thomas Weißschuh wrote:
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Changes since v2:
> * Style cleanups
> * Test for usability during probing
> * DMI-based whitelist
> * CC hwmon maintainers
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>

This looks good, one small nitpick:

I know this is a touchy subject for some, but we are trying to move away
from the whitelist/blacklist naming where possible and we don't want to
introduce any new cases, see:

https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst#4-naming

The driver currently uses this twice:
"Force loading on non-whitelisted platform"
"Forcing loading on non-whitelisted platform"

Interestingly enough you already avoided naming the dmi_system_id table
a whitelist (good).

I would like to see "non-whitelisted" replaced with "unknown" so that we end up with:

"Force loading on unknown platform"
"Forcing loading on unknown platform"

And while at it, I think for the second sentence this would be better English
(I'm not a native speaker myself):

"Forcing load on unknown platform"

If you are ok with these changes I can fix this up while merging, no need
to send a v4. Although if you prefer to send a v4 that is fine too.

Either way let me know.

Regards,

Hans

p.s.

For v4 or for a next patch, the way to add the changelog so that it does
not get picked up / automatically gets snipped by git am is to put it
below the Signed-off-by at the end of the commit message like this:

Signed-off-by: Thomas Weißschuh <[email protected]>
---
Changes since v1:
* Incorporate feedback from Barnabás Pőcze
* Use a WMI driver instead of a platform driver
* Let the kernel manage the driver lifecycle
* Fix errno/ACPI error confusion
* Fix resource cleanup
* Document reason for integer casting

Changes since v2:
* Style cleanups
* Test for usability during probing
* DMI-based whitelist
* CC hwmon maintainers

> ---
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 11 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 194 ++++++++++++++++++++++++++++
> 4 files changed, 212 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d92f85ca831d..9c10cfc00fe8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
> F: fs/gfs2/
> F: include/uapi/linux/gfs2_ondisk.h
>
> +GIGABYTE WMI DRIVER
> +M: Thomas Weißschuh <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/platform/x86/gigabyte-wmi.c
> +
> GNSS SUBSYSTEM
> M: Johan Hovold <[email protected]>
> S: Maintained
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..fb4e6d4c1823
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define NUM_TEMPERATURE_SENSORS 6
> +
> +static bool force_load;
> +module_param(force_load, bool, 0);
> +MODULE_PARM_DESC(force_load, "Force loading on non-whitelisted platform");
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
> +
> + if ACPI_FAILURE(ret)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
> + if (ret)
> + return ret;
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER)
> + *res = obj->integer.value;
> + else
> + ret = -EIO;
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0) {
> + if (temp == 0)
> + return -ENODEV;
> + *res = (s8)temp * 1000; // value is a signed 8-bit integer
> + }
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct wmi_device *wdev = dev_get_drvdata(dev);
> +
> + return gigabyte_wmi_temperature(wdev, channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static int gigabyte_wmi_validate_sensor_presence(struct wmi_device *wdev)
> +{
> + int working_sensors = 0, i;
> + long temp;
> +
> + for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
> + if (!gigabyte_wmi_temperature(wdev, i, &temp))
> + working_sensors++;
> + }
> + return working_sensors ? 0 : -ENODEV;
> +}
> +
> +static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
> + }},
> + { }
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev;
> + int ret;
> +
> + if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
> + if (force_load)
> + dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");
> + else
> + return -ENODEV;
> + }
> +
> + ret = gigabyte_wmi_validate_sensor_presence(wdev);
> + if (ret) {
> + dev_info(&wdev->dev, "No temperature sensors usable");
> + return ret;
> + }
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { }
> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>

2021-04-10 15:18:52

[permalink] [raw]

Subject: Re: [PATCH v3] platform/x86: add Gigabyte WMI temperature driver

On 4/10/21 7:40 AM, Thomas Weißschuh wrote:
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Changes since v2:
> * Style cleanups
> * Test for usability during probing
> * DMI-based whitelist
> * CC hwmon maintainers
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 11 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 194 ++++++++++++++++++++++++++++
> 4 files changed, 212 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d92f85ca831d..9c10cfc00fe8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
> F: fs/gfs2/
> F: include/uapi/linux/gfs2_ondisk.h
>
> +GIGABYTE WMI DRIVER
> +M: Thomas Weißschuh <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/platform/x86/gigabyte-wmi.c
> +
> GNSS SUBSYSTEM
> M: Johan Hovold <[email protected]>
> S: Maintained
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..fb4e6d4c1823
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define NUM_TEMPERATURE_SENSORS 6

Style: #define<space>name<tab>value

but of course that is Hans' call.

> +
> +static bool force_load;
> +module_param(force_load, bool, 0);
> +MODULE_PARM_DESC(force_load, "Force loading on non-whitelisted platform");
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
> +
> + if ACPI_FAILURE(ret)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
> + if (ret)
> + return ret;
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER)
> + *res = obj->integer.value;
> + else
> + ret = -EIO;
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0) {
> + if (temp == 0)
> + return -ENODEV;

That should be checked in gigabyte_wmi_hwmon_is_visible(); that is what that
function is for.

> + *res = (s8)temp * 1000; // value is a signed 8-bit integer
> + }
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct wmi_device *wdev = dev_get_drvdata(dev);
> +
> + return gigabyte_wmi_temperature(wdev, channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static int gigabyte_wmi_validate_sensor_presence(struct wmi_device *wdev)
> +{
> + int working_sensors = 0, i;
> + long temp;
> +
> + for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
> + if (!gigabyte_wmi_temperature(wdev, i, &temp))
> + working_sensors++;
> + }
> + return working_sensors ? 0 : -ENODEV;
> +}
> +
> +static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
> + }},
> + { }
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev;
> + int ret;
> +
> + if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
> + if (force_load)
> + dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");
> + else
> + return -ENODEV;

Style:
if (!force_load)
return -ENODEV;
dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");

> + }
> +
> + ret = gigabyte_wmi_validate_sensor_presence(wdev);
> + if (ret) {
> + dev_info(&wdev->dev, "No temperature sensors usable");

Normally one does not display a message if a probe function returns -ENODEV,
unless it is an error, to avoid polluting the kernel log.

> + return ret;
> + }
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { }
> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>

2021-04-10 15:21:58

[permalink] [raw]

Subject: Re: [PATCH v3] platform/x86: add Gigabyte WMI temperature driver

On 4/10/21 7:57 AM, Hans de Goede wrote:
> Hi,
>
> On 4/10/21 4:40 PM, Thomas Weißschuh wrote:
>> Changes since v1:
>> * Incorporate feedback from Barnabás Pőcze
>> * Use a WMI driver instead of a platform driver
>> * Let the kernel manage the driver lifecycle
>> * Fix errno/ACPI error confusion
>> * Fix resource cleanup
>> * Document reason for integer casting
>>
>> Changes since v2:
>> * Style cleanups
>> * Test for usability during probing
>> * DMI-based whitelist
>> * CC hwmon maintainers
>>
>> -- >8 --
>>
>> Tested with a X570 I Aorus Pro Wifi.
>> The mainboard contains an ITE IT8688E chip for management.
>> This chips is also handled by drivers/hwmon/i87.c but as it is also used
>> by the firmware itself it needs an ACPI driver.
>>
>> Unfortunately not all sensor registers are handled by the firmware and even
>> less are exposed via WMI.
>>
>> Signed-off-by: Thomas Weißschuh <[email protected]>
>
> This looks good, one small nitpick:
>
> I know this is a touchy subject for some, but we are trying to move away
> from the whitelist/blacklist naming where possible and we don't want to
> introduce any new cases, see:
>
> https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst#4-naming
>
> The driver currently uses this twice:
> "Force loading on non-whitelisted platform"
> "Forcing loading on non-whitelisted platform"
>
> Interestingly enough you already avoided naming the dmi_system_id table
> a whitelist (good).
>
> I would like to see "non-whitelisted" replaced with "unknown" so that we end up with:
>
> "Force loading on unknown platform"
> "Forcing loading on unknown platform"
>
> And while at it, I think for the second sentence this would be better English
> (I'm not a native speaker myself):
>
> "Forcing load on unknown platform"
>

Not native either, but I think it is either "Forcing load" or "Force loading".

> If you are ok with these changes I can fix this up while merging, no need
> to send a v4. Although if you prefer to send a v4 that is fine too.
>

Please consider adding an existence check into the is_visible function.
Sysfs attributes for non-existing sensors should not be created.

Thanks,
Guenter

> Either way let me know.
>
> Regards,
>
> Hans
>
> p.s.
>
> For v4 or for a next patch, the way to add the changelog so that it does
> not get picked up / automatically gets snipped by git am is to put it
> below the Signed-off-by at the end of the commit message like this:
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> Changes since v1:
> * Incorporate feedback from Barnabás Pőcze
> * Use a WMI driver instead of a platform driver
> * Let the kernel manage the driver lifecycle
> * Fix errno/ACPI error confusion
> * Fix resource cleanup
> * Document reason for integer casting
>
> Changes since v2:
> * Style cleanups
> * Test for usability during probing
> * DMI-based whitelist
> * CC hwmon maintainers
>
>
>
>
>
>
>
>> ---
>> MAINTAINERS | 6 +
>> drivers/platform/x86/Kconfig | 11 ++
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/gigabyte-wmi.c | 194 ++++++++++++++++++++++++++++
>> 4 files changed, 212 insertions(+)
>> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d92f85ca831d..9c10cfc00fe8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
>> F: fs/gfs2/
>> F: include/uapi/linux/gfs2_ondisk.h
>>
>> +GIGABYTE WMI DRIVER
>> +M: Thomas Weißschuh <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/platform/x86/gigabyte-wmi.c
>> +
>> GNSS SUBSYSTEM
>> M: Johan Hovold <[email protected]>
>> S: Maintained
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index ad4e630e73e2..96622a2106f7 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -123,6 +123,17 @@ config XIAOMI_WMI
>> To compile this driver as a module, choose M here: the module will
>> be called xiaomi-wmi.
>>
>> +config GIGABYTE_WMI
>> + tristate "Gigabyte WMI temperature driver"
>> + depends on ACPI_WMI
>> + depends on HWMON
>> + help
>> + Say Y here if you want to support WMI-based temperature reporting on
>> + Gigabyte mainboards.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called gigabyte-wmi.
>> +
>> config ACERHDF
>> tristate "Acer Aspire One temperature and fan driver"
>> depends on ACPI && THERMAL
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 60d554073749..1621ebfd04fd 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
>> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
>> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
>> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
>> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>>
>> # Acer
>> obj-$(CONFIG_ACERHDF) += acerhdf.o
>> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
>> new file mode 100644
>> index 000000000000..fb4e6d4c1823
>> --- /dev/null
>> +++ b/drivers/platform/x86/gigabyte-wmi.c
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
>> + */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
>> +#define NUM_TEMPERATURE_SENSORS 6
>> +
>> +static bool force_load;
>> +module_param(force_load, bool, 0);
>> +MODULE_PARM_DESC(force_load, "Force loading on non-whitelisted platform");
>> +
>> +enum gigabyte_wmi_commandtype {
>> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
>> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
>> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
>> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
>> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
>> +};
>> +
>> +struct gigabyte_wmi_args {
>> + u32 arg1;
>> +};
>> +
>> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
>> + enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
>> +{
>> + const struct acpi_buffer in = {
>> + .length = sizeof(*args),
>> + .pointer = args,
>> + };
>> +
>> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
>> +
>> + if ACPI_FAILURE(ret)
>> + return -EIO;
>> +
>> + return 0;
>> +}
>> +
>> +static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
>> + enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, u64 *res)
>> +{
>> + union acpi_object *obj;
>> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
>> + int ret;
>> +
>> + ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
>> + if (ret)
>> + return ret;
>> + obj = result.pointer;
>> + if (obj && obj->type == ACPI_TYPE_INTEGER)
>> + *res = obj->integer.value;
>> + else
>> + ret = -EIO;
>> + kfree(result.pointer);
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
>> +{
>> + struct gigabyte_wmi_args args = {
>> + .arg1 = sensor,
>> + };
>> + u64 temp;
>> + acpi_status ret;
>> +
>> + ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
>> + if (ret == 0) {
>> + if (temp == 0)
>> + return -ENODEV;
>> + *res = (s8)temp * 1000; // value is a signed 8-bit integer
>> + }
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct wmi_device *wdev = dev_get_drvdata(dev);
>> +
>> + return gigabyte_wmi_temperature(wdev, channel, val);
>> +}
>> +
>> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + return 0444;
>> +}
>> +
>> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT),
>> + NULL
>> +};
>> +
>> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
>> + .read = gigabyte_wmi_hwmon_read,
>> + .is_visible = gigabyte_wmi_hwmon_is_visible,
>> +};
>> +
>> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
>> + .ops = &gigabyte_wmi_hwmon_ops,
>> + .info = gigabyte_wmi_hwmon_info,
>> +};
>> +
>> +static int gigabyte_wmi_validate_sensor_presence(struct wmi_device *wdev)
>> +{
>> + int working_sensors = 0, i;
>> + long temp;
>> +
>> + for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
>> + if (!gigabyte_wmi_temperature(wdev, i, &temp))
>> + working_sensors++;
>> + }
>> + return working_sensors ? 0 : -ENODEV;
>> +}
>> +
>> +static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
>> + }},
>> + { }
>> +};
>> +
>> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> + struct device *hwmon_dev;
>> + int ret;
>> +
>> + if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
>> + if (force_load)
>> + dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");
>> + else
>> + return -ENODEV;
>> + }
>> +
>> + ret = gigabyte_wmi_validate_sensor_presence(wdev);
>> + if (ret) {
>> + dev_info(&wdev->dev, "No temperature sensors usable");
>> + return ret;
>> + }
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
>> + &gigabyte_wmi_hwmon_chip_info, NULL);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
>> + { GIGABYTE_WMI_GUID, NULL },
>> + { }
>> +};
>> +
>> +static struct wmi_driver gigabyte_wmi_driver = {
>> + .driver = {
>> + .name = "gigabyte-wmi",
>> + },
>> + .id_table = gigabyte_wmi_id_table,
>> + .probe = gigabyte_wmi_probe,
>> +};
>> +module_wmi_driver(gigabyte_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
>> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
>> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>>
>

2021-04-10 15:25:08

[permalink] [raw]

Subject: Re: [PATCH v3] platform/x86: add Gigabyte WMI temperature driver

Hi,

On 4/10/21 5:15 PM, Guenter Roeck wrote:
> On 4/10/21 7:40 AM, Thomas Weißschuh wrote:
>> Changes since v1:
>> * Incorporate feedback from Barnabás Pőcze
>> * Use a WMI driver instead of a platform driver
>> * Let the kernel manage the driver lifecycle
>> * Fix errno/ACPI error confusion
>> * Fix resource cleanup
>> * Document reason for integer casting
>>
>> Changes since v2:
>> * Style cleanups
>> * Test for usability during probing
>> * DMI-based whitelist
>> * CC hwmon maintainers
>>
>> -- >8 --
>>
>> Tested with a X570 I Aorus Pro Wifi.
>> The mainboard contains an ITE IT8688E chip for management.
>> This chips is also handled by drivers/hwmon/i87.c but as it is also used
>> by the firmware itself it needs an ACPI driver.
>>
>> Unfortunately not all sensor registers are handled by the firmware and even
>> less are exposed via WMI.
>>
>> Signed-off-by: Thomas Weißschuh <[email protected]>
>> ---
>> MAINTAINERS | 6 +
>> drivers/platform/x86/Kconfig | 11 ++
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/gigabyte-wmi.c | 194 ++++++++++++++++++++++++++++
>> 4 files changed, 212 insertions(+)
>> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d92f85ca831d..9c10cfc00fe8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
>> F: fs/gfs2/
>> F: include/uapi/linux/gfs2_ondisk.h
>>
>> +GIGABYTE WMI DRIVER
>> +M: Thomas Weißschuh <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/platform/x86/gigabyte-wmi.c
>> +
>> GNSS SUBSYSTEM
>> M: Johan Hovold <[email protected]>
>> S: Maintained
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index ad4e630e73e2..96622a2106f7 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -123,6 +123,17 @@ config XIAOMI_WMI
>> To compile this driver as a module, choose M here: the module will
>> be called xiaomi-wmi.
>>
>> +config GIGABYTE_WMI
>> + tristate "Gigabyte WMI temperature driver"
>> + depends on ACPI_WMI
>> + depends on HWMON
>> + help
>> + Say Y here if you want to support WMI-based temperature reporting on
>> + Gigabyte mainboards.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called gigabyte-wmi.
>> +
>> config ACERHDF
>> tristate "Acer Aspire One temperature and fan driver"
>> depends on ACPI && THERMAL
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 60d554073749..1621ebfd04fd 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
>> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
>> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
>> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
>> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>>
>> # Acer
>> obj-$(CONFIG_ACERHDF) += acerhdf.o
>> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
>> new file mode 100644
>> index 000000000000..fb4e6d4c1823
>> --- /dev/null
>> +++ b/drivers/platform/x86/gigabyte-wmi.c
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
>> + */
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
>> +#define NUM_TEMPERATURE_SENSORS 6
>
> Style: #define<space>name<tab>value
>
> but of course that is Hans' call.

I agree that aligning the 2 define values with tabs would be better.

>
>> +
>> +static bool force_load;
>> +module_param(force_load, bool, 0);
>> +MODULE_PARM_DESC(force_load, "Force loading on non-whitelisted platform");
>> +
>> +enum gigabyte_wmi_commandtype {
>> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
>> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
>> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
>> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
>> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
>> +};
>> +
>> +struct gigabyte_wmi_args {
>> + u32 arg1;
>> +};
>> +
>> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
>> + enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
>> +{
>> + const struct acpi_buffer in = {
>> + .length = sizeof(*args),
>> + .pointer = args,
>> + };
>> +
>> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
>> +
>> + if ACPI_FAILURE(ret)
>> + return -EIO;
>> +
>> + return 0;
>> +}
>> +
>> +static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
>> + enum gigabyte_wmi_commandtype command,
>> + struct gigabyte_wmi_args *args, u64 *res)
>> +{
>> + union acpi_object *obj;
>> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
>> + int ret;
>> +
>> + ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
>> + if (ret)
>> + return ret;
>> + obj = result.pointer;
>> + if (obj && obj->type == ACPI_TYPE_INTEGER)
>> + *res = obj->integer.value;
>> + else
>> + ret = -EIO;
>> + kfree(result.pointer);
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
>> +{
>> + struct gigabyte_wmi_args args = {
>> + .arg1 = sensor,
>> + };
>> + u64 temp;
>> + acpi_status ret;
>> +
>> + ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
>> + if (ret == 0) {
>> + if (temp == 0)
>> + return -ENODEV;
>
> That should be checked in gigabyte_wmi_hwmon_is_visible(); that is what that
> function is for.

Good point, actually the way I think this should be done is cache the result of
the initial probe done in gigabyte_wmi_validate_sensor_presence() and use that in
is_visible to return either 0 (not visible) or 0444, this way you can also
hide sensors when there is a whole in the range of sensors somehow.

So I guess we do need a v4 of this patch after all.

>
>> + *res = (s8)temp * 1000; // value is a signed 8-bit integer
>> + }
>> + return ret;
>> +}
>> +
>> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct wmi_device *wdev = dev_get_drvdata(dev);
>> +
>> + return gigabyte_wmi_temperature(wdev, channel, val);
>> +}
>> +
>> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + return 0444;
>> +}
>> +
>> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT,
>> + HWMON_T_INPUT),
>> + NULL
>> +};
>> +
>> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
>> + .read = gigabyte_wmi_hwmon_read,
>> + .is_visible = gigabyte_wmi_hwmon_is_visible,
>> +};
>> +
>> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
>> + .ops = &gigabyte_wmi_hwmon_ops,
>> + .info = gigabyte_wmi_hwmon_info,
>> +};
>> +
>> +static int gigabyte_wmi_validate_sensor_presence(struct wmi_device *wdev)
>> +{
>> + int working_sensors = 0, i;
>> + long temp;
>> +
>> + for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
>> + if (!gigabyte_wmi_temperature(wdev, i, &temp))
>> + working_sensors++;
>> + }
>> + return working_sensors ? 0 : -ENODEV;
>> +}
>> +
>> +static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
>> + }},
>> + { .matches = {
>> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
>> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
>> + }},
>> + { }
>> +};
>> +
>> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> + struct device *hwmon_dev;
>> + int ret;
>> +
>> + if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
>> + if (force_load)
>> + dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");
>> + else
>> + return -ENODEV;
>
> Style:
> if (!force_load)
> return -ENODEV;
> dev_warn(&wdev->dev, "Forcing loading on non-whitelisted platform");

Ack, that would be better.

>
>> + }
>> +
>> + ret = gigabyte_wmi_validate_sensor_presence(wdev);
>> + if (ret) {
>> + dev_info(&wdev->dev, "No temperature sensors usable");
>
> Normally one does not display a message if a probe function returns -ENODEV,
> unless it is an error, to avoid polluting the kernel log.

This will normally only be shown when the force_load module parameter is used,
at which point I think it makes sense to explain why the driver is still not
loading.

Regards,

Hans

>
>> + return ret;
>> + }
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
>> + &gigabyte_wmi_hwmon_chip_info, NULL);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
>> + { GIGABYTE_WMI_GUID, NULL },
>> + { }
>> +};
>> +
>> +static struct wmi_driver gigabyte_wmi_driver = {
>> + .driver = {
>> + .name = "gigabyte-wmi",
>> + },
>> + .id_table = gigabyte_wmi_id_table,
>> + .probe = gigabyte_wmi_probe,
>> +};
>> +module_wmi_driver(gigabyte_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
>> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
>> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
>> +MODULE_LICENSE("GPL");
>>
>> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>>
>

2021-04-10 18:21:18

[permalink] [raw]

Subject: [PATCH v4] platform/x86: add Gigabyte WMI temperature driver

Changes since v3:
* Completely hide unusable sensors
* Expose force_load parameter read-only via sysfs
* Naming
* Style cleanups

-- >8 --

Tested with
* X570 I Aorus Pro Wifi (rev 1.0)
* B550M DS3H
* B550 Gaming X V2 (rev.1.x)
* Z390 I AORUS PRO WIFI (rev. 1.0)

The mainboard contains an ITE IT8688E chip for management.
This chips is also handled by drivers/hwmon/i87.c but as it is also used
by the firmware itself it needs an ACPI driver.

Unfortunately not all sensor registers are handled by the firmware and even
less are exposed via WMI.

Signed-off-by: Thomas Weißschuh <[email protected]>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/gigabyte-wmi.c | 195 ++++++++++++++++++++++++++++
4 files changed, 213 insertions(+)
create mode 100644 drivers/platform/x86/gigabyte-wmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..9c10cfc00fe8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
F: fs/gfs2/
F: include/uapi/linux/gfs2_ondisk.h

+GIGABYTE WMI DRIVER
+M: Thomas Weißschuh <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/platform/x86/gigabyte-wmi.c
+
GNSS SUBSYSTEM
M: Johan Hovold <[email protected]>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..96622a2106f7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,17 @@ config XIAOMI_WMI
To compile this driver as a module, choose M here: the module will
be called xiaomi-wmi.

+config GIGABYTE_WMI
+ tristate "Gigabyte WMI temperature driver"
+ depends on ACPI_WMI
+ depends on HWMON
+ help
+ Say Y here if you want to support WMI-based temperature reporting on
+ Gigabyte mainboards.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gigabyte-wmi.
+
config ACERHDF
tristate "Acer Aspire One temperature and fan driver"
depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..1621ebfd04fd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
+obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o

# Acer
obj-$(CONFIG_ACERHDF) += acerhdf.o
diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
new file mode 100644
index 000000000000..c17e51fcf000
--- /dev/null
+++ b/drivers/platform/x86/gigabyte-wmi.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
+#define NUM_TEMPERATURE_SENSORS 6
+
+static bool force_load;
+module_param(force_load, bool, 0444);
+MODULE_PARM_DESC(force_load, "Force loading on unknown platform");
+
+static u8 usable_sensors_mask;
+
+enum gigabyte_wmi_commandtype {
+ GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
+ GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
+ GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
+ GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
+ GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
+};
+
+struct gigabyte_wmi_args {
+ u32 arg1;
+};
+
+static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, struct acpi_buffer *out)
+{
+ const struct acpi_buffer in = {
+ .length = sizeof(*args),
+ .pointer = args,
+ };
+
+ acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
+
+ if ACPI_FAILURE(ret)
+ return -EIO;
+
+ return 0;
+}
+
+static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, u64 *res)
+{
+ union acpi_object *obj;
+ struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+ int ret;
+
+ ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
+ if (ret)
+ return ret;
+ obj = result.pointer;
+ if (obj && obj->type == ACPI_TYPE_INTEGER)
+ *res = obj->integer.value;
+ else
+ ret = -EIO;
+ kfree(result.pointer);
+ return ret;
+}
+
+static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
+{
+ struct gigabyte_wmi_args args = {
+ .arg1 = sensor,
+ };
+ u64 temp;
+ acpi_status ret;
+
+ ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
+ if (ret == 0) {
+ if (temp == 0)
+ return -ENODEV;
+ *res = (s8)temp * 1000; // value is a signed 8-bit integer
+ }
+ return ret;
+}
+
+static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct wmi_device *wdev = dev_get_drvdata(dev);
+
+ return gigabyte_wmi_temperature(wdev, channel, val);
+}
+
+static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return usable_sensors_mask & BIT(channel) ? 0444 : 0;
+}
+
+static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT),
+ NULL
+};
+
+static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
+ .read = gigabyte_wmi_hwmon_read,
+ .is_visible = gigabyte_wmi_hwmon_is_visible,
+};
+
+static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
+ .ops = &gigabyte_wmi_hwmon_ops,
+ .info = gigabyte_wmi_hwmon_info,
+};
+
+static u8 gigabyte_wmi_detect_sensor_usability(struct wmi_device *wdev)
+{
+ int i;
+ long temp;
+ u8 r = 0;
+
+ for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
+ if (!gigabyte_wmi_temperature(wdev, i, &temp))
+ r |= BIT(i);
+ }
+ return r;
+}
+
+static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
+ }},
+ { }
+};
+
+static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+
+ if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
+ if (!force_load)
+ return -ENODEV;
+ dev_warn(&wdev->dev, "Forcing load on unknown platform");
+ }
+
+ usable_sensors_mask = gigabyte_wmi_detect_sensor_usability(wdev);
+ if (!usable_sensors_mask) {
+ dev_info(&wdev->dev, "No temperature sensors usable");
+ return -ENODEV;
+ }
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
+ &gigabyte_wmi_hwmon_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct wmi_device_id gigabyte_wmi_id_table[] = {
+ { GIGABYTE_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver gigabyte_wmi_driver = {
+ .driver = {
+ .name = "gigabyte-wmi",
+ },
+ .id_table = gigabyte_wmi_id_table,
+ .probe = gigabyte_wmi_probe,
+};
+module_wmi_driver(gigabyte_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
+MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
+MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
+MODULE_LICENSE("GPL");

base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
--
2.31.1

2021-04-11 01:00:05

[permalink] [raw]

Subject: Re: [PATCH v4] platform/x86: add Gigabyte WMI temperature driver

On 4/10/21 11:18 AM, Thomas Weißschuh wrote:
> Changes since v3:
> * Completely hide unusable sensors
> * Expose force_load parameter read-only via sysfs
> * Naming
> * Style cleanups
>
> -- >8 --
>
> Tested with
> * X570 I Aorus Pro Wifi (rev 1.0)
> * B550M DS3H
> * B550 Gaming X V2 (rev.1.x)
> * Z390 I AORUS PRO WIFI (rev. 1.0)
>
> The mainboard contains an ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>

Good enough, though you may want to improve your description.

Reviewed-by: Guenter Roeck <[email protected]>

FWIW, on B450 AORUS M:

gigabyte-wmi DEADBEEF-2001-0000-00A0-C90629100000: Forcing load on unknown platform
gigabyte-wmi DEADBEEF-2001-0000-00A0-C90629100000: No temperature sensors usable

Wonder who came up with that GUID.

> ---

Change log and everything that is not supposed to show up in the git history
is supposed to go here.

Guenter

> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 11 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/gigabyte-wmi.c | 195 ++++++++++++++++++++++++++++
> 4 files changed, 213 insertions(+)
> create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d92f85ca831d..9c10cfc00fe8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
> F: fs/gfs2/
> F: include/uapi/linux/gfs2_ondisk.h
>
> +GIGABYTE WMI DRIVER
> +M: Thomas Weißschuh <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/platform/x86/gigabyte-wmi.c
> +
> GNSS SUBSYSTEM
> M: Johan Hovold <[email protected]>
> S: Maintained
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..96622a2106f7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,17 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> + tristate "Gigabyte WMI temperature driver"
> + depends on ACPI_WMI
> + depends on HWMON
> + help
> + Say Y here if you want to support WMI-based temperature reporting on
> + Gigabyte mainboards.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gigabyte-wmi.
> +
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> # Acer
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..c17e51fcf000
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define NUM_TEMPERATURE_SENSORS 6
> +
> +static bool force_load;
> +module_param(force_load, bool, 0444);
> +MODULE_PARM_DESC(force_load, "Force loading on unknown platform");
> +
> +static u8 usable_sensors_mask;
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
> +
> + if ACPI_FAILURE(ret)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, u64 *res)
> +{
> + union acpi_object *obj;
> + struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> + int ret;
> +
> + ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
> + if (ret)
> + return ret;
> + obj = result.pointer;
> + if (obj && obj->type == ACPI_TYPE_INTEGER)
> + *res = obj->integer.value;
> + else
> + ret = -EIO;
> + kfree(result.pointer);
> + return ret;
> +}
> +
> +static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
> +{
> + struct gigabyte_wmi_args args = {
> + .arg1 = sensor,
> + };
> + u64 temp;
> + acpi_status ret;
> +
> + ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> + if (ret == 0) {
> + if (temp == 0)
> + return -ENODEV;
> + *res = (s8)temp * 1000; // value is a signed 8-bit integer
> + }
> + return ret;
> +}
> +
> +static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct wmi_device *wdev = dev_get_drvdata(dev);
> +
> + return gigabyte_wmi_temperature(wdev, channel, val);
> +}
> +
> +static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + return usable_sensors_mask & BIT(channel) ? 0444 : 0;
> +}
> +
> +static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
> + .read = gigabyte_wmi_hwmon_read,
> + .is_visible = gigabyte_wmi_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
> + .ops = &gigabyte_wmi_hwmon_ops,
> + .info = gigabyte_wmi_hwmon_info,
> +};
> +
> +static u8 gigabyte_wmi_detect_sensor_usability(struct wmi_device *wdev)
> +{
> + int i;
> + long temp;
> + u8 r = 0;
> +
> + for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
> + if (!gigabyte_wmi_temperature(wdev, i, &temp))
> + r |= BIT(i);
> + }
> + return r;
> +}
> +
> +static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
> + }},
> + { .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
> + }},
> + { }
> +};
> +
> +static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct device *hwmon_dev;
> +
> + if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
> + if (!force_load)
> + return -ENODEV;
> + dev_warn(&wdev->dev, "Forcing load on unknown platform");
> + }
> +
> + usable_sensors_mask = gigabyte_wmi_detect_sensor_usability(wdev);
> + if (!usable_sensors_mask) {
> + dev_info(&wdev->dev, "No temperature sensors usable");
> + return -ENODEV;
> + }
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
> + &gigabyte_wmi_hwmon_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct wmi_device_id gigabyte_wmi_id_table[] = {
> + { GIGABYTE_WMI_GUID, NULL },
> + { }
> +};
> +
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
>

2021-04-11 18:55:57

by Barnabás Pőcze

[permalink] [raw]

Subject: Re: [PATCH v4] platform/x86: add Gigabyte WMI temperature driver

Hi

2021. április 10., szombat 20:18 keltezéssel, Thomas Weißschuh írta:

> [...]
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..c17e51fcf000
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define NUM_TEMPERATURE_SENSORS 6
> +
> +static bool force_load;
> +module_param(force_load, bool, 0444);
> +MODULE_PARM_DESC(force_load, "Force loading on unknown platform");
> +
> +static u8 usable_sensors_mask;
> +
> +enum gigabyte_wmi_commandtype {
> + GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
> + GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
> + GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
> + GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
> + GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
> +};
> +
> +struct gigabyte_wmi_args {
> + u32 arg1;
> +};
> +
> +static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
> + enum gigabyte_wmi_commandtype command,
> + struct gigabyte_wmi_args *args, struct acpi_buffer *out)
> +{
> + const struct acpi_buffer in = {
> + .length = sizeof(*args),
> + .pointer = args,
> + };
> +
> + acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
> +
> + if ACPI_FAILURE(ret)

Please use `if (...)`.

> + return -EIO;
> +
> + return 0;
> +}
> [...]
> +static struct wmi_driver gigabyte_wmi_driver = {
> + .driver = {
> + .name = "gigabyte-wmi",
> + },
> + .id_table = gigabyte_wmi_id_table,
> + .probe = gigabyte_wmi_probe,
> +};
> +module_wmi_driver(gigabyte_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
> +MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
> +MODULE_DESCRIPTION("Gigabyte WMI temperature Driver");
^
It's a minor thing, but I think a lowercase 'd' would be better.

> +MODULE_LICENSE("GPL");
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1

Regards,
Barnabás Pőcze

2021-04-13 01:55:19

[permalink] [raw]

Subject: [PATCH v5] platform/x86: add Gigabyte WMI temperature driver

Tested with
* X570 I Aorus Pro Wifi (rev 1.0)
* B550M DS3H
* B550 Gaming X V2 (rev.1.x)
* Z390 I AORUS PRO WIFI (rev. 1.0)

Those mainboards contain an ITE chips for management and
monitoring.

They could also be handled by drivers/hwmon/i87.c.
But the SuperIO range used by i87 is already claimed and used by the
firmware.

The following warning is printed at boot:

kernel: ACPI Warning: SystemIO range 0x0000000000000A45-0x0000000000000A46 conflicts with OpRegion 0x0000000000000A45-0x0000000000000A46 (\GSA1.SIO1) (20200528/utaddress-204)
kernel: ACPI: This conflict may cause random problems and system instability
kernel: ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver

This driver implements such an ACPI driver.

Unfortunately not all sensor registers are handled by the firmware and even
less are exposed via WMI.

Signed-off-by: Thomas Weißschuh <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>

---

Changes since v4:
* Style
* Wording
* Alignment of email addresses
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/gigabyte-wmi.c | 195 ++++++++++++++++++++++++++++
4 files changed, 213 insertions(+)
create mode 100644 drivers/platform/x86/gigabyte-wmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..7fb5e2ba489b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7543,6 +7543,12 @@ F: Documentation/filesystems/gfs2*
F: fs/gfs2/
F: include/uapi/linux/gfs2_ondisk.h

+GIGABYTE WMI DRIVER
+M: Thomas Weißschuh <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/platform/x86/gigabyte-wmi.c
+
GNSS SUBSYSTEM
M: Johan Hovold <[email protected]>
S: Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..96622a2106f7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,17 @@ config XIAOMI_WMI
To compile this driver as a module, choose M here: the module will
be called xiaomi-wmi.

+config GIGABYTE_WMI
+ tristate "Gigabyte WMI temperature driver"
+ depends on ACPI_WMI
+ depends on HWMON
+ help
+ Say Y here if you want to support WMI-based temperature reporting on
+ Gigabyte mainboards.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gigabyte-wmi.
+
config ACERHDF
tristate "Acer Aspire One temperature and fan driver"
depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..1621ebfd04fd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
+obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o

# Acer
obj-$(CONFIG_ACERHDF) += acerhdf.o
diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
new file mode 100644
index 000000000000..bb1b0b205fa7
--- /dev/null
+++ b/drivers/platform/x86/gigabyte-wmi.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Thomas Weißschuh <[email protected]>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
+#define NUM_TEMPERATURE_SENSORS 6
+
+static bool force_load;
+module_param(force_load, bool, 0444);
+MODULE_PARM_DESC(force_load, "Force loading on unknown platform");
+
+static u8 usable_sensors_mask;
+
+enum gigabyte_wmi_commandtype {
+ GIGABYTE_WMI_BUILD_DATE_QUERY = 0x1,
+ GIGABYTE_WMI_MAINBOARD_TYPE_QUERY = 0x2,
+ GIGABYTE_WMI_FIRMWARE_VERSION_QUERY = 0x4,
+ GIGABYTE_WMI_MAINBOARD_NAME_QUERY = 0x5,
+ GIGABYTE_WMI_TEMPERATURE_QUERY = 0x125,
+};
+
+struct gigabyte_wmi_args {
+ u32 arg1;
+};
+
+static int gigabyte_wmi_perform_query(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, struct acpi_buffer *out)
+{
+ const struct acpi_buffer in = {
+ .length = sizeof(*args),
+ .pointer = args,
+ };
+
+ acpi_status ret = wmidev_evaluate_method(wdev, 0x0, command, &in, out);
+
+ if (ACPI_FAILURE(ret))
+ return -EIO;
+
+ return 0;
+}
+
+static int gigabyte_wmi_query_integer(struct wmi_device *wdev,
+ enum gigabyte_wmi_commandtype command,
+ struct gigabyte_wmi_args *args, u64 *res)
+{
+ union acpi_object *obj;
+ struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+ int ret;
+
+ ret = gigabyte_wmi_perform_query(wdev, command, args, &result);
+ if (ret)
+ return ret;
+ obj = result.pointer;
+ if (obj && obj->type == ACPI_TYPE_INTEGER)
+ *res = obj->integer.value;
+ else
+ ret = -EIO;
+ kfree(result.pointer);
+ return ret;
+}
+
+static int gigabyte_wmi_temperature(struct wmi_device *wdev, u8 sensor, long *res)
+{
+ struct gigabyte_wmi_args args = {
+ .arg1 = sensor,
+ };
+ u64 temp;
+ acpi_status ret;
+
+ ret = gigabyte_wmi_query_integer(wdev, GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
+ if (ret == 0) {
+ if (temp == 0)
+ return -ENODEV;
+ *res = (s8)temp * 1000; // value is a signed 8-bit integer
+ }
+ return ret;
+}
+
+static int gigabyte_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct wmi_device *wdev = dev_get_drvdata(dev);
+
+ return gigabyte_wmi_temperature(wdev, channel, val);
+}
+
+static umode_t gigabyte_wmi_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return usable_sensors_mask & BIT(channel) ? 0444 : 0;
+}
+
+static const struct hwmon_channel_info *gigabyte_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT,
+ HWMON_T_INPUT),
+ NULL
+};
+
+static const struct hwmon_ops gigabyte_wmi_hwmon_ops = {
+ .read = gigabyte_wmi_hwmon_read,
+ .is_visible = gigabyte_wmi_hwmon_is_visible,
+};
+
+static const struct hwmon_chip_info gigabyte_wmi_hwmon_chip_info = {
+ .ops = &gigabyte_wmi_hwmon_ops,
+ .info = gigabyte_wmi_hwmon_info,
+};
+
+static u8 gigabyte_wmi_detect_sensor_usability(struct wmi_device *wdev)
+{
+ int i;
+ long temp;
+ u8 r = 0;
+
+ for (i = 0; i < NUM_TEMPERATURE_SENSORS; i++) {
+ if (!gigabyte_wmi_temperature(wdev, i, &temp))
+ r |= BIT(i);
+ }
+ return r;
+}
+
+static const struct dmi_system_id gigabyte_wmi_known_working_platforms[] = {
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550 GAMING X V2"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "B550M DS3H"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "Z390 I AORUS PRO WIFI-CF"),
+ }},
+ { .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "Gigabyte Technology Co., Ltd."),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "X570 I AORUS PRO WIFI"),
+ }},
+ { }
+};
+
+static int gigabyte_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+
+ if (!dmi_check_system(gigabyte_wmi_known_working_platforms)) {
+ if (!force_load)
+ return -ENODEV;
+ dev_warn(&wdev->dev, "Forcing load on unknown platform");
+ }
+
+ usable_sensors_mask = gigabyte_wmi_detect_sensor_usability(wdev);
+ if (!usable_sensors_mask) {
+ dev_info(&wdev->dev, "No temperature sensors usable");
+ return -ENODEV;
+ }
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev, "gigabyte_wmi", wdev,
+ &gigabyte_wmi_hwmon_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct wmi_device_id gigabyte_wmi_id_table[] = {
+ { GIGABYTE_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver gigabyte_wmi_driver = {
+ .driver = {
+ .name = "gigabyte-wmi",
+ },
+ .id_table = gigabyte_wmi_id_table,
+ .probe = gigabyte_wmi_probe,
+};
+module_wmi_driver(gigabyte_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, gigabyte_wmi_id_table);
+MODULE_AUTHOR("Thomas Weißschuh <[email protected]>");
+MODULE_DESCRIPTION("Gigabyte WMI temperature driver");
+MODULE_LICENSE("GPL");

base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
--
2.31.1

2021-04-13 14:06:02