Re: [RFC 2/2] asus-wmi: add fan control

From: Corentin Chary
Date: Sat May 02 2015 - 08:37:27 EST

Next message: Peter Zijlstra: "Re: [PATCH] x86: Optimize variable_test_bit()"
Previous message: Gabriele Mazzotta: "[PATCH] iio: kfifo: Set update_needed to false only if a buffer was allocated"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Mostly comments about the code, I don't have anything against the idea.

On Wed, Apr 22, 2015 at 3:12 PM, Kast Bernd <kastbernd@xxxxxx> wrote:
> This patch is partially based on Felipe Contrera's earlier patch, that
> was discussed here: https://lkml.org/lkml/2013/10/8/800
> Some problems of that patch are fixed, now:
>
> 1) The main downside of the earlier patch seemed to be the use of
> virt_to_phys, thus an acpi-version of that function is used now.
> (provided by the first patch of this patchset)
>
> 2) random memory corruption occurred on my notebook, thus DMA-able memory
> is allocated now, which solves this problem
>
> 3) hwmon interface is used instead of the thermal interface, as a
> hwmon device is already set up by this driver and seemed more
> appropriate than the thermal interface
>
> 4) Calling the ACPI-functions was modularized thus it's possible to call
> some multifunctions easily, now (by using
> asus_wmi_evaluate_method_agfn).
>
> Unfortunately the WMI doesn't support controlling both fans on
> a dual-fan notebooks because of an restriction in the acpi-method
> "SFNS", that is callable through the wmi. If "SFNV" would be called
> direcly even dual fan configurations could be controlled, but not by using
> wmi.
>
> Speed readings only work on auto-mode, thus "-1" will be reported in
> manual mode.
> Additionally the speed readings are reported as hundreds of RPM thus
> they are not too precize.
>
> This patch is tested only on one notebook (N551JK) but a similar module,
> that contained some code to try to control the second fan also, was
> reported to work on an UX32VD, at least for the first fan.
>
> As Felipe already mentioned the low-level functions are described here:
> http://forum.notebookreview.com/threads/fan-control-on-asus-prime-ux31-ux31a-ux32a-ux32vd.705656/
>
> Signed-off-by: Kast Bernd <kastbernd@xxxxxx>
> ---
> drivers/platform/x86/asus-wmi.c | 297 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 277 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 7543a56..b16658a 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -78,6 +78,7 @@ MODULE_LICENSE("GPL");
> #define ASUS_WMI_METHODID_GPID 0x44495047 /* Get Panel ID?? (Resol) */
> #define ASUS_WMI_METHODID_QMOD 0x444F4D51 /* Quiet MODe */
> #define ASUS_WMI_METHODID_SPLV 0x4C425053 /* Set Panel Light Value */
> +#define ASUS_WMI_METHODID_AGFN 0x4E464741 /* ?? FaN?*/
> #define ASUS_WMI_METHODID_SFUN 0x4E554653 /* FUNCtionalities */
> #define ASUS_WMI_METHODID_SDSP 0x50534453 /* Set DiSPlay output */
> #define ASUS_WMI_METHODID_GDSP 0x50534447 /* Get DiSPlay output */
> @@ -150,11 +151,27 @@ MODULE_LICENSE("GPL");
> #define ASUS_WMI_DSTS_BRIGHTNESS_MASK 0x000000FF
> #define ASUS_WMI_DSTS_MAX_BRIGTH_MASK 0x0000FF00
>
> +#define ASUS_FAN_DESC "cpu_fan"
> +
> struct bios_args {
> u32 arg0;
> u32 arg1;
> } __packed;
>
> +struct agfn_args {
> + u16 mfun;
> + u16 sfun;
> + u16 len;
> + u8 stas;
> + u8 err;
> +} __packed;
> +
> +struct fan_args {
> + struct agfn_args agfn;
> + u8 fan;
> + u32 speed;
> +} __packed;
> +
> /*
> * <platform>/ - debugfs root directory
> * dev_id - current dev_id
> @@ -204,6 +221,10 @@ struct asus_wmi {
> struct asus_rfkill gps;
> struct asus_rfkill uwb;
>
> + int asus_hwmon_pwm;
> + int asus_hwmon_num_fans;
> + int asus_hwmon_fan_manual_mode;
> +
> struct hotplug_slot *hotplug_slot;
> struct mutex hotplug_lock;
> struct mutex wmi_lock;
> @@ -294,6 +315,39 @@ exit:
> return 0;
> }
>
> +static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
> +{
> + struct acpi_buffer input;
> + u32 status;
> + u64 phys_addr;
> + u32 retval;
> +
> + /* copy to dma capable address */
> + input.pointer = kzalloc(args.length, GFP_DMA | GFP_KERNEL);

Maybe add a comment here to explain why GFP_DMA.

> + input.length = args.length;
> + if (!input.pointer)
> + return -ENOMEM;
> +
> + if (acpi_os_get_physical_address(input.pointer, &phys_addr) != AE_OK)
> + goto fail;
> +
> + memcpy(input.pointer, args.pointer, args.length);
> +
> + status = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN, phys_addr, 0,
> + &retval);
> + if (status < 0)
> + goto fail;
> +
> + memcpy(args.pointer, input.pointer, args.length);
> +
> + kfree(input.pointer);
> + return retval;
> +
> +fail:
> + kfree(input.pointer);
> + return -1;
> +}
> +
> static int asus_wmi_get_devstate(struct asus_wmi *asus, u32 dev_id, u32 *retval)
> {
> return asus_wmi_evaluate_method(asus->dsts_id, dev_id, 0, retval);
> @@ -1022,35 +1076,187 @@ exit:
> /*
> * Hwmon device
> */
> -static ssize_t asus_hwmon_pwm1(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> +static int asus_hwmon_agfn_fan_speed(struct asus_wmi *asus, int write, int fan,
> + int *speed)

write could be a bool here. Even if it's a bool you may have one
function to read and one to write
because it makes it easier to understand what the function is doing.

> +{
> + int status;
> +
> + struct fan_args args = {
> + .agfn.len = sizeof(args),
> + .agfn.mfun = 0x13,
> + .agfn.sfun = write ? 0x07 : 0x06,

Could there be a comment, maybe in the structure, explaining what mfun
and sfun are ? And maybe add these values as constants.

> + .fan = fan,
> + .speed = speed ? write ? *speed : 0 : 0,
> + };
> +
> + struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +
> + status = asus_wmi_evaluate_method_agfn(input);
> +
> + if (status || args.agfn.err)
> + return -1;
> +
> + if (!speed)
> + return 0;
> +
> + if (write) {
> + if (fan == 1 || fan == 2)
> + asus->asus_hwmon_pwm = fan > 0 ? *speed : -1;

Add some kind of logging if fan is not 1 or 2 ?

> + } else {
> + *speed = args.speed;
> + }
> +
> + return 0;
> +}
> +
> +static int asus_hwmon_get_fan_number(struct asus_wmi *asus, int *num_fans)
> +{
> + int status;
> + int speed = 0;
> +
> + *num_fans = 0;
> +
> + status = asus_hwmon_agfn_fan_speed(asus, 0, 1, &speed);
> + if (status)
> + return 0;

Add a comment that you just check if there is enough to control one
fan, and if yes assume that there is one.

> + *num_fans = 1;
> + return 0;
> +}
> +
> +static int asus_hwmon_fan_set_auto(struct asus_wmi *asus)
> {
> + int status;
> +
> + status = asus_hwmon_agfn_fan_speed(asus, 1, 0, 0);
> + if (!status) {
> + asus->asus_hwmon_fan_manual_mode = 0;
> + return 0;
> + } else {
> + return -1;

Return proper error codes here (and other places)

> + }
> +}
> +
> +static int asus_hwmon_fan_rpm_show(struct device *dev, int fan)
> +{
> + int value;
> + int state;
> struct asus_wmi *asus = dev_get_drvdata(dev);
> - u32 value;
> +
> + /* no speed readable on manual mode */
> + if (asus->asus_hwmon_fan_manual_mode) {
> + value = -1;
> + } else {
> + state = asus_hwmon_agfn_fan_speed(asus, 0, fan+1, &value);
> + if (state) {
> + value = -1;
> + pr_warn("reading fan speed failed: %d\n", state);
> + }
> + }
> + return value;
> +}
> +
> +static void asus_hwmon_pwm_show(struct asus_wmi *asus, int fan, int *value)
> +{
> int err;
>
> - err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, &value);
> + if (asus->asus_hwmon_pwm >= 0) {
> + *value = asus->asus_hwmon_pwm;
> + return;
> + }
>
> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_CTRL, value);
> if (err < 0)
> - return err;
> -
> - value &= 0xFF;
> + return;
>
> - if (value == 1) /* Low Speed */
> - value = 85;
> - else if (value == 2)
> - value = 170;
> - else if (value == 3)
> - value = 255;
> - else if (value != 0) {
> - pr_err("Unknown fan speed %#x\n", value);
> - value = -1;
> + *value &= 0xFF;
> +
> + if (*value == 1) /* Low Speed */
> + *value = 85;
> + else if (*value == 2)
> + *value = 170;
> + else if (*value == 3)
> + *value = 255;
> + else if (*value) {
> + pr_err("Unknown fan speed %#x\n", *value);
> + *value = -1;
> }
> +}
>
> +static ssize_t asus_hwmon_pwm1_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> + int value;
> +
> + asus_hwmon_pwm_show(asus, 0, &value);
> return sprintf(buf, "%d\n", value);
> }
>
> +static ssize_t asus_hwmon_pwm1_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count) {
> + int value;
> + int state;
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + kstrtouint(buf, 10, &value);
> + if (value > 255)
> + value = 255;

I think there is a function to clamp values somewhere already.

> +
> + state = asus_hwmon_agfn_fan_speed(asus, 1, 1, &value);
> + if (state)
> + pr_warn("Setting fan speed failed: %d\n", state);
> + else
> + asus->asus_hwmon_fan_manual_mode = 1;
> +
> + return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_rpm_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int value = asus_hwmon_fan_rpm_show(dev, 0);
> +
> + return sprintf(buf, "%d\n", value < 0 ? value : value*100);
> +
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", asus->asus_hwmon_fan_manual_mode);
> +}
> +
> +static ssize_t asus_hwmon_cur_control_state_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int state;
> + int status;
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + kstrtouint(buf, 10, &state);
> + if (state == 0 || state == 2)

state could be a constant here to make it more obvious what it is.

> + status = asus_hwmon_fan_set_auto(asus);
> + else if (state == 1)
> + asus->asus_hwmon_fan_manual_mode = state;
> +
> + return count;
> +}
> +
> +static ssize_t asus_hwmon_fan1_label_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%s\n", ASUS_FAN_DESC);
> +}
> +
> static ssize_t asus_hwmon_temp1(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -1069,11 +1275,24 @@ static ssize_t asus_hwmon_temp1(struct device *dev,
> return sprintf(buf, "%d\n", value);
> }
>
> -static DEVICE_ATTR(pwm1, S_IRUGO, asus_hwmon_pwm1, NULL);
> +/* Fan1 */
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, asus_hwmon_pwm1_show,
> + asus_hwmon_pwm1_store);
> +static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> + asus_hwmon_cur_control_state_show,
> + asus_hwmon_cur_control_state_store);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, asus_hwmon_fan1_rpm_show, NULL);
> +static DEVICE_ATTR(fan1_label, S_IRUGO, asus_hwmon_fan1_label_show, NULL);
> +
> +/* Temperature */
> static DEVICE_ATTR(temp1_input, S_IRUGO, asus_hwmon_temp1, NULL);
>
> static struct attribute *hwmon_attributes[] = {
> &dev_attr_pwm1.attr,
> + &dev_attr_pwm1_enable.attr,
> + &dev_attr_fan1_input.attr,
> + &dev_attr_fan1_label.attr,
> +
> &dev_attr_temp1_input.attr,
> NULL
> };
> @@ -1086,6 +1305,7 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> struct asus_wmi *asus = platform_get_drvdata(pdev);
> bool ok = true;
> int dev_id = -1;
> + int fan_attr = -1;
> u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
>
> if (attr == &dev_attr_pwm1.attr)
> @@ -1093,10 +1313,18 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> else if (attr == &dev_attr_temp1_input.attr)
> dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
>
> +
> + if (attr == &dev_attr_fan1_input.attr
> + || attr == &dev_attr_fan1_label.attr
> + || attr == &dev_attr_pwm1.attr
> + || attr == &dev_attr_pwm1_enable.attr) {
> + fan_attr = 1;
> + }
> +
> if (dev_id != -1) {
> int err = asus_wmi_get_devstate(asus, dev_id, &value);
>
> - if (err < 0)
> + if (err < 0 && fan_attr == -1)
> return 0; /* can't return negative here */
> }
>
> @@ -1112,10 +1340,16 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
> || (!asus->sfun && !(value & ASUS_WMI_DSTS_PRESENCE_BIT)))
> ok = false;
> + else
> + ok = fan_attr <= asus->asus_hwmon_num_fans;
> } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {
> /* If value is zero, something is clearly wrong */
> - if (value == 0)
> + if (!value)
> ok = false;
> + } else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
> + ok = true;
> + } else {
> + ok = false;
> }
>
> return ok ? attr->mode : 0;
> @@ -1723,6 +1957,25 @@ error_debugfs:
> return -ENOMEM;
> }
>
> +static int asus_wmi_fan_init(struct asus_wmi *asus)
> +{
> + int status;
> +
> + asus->asus_hwmon_pwm = -1;
> + asus->asus_hwmon_num_fans = -1;
> + asus->asus_hwmon_fan_manual_mode = 0;
> +
> + status = asus_hwmon_get_fan_number(asus, &asus->asus_hwmon_num_fans);
> + if (status) {
> + asus->asus_hwmon_num_fans = 0;
> + pr_warn("Could not determine number of fans: %d\n", status);
> + return -1;
> + }
> +
> + pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
> + return 0;
> +}
> +
> /*
> * WMI Driver
> */
> @@ -1756,6 +2009,9 @@ static int asus_wmi_add(struct platform_device *pdev)
> if (err)
> goto fail_input;
>
> + err = asus_wmi_fan_init(asus); /* probably no problems on error */
> + asus_hwmon_fan_set_auto(asus);
> +
> err = asus_wmi_hwmon_init(asus);
> if (err)
> goto fail_hwmon;
> @@ -1832,6 +2088,7 @@ static int asus_wmi_remove(struct platform_device *device)
> asus_wmi_rfkill_exit(asus);
> asus_wmi_debugfs_exit(asus);
> asus_wmi_platform_exit(asus);
> + asus_hwmon_fan_set_auto(asus);
>
> kfree(asus);
> return 0;
> --
> 2.3.5
>

--
Corentin Chary
http://xf.iksaif.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Peter Zijlstra: "Re: [PATCH] x86: Optimize variable_test_bit()"
Previous message: Gabriele Mazzotta: "[PATCH] iio: kfifo: Set update_needed to false only if a buffer was allocated"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]