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 <[email protected]> > --- > 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 >
