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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab3a36dc-fa5a-4669-2f99-aa55a4083db2@roeck-us.net>
Date:   Sun, 30 Sep 2018 13:55:18 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Nicolin Chen <nicoleotsuka@...il.com>, jdelvare@...e.com,
        robh+dt@...nel.org, mark.rutland@....com, corbet@....net
Cc:     afd@...com, linux-hwmon@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v8 2/2] hwmon: ina3221: Read channel input source info
 from DT

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@...il.com>

[ ... ]

> +
> +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