Message-Id: <BLFOYQ.DC67MOSNFFNW2@ljones.dev>
Date:   Tue, 31 Aug 2021 11:51:11 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Barnabás Pőcze <pobrn@...tonmail.com>
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

On Mon, Aug 30 2021 at 21:28:18 +0000, Barnabás Pőcze 
<pobrn@...tonmail.com> 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@...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.

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

Powered by blists - more mailing lists