[PATCH 0/3] i8k: Rework fan_mult and fan_max code

[PATCH 0/3] i8k: Rework fan_mult and fan_max code

[Pali Rohár]

This patch series adds autodetection of fan_mult and fan_max.

Pali Rohár (3):
  i8k: cosmetic: distinguish between fan speed and fan rpm
  i8k: Autodetect maximal fan speed and fan RPM multiplier
  i8k: Remove laptop specific config data (fan_mult, fan_max) from
    driver

 drivers/char/i8k.c       |  258 ++++++++++++++++++++++------------------------
 include/uapi/linux/i8k.h |    4 +-
 2 files changed, 129 insertions(+), 133 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] i8k: cosmetic: distinguish between fan speed and fan rpm

[Pali Rohár]

Driver mix speed and rpm. Fan speed is value (0, 1, 2) which is used for
configuring fan. This patch change comments, function names and other
definitions so code should be unambiguous now.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
This patch is cosmetic and does not bring any change to code.
---
 drivers/char/i8k.c |   78 ++++++++++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 48d701c..31e4beb 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -39,9 +39,9 @@
 
 #define I8K_SMM_FN_STATUS	0x0025
 #define I8K_SMM_POWER_STATUS	0x0069
-#define I8K_SMM_SET_FAN		0x01a3
-#define I8K_SMM_GET_FAN		0x00a3
-#define I8K_SMM_GET_SPEED	0x02a3
+#define I8K_SMM_GET_FAN_SPEED	0x00a3
+#define I8K_SMM_SET_FAN_SPEED	0x01a3
+#define I8K_SMM_GET_FAN_RPM	0x02a3
 #define I8K_SMM_GET_TEMP	0x10a3
 #define I8K_SMM_GET_TEMP_TYPE	0x11a3
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
@@ -97,7 +97,7 @@ MODULE_PARM_DESC(power_status, "Report power status in /proc/i8k");
 
 static int fan_mult = I8K_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 rpm with");
 
 static int fan_max = I8K_FAN_HIGH;
 module_param(fan_max, int, 0);
@@ -254,38 +254,38 @@ static int i8k_get_power_status(void)
 }
 
 /*
- * Read the fan status.
+ * Read the fan speed value (status).
  */
-static int i8k_get_fan_status(int fan)
+static int i8k_get_fan_speed(int fan)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_SPEED, };
 
 	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }
 
 /*
- * Read the fan speed in RPM.
+ * Read the fan rpm.
  */
-static int i8k_get_fan_speed(int fan)
+static int i8k_get_fan_rpm(int fan)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
+	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_RPM, };
 
 	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
 }
 
 /*
- * Set the fan speed (off, low, high). Returns the new fan status.
+ * Set the fan speed (off, low, high). Returns the new fan speed.
  */
-static int i8k_set_fan(int fan, int speed)
+static int i8k_set_fan_speed(int fan, int speed)
 {
-	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };
+	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN_SPEED, };
 
 	speed = (speed < 0) ? 0 : ((speed > i8k_fan_max) ? i8k_fan_max : speed);
 	regs.ebx = (fan & 0xff) | (speed << 8);
 
-	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
+	return i8k_smm(&regs) ? : i8k_get_fan_speed(fan);
 }
 
 static int i8k_get_temp_type(int sensor)
@@ -389,14 +389,14 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;
 
-		val = i8k_get_fan_speed(val);
+		val = i8k_get_fan_rpm(val);
 		break;
 
 	case I8K_GET_FAN:
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;
 
-		val = i8k_get_fan_status(val);
+		val = i8k_get_fan_speed(val);
 		break;
 
 	case I8K_SET_FAN:
@@ -409,7 +409,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;
 
-		val = i8k_set_fan(val, speed);
+		val = i8k_set_fan_speed(val, speed);
 		break;
 
 	default:
@@ -457,13 +457,13 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 static int i8k_proc_show(struct seq_file *seq, void *offset)
 {
 	int fn_key, cpu_temp, ac_power;
-	int left_fan, right_fan, left_speed, right_speed;
+	int left_fan, right_fan, left_rpm, right_rpm;
 
 	cpu_temp	= i8k_get_temp(0);			/* 11100 µs */
-	left_fan	= i8k_get_fan_status(I8K_FAN_LEFT);	/*   580 µs */
-	right_fan	= i8k_get_fan_status(I8K_FAN_RIGHT);	/*   580 µs */
-	left_speed	= i8k_get_fan_speed(I8K_FAN_LEFT);	/*   580 µs */
-	right_speed	= i8k_get_fan_speed(I8K_FAN_RIGHT);	/*   580 µs */
+	left_fan	= i8k_get_fan_speed(I8K_FAN_LEFT);	/*   580 µs */
+	right_fan	= i8k_get_fan_speed(I8K_FAN_RIGHT);	/*   580 µs */
+	left_rpm	= i8k_get_fan_rpm(I8K_FAN_LEFT);	/*   580 µs */
+	right_rpm	= i8k_get_fan_rpm(I8K_FAN_RIGHT);	/*   580 µs */
 	fn_key		= i8k_get_fn_status();			/*   750 µs */
 	if (power_status)
 		ac_power = i8k_get_power_status();		/* 14700 µs */
@@ -477,10 +477,10 @@ static int i8k_proc_show(struct seq_file *seq, void *offset)
 	 * 2)  BIOS version
 	 * 3)  BIOS machine ID
 	 * 4)  Cpu temperature
-	 * 5)  Left fan status
-	 * 6)  Right fan status
-	 * 7)  Left fan speed
-	 * 8)  Right fan speed
+	 * 5)  Left fan speed (status)
+	 * 6)  Right fan speed (status)
+	 * 7)  Left fan rpm
+	 * 8)  Right fan rpm
 	 * 9)  AC power
 	 * 10) Fn Key status
 	 */
@@ -489,7 +489,7 @@ static int i8k_proc_show(struct seq_file *seq, void *offset)
 			  bios_version,
 			  i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
 			  cpu_temp,
-			  left_fan, right_fan, left_speed, right_speed,
+			  left_fan, right_fan, left_rpm, right_rpm,
 			  ac_power, fn_key);
 }
 
@@ -544,12 +544,12 @@ static ssize_t i8k_hwmon_show_fan(struct device *dev,
 				  char *buf)
 {
 	int index = to_sensor_dev_attr(devattr)->index;
-	int fan_speed;
+	int fan_rpm;
 
-	fan_speed = i8k_get_fan_speed(index);
-	if (fan_speed < 0)
-		return fan_speed;
-	return sprintf(buf, "%d\n", fan_speed);
+	fan_rpm = i8k_get_fan_rpm(index);
+	if (fan_rpm < 0)
+		return fan_rpm;
+	return sprintf(buf, "%d\n", fan_rpm);
 }
 
 static ssize_t i8k_hwmon_show_pwm(struct device *dev,
@@ -557,12 +557,12 @@ static ssize_t i8k_hwmon_show_pwm(struct device *dev,
 				  char *buf)
 {
 	int index = to_sensor_dev_attr(devattr)->index;
-	int status;
+	int fan_speed;
 
-	status = i8k_get_fan_status(index);
-	if (status < 0)
+	fan_speed = i8k_get_fan_speed(index);
+	if (fan_speed < 0)
 		return -EIO;
-	return sprintf(buf, "%d\n", clamp_val(status * i8k_pwm_mult, 0, 255));
+	return sprintf(buf, "%d\n", clamp_val(fan_speed * i8k_pwm_mult, 0, 255));
 }
 
 static ssize_t i8k_hwmon_set_pwm(struct device *dev,
@@ -579,7 +579,7 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
 	val = clamp_val(DIV_ROUND_CLOSEST(val, i8k_pwm_mult), 0, i8k_fan_max);
 
 	mutex_lock(&i8k_mutex);
-	err = i8k_set_fan(index, val);
+	err = i8k_set_fan_speed(index, val);
 	mutex_unlock(&i8k_mutex);
 
 	return err < 0 ? -EIO : count;
@@ -675,12 +675,12 @@ static int __init i8k_init_hwmon(void)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
 
 	/* Left fan attributes, if left fan is present */
-	err = i8k_get_fan_status(I8K_FAN_LEFT);
+	err = i8k_get_fan_speed(I8K_FAN_LEFT);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
 
 	/* Right fan attributes, if right fan is present */
-	err = i8k_get_fan_status(I8K_FAN_RIGHT);
+	err = i8k_get_fan_speed(I8K_FAN_RIGHT);
 	if (err >= 0)
 		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
 
-- 
1.7.9.5

Re: [PATCH 1/3] i8k: cosmetic: distinguish between fan speed and fan rpm

[Guenter Roeck]

On Tue, Dec 09, 2014 at 09:06:59PM +0100, Pali Rohár wrote:
> Driver mix speed and rpm. Fan speed is value (0, 1, 2) which is used for
> configuring fan. This patch change comments, function names and other
> definitions so code should be unambiguous now.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> This patch is cosmetic and does not bring any change to code.

For me "speed" and "rpm" are synonyms. So you are not really clarifying
anything. If anything, you make the code even more confusing.

Guenter

Re: [PATCH 1/3] i8k: cosmetic: distinguish between fan speed and fan rpm

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 940 bytes --]

On Tuesday 09 December 2014 21:23:04 Guenter Roeck wrote:
> On Tue, Dec 09, 2014 at 09:06:59PM +0100, Pali Rohár wrote:
> > Driver mix speed and rpm. Fan speed is value (0, 1, 2) which
> > is used for configuring fan. This patch change comments,
> > function names and other definitions so code should be
> > unambiguous now.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > This patch is cosmetic and does not bring any change to
> > code.
> 
> For me "speed" and "rpm" are synonyms. So you are not really
> clarifying anything. If anything, you make the code even more
> confusing.
> 
> Guenter

Ok, what do you want to use instead "speed" and "rpm" to make it 
clear? We have function which returns RPM and other functions 
which get/set fan speed value (which is 0, 1 or 2). And new 
function (from patch 2) returns nominal RPM for fan speed value.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] i8k: cosmetic: distinguish between fan speed and fan rpm

[Guenter Roeck]

On Tue, Dec 09, 2014 at 09:39:29PM +0100, Pali Rohár wrote:
> On Tuesday 09 December 2014 21:23:04 Guenter Roeck wrote:
> > On Tue, Dec 09, 2014 at 09:06:59PM +0100, Pali Rohár wrote:
> > > Driver mix speed and rpm. Fan speed is value (0, 1, 2) which
> > > is used for configuring fan. This patch change comments,
> > > function names and other definitions so code should be
> > > unambiguous now.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > This patch is cosmetic and does not bring any change to
> > > code.
> > 
> > For me "speed" and "rpm" are synonyms. So you are not really
> > clarifying anything. If anything, you make the code even more
> > confusing.
> > 
> > Guenter
> 
> Ok, what do you want to use instead "speed" and "rpm" to make it 
> clear? We have function which returns RPM and other functions 
> which get/set fan speed value (which is 0, 1 or 2). And new 
> function (from patch 2) returns nominal RPM for fan speed value.
> 
I would not touch the existing code at all.

Guenter

[PATCH 2/3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

This patch adds new function i8k_get_fan_nominal_rpm() 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 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 was 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@gmail.com>
---
I tested this patch only on my Dell Latitude E6440 and autodetection worked fine
Before appying this patch it should be tested on some other dell machines too
but if machine does not support i8k_get_fan_nominal_rpm() driver should fallback
to old values. So patch should be without regressions.
---
 drivers/char/i8k.c       |  122 ++++++++++++++++++++++++++++++++++++++--------
 include/uapi/linux/i8k.h |    4 +-
 2 files changed, 104 insertions(+), 22 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 31e4beb..8bdbed2 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -42,12 +42,14 @@
 #define I8K_SMM_GET_FAN_SPEED	0x00a3
 #define I8K_SMM_SET_FAN_SPEED	0x01a3
 #define I8K_SMM_GET_FAN_RPM	0x02a3
+#define I8K_SMM_GET_FAN_NOM_RPM	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 +66,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[I8K_FAN_COUNT];
+static int i8k_pwm_mult[I8K_FAN_COUNT];
+static int i8k_fan_max[I8K_FAN_COUNT];
 
 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -95,13 +97,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 rpm with");
+MODULE_PARM_DESC(fan_mult, "Factor to multiply fan rpm 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 +273,25 @@ static int i8k_get_fan_rpm(int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_RPM, };
 
+	if (fan < 0 || fan >= I8K_FAN_COUNT)
+		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_rpm(int fan, int speed)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_NOM_RPM, };
+
+	if (fan < 0 || fan >= I8K_FAN_COUNT)
+		return -EINVAL;
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult[fan];
 }
 
 /*
@@ -282,9 +301,12 @@ static int i8k_set_fan_speed(int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN_SPEED, };
 
-	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 < I8K_FAN_COUNT && speed > i8k_fan_max[fan])
+		speed = i8k_fan_max[fan];
 
+	regs.ebx = (fan & 0xff) | (speed << 8);
 	return i8k_smm(&regs) ? : i8k_get_fan_speed(fan);
 }
 
@@ -559,10 +581,14 @@ static ssize_t i8k_hwmon_show_pwm(struct device *dev,
 	int index = to_sensor_dev_attr(devattr)->index;
 	int fan_speed;
 
+	if (index < 0 || index >= I8K_FAN_COUNT)
+		return -EINVAL;
+
 	fan_speed = i8k_get_fan_speed(index);
 	if (fan_speed < 0)
 		return -EIO;
-	return sprintf(buf, "%d\n", clamp_val(fan_speed * i8k_pwm_mult, 0, 255));
+	return sprintf(buf, "%d\n", clamp_val(fan_speed * i8k_pwm_mult[index],
+					      0, 255));
 }
 
 static ssize_t i8k_hwmon_set_pwm(struct device *dev,
@@ -573,10 +599,14 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
 	unsigned long val;
 	int err;
 
+	if (index < 0 || index >= I8K_FAN_COUNT)
+		return -EINVAL;
+
 	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_speed(index, val);
@@ -854,7 +884,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 +915,66 @@ 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 == 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_mult <= 0) {
+		/*
+		 * 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
+		 */
+		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
+			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
+			for (val = 0; val < 256; ++val) {
+				ret = i8k_get_fan_nominal_rpm(fan, val);
+				if (ret > I8K_FAN_MAX_RPM) {
+					i8k_fan_mult[fan] = 1;
+					break;
+				} else if (ret < 0) {
+					break;
+				}
+			}
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
+			i8k_fan_mult[fan] = fan_mult;
 	}
-	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
+
+	if (fan_max <= 0) {
+		/*
+		 * Autodetect maximal fan speed value for each fan
+		 * Speed value is valid if i8k_get_fan_nominal_rpm() not fail
+		 */
+		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
+			for (val = 0; val < 256; ++val) {
+				if (i8k_get_fan_nominal_rpm(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;
+		}
+	} else {
+		/* Maximal fan speed was specified in module param or in dmi */
+		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
+			i8k_fan_max[fan] = fan_max;
+	}
+
+	for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
+		i8k_pwm_mult[fan] = DIV_ROUND_UP(255, i8k_fan_max[fan]);
 
 	return 0;
 }
diff --git a/include/uapi/linux/i8k.h b/include/uapi/linux/i8k.h
index 133d02f..60ef0ea 100644
--- a/include/uapi/linux/i8k.h
+++ b/include/uapi/linux/i8k.h
@@ -29,8 +29,10 @@
 #define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
 #define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
 
-#define I8K_FAN_LEFT		1
 #define I8K_FAN_RIGHT		0
+#define I8K_FAN_LEFT		1
+#define I8K_FAN_COUNT		(I8K_FAN_LEFT + 1)
+
 #define I8K_FAN_OFF		0
 #define I8K_FAN_LOW		1
 #define I8K_FAN_HIGH		2
-- 
1.7.9.5

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

[Guenter Roeck]

On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár wrote:
> This patch adds new function i8k_get_fan_nominal_rpm() 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 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 was 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@gmail.com>
> ---
> I tested this patch only on my Dell Latitude E6440 and autodetection worked fine
> Before appying this patch it should be tested on some other dell machines too
> but if machine does not support i8k_get_fan_nominal_rpm() driver should fallback
> to old values. So patch should be without regressions.

It looks like many of your error checks are unnecessary.
Why did you add those ?

Please refrain from adding unnecessary code.

Guenter

> ---
>  drivers/char/i8k.c       |  122 ++++++++++++++++++++++++++++++++++++++--------
>  include/uapi/linux/i8k.h |    4 +-
>  2 files changed, 104 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 31e4beb..8bdbed2 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -42,12 +42,14 @@
>  #define I8K_SMM_GET_FAN_SPEED	0x00a3
>  #define I8K_SMM_SET_FAN_SPEED	0x01a3
>  #define I8K_SMM_GET_FAN_RPM	0x02a3
> +#define I8K_SMM_GET_FAN_NOM_RPM	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 +66,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[I8K_FAN_COUNT];
> +static int i8k_pwm_mult[I8K_FAN_COUNT];
> +static int i8k_fan_max[I8K_FAN_COUNT];
>  
>  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
>  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> @@ -95,13 +97,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 rpm with");
> +MODULE_PARM_DESC(fan_mult, "Factor to multiply fan rpm 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 +273,25 @@ static int i8k_get_fan_rpm(int fan)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_RPM, };
>  
> +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> +		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_rpm(int fan, int speed)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_NOM_RPM, };
> +
> +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> +		return -EINVAL;
> +
> +	regs.ebx = (fan & 0xff) | (speed << 8);
> +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult[fan];
>  }
>  
>  /*
> @@ -282,9 +301,12 @@ static int i8k_set_fan_speed(int fan, int speed)
>  {
>  	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN_SPEED, };
>  
> -	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 < I8K_FAN_COUNT && speed > i8k_fan_max[fan])
> +		speed = i8k_fan_max[fan];
>  
> +	regs.ebx = (fan & 0xff) | (speed << 8);
>  	return i8k_smm(&regs) ? : i8k_get_fan_speed(fan);
>  }
>  
> @@ -559,10 +581,14 @@ static ssize_t i8k_hwmon_show_pwm(struct device *dev,
>  	int index = to_sensor_dev_attr(devattr)->index;
>  	int fan_speed;
>  
> +	if (index < 0 || index >= I8K_FAN_COUNT)
> +		return -EINVAL;
> +
>  	fan_speed = i8k_get_fan_speed(index);
>  	if (fan_speed < 0)
>  		return -EIO;
> -	return sprintf(buf, "%d\n", clamp_val(fan_speed * i8k_pwm_mult, 0, 255));
> +	return sprintf(buf, "%d\n", clamp_val(fan_speed * i8k_pwm_mult[index],
> +					      0, 255));
>  }
>  
>  static ssize_t i8k_hwmon_set_pwm(struct device *dev,
> @@ -573,10 +599,14 @@ static ssize_t i8k_hwmon_set_pwm(struct device *dev,
>  	unsigned long val;
>  	int err;
>  
> +	if (index < 0 || index >= I8K_FAN_COUNT)
> +		return -EINVAL;
> +
>  	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_speed(index, val);
> @@ -854,7 +884,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 +915,66 @@ 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 == 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_mult <= 0) {
> +		/*
> +		 * 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
> +		 */
> +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> +			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> +			for (val = 0; val < 256; ++val) {
> +				ret = i8k_get_fan_nominal_rpm(fan, val);
> +				if (ret > I8K_FAN_MAX_RPM) {
> +					i8k_fan_mult[fan] = 1;
> +					break;
> +				} else if (ret < 0) {
> +					break;
> +				}
> +			}
> +		}
> +	} else {
> +		/* Fan multiplier was specified in module param or in dmi */
> +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> +			i8k_fan_mult[fan] = fan_mult;
>  	}
> -	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
> +
> +	if (fan_max <= 0) {
> +		/*
> +		 * Autodetect maximal fan speed value for each fan
> +		 * Speed value is valid if i8k_get_fan_nominal_rpm() not fail
> +		 */
> +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> +			for (val = 0; val < 256; ++val) {
> +				if (i8k_get_fan_nominal_rpm(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;
> +		}
> +	} else {
> +		/* Maximal fan speed was specified in module param or in dmi */
> +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> +			i8k_fan_max[fan] = fan_max;
> +	}
> +
> +	for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> +		i8k_pwm_mult[fan] = DIV_ROUND_UP(255, i8k_fan_max[fan]);
>  
>  	return 0;
>  }
> diff --git a/include/uapi/linux/i8k.h b/include/uapi/linux/i8k.h
> index 133d02f..60ef0ea 100644
> --- a/include/uapi/linux/i8k.h
> +++ b/include/uapi/linux/i8k.h
> @@ -29,8 +29,10 @@
>  #define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
>  #define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
>  
> -#define I8K_FAN_LEFT		1
>  #define I8K_FAN_RIGHT		0
> +#define I8K_FAN_LEFT		1
> +#define I8K_FAN_COUNT		(I8K_FAN_LEFT + 1)
> +
>  #define I8K_FAN_OFF		0
>  #define I8K_FAN_LOW		1
>  #define I8K_FAN_HIGH		2
> -- 
> 1.7.9.5
> 

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 10362 bytes --]

On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
> On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár wrote:
> > This patch adds new function i8k_get_fan_nominal_rpm() 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 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 was 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@gmail.com>
> > ---
> > I tested this patch only on my Dell Latitude E6440 and
> > autodetection worked fine Before appying this patch it
> > should be tested on some other dell machines too but if
> > machine does not support i8k_get_fan_nominal_rpm() driver
> > should fallback to old values. So patch should be without
> > regressions.
> 
> It looks like many of your error checks are unnecessary.
> Why did you add those ?
> 
> Please refrain from adding unnecessary code.
> 
> Guenter
> 

Which error checks do you mean?

> > ---
> > 
> >  drivers/char/i8k.c       |  122
> >  ++++++++++++++++++++++++++++++++++++++--------
> >  include/uapi/linux/i8k.h |    4 +-
> >  2 files changed, 104 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > index 31e4beb..8bdbed2 100644
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -42,12 +42,14 @@
> > 
> >  #define I8K_SMM_GET_FAN_SPEED	0x00a3
> >  #define I8K_SMM_SET_FAN_SPEED	0x01a3
> >  #define I8K_SMM_GET_FAN_RPM	0x02a3
> > 
> > +#define I8K_SMM_GET_FAN_NOM_RPM	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 +66,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[I8K_FAN_COUNT];
> > +static int i8k_pwm_mult[I8K_FAN_COUNT];
> > +static int i8k_fan_max[I8K_FAN_COUNT];
> > 
> >  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
> >  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> > 
> > @@ -95,13 +97,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 rpm
> > with"); +MODULE_PARM_DESC(fan_mult, "Factor to multiply fan
> > rpm 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 +273,25 @@ static int i8k_get_fan_rpm(int fan)
> > 
> >  {
> >  
> >  	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_RPM, };
> > 
> > +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> > +		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_rpm(int fan, int speed)
> > +{
> > +	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_NOM_RPM,
> > }; +
> > +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> > +		return -EINVAL;
> > +
> > +	regs.ebx = (fan & 0xff) | (speed << 8);
> > +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) *
> > i8k_fan_mult[fan];
> > 
> >  }
> >  
> >  /*
> > 
> > @@ -282,9 +301,12 @@ static int i8k_set_fan_speed(int fan,
> > int speed)
> > 
> >  {
> >  
> >  	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN_SPEED, };
> > 
> > -	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 < I8K_FAN_COUNT && speed >
> > i8k_fan_max[fan]) +		speed = i8k_fan_max[fan];
> > 
> > +	regs.ebx = (fan & 0xff) | (speed << 8);
> > 
> >  	return i8k_smm(&regs) ? : i8k_get_fan_speed(fan);
> >  
> >  }
> > 
> > @@ -559,10 +581,14 @@ static ssize_t
> > i8k_hwmon_show_pwm(struct device *dev,
> > 
> >  	int index = to_sensor_dev_attr(devattr)->index;
> >  	int fan_speed;
> > 
> > +	if (index < 0 || index >= I8K_FAN_COUNT)
> > +		return -EINVAL;
> > +
> > 
> >  	fan_speed = i8k_get_fan_speed(index);
> >  	if (fan_speed < 0)
> >  	
> >  		return -EIO;
> > 
> > -	return sprintf(buf, "%d\n", clamp_val(fan_speed *
> > i8k_pwm_mult, 0, 255)); +	return sprintf(buf, "%d\n",
> > clamp_val(fan_speed * i8k_pwm_mult[index], +					
      0,
> > 255));
> > 
> >  }
> >  
> >  static ssize_t i8k_hwmon_set_pwm(struct device *dev,
> > 
> > @@ -573,10 +599,14 @@ static ssize_t
> > i8k_hwmon_set_pwm(struct device *dev,
> > 
> >  	unsigned long val;
> >  	int err;
> > 
> > +	if (index < 0 || index >= I8K_FAN_COUNT)
> > +		return -EINVAL;
> > +
> > 
> >  	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_speed(index, val);
> > 
> > @@ -854,7 +884,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 +915,66 @@ 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 == 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_mult <= 0) {
> > +		/*
> > +		 * 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 +		 */
> > +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > +			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > +			for (val = 0; val < 256; ++val) {
> > +				ret = i8k_get_fan_nominal_rpm(fan, val);
> > +				if (ret > I8K_FAN_MAX_RPM) {
> > +					i8k_fan_mult[fan] = 1;
> > +					break;
> > +				} else if (ret < 0) {
> > +					break;
> > +				}
> > +			}
> > +		}
> > +	} else {
> > +		/* Fan multiplier was specified in module param or in 
dmi
> > */ +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > +			i8k_fan_mult[fan] = fan_mult;
> > 
> >  	}
> > 
> > -	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
> > +
> > +	if (fan_max <= 0) {
> > +		/*
> > +		 * Autodetect maximal fan speed value for each fan
> > +		 * Speed value is valid if i8k_get_fan_nominal_rpm() 
not
> > fail +		 */
> > +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > +			for (val = 0; val < 256; ++val) {
> > +				if (i8k_get_fan_nominal_rpm(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;
> > +		}
> > +	} else {
> > +		/* Maximal fan speed was specified in module param or 
in
> > dmi */ +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > +			i8k_fan_max[fan] = fan_max;
> > +	}
> > +
> > +	for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > +		i8k_pwm_mult[fan] = DIV_ROUND_UP(255, 
i8k_fan_max[fan]);
> > 
> >  	return 0;
> >  
> >  }
> > 
> > diff --git a/include/uapi/linux/i8k.h
> > b/include/uapi/linux/i8k.h index 133d02f..60ef0ea 100644
> > --- a/include/uapi/linux/i8k.h
> > +++ b/include/uapi/linux/i8k.h
> > @@ -29,8 +29,10 @@
> > 
> >  #define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
> >  #define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
> > 
> > -#define I8K_FAN_LEFT		1
> > 
> >  #define I8K_FAN_RIGHT		0
> > 
> > +#define I8K_FAN_LEFT		1
> > +#define I8K_FAN_COUNT		(I8K_FAN_LEFT + 1)
> > +
> > 
> >  #define I8K_FAN_OFF		0
> >  #define I8K_FAN_LOW		1
> >  #define I8K_FAN_HIGH		2

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali Rohár wrote:
> On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
> > On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár wrote:
> > > This patch adds new function i8k_get_fan_nominal_rpm() 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 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 was 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@gmail.com>
> > > ---
> > > I tested this patch only on my Dell Latitude E6440 and
> > > autodetection worked fine Before appying this patch it
> > > should be tested on some other dell machines too but if
> > > machine does not support i8k_get_fan_nominal_rpm() driver
> > > should fallback to old values. So patch should be without
> > > regressions.
> > 
> > It looks like many of your error checks are unnecessary.
> > Why did you add those ?
> > 
> > Please refrain from adding unnecessary code.
> > 
> > Guenter
> > 
> 
> Which error checks do you mean?
> 
There are several you added. I noticed the ones around 'index', which
would only be hit on coding errors. At that point I stopped looking
further and did not verify which of the other added error checks are
unnecessary as well.

A quick additional check reveals that the fan variable range check in
i8k_get_fan_nominal_rpm is completely unnecessary - if the range
was wrong, the calling code would fail as well, since you unconditionally
write into an array indexed by the very same variable. Given the simplicity
of the calling code, it can even be mathematically proven that the error
condition you are checking can never happen.

With that I really stopped looking further.

Guenter

> > > ---
> > > 
> > >  drivers/char/i8k.c       |  122
> > >  ++++++++++++++++++++++++++++++++++++++--------
> > >  include/uapi/linux/i8k.h |    4 +-
> > >  2 files changed, 104 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > > index 31e4beb..8bdbed2 100644
> > > --- a/drivers/char/i8k.c
> > > +++ b/drivers/char/i8k.c
> > > @@ -42,12 +42,14 @@
> > > 
> > >  #define I8K_SMM_GET_FAN_SPEED	0x00a3
> > >  #define I8K_SMM_SET_FAN_SPEED	0x01a3
> > >  #define I8K_SMM_GET_FAN_RPM	0x02a3
> > > 
> > > +#define I8K_SMM_GET_FAN_NOM_RPM	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 +66,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[I8K_FAN_COUNT];
> > > +static int i8k_pwm_mult[I8K_FAN_COUNT];
> > > +static int i8k_fan_max[I8K_FAN_COUNT];
> > > 
> > >  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
> > >  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> > > 
> > > @@ -95,13 +97,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 rpm
> > > with"); +MODULE_PARM_DESC(fan_mult, "Factor to multiply fan
> > > rpm 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 +273,25 @@ static int i8k_get_fan_rpm(int fan)
> > > 
> > >  {
> > >  
> > >  	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_RPM, };
> > > 
> > > +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> > > +		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_rpm(int fan, int speed)
> > > +{
> > > +	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_NOM_RPM,
> > > }; +
> > > +	if (fan < 0 || fan >= I8K_FAN_COUNT)
> > > +		return -EINVAL;
> > > +
> > > +	regs.ebx = (fan & 0xff) | (speed << 8);
> > > +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) *
> > > i8k_fan_mult[fan];
> > > 
> > >  }
> > >  
> > >  /*
> > > 
> > > @@ -282,9 +301,12 @@ static int i8k_set_fan_speed(int fan,
> > > int speed)
> > > 
> > >  {
> > >  
> > >  	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN_SPEED, };
> > > 
> > > -	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 < I8K_FAN_COUNT && speed >
> > > i8k_fan_max[fan]) +		speed = i8k_fan_max[fan];
> > > 
> > > +	regs.ebx = (fan & 0xff) | (speed << 8);
> > > 
> > >  	return i8k_smm(&regs) ? : i8k_get_fan_speed(fan);
> > >  
> > >  }
> > > 
> > > @@ -559,10 +581,14 @@ static ssize_t
> > > i8k_hwmon_show_pwm(struct device *dev,
> > > 
> > >  	int index = to_sensor_dev_attr(devattr)->index;
> > >  	int fan_speed;
> > > 
> > > +	if (index < 0 || index >= I8K_FAN_COUNT)
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	fan_speed = i8k_get_fan_speed(index);
> > >  	if (fan_speed < 0)
> > >  	
> > >  		return -EIO;
> > > 
> > > -	return sprintf(buf, "%d\n", clamp_val(fan_speed *
> > > i8k_pwm_mult, 0, 255)); +	return sprintf(buf, "%d\n",
> > > clamp_val(fan_speed * i8k_pwm_mult[index], +					
>       0,
> > > 255));
> > > 
> > >  }
> > >  
> > >  static ssize_t i8k_hwmon_set_pwm(struct device *dev,
> > > 
> > > @@ -573,10 +599,14 @@ static ssize_t
> > > i8k_hwmon_set_pwm(struct device *dev,
> > > 
> > >  	unsigned long val;
> > >  	int err;
> > > 
> > > +	if (index < 0 || index >= I8K_FAN_COUNT)
> > > +		return -EINVAL;
> > > +
> > > 
> > >  	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_speed(index, val);
> > > 
> > > @@ -854,7 +884,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 +915,66 @@ 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 == 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_mult <= 0) {
> > > +		/*
> > > +		 * 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 +		 */
> > > +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > > +			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > > +			for (val = 0; val < 256; ++val) {
> > > +				ret = i8k_get_fan_nominal_rpm(fan, val);
> > > +				if (ret > I8K_FAN_MAX_RPM) {
> > > +					i8k_fan_mult[fan] = 1;
> > > +					break;
> > > +				} else if (ret < 0) {
> > > +					break;
> > > +				}
> > > +			}
> > > +		}
> > > +	} else {
> > > +		/* Fan multiplier was specified in module param or in 
> dmi
> > > */ +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > > +			i8k_fan_mult[fan] = fan_mult;
> > > 
> > >  	}
> > > 
> > > -	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
> > > +
> > > +	if (fan_max <= 0) {
> > > +		/*
> > > +		 * Autodetect maximal fan speed value for each fan
> > > +		 * Speed value is valid if i8k_get_fan_nominal_rpm() 
> not
> > > fail +		 */
> > > +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > > +			for (val = 0; val < 256; ++val) {
> > > +				if (i8k_get_fan_nominal_rpm(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;
> > > +		}
> > > +	} else {
> > > +		/* Maximal fan speed was specified in module param or 
> in
> > > dmi */ +		for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > > +			i8k_fan_max[fan] = fan_max;
> > > +	}
> > > +
> > > +	for (fan = 0; fan < I8K_FAN_COUNT; ++fan)
> > > +		i8k_pwm_mult[fan] = DIV_ROUND_UP(255, 
> i8k_fan_max[fan]);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > diff --git a/include/uapi/linux/i8k.h
> > > b/include/uapi/linux/i8k.h index 133d02f..60ef0ea 100644
> > > --- a/include/uapi/linux/i8k.h
> > > +++ b/include/uapi/linux/i8k.h
> > > @@ -29,8 +29,10 @@
> > > 
> > >  #define I8K_GET_FAN		_IOWR('i', 0x86, size_t)
> > >  #define I8K_SET_FAN		_IOWR('i', 0x87, size_t)
> > > 
> > > -#define I8K_FAN_LEFT		1
> > > 
> > >  #define I8K_FAN_RIGHT		0
> > > 
> > > +#define I8K_FAN_LEFT		1
> > > +#define I8K_FAN_COUNT		(I8K_FAN_LEFT + 1)
> > > +
> > > 
> > >  #define I8K_FAN_OFF		0
> > >  #define I8K_FAN_LOW		1
> > >  #define I8K_FAN_HIGH		2
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2756 bytes --]

On Tuesday 09 December 2014 23:42:08 Guenter Roeck wrote:
> On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali Rohár wrote:
> > On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
> > > On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár wrote:
> > > > This patch adds new function i8k_get_fan_nominal_rpm()
> > > > 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 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 was 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@gmail.com>
> > > > ---
> > > > I tested this patch only on my Dell Latitude E6440 and
> > > > autodetection worked fine Before appying this patch it
> > > > should be tested on some other dell machines too but if
> > > > machine does not support i8k_get_fan_nominal_rpm()
> > > > driver should fallback to old values. So patch should
> > > > be without regressions.
> > > 
> > > It looks like many of your error checks are unnecessary.
> > > Why did you add those ?
> > > 
> > > Please refrain from adding unnecessary code.
> > > 
> > > Guenter
> > 
> > Which error checks do you mean?
> 
> There are several you added. I noticed the ones around
> 'index', which would only be hit on coding errors. At that
> point I stopped looking further and did not verify which of
> the other added error checks are unnecessary as well.
> 
> A quick additional check reveals that the fan variable range
> check in i8k_get_fan_nominal_rpm is completely unnecessary -
> if the range was wrong, the calling code would fail as well,
> since you unconditionally write into an array indexed by the
> very same variable. Given the simplicity of the calling code,
> it can even be mathematically proven that the error condition
> you are checking can never happen.
> 
> With that I really stopped looking further.
> 
> Guenter
> 

Should I remove those access out-of-array checks?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On 12/10/2014 03:50 AM, Pali Rohár wrote:
> On Tuesday 09 December 2014 23:42:08 Guenter Roeck wrote:
>> On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali Rohár wrote:
>>> On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
>>>> On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár wrote:
>>>>> This patch adds new function i8k_get_fan_nominal_rpm()
>>>>> 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 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 was 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@gmail.com>
>>>>> ---
>>>>> I tested this patch only on my Dell Latitude E6440 and
>>>>> autodetection worked fine Before appying this patch it
>>>>> should be tested on some other dell machines too but if
>>>>> machine does not support i8k_get_fan_nominal_rpm()
>>>>> driver should fallback to old values. So patch should
>>>>> be without regressions.
>>>>
>>>> It looks like many of your error checks are unnecessary.
>>>> Why did you add those ?
>>>>
>>>> Please refrain from adding unnecessary code.
>>>>
>>>> Guenter
>>>
>>> Which error checks do you mean?
>>
>> There are several you added. I noticed the ones around
>> 'index', which would only be hit on coding errors. At that
>> point I stopped looking further and did not verify which of
>> the other added error checks are unnecessary as well.
>>
>> A quick additional check reveals that the fan variable range
>> check in i8k_get_fan_nominal_rpm is completely unnecessary -
>> if the range was wrong, the calling code would fail as well,
>> since you unconditionally write into an array indexed by the
>> very same variable. Given the simplicity of the calling code,
>> it can even be mathematically proven that the error condition
>> you are checking can never happen.
>>
>> With that I really stopped looking further.
>>
>> Guenter
>>
>
> Should I remove those access out-of-array checks?
>

If you want me to look into it further. In general, I don't accept
code like this, since it increases kernel size for no good reason.
It also makes it more difficult to find _real_ problems in the code
since it distracts from seeing those.

Guenter

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3280 bytes --]

On Wednesday 10 December 2014 15:08:11 Guenter Roeck wrote:
> On 12/10/2014 03:50 AM, Pali Rohár wrote:
> > On Tuesday 09 December 2014 23:42:08 Guenter Roeck wrote:
> >> On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali Rohár wrote:
> >>> On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
> >>>> On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár 
wrote:
> >>>>> This patch adds new function i8k_get_fan_nominal_rpm()
> >>>>> 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 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 was 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@gmail.com>
> >>>>> ---
> >>>>> I tested this patch only on my Dell Latitude E6440 and
> >>>>> autodetection worked fine Before appying this patch it
> >>>>> should be tested on some other dell machines too but if
> >>>>> machine does not support i8k_get_fan_nominal_rpm()
> >>>>> driver should fallback to old values. So patch should
> >>>>> be without regressions.
> >>>> 
> >>>> It looks like many of your error checks are unnecessary.
> >>>> Why did you add those ?
> >>>> 
> >>>> Please refrain from adding unnecessary code.
> >>>> 
> >>>> Guenter
> >>> 
> >>> Which error checks do you mean?
> >> 
> >> There are several you added. I noticed the ones around
> >> 'index', which would only be hit on coding errors. At that
> >> point I stopped looking further and did not verify which of
> >> the other added error checks are unnecessary as well.
> >> 
> >> A quick additional check reveals that the fan variable
> >> range check in i8k_get_fan_nominal_rpm is completely
> >> unnecessary - if the range was wrong, the calling code
> >> would fail as well, since you unconditionally write into
> >> an array indexed by the very same variable. Given the
> >> simplicity of the calling code, it can even be
> >> mathematically proven that the error condition you are
> >> checking can never happen.
> >> 
> >> With that I really stopped looking further.
> >> 
> >> Guenter
> > 
> > Should I remove those access out-of-array checks?
> 
> If you want me to look into it further. In general, I don't
> accept code like this, since it increases kernel size for no
> good reason. It also makes it more difficult to find _real_
> problems in the code since it distracts from seeing those.
> 
> Guenter

Ok, I will rework this patch and drop that first cosmetic.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On Thu, Dec 18, 2014 at 12:13:35PM +0100, Pali Rohár wrote:
> On Wednesday 10 December 2014 15:08:11 Guenter Roeck wrote:
> > On 12/10/2014 03:50 AM, Pali Rohár wrote:
> > > On Tuesday 09 December 2014 23:42:08 Guenter Roeck wrote:
> > >> On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali Rohár wrote:
> > >>> On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
> > >>>> On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali Rohár 
> wrote:
> > >>>>> This patch adds new function i8k_get_fan_nominal_rpm()
> > >>>>> 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 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 was 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@gmail.com>
> > >>>>> ---
> > >>>>> I tested this patch only on my Dell Latitude E6440 and
> > >>>>> autodetection worked fine Before appying this patch it
> > >>>>> should be tested on some other dell machines too but if
> > >>>>> machine does not support i8k_get_fan_nominal_rpm()
> > >>>>> driver should fallback to old values. So patch should
> > >>>>> be without regressions.
> > >>>> 
> > >>>> It looks like many of your error checks are unnecessary.
> > >>>> Why did you add those ?
> > >>>> 
> > >>>> Please refrain from adding unnecessary code.
> > >>>> 
> > >>>> Guenter
> > >>> 
> > >>> Which error checks do you mean?
> > >> 
> > >> There are several you added. I noticed the ones around
> > >> 'index', which would only be hit on coding errors. At that
> > >> point I stopped looking further and did not verify which of
> > >> the other added error checks are unnecessary as well.
> > >> 
> > >> A quick additional check reveals that the fan variable
> > >> range check in i8k_get_fan_nominal_rpm is completely
> > >> unnecessary - if the range was wrong, the calling code
> > >> would fail as well, since you unconditionally write into
> > >> an array indexed by the very same variable. Given the
> > >> simplicity of the calling code, it can even be
> > >> mathematically proven that the error condition you are
> > >> checking can never happen.
> > >> 
> > >> With that I really stopped looking further.
> > >> 
> > >> Guenter
> > > 
> > > Should I remove those access out-of-array checks?
> > 
> > If you want me to look into it further. In general, I don't
> > accept code like this, since it increases kernel size for no
> > good reason. It also makes it more difficult to find _real_
> > problems in the code since it distracts from seeing those.
> > 
> > Guenter
> 
> Ok, I will rework this patch and drop that first cosmetic.
> 
Fine, but as mentioned before I still dislike unnecessary
value range checks.

Guenter

[PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

Now we have autodetection code for fan multiplier and maximal fan speed so we do
not need to have those constants for each laptop in kernel driver code.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
!!!Please do not apply this patch until all affected machines will be tested!!!

I tested autodetection code only on Dell Latitude E6440 (where it worked).
Other machines which needs to be tested:

Dell Latitude D520
Dell Latitude E6540
Dell Precision WorkStation 490
Dell Studio
Dell XPS M140 (MXC051)
---
 drivers/char/i8k.c |   88 +---------------------------------------------------
 1 file changed, 1 insertion(+), 87 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 8bdbed2..bf74644 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -725,42 +725,6 @@ static int __init i8k_init_hwmon(void)
 	return 0;
 }
 
-struct i8k_config_data {
-	int fan_mult;
-	int fan_max;
-};
-
-enum i8k_configs {
-	DELL_LATITUDE_D520,
-	DELL_LATITUDE_E6540,
-	DELL_PRECISION_490,
-	DELL_STUDIO,
-	DELL_XPS_M140,
-};
-
-static const struct i8k_config_data i8k_config_data[] = {
-	[DELL_LATITUDE_D520] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_LATITUDE_E6540] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-	[DELL_PRECISION_490] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_TURBO,
-	},
-	[DELL_STUDIO] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-	[DELL_XPS_M140] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
-};
-
 static struct dmi_system_id i8k_dmi_table[] __initdata = {
 	{
 		.ident = "Dell Inspiron",
@@ -784,30 +748,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
 		},
 	},
 	{
-		.ident = "Dell Latitude D520",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
-	},
-	{
-		.ident = "Dell Latitude E6440",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
-		.ident = "Dell Latitude E6540",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
 		.ident = "Dell Latitude 2",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -822,22 +762,13 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
 		},
 	},
 	{
-		.ident = "Dell Inspiron 3",
+		.ident = "Dell Inspiron 4",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MP061"),
 		},
 	},
 	{
-		.ident = "Dell Precision 490",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME,
-				  "Precision WorkStation 490"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
-	},
-	{
 		.ident = "Dell Precision",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -864,7 +795,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
 	},
 	{
 		.ident = "Dell XPS M140",
@@ -872,7 +802,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
 		},
-		.driver_data = (void *)&i8k_config_data[DELL_XPS_M140],
 	},
 	{ }
 };
@@ -884,8 +813,6 @@ 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;
 
 	/*
@@ -915,19 +842,6 @@ static int __init i8k_probe(void)
 			return -ENODEV;
 	}
 
-	/*
-	 * 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) {
-		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) {
 		/*
 		 * Autodetect fan multiplier for each fan based on nominal rpm
-- 
1.7.9.5

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 731 bytes --]

On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> Now we have autodetection code for fan multiplier and maximal
> fan speed so we do not need to have those constants for each
> laptop in kernel driver code.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> !!!Please do not apply this patch until all affected machines
> will be tested!!!
> 
> I tested autodetection code only on Dell Latitude E6440 (where
> it worked). Other machines which needs to be tested:
> 
> Dell Latitude D520
> Dell Latitude E6540
> Dell Precision WorkStation 490
> Dell Studio
> Dell XPS M140 (MXC051)
> ---

Can somebody else with dell laptops test this patch series?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > Now we have autodetection code for fan multiplier and maximal
> > fan speed so we do not need to have those constants for each
> > laptop in kernel driver code.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > !!!Please do not apply this patch until all affected machines
> > will be tested!!!
> > 
> > I tested autodetection code only on Dell Latitude E6440 (where
> > it worked). Other machines which needs to be tested:
> > 
> > Dell Latitude D520
> > Dell Latitude E6540
> > Dell Precision WorkStation 490
> > Dell Studio
> > Dell XPS M140 (MXC051)
> > ---
> 
> Can somebody else with dell laptops test this patch series?
> 
> 

i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing changed
with this patch series applied.

Gabriele

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1734 bytes --]

On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > Now we have autodetection code for fan multiplier and
> > > maximal fan speed so we do not need to have those
> > > constants for each laptop in kernel driver code.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > > !!!Please do not apply this patch until all affected
> > > machines will be tested!!!
> > > 
> > > I tested autodetection code only on Dell Latitude E6440
> > > (where it worked). Other machines which needs to be
> > > tested:
> > > 
> > > Dell Latitude D520
> > > Dell Latitude E6540
> > > Dell Precision WorkStation 490
> > > Dell Studio
> > > Dell XPS M140 (MXC051)
> > > ---
> > 
> > Can somebody else with dell laptops test this patch series?
> 
> i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> changed with this patch series applied.
> 
> Gabriele

So your BIOS cannot report nominal_rpm and because your machine 
is not in dmi list, all 3 patches do nothing for your machine.

But you need to set multiplier to 1, right?

What about this patch? (on top of 3/3)

--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -850,6 +850,10 @@ static int __init i8k_probe(void)
 		 */
 		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
 			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
+			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
+				i8k_fan_mult[fan] = 1;
+				continue;
+			}
 			for (val = 0; val < 256; ++val) {
 				ret = i8k_get_fan_nominal_rpm(fan, val);
 				if (ret > I8K_FAN_MAX_RPM) {

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Thu, 18 Dec 2014 12:08:58 +0100, Pali Rohár said:

> So your BIOS cannot report nominal_rpm and because your machine=20
> is not in dmi list, all 3 patches do nothing for your machine.
> 
> But you need to set multiplier to 1, right?
> 
> What about this patch? (on top of 3/3)
> 
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
>  		 */
>  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
>  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> +				i8k_fan_mult[fan] = 1;
> +				continue;
> +			}
>  			for (val = 0; val < 256; ++val) {


Dell Latitude E6530, linux-next 20141208 plus the 3 patches you
posted on Nov 30 (support labels), plus the 4 of 3 patches in
this go around, and sensors reports:

i8k-virtual-0
Adapter: Virtual device
fan2:        2774 RPM

and under CPU load, that number rises to 3200 or so, so it seems to be
working....

So you probably should fold this into the current set of patches, and
feel free to put this in there:

Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1529 bytes --]

On Thursday 18 December 2014 16:08:51 Valdis.Kletnieks@vt.edu 
wrote:
> On Thu, 18 Dec 2014 12:08:58 +0100, Pali Rohár said:
> > So your BIOS cannot report nominal_rpm and because your
> > machine=20 is not in dmi list, all 3 patches do nothing for
> > your machine.
> > 
> > But you need to set multiplier to 1, right?
> > 
> > What about this patch? (on top of 3/3)
> > 
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > 
> >  		 */
> >  		
> >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> >  		
> >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > 
> > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > +				i8k_fan_mult[fan] = 1;
> > +				continue;
> > +			}
> > 
> >  			for (val = 0; val < 256; ++val) {
> 
> Dell Latitude E6530, linux-next 20141208 plus the 3 patches
> you posted on Nov 30 (support labels), plus the 4 of 3
> patches in this go around, and sensors reports:
> 
> i8k-virtual-0
> Adapter: Virtual device
> fan2:        2774 RPM
> 
> and under CPU load, that number rises to 3200 or so, so it
> seems to be working....
> 
> So you probably should fold this into the current set of
> patches, and feel free to put this in there:
> 
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

Thanks for testing. Anyway I would like to know if your dell 
machine supports i8k_get_fan_nominal_rpm(). Can you test without 
above 4 lines patch?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

On Thu, 18 Dec 2014 17:34:56 +0100, Pali Rohár said:

> Thanks for testing. Anyway I would like to know if your dell
> machine supports i8k_get_fan_nominal_rpm(). Can you test without
> above 4 lines patch?

I'll give that a try this evening..


[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > Now we have autodetection code for fan multiplier and
> > > > maximal fan speed so we do not need to have those
> > > > constants for each laptop in kernel driver code.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > > !!!Please do not apply this patch until all affected
> > > > machines will be tested!!!
> > > > 
> > > > I tested autodetection code only on Dell Latitude E6440
> > > > (where it worked). Other machines which needs to be
> > > > tested:
> > > > 
> > > > Dell Latitude D520
> > > > Dell Latitude E6540
> > > > Dell Precision WorkStation 490
> > > > Dell Studio
> > > > Dell XPS M140 (MXC051)
> > > > ---
> > > 
> > > Can somebody else with dell laptops test this patch series?
> > 
> > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> > changed with this patch series applied.
> > 
> > Gabriele
> 
> So your BIOS cannot report nominal_rpm and because your machine 
> is not in dmi list, all 3 patches do nothing for your machine.
> 
> But you need to set multiplier to 1, right?
> 
> What about this patch? (on top of 3/3)
> 
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
>  		 */
>  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
>  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> +				i8k_fan_mult[fan] = 1;
> +				continue;
> +			}
>  			for (val = 0; val < 256; ++val) {
>  				ret = i8k_get_fan_nominal_rpm(fan, val);
>  				if (ret > I8K_FAN_MAX_RPM) {
> 
> 

Hi,

I'm sorry for replying only now, but I couldn't follow this thread
closely and I'm a bit lost now. I haven't tested the suggested
change, but I don't think it would work in a reliable way. It's not
rare for the fan to be completely stopped, especially on boot. You
are right though, 1 is the right multiplier.


Guenter, while I was trying to catch up, I noticed that the support
for the XPS 13 [1] will be added. May I ask you which revision of
the laptop was tested? I own the 9333 one and I'm not sure that
having i8k automatically loaded is a good idea. The reason is that
reading and setting the speed of the right (and only) fan causes a
freeze of the laptop of some milliseconds, enough to be annoying and
noticeable. I'm worried of the effect this might have in the everyday
use, users might start noticing random freezes because of one
application that all the sudden is able to read the speed of the fan.
I don't if the same happens on all the other Dell systems, this is
the first and only Dell laptop I owned.

If the tested laptop wasn't the XPS13 9333 and this problem does not
exists, would it be possible to make the DMI_PRODUCT_NAME string such
that it matches only the tested revision? The string that identifies
my laptop is "XPS13 9333".

Gabriele

[1] http://www.spinics.net/lists/kernel/msg1878801.html

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta wrote:
> > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > Now we have autodetection code for fan multiplier and
> > > > > maximal fan speed so we do not need to have those
> > > > > constants for each laptop in kernel driver code.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > ---
> > > > > !!!Please do not apply this patch until all affected
> > > > > machines will be tested!!!
> > > > > 
> > > > > I tested autodetection code only on Dell Latitude E6440
> > > > > (where it worked). Other machines which needs to be
> > > > > tested:
> > > > > 
> > > > > Dell Latitude D520
> > > > > Dell Latitude E6540
> > > > > Dell Precision WorkStation 490
> > > > > Dell Studio
> > > > > Dell XPS M140 (MXC051)
> > > > > ---
> > > > 
> > > > Can somebody else with dell laptops test this patch series?
> > > 
> > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so nothing
> > > changed with this patch series applied.
> > > 
> > > Gabriele
> > 
> > So your BIOS cannot report nominal_rpm and because your machine 
> > is not in dmi list, all 3 patches do nothing for your machine.
> > 
> > But you need to set multiplier to 1, right?
> > 
> > What about this patch? (on top of 3/3)
> > 
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> >  		 */
> >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > +				i8k_fan_mult[fan] = 1;
> > +				continue;
> > +			}
> >  			for (val = 0; val < 256; ++val) {
> >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> >  				if (ret > I8K_FAN_MAX_RPM) {
> > 
> > 
> 
> Hi,
> 
> I'm sorry for replying only now, but I couldn't follow this thread
> closely and I'm a bit lost now. I haven't tested the suggested
> change, but I don't think it would work in a reliable way. It's not
> rare for the fan to be completely stopped, especially on boot. You
> are right though, 1 is the right multiplier.

I took a better look at the code and my laptop is indeed able to report
the nominal speed. The problem with the original patch was that the
first call of i8k_get_fan_nominal_rpm() correctly returned -22 (the fan
doesn't exist) and the loop was terminated because of that. I tested
the following patch http://www.spinics.net/lists/kernel/msg1892101.html
and the fan multiplier auto detection worked.

I confirm that the suggested change in case the nominal speed can't
be retrieved is not OK as its outcome depends on the current state of
the fans.

> Guenter, while I was trying to catch up, I noticed that the support
> for the XPS 13 [1] will be added. May I ask you which revision of
> the laptop was tested? I own the 9333 one and I'm not sure that
> having i8k automatically loaded is a good idea. The reason is that
> reading and setting the speed of the right (and only) fan causes a
> freeze of the laptop of some milliseconds, enough to be annoying and
> noticeable. I'm worried of the effect this might have in the everyday
> use, users might start noticing random freezes because of one
> application that all the sudden is able to read the speed of the fan.
> I don't if the same happens on all the other Dell systems, this is
> the first and only Dell laptop I owned.
> 
> If the tested laptop wasn't the XPS13 9333 and this problem does not
> exists, would it be possible to make the DMI_PRODUCT_NAME string such
> that it matches only the tested revision? The string that identifies
> my laptop is "XPS13 9333".
> 
> Gabriele
> 
> [1] http://www.spinics.net/lists/kernel/msg1878801.html

I modified i8k so that it prints the time required to execute the
assembly code in i8k_smm(). Here what I got:

[ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
[ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
[ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
[ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
[ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
[ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
[ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
[ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
[ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label

As you can see, the time required to read the speed of the fan is
substantially higher than the combined time of all the other reads.

Is the difference so big for the other systems that have been tested?

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 5376 bytes --]

On Saturday 27 December 2014 15:13:28 Gabriele Mazzotta wrote:
> On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> > On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta 
wrote:
> > > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > > Now we have autodetection code for fan multiplier
> > > > > > and maximal fan speed so we do not need to have
> > > > > > those constants for each laptop in kernel driver
> > > > > > code.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > ---
> > > > > > !!!Please do not apply this patch until all affected
> > > > > > machines will be tested!!!
> > > > > > 
> > > > > > I tested autodetection code only on Dell Latitude
> > > > > > E6440 (where it worked). Other machines which needs
> > > > > > to be tested:
> > > > > > 
> > > > > > Dell Latitude D520
> > > > > > Dell Latitude E6540
> > > > > > Dell Precision WorkStation 490
> > > > > > Dell Studio
> > > > > > Dell XPS M140 (MXC051)
> > > > > > ---
> > > > > 
> > > > > Can somebody else with dell laptops test this patch
> > > > > series?
> > > > 
> > > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so
> > > > nothing changed with this patch series applied.
> > > > 
> > > > Gabriele
> > > 
> > > So your BIOS cannot report nominal_rpm and because your
> > > machine is not in dmi list, all 3 patches do nothing for
> > > your machine.
> > > 
> > > But you need to set multiplier to 1, right?
> > > 
> > > What about this patch? (on top of 3/3)
> > > 
> > > --- a/drivers/char/i8k.c
> > > +++ b/drivers/char/i8k.c
> > > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > > 
> > >  		 */
> > >  		
> > >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > >  		
> > >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > > 
> > > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > > +				i8k_fan_mult[fan] = 1;
> > > +				continue;
> > > +			}
> > > 
> > >  			for (val = 0; val < 256; ++val) {
> > >  			
> > >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> > >  				if (ret > I8K_FAN_MAX_RPM) {
> > 
> > Hi,
> > 
> > I'm sorry for replying only now, but I couldn't follow this
> > thread closely and I'm a bit lost now. I haven't tested the
> > suggested change, but I don't think it would work in a
> > reliable way. It's not rare for the fan to be completely
> > stopped, especially on boot. You are right though, 1 is the
> > right multiplier.
> 
> I took a better look at the code and my laptop is indeed able
> to report the nominal speed. The problem with the original
> patch was that the first call of i8k_get_fan_nominal_rpm()
> correctly returned -22 (the fan doesn't exist) and the loop
> was terminated because of that. I tested the following patch
> http://www.spinics.net/lists/kernel/msg1892101.html and the
> fan multiplier auto detection worked.
> 
> I confirm that the suggested change in case the nominal speed
> can't be retrieved is not OK as its outcome depends on the
> current state of the fans.
> 
> > Guenter, while I was trying to catch up, I noticed that the
> > support for the XPS 13 [1] will be added. May I ask you
> > which revision of the laptop was tested? I own the 9333 one
> > and I'm not sure that having i8k automatically loaded is a
> > good idea. The reason is that reading and setting the speed
> > of the right (and only) fan causes a freeze of the laptop
> > of some milliseconds, enough to be annoying and noticeable.
> > I'm worried of the effect this might have in the everyday
> > use, users might start noticing random freezes because of
> > one application that all the sudden is able to read the
> > speed of the fan. I don't if the same happens on all the
> > other Dell systems, this is the first and only Dell laptop
> > I owned.
> > 
> > If the tested laptop wasn't the XPS13 9333 and this problem
> > does not exists, would it be possible to make the
> > DMI_PRODUCT_NAME string such that it matches only the
> > tested revision? The string that identifies my laptop is
> > "XPS13 9333".
> > 
> > Gabriele
> > 
> > [1] http://www.spinics.net/lists/kernel/msg1878801.html
> 
> I modified i8k so that it prints the time required to execute
> the assembly code in i8k_smm(). Here what I got:
> 
> [ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
> [ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
> [ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
> [ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
> [ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
> [ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
> [ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
> [ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
> [ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label
> 
> As you can see, the time required to read the speed of the fan
> is substantially higher than the combined time of all the
> other reads.
> 
> Is the difference so big for the other systems that have been
> tested?

It looks like we should not read fan rpm at startup for 
multiplier autodetection... and only fan nominal speed.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Guenter Roeck]

On Sun, Dec 28, 2014 at 09:22:29AM +0100, Pali Rohár wrote:
> On Saturday 27 December 2014 15:13:28 Gabriele Mazzotta wrote:
> > On Thursday 25 December 2014 22:54:34 Gabriele Mazzotta wrote:
> > > On Thursday 18 December 2014 12:08:58 Pali Rohár wrote:
> > > > On Wednesday 10 December 2014 14:32:16 Gabriele Mazzotta 
> wrote:
> > > > > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> > > > > > On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > > > > > > Now we have autodetection code for fan multiplier
> > > > > > > and maximal fan speed so we do not need to have
> > > > > > > those constants for each laptop in kernel driver
> > > > > > > code.
> > > > > > > 
> > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > > ---
> > > > > > > !!!Please do not apply this patch until all affected
> > > > > > > machines will be tested!!!
> > > > > > > 
> > > > > > > I tested autodetection code only on Dell Latitude
> > > > > > > E6440 (where it worked). Other machines which needs
> > > > > > > to be tested:
> > > > > > > 
> > > > > > > Dell Latitude D520
> > > > > > > Dell Latitude E6540
> > > > > > > Dell Precision WorkStation 490
> > > > > > > Dell Studio
> > > > > > > Dell XPS M140 (MXC051)
> > > > > > > ---
> > > > > > 
> > > > > > Can somebody else with dell laptops test this patch
> > > > > > series?
> > > > > 
> > > > > i8k_get_fan_nominal_rpm() returns -22 on my XPS13, so
> > > > > nothing changed with this patch series applied.
> > > > > 
> > > > > Gabriele
> > > > 
> > > > So your BIOS cannot report nominal_rpm and because your
> > > > machine is not in dmi list, all 3 patches do nothing for
> > > > your machine.
> > > > 
> > > > But you need to set multiplier to 1, right?
> > > > 
> > > > What about this patch? (on top of 3/3)
> > > > 
> > > > --- a/drivers/char/i8k.c
> > > > +++ b/drivers/char/i8k.c
> > > > @@ -850,6 +850,10 @@ static int __init i8k_probe(void)
> > > > 
> > > >  		 */
> > > >  		
> > > >  		for (fan = 0; fan < I8K_FAN_COUNT; ++fan) {
> > > >  		
> > > >  			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > > > 
> > > > +			if (i8k_get_fan_rpm(fan) > I8K_FAN_MAX_RPM) {
> > > > +				i8k_fan_mult[fan] = 1;
> > > > +				continue;
> > > > +			}
> > > > 
> > > >  			for (val = 0; val < 256; ++val) {
> > > >  			
> > > >  				ret = i8k_get_fan_nominal_rpm(fan, val);
> > > >  				if (ret > I8K_FAN_MAX_RPM) {
> > > 
> > > Hi,
> > > 
> > > I'm sorry for replying only now, but I couldn't follow this
> > > thread closely and I'm a bit lost now. I haven't tested the
> > > suggested change, but I don't think it would work in a
> > > reliable way. It's not rare for the fan to be completely
> > > stopped, especially on boot. You are right though, 1 is the
> > > right multiplier.
> > 
> > I took a better look at the code and my laptop is indeed able
> > to report the nominal speed. The problem with the original
> > patch was that the first call of i8k_get_fan_nominal_rpm()
> > correctly returned -22 (the fan doesn't exist) and the loop
> > was terminated because of that. I tested the following patch
> > http://www.spinics.net/lists/kernel/msg1892101.html and the
> > fan multiplier auto detection worked.
> > 
> > I confirm that the suggested change in case the nominal speed
> > can't be retrieved is not OK as its outcome depends on the
> > current state of the fans.
> > 
> > > Guenter, while I was trying to catch up, I noticed that the
> > > support for the XPS 13 [1] will be added. May I ask you
> > > which revision of the laptop was tested? I own the 9333 one
> > > and I'm not sure that having i8k automatically loaded is a
> > > good idea. The reason is that reading and setting the speed
> > > of the right (and only) fan causes a freeze of the laptop
> > > of some milliseconds, enough to be annoying and noticeable.
> > > I'm worried of the effect this might have in the everyday
> > > use, users might start noticing random freezes because of
> > > one application that all the sudden is able to read the
> > > speed of the fan. I don't if the same happens on all the
> > > other Dell systems, this is the first and only Dell laptop
> > > I owned.
> > > 
> > > If the tested laptop wasn't the XPS13 9333 and this problem
> > > does not exists, would it be possible to make the
> > > DMI_PRODUCT_NAME string such that it matches only the
> > > tested revision? The string that identifies my laptop is
> > > "XPS13 9333".
> > > 

I am several thousand miles away from the XPS13 right now, so I can not check
it. As far as I recall, it was a 9333.

> > > Gabriele
> > > 
> > > [1] http://www.spinics.net/lists/kernel/msg1878801.html
> > 
> > I modified i8k so that it prints the time required to execute
> > the assembly code in i8k_smm(). Here what I got:
> > 
> > [ 4697.485130] i8k_smm asm: 1965800 ns    #pwm1
> > [ 4697.487383] i8k_smm asm: 1375057 ns    #temp3_input
> > [ 4697.489477] i8k_smm asm: 1112493 ns    #temp3_label
> > [ 4697.991687] i8k_smm asm: 500796086 ns  #fan1_input
> > [ 4697.998245] i8k_smm asm: 1957365 ns    #fan1_label
> > [ 4698.009190] i8k_smm asm: 1770247 ns    #temp1_input
> > [ 4698.014416] i8k_smm asm: 749734 ns     #temp1_label
> > [ 4698.019103] i8k_smm asm: 2503962 ns    #temp2_input
> > [ 4698.023490] i8k_smm asm: 1738982 ns    #temp2_label
> > 
> > As you can see, the time required to read the speed of the fan
> > is substantially higher than the combined time of all the
> > other reads.
> > 
> > Is the difference so big for the other systems that have been
> > tested?
> 
> It looks like we should not read fan rpm at startup for 
> multiplier autodetection... and only fan nominal speed.
> 
Yes, agreed. After all, we still have the module parameter to override it
if reading the nominal speed (and thus auto-detection) is not successful.

Guenter

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 5751 bytes --]

Ok, here are new patches for testing... Those you are still reading this email thread and have your Dell 
machines near, can you test them (ideally with disabling dmi config data)?

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 8ec4c37..d6e8a26 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -64,9 +64,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 uint i8k_fan_mult;
+static uint i8k_pwm_mult;
+static uint i8k_fan_max = I8K_FAN_HIGH;
 
 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -95,12 +95,12 @@ 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;
-module_param(fan_mult, int, 0);
+static uint fan_mult = I8K_FAN_MULT;
+module_param(fan_mult, uint, 0);
 MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
 
-static int fan_max = I8K_FAN_HIGH;
-module_param(fan_max, int, 0);
+static uint fan_max = I8K_FAN_HIGH;
+module_param(fan_max, uint, 0);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
 
 static int i8k_open_fs(struct inode *inode, struct file *file);
@@ -696,8 +696,8 @@ static int __init i8k_init_hwmon(void)
 }
 
 struct i8k_config_data {
-	int fan_mult;
-	int fan_max;
+	uint fan_mult;
+	uint fan_max;
 };
 
 enum i8k_configs {



diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index d6e8a26..6ad0872 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
  * Hwmon integration:
  * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * 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_MAX_RPM		30000
 #define I8K_MAX_TEMP		127
 
 #define I8K_FN_NONE		0x00
@@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
 static char bios_version[4];
 static struct device *i8k_hwmon_dev;
 static u32 i8k_hwmon_flags;
-static uint i8k_fan_mult;
+static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 
@@ -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 uint fan_mult = I8K_FAN_MULT;
+static uint fan_mult;
 module_param(fan_mult, uint, 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 uint fan_max = I8K_FAN_HIGH;
+static uint fan_max;
 module_param(fan_max, uint, 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);
@@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int 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, };
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+}
+
+/*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
 static int i8k_set_fan(int fan, int speed)
@@ -863,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
 static int __init i8k_probe(void)
 {
 	const struct dmi_system_id *id;
+	int fan, ret;
 
 	/*
 	 * Get DMI information
@@ -891,19 +906,40 @@ 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 */
+	/*
+	 * Set 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;
-
-		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_mult && conf->fan_mult)
+			fan_mult = conf->fan_mult;
+		if (!fan_max && conf->fan_max)
+			fan_max = conf->fan_max;
 	}
+
+	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
 	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
 
+	if (!fan_mult) {
+		/*
+		 * Autodetect fan multiplier based on nominal rpm
+		 * If fan reports rpm value too high then set multiplier to 1
+		 */
+		for (fan = 0; fan < 2; ++fan) {
+			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			if (ret < 0)
+				continue;
+			if (ret > I8K_FAN_MAX_RPM)
+				i8k_fan_mult = 1;
+			break;
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		i8k_fan_mult = fan_mult;
+	}
+
 	return 0;
 }
 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> Ok, here are new patches for testing... Those you are still reading this email thread and have your Dell 
> machines near, can you test them (ideally with disabling dmi config data)?

Could you please tell me exactly against what should I apply these
patches? I can see that they depend on other patches in this thread,
but I can't find any tree that includes them.

> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 8ec4c37..d6e8a26 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -64,9 +64,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 uint i8k_fan_mult;
> +static uint i8k_pwm_mult;
> +static uint i8k_fan_max = I8K_FAN_HIGH;
>  
>  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
>  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> @@ -95,12 +95,12 @@ 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;
> -module_param(fan_mult, int, 0);
> +static uint fan_mult = I8K_FAN_MULT;
> +module_param(fan_mult, uint, 0);
>  MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with");
>  
> -static int fan_max = I8K_FAN_HIGH;
> -module_param(fan_max, int, 0);
> +static uint fan_max = I8K_FAN_HIGH;
> +module_param(fan_max, uint, 0);
>  MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed");
>  
>  static int i8k_open_fs(struct inode *inode, struct file *file);
> @@ -696,8 +696,8 @@ static int __init i8k_init_hwmon(void)
>  }
>  
>  struct i8k_config_data {
> -	int fan_mult;
> -	int fan_max;
> +	uint fan_mult;
> +	uint fan_max;
>  };
>  
>  enum i8k_configs {
> 
> 
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index d6e8a26..6ad0872 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -6,6 +6,7 @@
>   * Hwmon integration:
>   * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
>   * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>   *
>   * 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_MAX_RPM		30000
>  #define I8K_MAX_TEMP		127
>  
>  #define I8K_FN_NONE		0x00
> @@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
>  static char bios_version[4];
>  static struct device *i8k_hwmon_dev;
>  static u32 i8k_hwmon_flags;
> -static uint i8k_fan_mult;
> +static uint i8k_fan_mult = I8K_FAN_MULT;
>  static uint i8k_pwm_mult;
>  static uint i8k_fan_max = I8K_FAN_HIGH;
>  
> @@ -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 uint fan_mult = I8K_FAN_MULT;
> +static uint fan_mult;
>  module_param(fan_mult, uint, 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 uint fan_max = I8K_FAN_HIGH;
> +static uint fan_max;
>  module_param(fan_max, uint, 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);
> @@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int 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, };
> +
> +	regs.ebx = (fan & 0xff) | (speed << 8);
> +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
> +}
> +
> +/*
>   * Set the fan speed (off, low, high). Returns the new fan status.
>   */
>  static int i8k_set_fan(int fan, int speed)
> @@ -863,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
>  static int __init i8k_probe(void)
>  {
>  	const struct dmi_system_id *id;
> +	int fan, ret;
>  
>  	/*
>  	 * Get DMI information
> @@ -891,19 +906,40 @@ 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 */
> +	/*
> +	 * Set 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;
> -
> -		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_mult && conf->fan_mult)
> +			fan_mult = conf->fan_mult;
> +		if (!fan_max && conf->fan_max)
> +			fan_max = conf->fan_max;
>  	}
> +
> +	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>  	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
>  
> +	if (!fan_mult) {
> +		/*
> +		 * Autodetect fan multiplier based on nominal rpm
> +		 * If fan reports rpm value too high then set multiplier to 1
> +		 */
> +		for (fan = 0; fan < 2; ++fan) {
> +			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
> +			if (ret < 0)
> +				continue;
> +			if (ret > I8K_FAN_MAX_RPM)
> +				i8k_fan_mult = 1;
> +			break;
> +		}
> +	} else {
> +		/* Fan multiplier was specified in module param or in dmi */
> +		i8k_fan_mult = fan_mult;
> +	}
> +
>  	return 0;
>  }
>  
> 
> 

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 619 bytes --]

On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > Ok, here are new patches for testing... Those you are still
> > reading this email thread and have your Dell machines near,
> > can you test them (ideally with disabling dmi config data)?
> 
> Could you please tell me exactly against what should I apply
> these patches? I can see that they depend on other patches in
> this thread, but I can't find any tree that includes them.
> 

Two patches in previous email could apply on 3.19-rc1 tree.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > Ok, here are new patches for testing... Those you are still
> > > reading this email thread and have your Dell machines near,
> > > can you test them (ideally with disabling dmi config data)?
> > 
> > Could you please tell me exactly against what should I apply
> > these patches? I can see that they depend on other patches in
> > this thread, but I can't find any tree that includes them.
> > 
> 
> Two patches in previous email could apply on 3.19-rc1 tree.

I see that the second patch you've just sent depends on the followings:
https://patchwork.kernel.org/patch/5524791/ (I8K_SMM_GET_TEMP_TYPE)
https://patchwork.kernel.org/patch/5524731/ (Copyright)

If I'm not wrong, they are not included in 3.19-rc1.

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1168 bytes --]

On Sunday 28 December 2014 17:02:35 Gabriele Mazzotta wrote:
> On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> > On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > > Ok, here are new patches for testing... Those you are
> > > > still reading this email thread and have your Dell
> > > > machines near, can you test them (ideally with
> > > > disabling dmi config data)?
> > > 
> > > Could you please tell me exactly against what should I
> > > apply these patches? I can see that they depend on other
> > > patches in this thread, but I can't find any tree that
> > > includes them.
> > 
> > Two patches in previous email could apply on 3.19-rc1 tree.
> 
> I see that the second patch you've just sent depends on the
> followings: https://patchwork.kernel.org/patch/5524791/
> (I8K_SMM_GET_TEMP_TYPE)
> https://patchwork.kernel.org/patch/5524731/ (Copyright)
> 
> If I'm not wrong, they are not included in 3.19-rc1.

Yes, they are not included in 3.19-rc1 but patches from previous 
mail should work without them.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Sunday 28 December 2014 17:07:43 Pali Rohár wrote:
> On Sunday 28 December 2014 17:02:35 Gabriele Mazzotta wrote:
> > On Sunday 28 December 2014 16:48:54 Pali Rohár wrote:
> > > On Sunday 28 December 2014 16:25:21 Gabriele Mazzotta wrote:
> > > > On Sunday 28 December 2014 09:46:19 Pali Rohár wrote:
> > > > > Ok, here are new patches for testing... Those you are
> > > > > still reading this email thread and have your Dell
> > > > > machines near, can you test them (ideally with
> > > > > disabling dmi config data)?
> > > > 
> > > > Could you please tell me exactly against what should I
> > > > apply these patches? I can see that they depend on other
> > > > patches in this thread, but I can't find any tree that
> > > > includes them.
> > > 
> > > Two patches in previous email could apply on 3.19-rc1 tree.
> > 
> > I see that the second patch you've just sent depends on the
> > followings: https://patchwork.kernel.org/patch/5524791/
> > (I8K_SMM_GET_TEMP_TYPE)
> > https://patchwork.kernel.org/patch/5524731/ (Copyright)
> > 
> > If I'm not wrong, they are not included in 3.19-rc1.
> 
> Yes, they are not included in 3.19-rc1 but patches from previous 
> mail should work without them.

OK, I wanted to double check in case I had something missing.

Patches tested on my XPS13: the correct values for fan_mult and fan_man
are automatically selected.

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 408 bytes --]

On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> OK, I wanted to double check in case I had something missing.
> 
> Patches tested on my XPS13: the correct values for fan_mult
> and fan_man are automatically selected.

Great. Are there any other problems? Now probe time when loading 
module should be lower and could not freeze system, right?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Monday 29 December 2014 13:22:52 Pali Rohár wrote:
> On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> > OK, I wanted to double check in case I had something missing.
> > 
> > Patches tested on my XPS13: the correct values for fan_mult
> > and fan_man are automatically selected.
> 
> Great. Are there any other problems? Now probe time when loading 
> module should be lower and could not freeze system, right?

Yes, the system doesn't hang on module load. The only problem I have
right night now is that the system temporarily hangs when I read or set
the speed of the fan.

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Guenter Roeck]

On Mon, Dec 29, 2014 at 01:50:22PM +0100, Gabriele Mazzotta wrote:
> On Monday 29 December 2014 13:22:52 Pali Rohár wrote:
> > On Sunday 28 December 2014 17:17:14 Gabriele Mazzotta wrote:
> > > OK, I wanted to double check in case I had something missing.
> > > 
> > > Patches tested on my XPS13: the correct values for fan_mult
> > > and fan_man are automatically selected.
> > 
> > Great. Are there any other problems? Now probe time when loading 
> > module should be lower and could not freeze system, right?
> 
> Yes, the system doesn't hang on module load. The only problem I have
> right night now is that the system temporarily hangs when I read or set
> the speed of the fan.

This is a known problem on some Dell laptops; one of mine has the same problem.
Nothing we can do about that, unfortunately. Only thing you can do is
to not read the fan speed.

Guenter

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 917 bytes --]

On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> > Now we have autodetection code for fan multiplier and
> > maximal fan speed so we do not need to have those constants
> > for each laptop in kernel driver code.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > !!!Please do not apply this patch until all affected
> > machines will be tested!!!
> > 
> > I tested autodetection code only on Dell Latitude E6440
> > (where it worked). Other machines which needs to be tested:
> > 
> > Dell Latitude D520
> > Dell Latitude E6540
> > Dell Precision WorkStation 490
> > Dell Studio
> > Dell XPS M140 (MXC051)
> > ---
> 
> Can somebody else with dell laptops test this patch series?
 
Steven Honeyman, can you test this autodetection patch on your 
Latitude E6540?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Steven Honeyman]

On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
>> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
>> > Now we have autodetection code for fan multiplier and
>> > maximal fan speed so we do not need to have those constants
>> > for each laptop in kernel driver code.
>> >
>> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>> > ---
>> > !!!Please do not apply this patch until all affected
>> > machines will be tested!!!
>> >
>> > I tested autodetection code only on Dell Latitude E6440
>> > (where it worked). Other machines which needs to be tested:
>> >
>> > Dell Latitude D520
>> > Dell Latitude E6540
>> > Dell Precision WorkStation 490
>> > Dell Studio
>> > Dell XPS M140 (MXC051)
>> > ---
>>
>> Can somebody else with dell laptops test this patch series?
>
> Steven Honeyman, can you test this autodetection patch on your
> Latitude E6540?

Sure. You picked the best possible time of day to ask (seriously!)
Just applied patches 1,2,3 and so far seems exactly as before so it
looks OK to me.

While I was spamming "sensors" I did notice this (output trimmed for
neatness) but it is unusual my laptop fan is ever off so it might have
always done this and I would have never noticed:
Each line is about 0.5 - 1 second apart:

fan2:        3186 RPM
fan2:        3231 RPM
<temp is low enough, fan stops>
fan2:        10000 RPM
fan2:        9544 RPM
fan2:        10000 RPM
fan2:           0 RPM
(stays at 0 while fan is off)
<temp back up, fan starts>
fan2:        3236 RPM
fan2:        3597 RPM

Ever seen this before? I'd roll back and test again but this room and
laptop are too warm for the fan to stop completely now.


Thanks,
Steven

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Wed, 17 Dec 2014 18:20:21 +0000, Steven Honeyman said:
> On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> wrote:

> >> > Dell Latitude E6540

> >> Can somebody else with dell laptops test this patch series?
> >
> > Steven Honeyman, can you test this autodetection patch on your
> > Latitude E6540?

I'm not Steven, but I have a E6530.  linux-next 20141208
reports fan wonkyness in 'sensors':

i8k-virtual-0
Adapter: Virtual device
fan2:        83070 RPM

I'll have to see if the patch series fixes that.

[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2213 bytes --]

On Wednesday 17 December 2014 19:20:21 Steven Honeyman wrote:
> On 17 December 2014 at 17:54, Pali Rohár <pali.rohar@gmail.com> 
wrote:
> > On Wednesday 10 December 2014 12:51:30 Pali Rohár wrote:
> >> On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> >> > Now we have autodetection code for fan multiplier and
> >> > maximal fan speed so we do not need to have those
> >> > constants for each laptop in kernel driver code.
> >> > 
> >> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >> > ---
> >> > !!!Please do not apply this patch until all affected
> >> > machines will be tested!!!
> >> > 
> >> > I tested autodetection code only on Dell Latitude E6440
> >> > (where it worked). Other machines which needs to be
> >> > tested:
> >> > 
> >> > Dell Latitude D520
> >> > Dell Latitude E6540
> >> > Dell Precision WorkStation 490
> >> > Dell Studio
> >> > Dell XPS M140 (MXC051)
> >> > ---
> >> 
> >> Can somebody else with dell laptops test this patch series?
> > 
> > Steven Honeyman, can you test this autodetection patch on
> > your Latitude E6540?
> 
> Sure. You picked the best possible time of day to ask
> (seriously!) Just applied patches 1,2,3 and so far seems
> exactly as before so it looks OK to me.
> 

Good, thanks for testing. Now we know that my patches detects 
correct multiplier for E6440 and E6540 machines which are in dmi 
list.

> While I was spamming "sensors" I did notice this (output
> trimmed for neatness) but it is unusual my laptop fan is ever
> off so it might have always done this and I would have never
> noticed:
> Each line is about 0.5 - 1 second apart:
> 
> fan2:        3186 RPM
> fan2:        3231 RPM
> <temp is low enough, fan stops>
> fan2:        10000 RPM
> fan2:        9544 RPM
> fan2:        10000 RPM
> fan2:           0 RPM
> (stays at 0 while fan is off)
> <temp back up, fan starts>
> fan2:        3236 RPM
> fan2:        3597 RPM
> 
> Ever seen this before? I'd roll back and test again but this
> room and laptop are too warm for the fan to stop completely
> now.
> 
> 
> Thanks,
> Steven

On my E6440 I have never seen 10000 RPM.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i8k: Remove laptop specific config data (fan_mult, fan_max) from driver

[Gabriele Mazzotta]

On Tuesday 09 December 2014 21:07:01 Pali Rohár wrote:
> Now we have autodetection code for fan multiplier and maximal fan speed so we do
> not need to have those constants for each laptop in kernel driver code.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> !!!Please do not apply this patch until all affected machines will be tested!!!
> 
> I tested autodetection code only on Dell Latitude E6440 (where it worked).
> Other machines which needs to be tested:
> 
> Dell Latitude D520
> Dell Latitude E6540
> Dell Precision WorkStation 490
> Dell Studio
> Dell XPS M140 (MXC051)

Just a note: "or in dmi" has to be removed from a couple of comments.

Gabriele

> ---
>  drivers/char/i8k.c |   88 +---------------------------------------------------
>  1 file changed, 1 insertion(+), 87 deletions(-)
> 
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 8bdbed2..bf74644 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -725,42 +725,6 @@ static int __init i8k_init_hwmon(void)
>  	return 0;
>  }
>  
> -struct i8k_config_data {
> -	int fan_mult;
> -	int fan_max;
> -};
> -
> -enum i8k_configs {
> -	DELL_LATITUDE_D520,
> -	DELL_LATITUDE_E6540,
> -	DELL_PRECISION_490,
> -	DELL_STUDIO,
> -	DELL_XPS_M140,
> -};
> -
> -static const struct i8k_config_data i8k_config_data[] = {
> -	[DELL_LATITUDE_D520] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_TURBO,
> -	},
> -	[DELL_LATITUDE_E6540] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -	[DELL_PRECISION_490] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_TURBO,
> -	},
> -	[DELL_STUDIO] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -	[DELL_XPS_M140] = {
> -		.fan_mult = 1,
> -		.fan_max = I8K_FAN_HIGH,
> -	},
> -};
> -
>  static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  	{
>  		.ident = "Dell Inspiron",
> @@ -784,30 +748,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  		},
>  	},
>  	{
> -		.ident = "Dell Latitude D520",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude D520"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
> -	},
> -	{
> -		.ident = "Dell Latitude E6440",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
> -	},
> -	{
> -		.ident = "Dell Latitude E6540",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
> -	},
> -	{
>  		.ident = "Dell Latitude 2",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> @@ -822,22 +762,13 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  		},
>  	},
>  	{
> -		.ident = "Dell Inspiron 3",
> +		.ident = "Dell Inspiron 4",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "MP061"),
>  		},
>  	},
>  	{
> -		.ident = "Dell Precision 490",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> -			DMI_MATCH(DMI_PRODUCT_NAME,
> -				  "Precision WorkStation 490"),
> -		},
> -		.driver_data = (void *)&i8k_config_data[DELL_PRECISION_490],
> -	},
> -	{
>  		.ident = "Dell Precision",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> @@ -864,7 +795,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "Studio"),
>  		},
> -		.driver_data = (void *)&i8k_config_data[DELL_STUDIO],
>  	},
>  	{
>  		.ident = "Dell XPS M140",
> @@ -872,7 +802,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>  			DMI_MATCH(DMI_PRODUCT_NAME, "MXC051"),
>  		},
> -		.driver_data = (void *)&i8k_config_data[DELL_XPS_M140],
>  	},
>  	{ }
>  };
> @@ -884,8 +813,6 @@ 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;
>  
>  	/*
> @@ -915,19 +842,6 @@ static int __init i8k_probe(void)
>  			return -ENODEV;
>  	}
>  
> -	/*
> -	 * 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) {
> -		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) {
>  		/*
>  		 * Autodetect fan multiplier for each fan based on nominal rpm
> 

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

[Pali Rohár]

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@gmail.com>
Tested-By: Pali Rohár <pali.rohar@gmail.com>
Tested-By: Steven Honeyman <stevenhoneyman@gmail.com>
Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
---
 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@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * 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) {
+		/*
+		 * 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;
+			}
+			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;
+				}
+			}
+		}
+	} 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;
+		}
+	} 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;
 	}
-	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;
 }
-- 
1.7.9.5

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

[Guenter Roeck]

On Fri, Dec 19, 2014 at 07:04:27PM +0100, 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@gmail.com>
> Tested-By: Pali Rohár <pali.rohar@gmail.com>
> Tested-By: Steven Honeyman <stevenhoneyman@gmail.com>
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> ---
>  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@suse.de>
>   * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>   *
>   * 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];
>  
The rationale for this change is not explained in the commit log.

Do you have any indication that those values would ever be different
for the two fans, ie that you actually need arrays here ?

>  #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;
> +

This range check (and probably others) is still unnecessary.

Guenter

>  	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) {
> +		/*
> +		 * 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;
> +			}
> +			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;
> +				}
> +			}
> +		}
> +	} 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;
> +		}
> +	} 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;
>  	}
> -	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;
>  }
> -- 
> 1.7.9.5
> 

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1365 bytes --]

On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
> > -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];
> 
> The rationale for this change is not explained in the commit
> log.
> 
> Do you have any indication that those values would ever be
> different for the two fans, ie that you actually need arrays
> here ?
> 

I do not know... But if we decide to use only single value for 
multiplier and max value which fan to use for autodetection?

> > @@ -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;
> > +
> 
> This range check (and probably others) is still unnecessary.
> 
> Guenter
> 

No, it is necessary. Function i8k_get_fan_speed is called from 
ioctl callback with value which comes from userspace. If 
userspace specify fan out of that array we can get kernel panic.

> >  	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]; +}

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
> > > -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];
> > 
> > The rationale for this change is not explained in the commit
> > log.
> > 
> > Do you have any indication that those values would ever be
> > different for the two fans, ie that you actually need arrays
> > here ?
> > 
> 
> I do not know... But if we decide to use only single value for 
> multiplier and max value which fan to use for autodetection?
> 
That does not answer my question. That you can not decide which
fan to use for auto-detection does not mean that the result of
that auto-detection would be different for different fans.

> > > @@ -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;
> > > +
> > 
> > This range check (and probably others) is still unnecessary.
> > 
> > Guenter
> > 
> 
> No, it is necessary. Function i8k_get_fan_speed is called from 
> ioctl callback with value which comes from userspace. If 
> userspace specify fan out of that array we can get kernel panic.
> 
Yes, but just because you introduced an array for various variables,
and you still have the unnecessary check for other callers.

If you want to return -EINVAL for bad ioctl parameters, add a range
check check there. But that would be a separate patch.

Guenter

> > >  	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]; +}
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2429 bytes --]

On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
> > On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
> > > > -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];
> > > 
> > > The rationale for this change is not explained in the
> > > commit log.
> > > 
> > > Do you have any indication that those values would ever be
> > > different for the two fans, ie that you actually need
> > > arrays here ?
> > 
> > I do not know... But if we decide to use only single value
> > for multiplier and max value which fan to use for
> > autodetection?
> 
> That does not answer my question. That you can not decide
> which fan to use for auto-detection does not mean that the
> result of that auto-detection would be different for
> different fans.
> 

Really I do not know if some dell products which have more fans 
(some Precision models have 2) and each fan is using different 
multiplier or has different max speed value.

> > > > @@ -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;
> > > > +
> > > 
> > > This range check (and probably others) is still
> > > unnecessary.
> > > 
> > > Guenter
> > 
> > No, it is necessary. Function i8k_get_fan_speed is called
> > from ioctl callback with value which comes from userspace.
> > If userspace specify fan out of that array we can get
> > kernel panic.
> 
> Yes, but just because you introduced an array for various
> variables, and you still have the unnecessary check for other
> callers.
> 
> If you want to return -EINVAL for bad ioctl parameters, add a
> range check check there. But that would be a separate patch.
> 
> Guenter
> 

So you want to move that checks into ioctl code and do not do 
checks in smm functions?

> > > >  	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]; +}

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On 12/20/2014 12:57 AM, Pali Rohár wrote:
> On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
>> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
>>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
>>>>> -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];
>>>>
>>>> The rationale for this change is not explained in the
>>>> commit log.
>>>>
>>>> Do you have any indication that those values would ever be
>>>> different for the two fans, ie that you actually need
>>>> arrays here ?
>>>
>>> I do not know... But if we decide to use only single value
>>> for multiplier and max value which fan to use for
>>> autodetection?
>>
>> That does not answer my question. That you can not decide
>> which fan to use for auto-detection does not mean that the
>> result of that auto-detection would be different for
>> different fans.
>>
>
> Really I do not know if some dell products which have more fans
> (some Precision models have 2) and each fan is using different
> multiplier or has different max speed value.
>

"I do not know" is not a reason for introducing such code.

Guenter

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2067 bytes --]

On Saturday 20 December 2014 13:04:03 Guenter Roeck wrote:
> On 12/20/2014 12:57 AM, Pali Rohár wrote:
> > On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
> >> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
> >>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
> >>>>> -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];
> >>>> 
> >>>> The rationale for this change is not explained in the
> >>>> commit log.
> >>>> 
> >>>> Do you have any indication that those values would ever
> >>>> be different for the two fans, ie that you actually need
> >>>> arrays here ?
> >>> 
> >>> I do not know... But if we decide to use only single value
> >>> for multiplier and max value which fan to use for
> >>> autodetection?
> >> 
> >> That does not answer my question. That you can not decide
> >> which fan to use for auto-detection does not mean that the
> >> result of that auto-detection would be different for
> >> different fans.
> > 
> > Really I do not know if some dell products which have more
> > fans (some Precision models have 2) and each fan is using
> > different multiplier or has different max speed value.
> 
> "I do not know" is not a reason for introducing such code.
> 
> Guenter

And having broken fan rpm output in userspace is not good.

My code introduce fan rpm detection for each fan. I do not see 
any problem for doing this detection per fan. You suggest to do 
some global detection and use it for each fan. And for your 
suggestion I have 2 objections:

1) I do not know if global multiplier will work for all fans. 
Detection per fan does not have this problem "I do not know".

2) How to do that global multiplier detection? I have no idea how 
you want to do that.

So I would say, "I do not know" refers yo your suggestion, not to 
my current code.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On 12/20/2014 04:18 AM, Pali Rohár wrote:
> On Saturday 20 December 2014 13:04:03 Guenter Roeck wrote:
>> On 12/20/2014 12:57 AM, Pali Rohár wrote:
>>> On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
>>>> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
>>>>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
>>>>>>> -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];
>>>>>>
>>>>>> The rationale for this change is not explained in the
>>>>>> commit log.
>>>>>>
>>>>>> Do you have any indication that those values would ever
>>>>>> be different for the two fans, ie that you actually need
>>>>>> arrays here ?
>>>>>
>>>>> I do not know... But if we decide to use only single value
>>>>> for multiplier and max value which fan to use for
>>>>> autodetection?
>>>>
>>>> That does not answer my question. That you can not decide
>>>> which fan to use for auto-detection does not mean that the
>>>> result of that auto-detection would be different for
>>>> different fans.
>>>
>>> Really I do not know if some dell products which have more
>>> fans (some Precision models have 2) and each fan is using
>>> different multiplier or has different max speed value.
>>
>> "I do not know" is not a reason for introducing such code.
>>
>> Guenter
>
> And having broken fan rpm output in userspace is not good.
>

Sure, but only if it is in fact broken.

> My code introduce fan rpm detection for each fan. I do not see
> any problem for doing this detection per fan. You suggest to do
> some global detection and use it for each fan. And for your
> suggestion I have 2 objections:
>
The problem is that there is no evidence that a per-fan multiplier
is needed. You may not see it that way, but unnecessary code _is_
a problem since it increases code size for no good reason. Furthermore,
people not involved in this discussion will be inclined to believe
that the code is necessary, in the wrong assumption that it would
not have been introduced unless it was necessary.

> 1) I do not know if global multiplier will work for all fans.
> Detection per fan does not have this problem "I do not know".
>
It has been working all along, with no evidence that it doesn't work.

> 2) How to do that global multiplier detection? I have no idea how
> you want to do that.
>
A single multiplier worked all along. It doesn't matter which fan is
used to auto-detect the multiplier, it is still a single multiplier.

Again, introducing code if you don't know that or if it may be
needed is not an argument for introducing such code. Introduce it
if and when there is evidence that it is needed.

Thanks,
Guenter

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3598 bytes --]

On Saturday 20 December 2014 13:44:59 Guenter Roeck wrote:
> On 12/20/2014 04:18 AM, Pali Rohár wrote:
> > On Saturday 20 December 2014 13:04:03 Guenter Roeck wrote:
> >> On 12/20/2014 12:57 AM, Pali Rohár wrote:
> >>> On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
> >>>> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár 
wrote:
> >>>>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
> >>>>>>> -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];
> >>>>>> 
> >>>>>> The rationale for this change is not explained in the
> >>>>>> commit log.
> >>>>>> 
> >>>>>> Do you have any indication that those values would ever
> >>>>>> be different for the two fans, ie that you actually
> >>>>>> need arrays here ?
> >>>>> 
> >>>>> I do not know... But if we decide to use only single
> >>>>> value for multiplier and max value which fan to use for
> >>>>> autodetection?
> >>>> 
> >>>> That does not answer my question. That you can not decide
> >>>> which fan to use for auto-detection does not mean that
> >>>> the result of that auto-detection would be different for
> >>>> different fans.
> >>> 
> >>> Really I do not know if some dell products which have more
> >>> fans (some Precision models have 2) and each fan is using
> >>> different multiplier or has different max speed value.
> >> 
> >> "I do not know" is not a reason for introducing such code.
> >> 
> >> Guenter
> > 
> > And having broken fan rpm output in userspace is not good.
> 
> Sure, but only if it is in fact broken.
> 
> > My code introduce fan rpm detection for each fan. I do not
> > see any problem for doing this detection per fan. You
> > suggest to do some global detection and use it for each
> > fan. And for your
> 
> > suggestion I have 2 objections:
> The problem is that there is no evidence that a per-fan
> multiplier is needed. You may not see it that way, but
> unnecessary code _is_ a problem since it increases code size
> for no good reason. Furthermore, people not involved in this
> discussion will be inclined to believe that the code is
> necessary, in the wrong assumption that it would not have
> been introduced unless it was necessary.
> 
> > 1) I do not know if global multiplier will work for all
> > fans. Detection per fan does not have this problem "I do
> > not know".
> 
> It has been working all along, with no evidence that it
> doesn't work.
> 
> > 2) How to do that global multiplier detection? I have no
> > idea how you want to do that.
> 
> A single multiplier worked all along. It doesn't matter which
> fan is used to auto-detect the multiplier, it is still a
> single multiplier.
> 
> Again, introducing code if you don't know that or if it may be
> needed is not an argument for introducing such code. Introduce
> it if and when there is evidence that it is needed.
> 
> Thanks,
> Guenter

Ok, so what you suggest now? We have function which reports on 
supported models if speed level X is supported for fan Y and 
which RPM speed Z is expected for that level X. I have no problem 
to (re)write for fan multiplier detection and maximal fan speed 
level, but I need to know how to do that (which approach you 
accept and which not). What I want to have is correct data in 
userspace and better without hardcoded constants for each laptop 
in driver code.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On 12/20/2014 04:54 AM, Pali Rohár wrote:
> On Saturday 20 December 2014 13:44:59 Guenter Roeck wrote:
>> On 12/20/2014 04:18 AM, Pali Rohár wrote:
>>> On Saturday 20 December 2014 13:04:03 Guenter Roeck wrote:
>>>> On 12/20/2014 12:57 AM, Pali Rohár wrote:
>>>>> On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
>>>>>> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár
> wrote:
>>>>>>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
>>>>>>>>> -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];
>>>>>>>>
>>>>>>>> The rationale for this change is not explained in the
>>>>>>>> commit log.
>>>>>>>>
>>>>>>>> Do you have any indication that those values would ever
>>>>>>>> be different for the two fans, ie that you actually
>>>>>>>> need arrays here ?
>>>>>>>
>>>>>>> I do not know... But if we decide to use only single
>>>>>>> value for multiplier and max value which fan to use for
>>>>>>> autodetection?
>>>>>>
>>>>>> That does not answer my question. That you can not decide
>>>>>> which fan to use for auto-detection does not mean that
>>>>>> the result of that auto-detection would be different for
>>>>>> different fans.
>>>>>
>>>>> Really I do not know if some dell products which have more
>>>>> fans (some Precision models have 2) and each fan is using
>>>>> different multiplier or has different max speed value.
>>>>
>>>> "I do not know" is not a reason for introducing such code.
>>>>
>>>> Guenter
>>>
>>> And having broken fan rpm output in userspace is not good.
>>
>> Sure, but only if it is in fact broken.
>>
>>> My code introduce fan rpm detection for each fan. I do not
>>> see any problem for doing this detection per fan. You
>>> suggest to do some global detection and use it for each
>>> fan. And for your
>>
>>> suggestion I have 2 objections:
>> The problem is that there is no evidence that a per-fan
>> multiplier is needed. You may not see it that way, but
>> unnecessary code _is_ a problem since it increases code size
>> for no good reason. Furthermore, people not involved in this
>> discussion will be inclined to believe that the code is
>> necessary, in the wrong assumption that it would not have
>> been introduced unless it was necessary.
>>
>>> 1) I do not know if global multiplier will work for all
>>> fans. Detection per fan does not have this problem "I do
>>> not know".
>>
>> It has been working all along, with no evidence that it
>> doesn't work.
>>
>>> 2) How to do that global multiplier detection? I have no
>>> idea how you want to do that.
>>
>> A single multiplier worked all along. It doesn't matter which
>> fan is used to auto-detect the multiplier, it is still a
>> single multiplier.
>>
>> Again, introducing code if you don't know that or if it may be
>> needed is not an argument for introducing such code. Introduce
>> it if and when there is evidence that it is needed.
>>
>> Thanks,
>> Guenter
>
> Ok, so what you suggest now? We have function which reports on
> supported models if speed level X is supported for fan Y and
> which RPM speed Z is expected for that level X. I have no problem
> to (re)write for fan multiplier detection and maximal fan speed
> level, but I need to know how to do that (which approach you
> accept and which not). What I want to have is correct data in
> userspace and better without hardcoded constants for each laptop
> in driver code.
>

Take your patch, run 's/\[fan\]//', see where it takes you.
Essentially assume that the auto-detected parameters for one fan
apply to all fans. I would think that it should be relatively safe
to assume that the first fan always exists, so you can start with
using that to detect parameters and see where it takes you.

Guenter

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

[Steven Honeyman]

On 20 December 2014 at 17:20, Guenter Roeck <linux@roeck-us.net> wrote:
> I would think that it should be relatively safe
> to assume that the first fan always exists

That would be a bad idea, as this is definitely not the case:

i8k-virtual-0
Adapter: Virtual device
fan2:        3181 RPM
CPU:          +43.0°C
Ambient:      +42.0°C
SODIMM:       +36.0°C


Thanks,
Steven

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

[Guenter Roeck]

On 12/20/2014 09:27 AM, Steven Honeyman wrote:
> On 20 December 2014 at 17:20, Guenter Roeck <linux@roeck-us.net> wrote:
>> I would think that it should be relatively safe
>> to assume that the first fan always exists
>
> That would be a bad idea, as this is definitely not the case:
>
> i8k-virtual-0
> Adapter: Virtual device
> fan2:        3181 RPM
> CPU:          +43.0°C
> Ambient:      +42.0°C
> SODIMM:       +36.0°C
>

Well, then he'll have to loop through fans until finding one which exists.

Guenter

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 718 bytes --]

On Saturday 20 December 2014 19:07:49 Guenter Roeck wrote:
> On 12/20/2014 09:27 AM, Steven Honeyman wrote:
> > On 20 December 2014 at 17:20, Guenter Roeck <linux@roeck-
us.net> wrote:
> >> I would think that it should be relatively safe
> >> to assume that the first fan always exists
> > 
> > That would be a bad idea, as this is definitely not the
> > case:
> > 
> > i8k-virtual-0
> > Adapter: Virtual device
> > fan2:        3181 RPM
> > CPU:          +43.0°C
> > Ambient:      +42.0°C
> > SODIMM:       +36.0°C
> 
> Well, then he'll have to loop through fans until finding one
> which exists.
> 
> Guenter

Ok, I will send new version of patch.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

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@gmail.com>
> Tested-By: Pali Rohár <pali.rohar@gmail.com>
> Tested-By: Steven Honeyman <stevenhoneyman@gmail.com>
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> ---
>   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@suse.de>
>    * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>    *
>    * 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;
>   }
>

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3411 bytes --]

On Saturday 20 December 2014 19:38:32 Guenter Roeck wrote:
> > +	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.
> 

In some other kernel modules negative value means autodetect. I 
think we can do that too.

> > +		/*
> > +		 * 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 ?
> 

See my other email "[PATCH] i8k: Add support for fan labels". DOS 
binary check for fan presence by another function. I bet it is 
because fan could have same problem as temperature sensors. If 
fan is on GPU and GPU is turned off reading fan rpm will fail. 
But my laptop (with GPU which can be turned on/off) does not have 
GPU fan so this is just my assumption.

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

Ok.

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

Ok, I will change code.

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

Now when I reduced fan detection for first available fan code is 
smaller.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Guenter Roeck]

On 12/21/2014 01:13 AM, Pali Rohár wrote:
> On Saturday 20 December 2014 19:38:32 Guenter Roeck wrote:
>>> +	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.
>>
>
> In some other kernel modules negative value means autodetect. I
> think we can do that too.
>
But 0 already implies autodetect, so that would be redundant.

>>> +		/*
>>> +		 * 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 ?
>>
>
> See my other email "[PATCH] i8k: Add support for fan labels". DOS
> binary check for fan presence by another function. I bet it is
> because fan could have same problem as temperature sensors. If
> fan is on GPU and GPU is turned off reading fan rpm will fail.
> But my laptop (with GPU which can be turned on/off) does not have
> GPU fan so this is just my assumption.
>
Good point.

Guenter

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

[Guenter Roeck]

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@gmail.com>
> Tested-By: Pali Rohár <pali.rohar@gmail.com>
> Tested-By: Steven Honeyman <stevenhoneyman@gmail.com>
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>
> ---
>   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@suse.de>
>    * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>    *
>    * 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) {
> +		/*
> +		 * 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;
> +			}

Another note: On one of my systems, reading the current fan speed literally halts
the entire system for a second or two. Given this, is the above really necessary ?
It might be better to just try to use the nominal fan speeds and only read
the actual fan speed if that doesn't work.

> +			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;
> +				}
> +			}

I wonder if it would help to detect the maximum fan speed first. Once that
is known, it should be possible to just read the nominal fan speed once,
for the known maximum speed. With this, the 'val' loop here would be unnecessary.

Thanks,
Guenter

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1279 bytes --]

On Saturday 20 December 2014 20:02:31 Guenter Roeck wrote:
> > +		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;
> > +			}
> 
> Another note: On one of my systems, reading the current fan
> speed literally halts the entire system for a second or two.
> Given this, is the above really necessary ? It might be
> better to just try to use the nominal fan speeds and only
> read the actual fan speed if that doesn't work.
> 

Ok. I added fallback to i8k_get_fan_speed() only if function 
i8k_get_fan_nominal_speed() failed.

> > +			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;
> > +				}
> > +			}
> 
> I wonder if it would help to detect the maximum fan speed
> first. Once that is known, it should be possible to just read
> the nominal fan speed once, for the known maximum speed. With
> this, the 'val' loop here would be unnecessary.
> 

Right, good point.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

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@gmail.com>
Tested-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/char/i8k.c |   89 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 76 insertions(+), 13 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 48d701c..ce2147c 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
  * Hwmon integration:
  * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * 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,13 @@
 #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_MAX_RPM		30000
 #define I8K_MAX_TEMP		127
 
 #define I8K_FN_NONE		0x00
@@ -64,7 +66,7 @@ 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_fan_mult = 30;
 static int i8k_pwm_mult;
 static int i8k_fan_max = I8K_FAN_HIGH;
 
@@ -95,13 +97,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);
@@ -276,6 +278,17 @@ static int i8k_get_fan_speed(int 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, };
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+}
+
+/*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
 static int i8k_set_fan(int fan, int speed)
@@ -854,7 +867,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,19 +898,67 @@ 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 == 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
+		 * Speed value is valid if i8k_get_fan_nominal_speed() not fail
+		 */
+		for (fan = 0; fan < 2; ++fan) {
+			for (val = 1; val < 256; ++val) {
+				if (i8k_get_fan_nominal_speed(fan, val) < 0)
+					break;
+				fan_max = val; /* Must not be 0 */
+			}
+			if (fan_max != 0) {
+				i8k_fan_max = fan_max;
+				break;
+			}
+		}
+	} else {
+		/* Maximal fan speed was specified in module param or in dmi */
+		i8k_fan_max = fan_max;
 	}
+
 	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
 
+	if (fan_mult <= 0) {
+		/*
+		 * Autodetect fan multiplier based on nominal rpm
+		 * If fan 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 < 2; ++fan) {
+			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			if (ret < 0)
+				ret = i8k_get_fan_speed(fan);
+			if (ret < 0)
+				continue;
+			if (ret > I8K_FAN_MAX_RPM)
+				i8k_fan_mult = 1;
+			break;
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		i8k_fan_mult = fan_mult;
+	}
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On 12/21/2014 01:20 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@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>   drivers/char/i8k.c |   89 ++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 76 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index 48d701c..ce2147c 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -6,6 +6,7 @@
>    * Hwmon integration:
>    * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
>    * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
> + * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
>    *
>    * 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,13 @@
>   #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_MAX_RPM		30000
>   #define I8K_MAX_TEMP		127
>
>   #define I8K_FN_NONE		0x00
> @@ -64,7 +66,7 @@ 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_fan_mult = 30;

Why did you drop I8K_FAN_MULT ?

>   static int i8k_pwm_mult;
>   static int i8k_fan_max = I8K_FAN_HIGH;
>
> @@ -95,13 +97,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);
> @@ -276,6 +278,17 @@ static int i8k_get_fan_speed(int 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, };
> +
> +	regs.ebx = (fan & 0xff) | (speed << 8);
> +	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
> +}
> +
> +/*
>    * Set the fan speed (off, low, high). Returns the new fan status.
>    */
>   static int i8k_set_fan(int fan, int speed)
> @@ -854,7 +867,9 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
>    */
>   static int __init i8k_probe(void)
>   {
> +	const struct i8k_config_data *conf;

Why did you move this variable declaration ?

>   	const struct dmi_system_id *id;
> +	int fan, val, ret;
>
>   	/*
>   	 * Get DMI information
> @@ -883,19 +898,67 @@ 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)

I still don't see the value in accepting fan_mult < 0 (compeared to == 0).

> +			fan_mult = conf->fan_mult;
> +		if (fan_max <= 0 && conf->fan_max > 0)

Same for fan_max < 0.

> +			fan_max = conf->fan_max;
> +	}
>
> -		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) {

Same here.

> +		/*
> +		 * Autodetect maximal fan speed value
> +		 * Speed value is valid if i8k_get_fan_nominal_speed() not fail
> +		 */
> +		for (fan = 0; fan < 2; ++fan) {
> +			for (val = 1; val < 256; ++val) {
> +				if (i8k_get_fan_nominal_speed(fan, val) < 0)
> +					break;
> +				fan_max = val; /* Must not be 0 */
> +			}
> +			if (fan_max != 0) {
> +				i8k_fan_max = fan_max;
> +				break;
> +			}
> +		}
> +	} else {
> +		/* Maximal fan speed was specified in module param or in dmi */
> +		i8k_fan_max = fan_max;
>   	}
> +
>   	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
>
> +	if (fan_mult <= 0) {

Same here.

Thanks,
Guenter

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1323 bytes --]

On Sunday 21 December 2014 12:57:08 Guenter Roeck wrote:
> > -#define I8K_FAN_MULT		30
> > +#define I8K_FAN_MAX_RPM		30000
> > 
> >   #define I8K_MAX_TEMP		127
> >   
> >   #define I8K_FN_NONE		0x00
> > 
> > @@ -64,7 +66,7 @@ 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_fan_mult = 30;
> 
> Why did you drop I8K_FAN_MULT ?
> 

Because I think it is not needed anymore... It is used only in 
one place (there ^). But if you want I can revert it back.

> >   static int __init i8k_probe(void)
> >   {
> > 
> > +	const struct i8k_config_data *conf;
> 
> Why did you move this variable declaration ?
> 

Comes from previous version of patches where I moved all 
variables to start of function. I will revert this change.

> > 
> > -		const struct i8k_config_data *conf = id->driver_data;
> > +		conf = id->driver_data;
> > +		if (fan_mult <= 0 && conf->fan_mult > 0)
> 
> I still don't see the value in accepting fan_mult < 0
> (compeared to == 0).
> 

Ok. What kernel driver should do if user load it with negative 
parameter? We should not propagate negative value to functions.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On 12/21/2014 04:09 AM, Pali Rohár wrote:
> On Sunday 21 December 2014 12:57:08 Guenter Roeck wrote:
>>> -#define I8K_FAN_MULT		30
>>> +#define I8K_FAN_MAX_RPM		30000
>>>
>>>    #define I8K_MAX_TEMP		127
>>>
>>>    #define I8K_FN_NONE		0x00
>>>
>>> @@ -64,7 +66,7 @@ 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_fan_mult = 30;
>>
>> Why did you drop I8K_FAN_MULT ?
>>
>
> Because I think it is not needed anymore... It is used only in
> one place (there ^). But if you want I can revert it back.
>
That is not a reason to drop a define.

>>>    static int __init i8k_probe(void)
>>>    {
>>>
>>> +	const struct i8k_config_data *conf;
>>
>> Why did you move this variable declaration ?
>>
>
> Comes from previous version of patches where I moved all
> variables to start of function. I will revert this change.
>
>>>
>>> -		const struct i8k_config_data *conf = id->driver_data;
>>> +		conf = id->driver_data;
>>> +		if (fan_mult <= 0 && conf->fan_mult > 0)
>>
>> I still don't see the value in accepting fan_mult < 0
>> (compeared to == 0).
>>
>
> Ok. What kernel driver should do if user load it with negative
> parameter? We should not propagate negative value to functions.
>
You have multiple options: Ignore it (bad idea ;-), abort loading
the module with -EINVAL, or make the module parameter an unsigned.

I would prefer the latter. Either case, that should be a separate patch
(different logical change).

Thanks,
Guenter

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2061 bytes --]

On Sunday 21 December 2014 13:23:32 Guenter Roeck wrote:
> On 12/21/2014 04:09 AM, Pali Rohár wrote:
> > On Sunday 21 December 2014 12:57:08 Guenter Roeck wrote:
> >>> -#define I8K_FAN_MULT		30
> >>> +#define I8K_FAN_MAX_RPM		30000
> >>> 
> >>>    #define I8K_MAX_TEMP		127
> >>>    
> >>>    #define I8K_FN_NONE		0x00
> >>> 
> >>> @@ -64,7 +66,7 @@ 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_fan_mult = 30;
> >> 
> >> Why did you drop I8K_FAN_MULT ?
> > 
> > Because I think it is not needed anymore... It is used only
> > in one place (there ^). But if you want I can revert it
> > back.
> 
> That is not a reason to drop a define.
> 
> >>>    static int __init i8k_probe(void)
> >>>    {
> >>> 
> >>> +	const struct i8k_config_data *conf;
> >> 
> >> Why did you move this variable declaration ?
> > 
> > Comes from previous version of patches where I moved all
> > variables to start of function. I will revert this change.
> > 
> >>> -		const struct i8k_config_data *conf = id->driver_data;
> >>> +		conf = id->driver_data;
> >>> +		if (fan_mult <= 0 && conf->fan_mult > 0)
> >> 
> >> I still don't see the value in accepting fan_mult < 0
> >> (compeared to == 0).
> > 
> > Ok. What kernel driver should do if user load it with
> > negative parameter? We should not propagate negative value
> > to functions.
> 
> You have multiple options: Ignore it (bad idea ;-), abort
> loading the module with -EINVAL, or make the module parameter
> an unsigned.
> 

And how to make module parameter as unsigned? It is possible?

Code

module_param(fan_mult, unsigned int, 0);

cause compile error:

i8k.c:99:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘int’
i8k.c:99:1: error: ‘param_ops_unsigned’ undeclared here (not in a function)

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Steven Honeyman]

On 21 December 2014 at 16:37, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Sunday 21 December 2014 13:23:32 Guenter Roeck wrote:
>> On 12/21/2014 04:09 AM, Pali Rohár wrote:
>> > On Sunday 21 December 2014 12:57:08 Guenter Roeck wrote:
>> >>> -#define I8K_FAN_MULT             30
>> >>> +#define I8K_FAN_MAX_RPM          30000
>> >>>
>> >>>    #define I8K_MAX_TEMP           127
>> >>>
>> >>>    #define I8K_FN_NONE            0x00
>> >>>
>> >>> @@ -64,7 +66,7 @@ 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_fan_mult = 30;
>> >>
>> >> Why did you drop I8K_FAN_MULT ?
>> >
>> > Because I think it is not needed anymore... It is used only
>> > in one place (there ^). But if you want I can revert it
>> > back.
>>
>> That is not a reason to drop a define.
>>
>> >>>    static int __init i8k_probe(void)
>> >>>    {
>> >>>
>> >>> + const struct i8k_config_data *conf;
>> >>
>> >> Why did you move this variable declaration ?
>> >
>> > Comes from previous version of patches where I moved all
>> > variables to start of function. I will revert this change.
>> >
>> >>> -         const struct i8k_config_data *conf = id->driver_data;
>> >>> +         conf = id->driver_data;
>> >>> +         if (fan_mult <= 0 && conf->fan_mult > 0)
>> >>
>> >> I still don't see the value in accepting fan_mult < 0
>> >> (compeared to == 0).
>> >
>> > Ok. What kernel driver should do if user load it with
>> > negative parameter? We should not propagate negative value
>> > to functions.
>>
>> You have multiple options: Ignore it (bad idea ;-), abort
>> loading the module with -EINVAL, or make the module parameter
>> an unsigned.
>>
>
> And how to make module parameter as unsigned? It is possible?
>
> Code
>
> module_param(fan_mult, unsigned int, 0);
>
> cause compile error:
>
> i8k.c:99:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘int’
> i8k.c:99:1: error: ‘param_ops_unsigned’ undeclared here (not in a function)


module_param(fan_mult, uint, 0);


Steven.

Re: [PATCH v3] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2650 bytes --]

On Sunday 21 December 2014 17:55:51 Steven Honeyman wrote:
> On 21 December 2014 at 16:37, Pali Rohár <pali.rohar@gmail.com> 
wrote:
> > On Sunday 21 December 2014 13:23:32 Guenter Roeck wrote:
> >> On 12/21/2014 04:09 AM, Pali Rohár wrote:
> >> > On Sunday 21 December 2014 12:57:08 Guenter Roeck wrote:
> >> >>> -#define I8K_FAN_MULT             30
> >> >>> +#define I8K_FAN_MAX_RPM          30000
> >> >>> 
> >> >>>    #define I8K_MAX_TEMP           127
> >> >>>    
> >> >>>    #define I8K_FN_NONE            0x00
> >> >>> 
> >> >>> @@ -64,7 +66,7 @@ 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_fan_mult = 30;
> >> >> 
> >> >> Why did you drop I8K_FAN_MULT ?
> >> > 
> >> > Because I think it is not needed anymore... It is used
> >> > only in one place (there ^). But if you want I can
> >> > revert it back.
> >> 
> >> That is not a reason to drop a define.
> >> 
> >> >>>    static int __init i8k_probe(void)
> >> >>>    {
> >> >>> 
> >> >>> + const struct i8k_config_data *conf;
> >> >> 
> >> >> Why did you move this variable declaration ?
> >> > 
> >> > Comes from previous version of patches where I moved all
> >> > variables to start of function. I will revert this
> >> > change.
> >> > 
> >> >>> -         const struct i8k_config_data *conf =
> >> >>> id->driver_data; +         conf = id->driver_data;
> >> >>> +         if (fan_mult <= 0 && conf->fan_mult > 0)
> >> >> 
> >> >> I still don't see the value in accepting fan_mult < 0
> >> >> (compeared to == 0).
> >> > 
> >> > Ok. What kernel driver should do if user load it with
> >> > negative parameter? We should not propagate negative
> >> > value to functions.
> >> 
> >> You have multiple options: Ignore it (bad idea ;-), abort
> >> loading the module with -EINVAL, or make the module
> >> parameter an unsigned.
> > 
> > And how to make module parameter as unsigned? It is
> > possible?
> > 
> > Code
> > 
> > module_param(fan_mult, unsigned int, 0);
> > 
> > cause compile error:
> > 
> > i8k.c:99:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
> > ‘__attribute__’ before ‘int’ i8k.c:99:1: error:
> > ‘param_ops_unsigned’ undeclared here (not in a function)
> 
> module_param(fan_mult, uint, 0);
> 
> 
> Steven.

Thanks! I sent new patch which change module param to uint and 
after then I sent new version v4 of autodetect patch.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

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@gmail.com>
Tested-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/char/i8k.c |   85 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 11 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 175bb9f..49e756e 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
  * Hwmon integration:
  * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * 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_MAX_RPM		30000
 #define I8K_MAX_TEMP		127
 
 #define I8K_FN_NONE		0x00
@@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
 static char bios_version[4];
 static struct device *i8k_hwmon_dev;
 static u32 i8k_hwmon_flags;
-static uint i8k_fan_mult;
+static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 
@@ -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 uint fan_mult = I8K_FAN_MULT;
+static uint fan_mult;
 module_param(fan_mult, uint, 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 uint fan_max = I8K_FAN_HIGH;
+static uint fan_max;
 module_param(fan_max, uint, 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);
@@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int 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, };
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+}
+
+/*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
 static int i8k_set_fan(int fan, int speed)
@@ -855,6 +869,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
 static int __init i8k_probe(void)
 {
 	const struct dmi_system_id *id;
+	int fan, val, ret;
 
 	/*
 	 * Get DMI information
@@ -883,19 +898,67 @@ 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;
+		if (!fan_mult && conf->fan_mult)
+			fan_mult = conf->fan_mult;
+		if (!fan_max && conf->fan_max)
+			fan_max = conf->fan_max;
+	}
 
-		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) {
+		/*
+		 * Autodetect maximal fan speed value
+		 * Speed value is valid if i8k_get_fan_nominal_speed() not fail
+		 */
+		for (fan = 0; fan < 2; ++fan) {
+			for (val = 1; val < 256; ++val) {
+				if (i8k_get_fan_nominal_speed(fan, val) < 0)
+					break;
+				fan_max = val; /* Must not be 0 */
+			}
+			if (fan_max) {
+				i8k_fan_max = fan_max;
+				break;
+			}
+		}
+	} else {
+		/* Maximal fan speed was specified in module param or in dmi */
+		i8k_fan_max = fan_max;
 	}
+
 	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
 
+	if (!fan_mult) {
+		/*
+		 * Autodetect fan multiplier based on nominal rpm
+		 * If fan 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 < 2; ++fan) {
+			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			if (ret < 0)
+				ret = i8k_get_fan_speed(fan);
+			if (ret < 0)
+				continue;
+			if (ret > I8K_FAN_MAX_RPM)
+				i8k_fan_mult = 1;
+			break;
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		i8k_fan_mult = fan_mult;
+	}
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On 12/21/2014 09:23 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@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>

Tested-by the submitter is kind of implied.

Anyway, this patch does not apply to 3.19-rc1, nor to 3.18.
What is your baseline ? Can you rebase to 3.19-rc1 ?
Or do I need to apply some other patch first ?

Thanks,
Guenter

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1850 bytes --]

On Sunday 21 December 2014 19:27:34 Guenter Roeck wrote:
> On 12/21/2014 09:23 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@gmail.com>
> > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Tested-by the submitter is kind of implied.
> 
> Anyway, this patch does not apply to 3.19-rc1, nor to 3.18.
> What is your baseline ? Can you rebase to 3.19-rc1 ?
> Or do I need to apply some other patch first ?
> 
> Thanks,
> Guenter

You need to apply these old patches:
i8k: Add support for temperature sensor labels
i8k: Register only temperature sensors which have labels
i8k: Return -ENODATA for invalid temperature
i8k: Rework error retries

(they are not in 3.18/3.19 yet, but you already reviewed them)

And after that you need to apply new pach which I sent hour ago:
[PATCH] i8k: Make fan module parameters an unsigned

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On 12/21/2014 10:40 AM, Pali Rohár wrote:
> On Sunday 21 December 2014 19:27:34 Guenter Roeck wrote:
>> On 12/21/2014 09:23 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@gmail.com>
>>> Tested-by: Pali Rohár <pali.rohar@gmail.com>
>>
>> Tested-by the submitter is kind of implied.
>>
>> Anyway, this patch does not apply to 3.19-rc1, nor to 3.18.
>> What is your baseline ? Can you rebase to 3.19-rc1 ?
>> Or do I need to apply some other patch first ?
>>
>> Thanks,
>> Guenter
>
> You need to apply these old patches:
> i8k: Add support for temperature sensor labels
> i8k: Register only temperature sensors which have labels
> i8k: Return -ENODATA for invalid temperature
> i8k: Rework error retries
>

I think it would make sense to re-send the entire series at this
point (including my patch and all Reviewed-by: tags). Problem is
that not all patches have a version number (for example there
are multiple versions of the label patch, but the version
I reviewed does not have version information). This makes it
very difficult to find the correct (reviewed) version, and I
suspect that Greg may neither have the time nor the desire
to do it.

Thanks,
Guenter

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2440 bytes --]

On Sunday 21 December 2014 19:51:19 Guenter Roeck wrote:
> On 12/21/2014 10:40 AM, Pali Rohár wrote:
> > On Sunday 21 December 2014 19:27:34 Guenter Roeck wrote:
> >> On 12/21/2014 09:23 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@gmail.com>
> >>> Tested-by: Pali Rohár <pali.rohar@gmail.com>
> >> 
> >> Tested-by the submitter is kind of implied.
> >> 
> >> Anyway, this patch does not apply to 3.19-rc1, nor to 3.18.
> >> What is your baseline ? Can you rebase to 3.19-rc1 ?
> >> Or do I need to apply some other patch first ?
> >> 
> >> Thanks,
> >> Guenter
> > 
> > You need to apply these old patches:
> > i8k: Add support for temperature sensor labels
> > i8k: Register only temperature sensors which have labels
> > i8k: Return -ENODATA for invalid temperature
> > i8k: Rework error retries
> 
> I think it would make sense to re-send the entire series at
> this point (including my patch and all Reviewed-by: tags).
> Problem is that not all patches have a version number (for
> example there are multiple versions of the label patch, but
> the version I reviewed does not have version information).
> This makes it very difficult to find the correct (reviewed)
> version, and I suspect that Greg may neither have the time
> nor the desire to do it.
> 
> Thanks,
> Guenter

Ok, I sent all patches which was not merged to char-misc tree yet

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On 12/21/2014 09:23 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@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>

Auto-detection of both multiplier and maximum speed tested working on M140
(after removing its configuration entry).

On Studio 1555, multiplier auto-detection works, but fan_max auto-detection fails.
A speed value of '3' is accepted, but it does not set the fan speed to its maximum.
Also, after setting the speed value to '3', reading it back returns to old value.
No idea what it does or is expected to do. Reading the nominal speed
does return a valid value.

Given that, I think we should not try to auto-detect fan_max, but keep the current
code (meaning either use 2 or 3 depending on the configuration data, with 2 as default
if nothing else is known).

Thanks,
Guenter

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2317 bytes --]

On Sunday 21 December 2014 20:51:14 Guenter Roeck wrote:
> On 12/21/2014 09:23 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@gmail.com>
> > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Auto-detection of both multiplier and maximum speed tested
> working on M140 (after removing its configuration entry).
> 
> On Studio 1555, multiplier auto-detection works, but fan_max
> auto-detection fails. A speed value of '3' is accepted, but
> it does not set the fan speed to its maximum. Also, after
> setting the speed value to '3', reading it back returns to
> old value. No idea what it does or is expected to do. Reading
> the nominal speed does return a valid value.
> 
> Given that, I think we should not try to auto-detect fan_max,
> but keep the current code (meaning either use 2 or 3
> depending on the configuration data, with 2 as default if
> nothing else is known).
> 
> Thanks,
> Guenter

Ok. In this case I will remove max fan speed detection code and 
we will use config data for those some machines which support 
TURBO speed. If multiplier detection on Studio is working fine, I 
can send another patch which remove config data for Studio 
(because default multiplier is 2).

Can you check which nominal value Studio returns for speed 3? 
Maybe it is some special?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Guenter Roeck]

On Mon, Dec 22, 2014 at 04:07:34PM +0100, Pali Rohár wrote:
> On Sunday 21 December 2014 20:51:14 Guenter Roeck wrote:
> > On 12/21/2014 09:23 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@gmail.com>
> > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > Auto-detection of both multiplier and maximum speed tested
> > working on M140 (after removing its configuration entry).
> > 
> > On Studio 1555, multiplier auto-detection works, but fan_max
> > auto-detection fails. A speed value of '3' is accepted, but
> > it does not set the fan speed to its maximum. Also, after
> > setting the speed value to '3', reading it back returns to
> > old value. No idea what it does or is expected to do. Reading
> > the nominal speed does return a valid value.
> > 
> > Given that, I think we should not try to auto-detect fan_max,
> > but keep the current code (meaning either use 2 or 3
> > depending on the configuration data, with 2 as default if
> > nothing else is known).
> > 
> > Thanks,
> > Guenter
> 
> Ok. In this case I will remove max fan speed detection code and 
> we will use config data for those some machines which support 
> TURBO speed. If multiplier detection on Studio is working fine, I 
> can send another patch which remove config data for Studio 
> (because default multiplier is 2).
> 
> Can you check which nominal value Studio returns for speed 3? 
> Maybe it is some special?
> 
It was a bit higher than the value returned for speed 2. Nothing special
as far as I could see, only that setting a value of 3 did not modify the
fan speed.

Note that I won't be able to test further in the next two weeks;
I am about 6,000 miles away from my Dell laptops right now.

Guenter

Re: [PATCH v4] i8k: Autodetect maximal fan speed and fan RPM multiplier

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 7496 bytes --]

On Tuesday 23 December 2014 14:52:10 Guenter Roeck wrote:
> On Mon, Dec 22, 2014 at 04:07:34PM +0100, Pali Rohár wrote:
> > On Sunday 21 December 2014 20:51:14 Guenter Roeck wrote:
> > > On 12/21/2014 09:23 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@gmail.com>
> > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > Auto-detection of both multiplier and maximum speed tested
> > > working on M140 (after removing its configuration entry).
> > > 
> > > On Studio 1555, multiplier auto-detection works, but
> > > fan_max auto-detection fails. A speed value of '3' is
> > > accepted, but it does not set the fan speed to its
> > > maximum. Also, after setting the speed value to '3',
> > > reading it back returns to old value. No idea what it
> > > does or is expected to do. Reading the nominal speed does
> > > return a valid value.
> > > 
> > > Given that, I think we should not try to auto-detect
> > > fan_max, but keep the current code (meaning either use 2
> > > or 3 depending on the configuration data, with 2 as
> > > default if nothing else is known).
> > > 
> > > Thanks,
> > > Guenter
> > 
> > Ok. In this case I will remove max fan speed detection code
> > and we will use config data for those some machines which
> > support TURBO speed. If multiplier detection on Studio is
> > working fine, I can send another patch which remove config
> > data for Studio (because default multiplier is 2).
> > 
> > Can you check which nominal value Studio returns for speed
> > 3? Maybe it is some special?
> 
> It was a bit higher than the value returned for speed 2.
> Nothing special as far as I could see, only that setting a
> value of 3 did not modify the fan speed.
> 

Ok. I removed detection of maximal fan speed. Patch is below.

> Note that I won't be able to test further in the next two
> weeks; I am about 6,000 miles away from my Dell laptops right
> now.
> 
> Guenter

Ok. When you will be back you can test new version:

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index d6e8a26..f0e21a0 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -6,6 +6,7 @@
  * Hwmon integration:
  * Copyright (C) 2011  Jean Delvare <jdelvare@suse.de>
  * Copyright (C) 2013, 2014  Guenter Roeck <linux@roeck-us.net>
+ * Copyright (C) 2014  Pali Rohár <pali.rohar@gmail.com>
  *
  * 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_MAX_RPM		30000
 #define I8K_MAX_TEMP		127
 
 #define I8K_FN_NONE		0x00
@@ -64,7 +67,7 @@ static DEFINE_MUTEX(i8k_mutex);
 static char bios_version[4];
 static struct device *i8k_hwmon_dev;
 static u32 i8k_hwmon_flags;
-static uint i8k_fan_mult;
+static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 
@@ -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 uint fan_mult = I8K_FAN_MULT;
+static uint fan_mult;
 module_param(fan_mult, uint, 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 uint fan_max = I8K_FAN_HIGH;
+static uint fan_max;
 module_param(fan_max, uint, 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);
@@ -276,6 +279,17 @@ static int i8k_get_fan_speed(int 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, };
+
+	regs.ebx = (fan & 0xff) | (speed << 8);
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+}
+
+/*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
 static int i8k_set_fan(int fan, int speed)
@@ -863,6 +877,7 @@ MODULE_DEVICE_TABLE(dmi, i8k_dmi_table);
 static int __init i8k_probe(void)
 {
 	const struct dmi_system_id *id;
+	int fan, ret;
 
 	/*
 	 * Get DMI information
@@ -891,19 +906,47 @@ 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 */
+	/*
+	 * Set 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;
-
-		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_mult && conf->fan_mult)
+			fan_mult = conf->fan_mult;
+		if (!fan_max && conf->fan_max)
+			fan_max = conf->fan_max;
 	}
+
+	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
 	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
 
+	if (!fan_mult) {
+		/*
+		 * Autodetect fan multiplier based on nominal rpm
+		 * If fan 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 < 2; ++fan) {
+			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			if (ret < 0)
+				ret = i8k_get_fan_speed(fan);
+			if (ret < 0)
+				continue;
+			if (ret > I8K_FAN_MAX_RPM)
+				i8k_fan_mult = 1;
+			break;
+		}
+	} else {
+		/* Fan multiplier was specified in module param or in dmi */
+		i8k_fan_mult = fan_mult;
+	}
+
 	return 0;
 }
 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 2/2] i8k: Remove DMI config data for Latitude E6x40

[Pali Rohár]

Both Dell Latitude machines were tested with new autodetection code and it is
working fine. So we do not need to maintain config data for them anymore.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-By: Pali Rohár <pali.rohar@gmail.com>
Tested-By: Steven Honeyman <stevenhoneyman@gmail.com>
---
 drivers/char/i8k.c |   21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
index 094a6b8..9075bbf 100644
--- a/drivers/char/i8k.c
+++ b/drivers/char/i8k.c
@@ -727,7 +727,6 @@ struct i8k_config_data {
 
 enum i8k_configs {
 	DELL_LATITUDE_D520,
-	DELL_LATITUDE_E6540,
 	DELL_PRECISION_490,
 	DELL_STUDIO,
 	DELL_XPS_M140,
@@ -738,10 +737,6 @@ static const struct i8k_config_data i8k_config_data[] = {
 		.fan_mult = 1,
 		.fan_max = I8K_FAN_TURBO,
 	},
-	[DELL_LATITUDE_E6540] = {
-		.fan_mult = 1,
-		.fan_max = I8K_FAN_HIGH,
-	},
 	[DELL_PRECISION_490] = {
 		.fan_mult = 1,
 		.fan_max = I8K_FAN_TURBO,
@@ -787,22 +782,6 @@ static struct dmi_system_id i8k_dmi_table[] __initdata = {
 		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_D520],
 	},
 	{
-		.ident = "Dell Latitude E6440",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
-		.ident = "Dell Latitude E6540",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
-		},
-		.driver_data = (void *)&i8k_config_data[DELL_LATITUDE_E6540],
-	},
-	{
 		.ident = "Dell Latitude 2",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-- 
1.7.9.5