Re: [PATCH v2 1/2] i8k: Autodetect maximal fan speed and fan RPM multiplier

From: Guenter Roeck
Date: Sat Dec 20 2014 - 13:39:08 EST

On 12/19/2014 10:04 AM, Pali RohÃr wrote:
This patch adds new function i8k_get_fan_nominal_speed() for doing SMM call
which will return nominal fan RPM for specified fan speed. It returns nominal
RPM value at which fan operate when speed (0, 1, 2, 3) is set. It looks like
RPM value is not accurate, but still provides very useful information.

First it can be used to validate if certain fan speed could be accepted by SMM
for setting fan speed and we can use this routine to detect maximal fan speed.

Second it returns RPM value, so we can check if value looks correct with
multiplier 30 or multiplier 1 (until now only these two multiplier were used).
If RPM value with multiplier 30 is too high, then multiplier 1 is used.

In case when SMM reports that new function is not supported we will fallback
to old hardcoded values. Maximal fan speed would be 2 and RPM multiplier 30.

Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
Tested-By: Pali RohÃr <pali.rohar@xxxxxxxxx>
Tested-By: Steven Honeyman <stevenhoneyman@xxxxxxxxx>
Tested-By: Valdis Kletnieks <valdis.kletnieks@xxxxxx>
---
drivers/char/i8k.c | 126 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 105 insertions(+), 21 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 48d701c..094a6b8 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
* Hwmon integration:
* Copyright (C) 2011 Jean Delvare <jdelvare@xxxxxxx>
* Copyright (C) 2013, 2014 Guenter Roeck <linux@xxxxxxxxxxxx>
+ * Copyright (C) 2014 Pali RohÃr <pali.rohar@xxxxxxxxx>
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the
@@ -42,12 +43,14 @@
#define I8K_SMM_SET_FAN 0x01a3
#define I8K_SMM_GET_FAN 0x00a3
#define I8K_SMM_GET_SPEED 0x02a3
+#define I8K_SMM_GET_NOM_SPEED 0x04a3
#define I8K_SMM_GET_TEMP 0x10a3
#define I8K_SMM_GET_TEMP_TYPE 0x11a3
#define I8K_SMM_GET_DELL_SIG1 0xfea3
#define I8K_SMM_GET_DELL_SIG2 0xffa3

-#define I8K_FAN_MULT 30
+#define I8K_FAN_DEFAULT_MULT 30
+#define I8K_FAN_MAX_RPM 30000
#define I8K_MAX_TEMP 127

#define I8K_FN_NONE 0x00
@@ -64,9 +67,9 @@ static DEFINE_MUTEX(i8k_mutex);
static char bios_version[4];
static struct device *i8k_hwmon_dev;
static u32 i8k_hwmon_flags;
-static int i8k_fan_mult;
-static int i8k_pwm_mult;
-static int i8k_fan_max = I8K_FAN_HIGH;
+static int i8k_fan_mult[2];
+static int i8k_pwm_mult[2];
+static int i8k_fan_max[2];

#define I8K_HWMON_HAVE_TEMP1 (1 << 0)
#define I8K_HWMON_HAVE_TEMP2 (1 << 1)
@@ -95,13 +98,13 @@ static bool power_status;
module_param(power_status, bool, 0600);
MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");

-static int fan_mult = I8K_FAN_MULT;
+static int fan_mult;
module_param(fan_mult, int, 0);
-MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
+MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");

-static int fan_max = I8K_FAN_HIGH;
+static int fan_max;
module_param(fan_max, int, 0);
-MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
+MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");

static int i8k_open_fs(struct inode *inode, struct file *file);
static long i8k_ioctl(struct file *, unsigned int, unsigned long);
@@ -271,8 +274,25 @@ static int i8k_get_fan_speed(int fan)
{
struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };

+ if (fan < 0 || fan >= ARRAY_SIZE(i8k_fan_mult))
+ return -EINVAL;
+
regs.ebx = fan & 0xff;
- return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+ return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult[fan];
+}
+
+/*
+ * Read the fan nominal rpm for specific fan speed.
+ */
+static int i8k_get_fan_nominal_speed(int fan, int speed)
+{
+ struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
+
+ if (fan < 0 || fan >= ARRAY_SIZE(i8k_fan_mult))
+ return -EINVAL;
+
+ regs.ebx = (fan & 0xff) | (speed << 8);
+ return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult[fan];
}

/*
@@ -282,9 +302,12 @@ static int i8k_set_fan(int fan, int speed)
{
struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };

- speed = (speed < 0) ? 0 : ((speed > i8k_fan_max) ? i8k_fan_max : speed);
- regs.ebx = (fan & 0xff) | (speed << 8);
+ if (speed < 0)
+ speed = 0;
+ if (fan >= 0 && fan < ARRAY_SIZE(i8k_fan_max) && speed > i8k_fan_max[fan])
+ speed = i8k_fan_max[fan];

+ regs.ebx = (fan & 0xff) | (speed << 8);
return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
}

@@ -562,7 +585,8 @@ static ssize_t i8k_hwmon_show_pwm(struct device *dev,
status = i8k_get_fan_status(index);
if (status < 0)
return -EIO;
- return sprintf(buf, "%d\n", clamp_val(status * i8k_pwm_mult, 0, 255));
+ return sprintf(buf, "%d\n", clamp_val(status * i8k_pwm_mult[index],
+ 0, 255));
}

static ssize_t i8k_hwmon_set_pwm(struct device *dev,
@@ -576,7 +600,8 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
err = kstrtoul(buf, 10, &val);
if (err)
return err;
- val = clamp_val(DIV_ROUND_CLOSEST(val, i8k_pwm_mult), 0, i8k_fan_max);
+ val = clamp_val(DIV_ROUND_CLOSEST(val, i8k_pwm_mult[index]), 0,
+ i8k_fan_max[index]);

mutex_lock(&i8k_mutex);
err = i8k_set_fan(index, val);
@@ -854,7 +879,9 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
*/
static int __init i8k_probe(void)
{
+ const struct i8k_config_data *conf;
const struct dmi_system_id *id;
+ int fan, val, ret;

/*
* Get DMI information
@@ -883,18 +910,75 @@ static int __init i8k_probe(void)
return -ENODEV;
}

- i8k_fan_mult = fan_mult;
- i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */
+ /*
+ * Autodetect fan multiplier and maximal fan speed from dmi config
+ * Values specified in module parameters override values from dmi
+ */
id = dmi_first_match(i8k_dmi_table);
if (id && id->driver_data) {
- const struct i8k_config_data *conf = id->driver_data;
+ conf = id->driver_data;
+ if (fan_mult <= 0 && conf->fan_mult > 0)
+ fan_mult = conf->fan_mult;
+ if (fan_max <= 0 && conf->fan_max > 0)
+ fan_max = conf->fan_max;
+ }
+
+ if (fan_mult <= 0) {

Wonder what to do in the < 0 case. It might be better to make the variable
and the module parameter an unsigned int to cover that case, since a negative
multiplier doesn't really make sense. Same for fan_max.

+ /*
+ * Autodetect fan multiplier for each fan based on nominal rpm
+ * First set default fan multiplier for each fan
+ * And if it reports rpm value too high then set multiplier to 1
+ *
+ * Try also setting multiplier from current rpm, but this will
+ * work only in case when fan is not turned off. It is better
+ * then nothing for machines which does not support nominal rpm
+ * SMM function.
+ */
+ for (fan = 0; fan < ARRAY_SIZE(i8k_fan_mult); ++fan) {
+ i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
+ if (i8k_get_fan_speed(fan) > I8K_FAN_MAX_RPM) {
+ i8k_fan_mult[fan] = 1;
+ continue;
+ }

What if i8k_get_fan_speed(fan) returns an error ? Doesn't that imply that the fan
does not exist, and that you would not need the second loop in the first place ?

+ for (val = 0; val < 256; ++val) {
+ ret = i8k_get_fan_nominal_speed(fan, val);
+ if (ret > I8K_FAN_MAX_RPM) {
+ i8k_fan_mult[fan] = 1;
+ break;
+ } else if (ret < 0) {
+ break;
+ }

Traditionally error checks come first.

+ }
+ }
+ } else {
+ /* Fan multiplier was specified in module param or in dmi */
+ for (fan = 0; fan < ARRAY_SIZE(i8k_fan_mult); ++fan)
+ i8k_fan_mult[fan] = fan_mult;
+ }

- if (fan_mult == I8K_FAN_MULT && conf->fan_mult)
- i8k_fan_mult = conf->fan_mult;
- if (fan_max == I8K_FAN_HIGH && conf->fan_max)
- i8k_fan_max = conf->fan_max;
+ if (fan_max <= 0) {
+ /*
+ * Autodetect maximal fan speed value for each fan
+ * Speed value is valid if i8k_get_fan_nominal_speed() not fail
+ */
+ for (fan = 0; fan < ARRAY_SIZE(i8k_fan_max); ++fan) {
+ for (val = 0; val < 256; ++val) {
+ if (i8k_get_fan_nominal_speed(fan, val) < 0)
+ break;
+ }
+ --val; /* set last value which not failed */
+ if (val <= 0) /* Must not be 0 (and non negative) */
+ val = I8K_FAN_HIGH;
+ i8k_fan_max[fan] = val;

I kind of dislike that you are (at least potentially) looping through all
the nominal speeds twice; not sure how long that will delay the boot process.
I don't know if or how that can be solved easily, though. Either case,
I would prefer something like
i8k_fan_max = I8K_FAN_HIGH;
for (val = 0; val < 256; ++val) {
if (i8k_get_fan_nominal_speed(fan, val) < 0)
break;
i8k_fan_max = val;
}

since that would take care of all the meddling at the end.

+ }
+ } else {
+ /* Maximal fan speed was specified in module param or in dmi */
+ for (fan = 0; fan < ARRAY_SIZE(i8k_fan_max); ++fan)
+ i8k_fan_max[fan] = fan_max;
}

Overall I think it would make sense to move the auto-detection code to a separate
function. This is getting a bit large to have it all in a single function.

Thanks,
Guenter

- i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
+
+ for (fan = 0; fan < ARRAY_SIZE(i8k_pwm_mult); ++fan)
+ i8k_pwm_mult[fan] = DIV_ROUND_UP(255, i8k_fan_max[fan]);

return 0;
}


--
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/