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

[Guenter Roeck]

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