Skip to contentSkip to conversation

hwmon: (f75375s) Catch some attempts to write to r/o registers

PATCH Applied Nikolaus Schulz 2 participants 7 messages Feb 28, 2012 – Mar 3, 2012

Subsystems (2)
Hardware monitoring Fintek F75375S hardware monitor and fan controller driver
lore
Share & tools
lore

Nikolaus Schulz

author
#0 lore raw

PATCH v2 4/4 hwmon: (f75375s) Catch some attempts to write to r/o registers

It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EINVAL instead of silently doing nothing.

Signed-off-by Nikolaus Schulz <[email protected]>
---
 drivers/hwmon/f75375s.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
drivers/hwmon/f75375s.c +27 -0
index 3fee82dc..5374501 100644
@@ -278,6 +278,21 @@ static bool duty_mode_enabled(u8 pwm_enable)
 	}
 }
 
+static bool auto_mode_enabled(u8 pwm_enable)
+{
+	switch (pwm_enable) {
+	case 0: /* Manual, duty mode (full speed) */
+	case 1: /* Manual, duty mode */
+	case 3: /* Manual, speed mode */
+		return false;
+	case 2: /* Auto, speed mode */
+	case 4: /* Auto, duty mode */
+		return true;
+	default:
+		BUG();
+	}
+}
+
 static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {

@@ -311,6 +326,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
+	if (auto_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+	if (data->kind = f75387 && duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
 	data->fan_target[nr] = rpm_to_reg(val);
 	f75375_write16(client, F75375_REG_FAN_EXP(nr), data->fan_target[nr]);

@@ -331,6 +351,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
+	if (auto_mode_enabled(data->pwm_enable[nr]) ||
+			!duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
 	data->pwm[nr] = SENSORS_LIMIT(val, 0, 255);
 	f75375_write_pwm(client, nr, data->pwm[nr]);

@@ -792,6 +816,9 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 	set_pwm_enable_direct(client, 0, f75375s_pdata->pwm_enable[0]);
 	set_pwm_enable_direct(client, 1, f75375s_pdata->pwm_enable[1]);
 	for (nr = 0; nr < 2; nr++) {
+		if (auto_mode_enabled(f75375s_pdata->pwm_enable[nr]) ||
+		   !duty_mode_enabled(f75375s_pdata->pwm_enable[nr]))
+			continue;
 		data->pwm[nr] = SENSORS_LIMIT(f75375s_pdata->pwm[nr], 0, 255);
 		f75375_write_pwm(client, nr, data->pwm[nr]);
 	}
Signature · 7 lines
1.7.9.1


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#1 lore raw
Quoted 6 lines · Nikolaus Schulz

It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EINVAL instead of silently doing nothing.

Signed-off-by Nikolaus Schulz <[email protected]>

Same as before - we can either hold the patch for 3.4, or split it into two parts. Let me know which way you prefer.

Thanks,
Guenter

Quoted 66 lines
---
 drivers/hwmon/f75375s.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
drivers/hwmon/f75375s.c +27 -0
index 3fee82dc..5374501 100644
@@ -278,6 +278,21 @@ static bool duty_mode_enabled(u8 pwm_enable)
 	}
 }
 
+static bool auto_mode_enabled(u8 pwm_enable)
+{
+	switch (pwm_enable) {
+	case 0: /* Manual, duty mode (full speed) */
+	case 1: /* Manual, duty mode */
+	case 3: /* Manual, speed mode */
+		return false;
+	case 2: /* Auto, speed mode */
+	case 4: /* Auto, duty mode */
+		return true;
+	default:
+		BUG();
+	}
+}
+
 static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {

@@ -311,6 +326,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
+	if (auto_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+	if (data->kind = f75387 && duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
 	data->fan_target[nr] = rpm_to_reg(val);
 	f75375_write16(client, F75375_REG_FAN_EXP(nr), data->fan_target[nr]);

@@ -331,6 +351,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
+	if (auto_mode_enabled(data->pwm_enable[nr]) ||
+			!duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
 	data->pwm[nr] = SENSORS_LIMIT(val, 0, 255);
 	f75375_write_pwm(client, nr, data->pwm[nr]);

@@ -792,6 +816,9 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 	set_pwm_enable_direct(client, 0, f75375s_pdata->pwm_enable[0]);
 	set_pwm_enable_direct(client, 1, f75375s_pdata->pwm_enable[1]);
 	for (nr = 0; nr < 2; nr++) {
+		if (auto_mode_enabled(f75375s_pdata->pwm_enable[nr]) ||
+		   !duty_mode_enabled(f75375s_pdata->pwm_enable[nr]))
+			continue;
 		data->pwm[nr] = SENSORS_LIMIT(f75375s_pdata->pwm[nr], 0, 255);
 		f75375_write_pwm(client, nr, data->pwm[nr]);
 	}
Signature · 1 line
1.7.9.1

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#2 lore raw
In reply to Nikolaus Schulz
Quoted 6 lines · Nikolaus Schulz

It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EINVAL instead of silently doing nothing.

Signed-off-by Nikolaus Schulz <[email protected]>

Applied (with minor formatting cleanup) to -next.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#3 lore raw
In reply to Nikolaus Schulz
Quoted 10 lines · Guenter Roeck
Quoted 6 lines · Nikolaus Schulz

It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EINVAL instead of silently doing nothing.

Signed-off-by Nikolaus Schulz <[email protected]>

Same as before - we can either hold the patch for 3.4, or split it into two parts. Let me know which way you prefer.

I see that this patch can indeed be split.

Quoted 34 lines
Quoted 34 lines
---
 drivers/hwmon/f75375s.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
drivers/hwmon/f75375s.c +15 -0
index 3fee82dc..5374501 100644
@@ -278,6 +278,21 @@ static bool duty_mode_enabled(u8 pwm_enable)
 	}
 }
 
+static bool auto_mode_enabled(u8 pwm_enable)
+{
+	switch (pwm_enable) {
+	case 0: /* Manual, duty mode (full speed) */
+	case 1: /* Manual, duty mode */
+	case 3: /* Manual, speed mode */
+		return false;
+	case 2: /* Auto, speed mode */
+	case 4: /* Auto, duty mode */
+		return true;
+	default:
+		BUG();
+	}
+}
+
 static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {

@@ -311,6 +326,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
Quoted 2 lines

drivers/hwmon/f75375s.c static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr, · line 330

Quoted 2 lines
drivers/hwmon/f75375s.c +2 -0
+	if (auto_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;

This is harmless as the register is r/o in this case, so the change need not go into v3.3.

Quoted 3 lines

drivers/hwmon/f75375s.c static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr, · line 333

Quoted 3 lines
drivers/hwmon/f75375s.c +3 -0
+	if (data->kind = f75387 && duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+

This is blocking a dangerous operation, so it is a good candidate for v3.3.

Quoted 11 lines

drivers/hwmon/f75375s.c static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, · line 357

Quoted 11 lines
drivers/hwmon/f75375s.c +4 -0
 	mutex_lock(&data->update_lock);
 	data->fan_target[nr] = rpm_to_reg(val);
 	f75375_write16(client, F75375_REG_FAN_EXP(nr), data->fan_target[nr]);

@@ -331,6 +351,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
+	if (auto_mode_enabled(data->pwm_enable[nr]) ||
+			!duty_mode_enabled(data->pwm_enable[nr]))
+		return -EINVAL;
+

This is combination of the two cases above. The check for auto mode is again only cosmetic, but the check for duty mode is blocking a dangerous operation for the F75387.

Quoted 10 lines

drivers/hwmon/f75375s.c static void f75375_init(struct i2c_client *client, struct f75375_data *data, · line 821

Quoted 10 lines
drivers/hwmon/f75375s.c +3 -0
 	mutex_lock(&data->update_lock);
 	data->pwm[nr] = SENSORS_LIMIT(val, 0, 255);
 	f75375_write_pwm(client, nr, data->pwm[nr]);

@@ -792,6 +816,9 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 	set_pwm_enable_direct(client, 0, f75375s_pdata->pwm_enable[0]);
 	set_pwm_enable_direct(client, 1, f75375s_pdata->pwm_enable[1]);
 	for (nr = 0; nr < 2; nr++) {
+		if (auto_mode_enabled(f75375s_pdata->pwm_enable[nr]) ||
+		   !duty_mode_enabled(f75375s_pdata->pwm_enable[nr]))
+			continue;

This is again combining a cosmetic and a safety check.

Quoted 5 lines
Quoted 5 lines
drivers/hwmon/f75375s.c +0 -0
 		data->pwm[nr] = SENSORS_LIMIT(f75375s_pdata->pwm[nr], 0, 255);
 		f75375_write_pwm(client, nr, data->pwm[nr]);
 	}
Signature · 1 line
1.7.9.1

How shall we proceed here? I can split this, but then I see that you have already applied the current patch to -next.

-- Nikolaus

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#4 lore raw
In reply to Nikolaus Schulz

On Fri, 2012-03-02 at 16:59 -0500, Nikolaus Schulz wrote:
[ ... ]

Quoted 4 lines

How shall we proceed here? I can split this, but then I see that you have already applied the current patch to -next.

With the other patch applied to 3.3, it made sense to apply this entire patch to 3.3 as well. So I did, and already sent it off to Linus.

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#5 lore raw
In reply to Nikolaus Schulz
Quoted 6 lines · Guenter Roeck
Quoted 3 lines · Nikolaus Schulz

How shall we proceed here? I can split this, but then I see that you have already applied the current patch to -next.

With the other patch applied to 3.3, it made sense to apply this entire patch to 3.3 as well. So I did, and already sent it off to Linus.

Excellent. What a relief! I was really worried about this. Thank you.

Nikolaus

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#6 lore raw
In reply to Nikolaus Schulz
Quoted 10 lines · Nikolaus Schulz
Quoted 6 lines · Guenter Roeck
Quoted 3 lines · Nikolaus Schulz

How shall we proceed here? I can split this, but then I see that you have already applied the current patch to -next.

With the other patch applied to 3.3, it made sense to apply this entire patch to 3.3 as well. So I did, and already sent it off to Linus.

Excellent. What a relief! I was really worried about this. Thank you.

... and thanks for your persistence.

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors