Skip to contentSkip to conversation

hwmon: pmbus: protect read-modify-write with lock

PATCH In review Adamski, Krzysztof (Nokia - PL/Wroclaw) 2 participants 4 messages May 28, 2019 – Jun 10, 2019

Subsystems (2)
I2C subsystem host drivers Hardware monitoring
lore
Share & tools
lore

Adamski, Krzysztof (Nokia - PL/Wroclaw)

author
#0 lore raw

PATCH 1/1 hwmon: pmbus: protect read-modify-write with lock

The operation done in the pmbus_update_fan() function is a read-modify-write operation but it lacks any kind of lock protection which may cause problems if run more than once simultaneously. This patch uses an existing update_lock mutex to fix this problem.

Signed-off-by Krzysztof Adamski <[email protected]>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/hwmon/pmbus/pmbus_core.c +8 -3
index ef7ee90ee785..94adbede7912 100644
@@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 	int rv;
 	u8 to;
 
+	mutex_lock(&data->update_lock);
 	from = pmbus_read_byte_data(client, page,
 				    pmbus_fan_config_registers[id]);
 	if (from < 0)

@@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		rv = pmbus_write_byte_data(client, page,
 					   pmbus_fan_config_registers[id], to);
 		if (rv < 0)
-			return rv;
+			goto out;
 	}
 
-	return _pmbus_write_word_data(client, page,
-				      pmbus_fan_command_registers[id], command);
+	rv = _pmbus_write_word_data(client, page,
+				    pmbus_fan_command_registers[id], command);
+
+out:
+	mutex_lock(&data->update_lock);
+	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_update_fan);
Signature · 1 line
2.20.1
#1 lore raw
Quoted 6 lines · Krzysztof (Nokia - PL/Wroclaw)

The operation done in the pmbus_update_fan() function is a read-modify-write operation but it lacks any kind of lock protection which may cause problems if run more than once simultaneously. This patch uses an existing update_lock mutex to fix this problem.

Signed-off-by Krzysztof Adamski <[email protected]>

Please use get_maintainer to find the people responsible for this file. It is not my realm.

drivers/hwmon/pmbus/pmbus_core.c int pmbus_update_fan(struct i2c_client *client, int page, int id, · line 289

Quoted 2 lines
drivers/hwmon/pmbus/pmbus_core.c +2 -0
+out:
+	mutex_lock(&data->update_lock);

Despite the above, have you tested the code? This likely should be mutex_unlock?

author
#2 lore raw
Quoted 8 lines · Wolfram Sang
Quoted 6 lines · Krzysztof (Nokia - PL/Wroclaw)

The operation done in the pmbus_update_fan() function is a read-modify-write operation but it lacks any kind of lock protection which may cause problems if run more than once simultaneously. This patch uses an existing update_lock mutex to fix this problem.

Signed-off-by Krzysztof Adamski <[email protected]>
Quoted 9 lines

Please use get_maintainer to find the people responsible for this file. It is not my realm.

drivers/hwmon/pmbus/pmbus_core.c int pmbus_update_fan(struct i2c_client *client, int page, int id, · line 289

Quoted 2 lines
drivers/hwmon/pmbus/pmbus_core.c +2 -0
+out:
+	mutex_lock(&data->update_lock);

Despite the above, have you tested the code? This likely should be mutex_unlock?

Sorry for that, I made a mistake and used wrong command to generate the patch and send it to the wrong list of people. You can ignore this patchset, it was resent to the proper mailing list instead.

Krzysztof

#3 lore raw

Hi Krzysztof,

Quoted 2 lines

patch and send it to the wrong list of people. You can ignore this patchset, it was resent to the proper mailing list instead.

Thanks for the heads up.

Regards,

Wolfram