Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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 <krzysztof.adamski@xxxxxxxxx>
>> ---
>>
>> 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