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