On Mon, Nov 12, 2018 at 08:23:24PM -0800, Nicolin Chen wrote:
> The shunt resistor values are used to calculate shunt voltages
> and currents. As a part of sysfs nodes, it would be better to
> get protected with the same mutex too as other sysfs ABI nodes,
> although this is not very critical because the mutex was added
> to mainly protect register access.
>
> So this patch adds the mutex lock to protect the shunt node.
>
I am missing something here. I don't see the point of this mutex.
It just reads a variable and reports the result. When setting,
it just writes a value. Protecting the conversion of the passed
value with a mutex is pointless. Protecting writing the value
against reading it is just as pointless.
Guenter
> Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
> ---
> drivers/hwmon/ina3221.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 86281afd2619..f7a09ab6c440 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -532,8 +532,15 @@ static ssize_t ina3221_show_shunt(struct device *dev,
> struct ina3221_data *ina = dev_get_drvdata(dev);
> unsigned int channel = sd_attr->index;
> struct ina3221_input *input = &ina->inputs[channel];
> + ssize_t ret;
>
> - return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
> + mutex_lock(&ina->lock);
> +
> + ret = snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
> +
> + mutex_unlock(&ina->lock);
> +
> + return ret;
> }
>
> static ssize_t ina3221_set_shunt(struct device *dev,
> @@ -547,14 +554,20 @@ static ssize_t ina3221_set_shunt(struct device *dev,
> int val;
> int ret;
>
> + mutex_lock(&ina->lock);
> +
> ret = kstrtoint(buf, 0, &val);
> - if (ret)
> + if (ret) {
> + mutex_unlock(&ina->lock);
> return ret;
> + }
>
> val = clamp_val(val, 1, INT_MAX);
>
> input->shunt_resistor = val;
>
> + mutex_unlock(&ina->lock);
> +
> return count;
> }
>
> --
> 2.17.1
>
