Re: [PATCH v7] asus-wmi: Add support for custom fan curves

From: Luke Jones
Date: Mon Aug 30 2021 - 19:51:36 EST

Next message: Mike Kravetz: "Re: [v2] hugetlbfs: Extend the definition of hugepages parameter to support node allocation"
Previous message: Kees Cook: "Re: linux-next: build warning after merge of the kspp tree"
In reply to: Barnabás Pőcze: "Re: [PATCH v7] asus-wmi: Add support for custom fan curves"
Next in thread: Luke Jones: "Re: [PATCH v7] asus-wmi: Add support for custom fan curves"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi

On Mon, Aug 30 2021 at 21:28:18 +0000, Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:

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@xxxxxxxxxx>
---
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.

Oops, see below inline reply:

+ * 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.

The return for the method we're calling in this patch returns 0 if the input arg has no match.

+ }
+
+ 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.

Got it. So something like this would be suitable?

if (obj && obj->type == ACPI_TYPE_BUFFER)
if (obj->buffer.length < size)
err = -ENOSPC;
if (!obj->buffer.length)
err = -ENODATA;
if (err) {
kfree(obj);
return err;
}
memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
}

if (obj && obj->type == ACPI_TYPE_INTEGER)
int_tmp = (u32) obj->integer.value;

kfree(obj);

if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
return -ENODEV;

/* There is at least one method that returns a 0 with no buffer */
if (obj == NULL || int_tmp == 0)
return -ENODATA;

return 0;

+
+ 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()

Sorry, I usually go by existing code as examples. I might submit a patch after this one that cleans up some of the other parts.

+}
+
+/*
+ * "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?

I'm not 100% certain on this. The WMI method 0x00110024 takes an arg 0,1,2 which then returns some factory stored fan profiles, these fit the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2 swapped.

Looking at the SET part, it seems to write to a different location than where the GET is fetching information.

Because of the fact there are three sets of curves to get, I thought it would be good for users to be able to set per profile. I don't think the set is retained in acpi if the profile is switched.

Do you think it would be best to not have the ability to store per profile in kernel? How would I choose which profile get to populate the initial data with if so?

+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?

Not really sure here. The DSDT I have is the following which looks like it writes as is. I don't really want to be responsible for allowing a reverse curve to be set.

// SET
If ((IIA0 == 0x00110024))
{
Return (^^PCI0.SBRG.EC0.SUFC (IIA1, IIA2, IIA3, IIA4, 0x40))
}

If ((IIA0 == 0x00110025))
{
Return (^^PCI0.SBRG.EC0.SUFC (IIA1, IIA2, IIA3, IIA4, 0x44))
}

Method (SUFC, 5, NotSerialized)
{
Name (DUBF, Buffer (0x10)
{
/* 0000 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // ........
/* 0008 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 // ........
})
Name (UFC0, Buffer (One)
{
0x00 // .
})
DUBF [Zero] = (Arg0 >> Zero)
DUBF [One] = (Arg0 >> 0x08)
DUBF [0x02] = (Arg0 >> 0x10)
DUBF [0x03] = (Arg0 >> 0x18)
DUBF [0x04] = (Arg1 >> Zero)
DUBF [0x05] = (Arg1 >> 0x08)
DUBF [0x06] = (Arg1 >> 0x10)
DUBF [0x07] = (Arg1 >> 0x18)
DUBF [0x08] = (Arg2 >> Zero)
DUBF [0x09] = (Arg2 >> 0x08)
DUBF [0x0A] = (Arg2 >> 0x10)
DUBF [0x0B] = (Arg2 >> 0x18)
DUBF [0x0C] = (Arg3 >> Zero)
DUBF [0x0D] = (Arg3 >> 0x08)
DUBF [0x0E] = (Arg3 >> 0x10)
DUBF [0x0F] = (Arg3 >> 0x18)
UFC0 [Zero] = Arg4
WEBC (0x20, One, UFC0)
Return (WEBC (0x22, 0x10, DUBF))
}

+
+ 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()

Ack

+}
+
+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`.

Thanks, I was hoping for a method liek this and had found the same last night but was unclear on how I could retain the data I need from the following?

static SENSOR_DEVICE_ATTR_2_RW(pwm2_auto_point1_pwm, fan_curve,
FAN_CURVE_DEV_GPU | FAN_CURVE_PWM_MASK, 0);

[...]
+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.

Ack

+};
+__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.

Ack. Sorry, I have been trying to keep using dev_err.

+ 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

Next message: Mike Kravetz: "Re: [v2] hugetlbfs: Extend the definition of hugepages parameter to support node allocation"
Previous message: Kees Cook: "Re: linux-next: build warning after merge of the kspp tree"
In reply to: Barnabás Pőcze: "Re: [PATCH v7] asus-wmi: Add support for custom fan curves"
Next in thread: Luke Jones: "Re: [PATCH v7] asus-wmi: Add support for custom fan curves"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]