Message-Id: <LL7PYQ.O4HURM6VWLGE3@ljones.dev>
Date:   Tue, 31 Aug 2021 21:56:09 +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

Additionally to the above I've taken in to account feedback for v7 
patch, and cleaned up plus tidied a few things.

I'll again attach for quick look over before I submit next patch as 
full review. I am thinking that this version (v9 here) is the proper 
and expected outcome.

Cheers,
Luke

On Tue, Aug 31 2021 at 20:58:49 +1200, Luke Jones <luke@...nes.dev> 
wrote:
> Hi Barnabás,
> 
> I did another refactor using hwmon_device_register_with_info() and 
> HWMON_CHANNEL_INFO(). I'm unsure if this is what you were looking for 
> so I'm going to attach the patch instead of submitting as a V8 for 
> now.
> 
> My main concern as that the use of the above removes the 
> pwm1_auto_point1_pwm + pwm1_auto_point1_temp format and gives two 
> hwmon<num>, one per cpu/gpu fan with:
> 
> device power
> fan1_input subsystem
> fan2_input temp1_input
> fan3_input temp2_input
> fan4_input temp3_input
> fan5_input temp4_input
> fan6_input temp5_input
> fan7_input temp6_input
> fan8_input temp7_input
> in0_enable temp8_input
> name uevent
> 
> cat -p /sys/devices/platform/asus-nb-wmi/hwmon/hwmon7/name
> asus_cpu_fan_custom_curve
> 
> I've named the root name of each as descriptive as possible to convey 
> exactly what each is
> 
> Oh and `sensors` now shows:
> 
> asus_cpu_fan_curve-isa-0000
> Adapter: ISA adapter
> fan1: 8 RPM
> fan2: 10 RPM
> fan3: 18 RPM
> fan4: 20 RPM
> fan5: 28 RPM
> fan6: 34 RPM
> fan7: 44 RPM
> fan8: 56 RPM
> temp1: +0.0°C
> temp2: +0.1°C
> temp3: +0.1°C
> temp4: +0.1°C
> temp5: +0.1°C
> temp6: +0.1°C
> temp7: +0.1°C
> temp8: +0.1°C
> 
> 
> > 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`.
> 
> So when you say this, did you mean *just* for the pwmX_enable 
> attributes?
> 
> 
> 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.
>> 
>> 
>>>  + * 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
> 


View attachment "v9-0001-asus-wmi-Add-support-for-custom-fan-curves.patch" of type "text/x-patch" (22724 bytes)

Powered by blists - more mailing lists