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@xxxxxxxxxx>
> Reviewed-by: Alex Qiu <xqiu@xxxxxxxxxx>
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;
>
