On Sun, Sep 30, 2018 at 01:55:18PM -0700, Guenter Roeck wrote:
> Hi Nicolin,
>
> On 09/28/2018 06:39 PM, Nicolin Chen wrote:
> >An ina3221 chip has three input ports. Each port is used
> >to measure the voltage and current of its input source.
> >
> >The DT binding now has defined bindings for their input
> >sources, so the driver should read these information and
> >handle accordingly.
> >
> >This patch adds a new structure of input source specific
> >information including input source label, shunt resistor
> >value and its connection status. It exposes these labels
> >via in[123]_label sysfs nodes upon available, and also
> >disables those channels where there are no input source
> >being connected. Meanwhile, it also adds in[123]_enable
> >sysfs nodes for users to get control of three channels,
> >and returns -ENODATA code for any sensor read according
> >to hwmon ABI.
> >
> >Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
>
> [ ... ]
>
> >+
> >+static ssize_t ina3221_set_enable(struct device *dev,
> >+ struct device_attribute *attr,
> >+ const char *buf, size_t count)
> >+{
> >+ struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> >+ struct ina3221_data *ina = dev_get_drvdata(dev);
> >+ unsigned int channel = sd_attr->index;
> >+ u16 config = ina->reg_config;
> >+ bool enable;
> >+ int ret;
> >+
> >+ ret = kstrtobool(buf, &enable);
> >+ if (ret)
> >+ return ret;
> >+
> >+ if (enable)
> >+ config |= INA3221_CONFIG_CHx_EN(channel);
> >+ else
> >+ config &= ~INA3221_CONFIG_CHx_EN(channel);
> >+
> >+ ret = regmap_write(ina->regmap, INA3221_CONFIG, config);
> >+ if (ret)
> >+ return ret;
> >+
> >+ ina->reg_config = config;
> >+
>
> Sorry it too me so long to realize this: The code above is racy.
> There could be multiple threads enabling and disabling a channel.
> Without a mutex, there is no guarantee that the final value of
> reg_config matches the value written into the chip.
Hmm..that's true...glad you found and pointed it out.
> You'll have to introduce a mutex and implement something like
> ret = kstrtobool(buf, &enable);
> if (ret)
> return ret;
>
> mutex_lock(&ina->update_lock);
> config = ina->reg_config;
> ...
> ret = regmap_write(ina->regmap, INA3221_CONFIG, config);
> if (ret) {
> count = ret;
> goto error;
> }
> ina->reg_config = config;
> error:
> mutex_unlock(&ina->update_lock);
> return count;
I would prefer to just add another regmap_read at the end for
the reg_config sync, and to use regmap_update_bits() for the
bit set/clear. It might not serialize things as the solution
above does but at least the reg_config would get the correct
value eventually. Since regmap has an internal mutex already,
we may get rid of another mutex by doing so.
I will send a v9 tomorrow.
Thanks for the review
Nicolin
