On Thu, May 30, 2019 at 10:21:20AM -0700, Guenter Roeck wrote: >Hi, > >On Thu, May 30, 2019 at 06:45:48AM +0000, Adamski, Krzysztof (Nokia - >PL/Wroclaw) wrote: >> 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]> >> --- >> >> I'm resending this patch to proper recipients this time. Sorry if the >> previous submission confused anybody. >> >> drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c >> b/drivers/hwmon/pmbus/pmbus_core.c >> index ef7ee90ee785..94adbede7912 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -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); > >Should be mutex_unlock(), meaning you have not tested this ;-). > >Either case, I think this is unnecessary. The function is (or should be) >always called with the lock already taken (ie with pmbus_set_sensor() >in the call path). If not, we would need a locked and an unlocked version >of this function to avoid lock recursion.
You've got me :) I did not test that as I do not have a workflow using this. I just stumbled opon this when looking at the code related to my other patches. So it was more like a - "hey, shouldn't there be a lock here?". But I was wrong, thanks. Krzysztof
