Message-ID: <N6sOrC__lJeA1mtEKUtB18DPy9hp5bSjL9rq1TfOXiRE7IAO5aih5oyPEpq-vyqdZZsF4W8FIe-9GWB15lO-3fQlqjWQrMTlTJvqLBBGYOQ=@protonmail.com>
Date:   Mon, 05 Apr 2021 17:13:48 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Thomas Weißschuh <linux@...ssschuh.net>
Cc:     "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        Mark Gross <mgross@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [PATCH] platform/x86: add Gigabyte WMI temperature driver

Hi


If you wanted to get some feedback regarding the code, then I added some comments;
otherwise please disregard this email.

I think the two most important things are:

 * consider using `devm_hwmon_device_register_with_info()` instead of manually
   specifying the attributes;
 * consider getting rid of the platform {driver,device} and use a single
   WMI driver.


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

> Hi,
>
> this patch adds support for temperature readings on Gigabyte Mainboards.
> (At least on mine)
>
> The current code should be usable as is.
> I'd like for people to test it though and report their results with different
> hardware.
>
> Further development I have some general questions:
>
> The ASL IndexField does not cover all relevant registers, can it be extended by
> driver code?
>
> * Not all registers are exposed via ACPI methods, can they be accessed via ACPI directly?
> * Some registers are exposed via ACPI methods but are not reachable directly from the WMI dispatcher.
>   * Does ASL have some sort of reflection that could enable those methods?
>   * Is it possible to call those methods directly, bypassing WMI?
>
> I suspect the answer to be "no" for all of these, but maybe I am wrong.
>
> Furthermore there are WMI methods to return information about the installed
> firmware.
>
> * Version
> * Build-date
> * Motherboard name
>
> Would it make sense to add this information as attributes to the
> platform_device?

I think if there aren't really users of the data, then just printing them
to the kernel message buffer is fine until there's a need
(in the probe function, for example). Does it provide information
that DMI doesn't?


>
> The ACPI tables can be found here:
> https://github.com/t-8ch/linux-gigabyte-wmi-driver/blob/main/ssdt8.dsl
>
> Thanks,
> Thomas
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains a 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 <linux@...ssschuh.net>
> ---
>  drivers/platform/x86/Kconfig        |  10 ++
>  drivers/platform/x86/Makefile       |   1 +
>  drivers/platform/x86/gigabyte-wmi.c | 178 ++++++++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..40d593ff1f01 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,16 @@ 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 on Gigabyte mainboards.

I'm not a native speaker, but maybe "WMI-based temperature reporting" would be better?


> +
> +	  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..a3749cf248cb
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Gigabyte WMI temperature driver
> + *
> + * Copyright (C) 2021 Thomas Weißschuh <linux@...ssschuh.net>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

It is usually preferred if the list is sorted alphabetically.


> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define DRVNAME "gigabyte-wmi"
> +
> +MODULE_AUTHOR("Thomas Weißschuh <thomas@...ssschuh.net>");
> +MODULE_DESCRIPTION("Gigabyte Generic WMI Driver");

The Kconfig says "Gigabyte WMI **temperature** driver". Which one is it?


> +MODULE_LICENSE("GPL");
> +
> +MODULE_ALIAS("wmi:" GIGABYTE_WMI_GUID);
> +
> +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,
> +};
> +
> +static int gigabyte_wmi_temperature(u8 sensor, s8 *res);

If you change the order of functions, you can get rid of this forward declaration.


> +
> +static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	int index = sattr->index;
> +	s8 temp;
> +	acpi_status res;
> +
> +	res = gigabyte_wmi_temperature(index, &temp);
> +	if (ACPI_FAILURE(res))
> +		return -res;

ACPI errors and errnos are not compatible, you can't return it like that. Or,
technically, you can, but it does not make sense. E.g. it'd "convert"
AE_NO_MEMORY to EINTR since both have the numeric value 4.


> +
> +	return sprintf(buf, "%d\n", temp * 1000);

Use `sysfs_emit()`.


> +}
> +
> +static SENSOR_DEVICE_ATTR_2_RO(temp1_input, temp, 0, 0);
> +static SENSOR_DEVICE_ATTR_2_RO(temp2_input, temp, 0, 1);
> +static SENSOR_DEVICE_ATTR_2_RO(temp3_input, temp, 0, 2);
> +static SENSOR_DEVICE_ATTR_2_RO(temp4_input, temp, 0, 3);
> +static SENSOR_DEVICE_ATTR_2_RO(temp5_input, temp, 0, 4);
> +static SENSOR_DEVICE_ATTR_2_RO(temp6_input, temp, 0, 5);
> +
> +static struct platform_device *gigabyte_wmi_pdev;
> +
> +
> +static struct attribute *gigabyte_wmi_hwmon_attributes_temp[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group gigabyte_wmi_hwmon_group_temp = {
> +	.attrs = gigabyte_wmi_hwmon_attributes_temp,
> +};
> +
> +static const struct attribute_group *gigabyte_wmi_hwmon_groups[] = {
> +	&gigabyte_wmi_hwmon_group_temp,
> +	NULL,
> +};

If you want to, you can get rid of these two definitions if you rename
`gigabyte_wmi_hwmon_attributes_temp` to `gigabyte_wmi_hwmon_temp_attrs`
and use

  ATTRIBUTE_GROUPS(gigabyte_wmi_hwmon_temp); // linux/sysfs.h

that'll define `gigabyte_wmi_hwmon_temp_group` and `gigabyte_wmi_hwmon_temp_groups`.


> +
> +static int gigabyte_wmi_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev;
> +	struct device *dev = &pdev->dev;
> +
> +	if (!wmi_has_guid(GIGABYTE_WMI_GUID))
> +		return -ENODEV;
> +	gigabyte_wmi_pdev = pdev;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> +					"gigabyte_wmi",
> +					NULL, gigabyte_wmi_hwmon_groups);

Is there a reason for not using `devm_hwmon_device_register_with_info()` and
the hwmon_{chip,channel}_info, hwmon_ops stuff? That way you wouldn't need to
touch any of the sysfs things.


> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver gigabyte_wmi_driver = {
> +	.driver = {
> +		.name	= DRVNAME,
> +	},
> +	.probe	= gigabyte_wmi_probe,
> +};

Is there any reason for using a platform driver instead of a `wmi_driver`?
I think you could get rid of both the `platform_driver` and the `platform_device`
and simply use a `wmi_driver`.


> +
> +struct args {
> +	u32 arg1;
> +	u32 arg2;
> +	u32 arg3;
> +};
> +
> +static acpi_status gigabyte_wmi_perform_query(
> +		enum gigabyte_wmi_commandtype command, struct args *args, struct acpi_buffer *out)

Long function signatures are usually split up in such a way that the first argument
remains in line with the function name.


> +{
> +	struct acpi_buffer in = {};
> +
> +	if (!args) {
> +		struct args empty_args = {};
> +
> +		args = &empty_args;

I don't think this is correct. `args` will be a dangling pointer since `empty_args`
goes out of scope - if I'm not mistaken.


> +	}
> +	in.length = sizeof(*args);
> +	in.pointer = args;
> +	return wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
> +}
> +
> +static int gigabyte_wmi_query_integer(
> +		enum gigabyte_wmi_commandtype command, struct args *args, u64 *res)
> +{
> +	union acpi_object *obj;
> +	struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };

The allocated buffer is not freed.


> +	acpi_status ret;
> +
> +	ret = gigabyte_wmi_perform_query(command, args, &result);
> +	if (ACPI_FAILURE(ret))
> +		return -ENXIO;
> +	obj = result.pointer;
> +	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
> +		pr_warn("Unexpected result type %d for command %d", obj->type, command);
> +		return -ENXIO;
> +	}
> +	*res = obj->integer.value;
> +	return AE_OK;
> +}
> +
> +static int gigabyte_wmi_temperature(u8 sensor, s8 *res)
> +{
> +	struct 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;

That cast will be done no matter if it's explicitly specified,
so you might want to drop it.


> +	return ret;
> +}
> +
> +static int __init gigabyte_wmi_init(void)
> +{
> +	struct platform_device *pdev;
> +	int err;
> +
> +	err = platform_driver_register(&gigabyte_wmi_driver);
> +	if (err)
> +		return err;
> +	pdev = platform_device_alloc(DRVNAME, -1);

Prefer `PLATFORM_DEVID_NONE` instead of -1.


> +	if (!pdev) {
> +		platform_driver_unregister(&gigabyte_wmi_driver);
> +		return -ENOMEM;
> +	}
> +	return platform_device_add(pdev);

The driver is not unregistered if this fails.


> +}
> +module_init(gigabyte_wmi_init);
> +
> +static void __exit gigabyte_wmi_exit(void)
> +{
> +	platform_device_unregister(gigabyte_wmi_pdev);
> +	platform_driver_unregister(&gigabyte_wmi_driver);
> +}
> +module_exit(gigabyte_wmi_exit);
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1
>


Regards,
Barnabás Pőcze

Powered by blists - more mailing lists