Re: [PATCH] hwmon: (pmbus) Add mutex locking for sysfs reads

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Nov 03, 2020 at 01:33:15PM -0600, Robert Hancock wrote:
> As part of commit a919ba06979a7 ("hwmon: (pmbus) Stop caching register
> values"), the update of the sensor value is now triggered directly by the
> sensor attribute value being read from sysfs. This created (or at least
> made much more likely) a locking issue, since nothing protected the device
> page selection from being unexpectedly modified by concurrent reads. If
> sensor values on different pages on the same device were being concurrently
> read by multiple threads, this could cause spurious read errors due to the
> page register not reading back the same value last written, or sensor
> values being read from the incorrect page.
> 
> Add locking of the update_lock mutex in pmbus_show_sensor and
> pmbus_show_samples so that these cannot result in concurrent reads from the
> underlying device.
> 
> Fixes: a919ba06979a7 ("hwmon: (pmbus) Stop caching register values")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> Reviewed-by: Alex Qiu <xqiu@google.com>

Good catch. Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 170a9f82ca61..b0e2820a2d57 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -941,12 +941,16 @@ static ssize_t pmbus_show_sensor(struct device *dev,
>  	struct i2c_client *client = to_i2c_client(dev->parent);
>  	struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
>  	struct pmbus_data *data = i2c_get_clientdata(client);
> +	ssize_t ret;
>  
> +	mutex_lock(&data->update_lock);
>  	pmbus_update_sensor_data(client, sensor);
>  	if (sensor->data < 0)
> -		return sensor->data;
> -
> -	return snprintf(buf, PAGE_SIZE, "%lld\n", pmbus_reg2data(data, sensor));
> +		ret = sensor->data;
> +	else
> +		ret = snprintf(buf, PAGE_SIZE, "%lld\n", pmbus_reg2data(data, sensor));
> +	mutex_unlock(&data->update_lock);
> +	return ret;
>  }
>  
>  static ssize_t pmbus_set_sensor(struct device *dev,
> @@ -2012,8 +2016,11 @@ static ssize_t pmbus_show_samples(struct device *dev,
>  	int val;
>  	struct i2c_client *client = to_i2c_client(dev->parent);
>  	struct pmbus_samples_reg *reg = to_samples_reg(devattr);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
>  
> +	mutex_lock(&data->update_lock);
>  	val = _pmbus_read_word_data(client, reg->page, 0xff, reg->attr->reg);
> +	mutex_unlock(&data->update_lock);
>  	if (val < 0)
>  		return val;
>