Message-ID: <1o94oJFiia_xvrFrSPI_zG1Xfv4FAlJNY96x39rg-zX3-3N5Czw4KmTiJtzCy1So7kYXLu0FTkRkmwUUudeuTyLHSsx5sJGhfsZaYrXKEic=@protonmail.com>
Date:   Mon, 30 Aug 2021 21:28:18 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     "Luke D. Jones" <luke@...nes.dev>
Cc:     linux-kernel@...r.kernel.org, hdegoede@...hat.com,
        linux@...ck-us.net, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v7] asus-wmi: Add support for custom fan curves

Hi


2021. augusztus 30., hétfő 13:31 keltezéssel, Luke D. Jones írta:
> Add support for custom fan curves found on some ASUS ROG laptops.
>
> These laptops have the ability to set a custom curve for the CPU
> and GPU fans via an ACPI method call. This patch enables this,
> additionally enabling custom fan curves per-profile, where profile
> here means each of the 3 levels of "throttle_thermal_policy".
>
> This patch adds two blocks of attributes to the hwmon sysfs,
> 1 block each for CPU and GPU fans.
>
> When the user switches profiles the associated curve data for that
> profile is then show/store enabled to allow users to rotate through
> the profiles and set a fan curve for each profile which then
> activates on profile switch if enabled.
>
> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 568 ++++++++++++++++++++-
>  include/linux/platform_data/x86/asus-wmi.h |   2 +
>  2 files changed, 566 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index cc5811844012..b594c2475034 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> [...]
> +/*
> + * Returns as an error if the method output is not a buffer. Typically this

It seems to me it will simply leave the output buffer uninitialized if something
other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and return 0.


> + * means that the method called is unsupported.
> + */
> +static int asus_wmi_evaluate_method_buf(u32 method_id,
> +		u32 arg0, u32 arg1, u8 *ret_buffer)
> +{
> +	struct bios_args args = {
> +		.arg0 = arg0,
> +		.arg1 = arg1,
> +		.arg2 = 0,
> +	};
> +	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	union acpi_object *obj;
> +	u32 int_tmp = 0;
> +
> +	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
> +				     &input, &output);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	obj = (union acpi_object *)output.pointer;
> +
> +	if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +		int_tmp = (u32) obj->integer.value;
> +		if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
> +			return -ENODEV;
> +		return int_tmp;

Is anything known about the possible values? You are later
using it as if it was an errno (e.g. in `custom_fan_check_present()`).

And `obj` is leaked in both of the previous two returns.


> +	}
> +
> +	if (obj && obj->type == ACPI_TYPE_BUFFER)
> +		memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);

I would suggest you add a "size_t size" argument to this function, and
return -ENOSPC/-ENODATA depending on whether the returned buffer is too
big/small. Maybe return -ENODATA if `obj` is NULL, too.


> +
> +	kfree(obj);
> +
> +	return 0;
> +}
> [...]
> +static ssize_t fan_curve_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	int value;
> +
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	int pwm = nr & FAN_CURVE_PWM_MASK;
> +
> +	if (pwm)
> +		value = 255 * data->percents[index] / 100;
> +	else
> +		value = data->temps[index];
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", value);

sysfs_emit()


> +}
> +
> +/*
> + * "dev" is the related WMI method such as ASUS_WMI_DEVID_CPU_FAN_CURVE.
> + */
> +static int fan_curve_write(struct asus_wmi *asus, u32 dev,
> +					struct fan_curve_data *data)
> +{
> +	int ret, i, shift = 0;
> +	u32 arg1, arg2, arg3, arg4;
> +
> +	arg1 = arg2 = arg3 = arg4 = 0;
> +
> +	for (i = 0; i < FAN_CURVE_POINTS / 2; i++) {
> +		arg1 += data->temps[i] << shift;
> +		arg2 += data->temps[i + 4] << shift;
> +		arg3 += data->percents[0] << shift;
> +		arg4 += data->percents[i + 4] << shift;
> +		shift += 8;
> +	}
> +
> +	return asus_wmi_evaluate_method5(ASUS_WMI_METHODID_DEVS, dev,
> +					arg1, arg2, arg3, arg4, &ret);
> +}
> +
> +/*
> + * Called only by throttle_thermal_policy_write()
> + */

Am I correct in thinking that the firmware does not actually
support specifying fan curves for each mode, only a single one,
and the fan curve switching is done by this driver when
the performance mode is changed?


> +static int fan_curve_write_data(struct asus_wmi *asus)
> +{
> +	struct fan_curve_data *cpu;
> +	struct fan_curve_data *gpu;
> +	int err, mode;
> +
> +	mode = asus->throttle_thermal_policy_mode;
> +	cpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_CPU];
> +	gpu = &asus->throttle_fan_curves[mode][FAN_CURVE_DEV_GPU];
> +
> +	if (cpu->enabled) {
> +		err = fan_curve_write(asus, ASUS_WMI_DEVID_CPU_FAN_CURVE, cpu);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (gpu->enabled) {
> +		err = fan_curve_write(asus, ASUS_WMI_DEVID_GPU_FAN_CURVE, gpu);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> [...]
> +static ssize_t fan_curve_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	u8 value, old_value;
> +	int err;
> +
> +	int index = to_sensor_dev_attr_2(attr)->index;
> +	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	int pwm = nr & FAN_CURVE_PWM_MASK;
> +
> +	err = kstrtou8(buf, 10, &value);
> +	if (err < 0)
> +		return err;
> +
> +	if (pwm) {
> +		old_value = data->percents[index];
> +		data->percents[index] = 100 * value / 255;
> +	} else {
> +		old_value = data->temps[index];
> +		data->temps[index] = value;
> +	}
> +	/*
> +	 * The check here forces writing a curve graph in reverse,
> +	 * from highest to lowest.
> +	 */
> +	err = fan_curve_verify(data);
> +	if (err) {
> +		if (pwm) {
> +			dev_err(dev, "a fan curve percentage was higher than the next in sequence\n");
> +			data->percents[index] = old_value;
> +		} else {
> +			dev_err(dev, "a fan curve temperature was higher than the next in sequence\n");
> +			data->temps[index] = old_value;
> +		}
> +		return err;
> +	}

Are such sequences rejected by the firmware itself?
Or is this just an extra layer of protection?


> +
> +	return count;
> +}
> +
> +static ssize_t fan_curve_enable_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", data->enabled);

sysfs_emit()


> +}
> +
> +static ssize_t fan_curve_enable_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct fan_curve_data *data = fan_curve_attr_data_select(dev, attr);
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	bool value;
> +	int err;
> +
> +	err = kstrtobool(buf, &value);
> +	if (err < 0)
> +		return err;
> +
> +	data->enabled = value;
> +	throttle_thermal_policy_write(asus);
> +
> +	return count;
> +}
> +
> +/* CPU */
> +// TODO: enable
> +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, fan_curve_enable,
> +				FAN_CURVE_DEV_CPU);

FYI, the pwmX_enable attributes can be created by the hwmon
subsystem itself if you use [devm_]hwmon_device_register_with_info()
with appropriately populated `struct hwmon_chip_info`.


> [...]
> +static const struct attribute_group fan_curve_attribute_group = {
> +	.is_visible = fan_curve_sysfs_is_visible,
> +	.attrs = fan_curve_attributes

Small thing, but it is customary to put commas after non-terminating
entries in initializers / enum definitions.


> +};
> +__ATTRIBUTE_GROUPS(fan_curve_attribute);
> +
> +static int asus_wmi_fan_curve_init(struct asus_wmi *asus)
> +{
> +	struct device *dev = &asus->platform_device->dev;
> +	struct device *hwmon;
> +
> +	hwmon = devm_hwmon_device_register_with_groups(dev, "asus", asus,
> +						fan_curve_attribute_groups);
> +
> +	if (IS_ERR(hwmon)) {
> +		pr_err("Could not register asus fan_curve device\n");

I think `dev_err()` would be better.


> +		return PTR_ERR(hwmon);
> +	}
> +
> +	return 0;
> +}
> [...]
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 17dc5cb6f3f2..a571b47ff362 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -77,6 +77,8 @@
>  #define ASUS_WMI_DEVID_THERMAL_CTRL	0x00110011
>  #define ASUS_WMI_DEVID_FAN_CTRL		0x00110012 /* deprecated */
>  #define ASUS_WMI_DEVID_CPU_FAN_CTRL	0x00110013
> +#define ASUS_WMI_DEVID_CPU_FAN_CURVE	0x00110024
> +#define ASUS_WMI_DEVID_GPU_FAN_CURVE	0x00110025
>
>  /* Power */
>  #define ASUS_WMI_DEVID_PROCESSOR_STATE	0x00120012
> --
> 2.31.1


Best regards,
Barnabás Pőcze

Powered by blists - more mailing lists