Re: [PATCH v8 2/2] hwmon: ina3221: Read channel input source info from DT

From: Guenter Roeck
Date: Sun Sep 30 2018 - 16:55:25 EST


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.

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;

Guenter