Message-ID: <247119aa-651f-4320-8522-971e2a6e4054@gmx.de>
Date:   Sun, 8 Oct 2023 20:41:07 +0200
From:   Armin Wolf <W_Armin@....de>
To:     Ai Chao <aichao@...inos.cn>, hdegoede@...hat.com,
        ilpo.jarvinen@...ux.intel.com, markgross@...nel.org
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] platform/x86: support to store/show powermode for Inspur

Am 08.10.23 um 11:21 schrieb Ai Chao:

> Support to store/show powermode for Inspur.
>
> Signed-off-by: Ai Chao <aichao@...inos.cn>
> ---
>   drivers/platform/x86/Kconfig      |  11 ++
>   drivers/platform/x86/Makefile     |   3 +
>   drivers/platform/x86/inspur-wmi.c | 180 ++++++++++++++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 drivers/platform/x86/inspur-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2a1070543391..9e565ee01a9f 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -988,6 +988,17 @@ config TOUCHSCREEN_DMI
>   	  the OS-image for the device. This option supplies the missing info.
>   	  Enable this for x86 tablets with Silead or Chipone touchscreens.
>
> +config INSPUR_WMI
> +	tristate "Inspur WMI hotkeys driver"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	help
> +	This driver provides support for Inspur WMI hotkeys.
> +	It's support to store/show powermode.
> +

Maybe "It supports to change the power mode."? A short explanation that this power mode
does would also be helpful here.

> +	To compile this driver as a module, choose M here: the module
> +	will be called inspur-wmi.
> +
>   source "drivers/platform/x86/x86-android-tablets/Kconfig"
>
>   config FW_ATTR_CLASS
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index b457de5abf7d..9285c252757e 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -98,6 +98,9 @@ obj-$(CONFIG_TOSHIBA_WMI)	+= toshiba-wmi.o
>   # before toshiba_acpi initializes
>   obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
>
> +# Inspur
> +obj-$(CONFIG_INSPUR_WMI)	+= inspur-wmi.o
> +
>   # Laptop drivers
>   obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
>   obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
> diff --git a/drivers/platform/x86/inspur-wmi.c b/drivers/platform/x86/inspur-wmi.c
> new file mode 100644
> index 000000000000..6c4d722e98dc
> --- /dev/null
> +++ b/drivers/platform/x86/inspur-wmi.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Inspur WMI hotkeys
> + *
> + *  Copyright (C) 2018	      Ai Chao <aichao@...inos.cn>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define WMI_INSPUR_POWERMODE_EVENT_GUID "854FA5AC-58C7-451D-AAB1-57D6F4E6DDD4"
> +#define WMI_INSPUR_POWERMODE_BIOS_GUID "596C31E3-332D-43C9-AEE9-585493284F5D"
> +
> +enum inspur_wmi_method_ids {
> +	INSPUR_WMI_GET_POWERMODE = 0x02,
> +	INSPUR_WMI_SET_POWERMODE = 0x03,
> +};
> +
> +struct inspur_wmi_priv {
> +	struct input_dev *idev;
> +};
> +
> +static int inspur_wmi_perform_query(char *guid,
> +		enum inspur_wmi_method_ids query_id,
> +		void *buffer, size_t insize, size_t outsize)
> +{
> +	union acpi_object *obj;
> +	int ret = 0;
> +	u32 wmi_outsize;
> +	struct acpi_buffer input = { insize, buffer};
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	wmi_evaluate_method(guid, 0, query_id, &input, &output);
> +

Please use wmidev_evaluate_method() instead of the deprecated GUID-based interface.
And check the return value of this function.

> +	obj = output.pointer;
> +	if (!obj)
> +		return -EINVAL;
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	/* Ignore output data of zero size */
> +	if (!outsize)
> +		goto out_free;
> +
> +	wmi_outsize = min_t(size_t, outsize, obj->buffer.length);

Please return an error if the size of the returned buffer does not match the size
requested by the caller. Otherwise the caller might process bogus values if the buffer size
is smaller than requested.

> +	memcpy(buffer, obj->buffer.pointer, wmi_outsize);
> +
> +out_free:
> +	kfree(obj);
> +	return ret;
> +}
> +
> +static ssize_t powermode_store(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	int ret, mode;
> +
> +	ret = kstrtoint(buf, 0, &mode);
> +	if (ret)
> +		return ret;
> +
> +	inspur_wmi_perform_query(WMI_INSPUR_POWERMODE_BIOS_GUID,
> +			INSPUR_WMI_SET_POWERMODE,
> +			&mode, sizeof(mode), sizeof(mode));

Maybe use a fixed-width integer like u32 here? Also the return value of
inspur_wmi_perform_query() should be checked.

> +
> +	return count;
> +}
> +
> +
> +static ssize_t powermode_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	int mode = 0;
> +	u8 ret;
> +	u8 *ret_code;
> +
> +	inspur_wmi_perform_query(WMI_INSPUR_POWERMODE_BIOS_GUID,
> +			INSPUR_WMI_GET_POWERMODE,
> +			&mode, sizeof(mode), sizeof(mode));

Same as above.

> +	/*
> +	 *Byte [0]: Return code, 0x0 No error, 0x01 Error
> +	 *Byte [1]: Power Mode
> +	 */
> +	ret_code = (u8 *)(&mode);
> +	ret = ret_code[1];

Please check ret_code[0] for 0x01, and return an appropriate error code in such a case.

> +
> +	return sprintf(buf, "%u\n", ret);
> +}
> +
> +DEVICE_ATTR_RW(powermode);

Please make this static.

> +
> +static struct attribute *inspur_wmi_attrs[] = {
> +	&dev_attr_powermode.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group inspur_wmi_group = {
> +	.attrs = inspur_wmi_attrs,
> +};
> +
> +const struct attribute_group *inspur_wmi_groups[] = {
> +	&inspur_wmi_group,
> +	NULL,
> +};

Same as above

> +
> +static void inspur_wmi_notify(struct wmi_device *wdev,
> +		union acpi_object *obj)
> +{
> +	//to do

Please insert something like "dev_notice(&wdev->dev, "Unknown WMI event: %u\n", event)" here
so people can find out the necessary hotkey events.

> +}
> +
> +static int inspur_wmi_input_setup(struct wmi_device *wdev)
> +{
> +	struct inspur_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> +	priv->idev = devm_input_allocate_device(&wdev->dev);
> +	if (!priv->idev)
> +		return -ENOMEM;
> +
> +	priv->idev->name = "Inspur WMI hotkeys";
> +	priv->idev->phys = "wmi/input0";
> +	priv->idev->id.bustype = BUS_HOST;
> +	priv->idev->dev.parent = &wdev->dev;
> +
> +	return input_register_device(priv->idev);
> +}
> +
> +static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct inspur_wmi_priv *priv;
> +	int err;
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(struct inspur_wmi_priv),
> +			GFP_KERNEL);

Please use "sizeof(*priv)" instead of "sizeof(struct inspur_wmi_priv)".
Also the alignment should match open parenthesis, please run "checkpatch --strict".

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	err = inspur_wmi_input_setup(wdev);
> +	return err;

The variable "err" can be omitted, just do "return inspur_wmi_input_setup(...)"

> +}
> +
> +static void inspur_wmi_remove(struct wmi_device *wdev)
> +{
> +	struct inspur_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> +	input_unregister_device(priv->idev);

The kerneldoc comment for devm_input_allocate_device() says:

"Managed input devices do not need to be explicitly unregistered or freed as it will be done
automatically when owner device unbinds from its driver (or binding fails)."

Please drop inspur_wmi_remove().

> +}
> +
> +static const struct wmi_device_id inspur_wmi_id_table[] = {
> +	{ .guid_string = WMI_INSPUR_POWERMODE_EVENT_GUID },
> +	{  }
> +};

Can you share the bmof buffer describing this WMI device? Currently, both WMI GUIDs seem
to be independent from each other, so it would make more sense to create two separate WMI drivers.
The first driver would continue to handle the hotkey events, while the second drivers allows to change
the power mode.

Thanks,
Armin Wolf

> +
> +static struct wmi_driver inspur_wmi_driver = {
> +	.driver = {
> +		.name = "inspur-wmi",
> +		.dev_groups = inspur_wmi_groups,
> +	},
> +	.id_table = inspur_wmi_id_table,
> +	.probe = inspur_wmi_probe,
> +	.notify = inspur_wmi_notify,
> +	.remove = inspur_wmi_remove,
> +};
> +
> +module_wmi_driver(inspur_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <aichao@...inos.cn>");
> +MODULE_DESCRIPTION("Inspur WMI hotkeys");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists