Skip to contentSkip to conversation

hwmon (ina3221) Add mutex lock to shunt nodes

PATCH In review Nicolin Chen 2 participants 3 messages Nov 13, 2018 Hardware monitoring

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 hwmon (ina3221) Add mutex lock to shunt nodes

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.

---
 drivers/hwmon/ina3221.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/hwmon/ina3221.c +15 -2
index 86281afd2619..f7a09ab6c440 100644
@@ -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;
 }
 
Signature · 1 line
2.17.1
maintainer
#1 lore raw
Quoted 8 lines · Nicolin Chen

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

Quoted 51 lines
---
 drivers/hwmon/ina3221.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/hwmon/ina3221.c +15 -2
index 86281afd2619..f7a09ab6c440 100644
@@ -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;
 }
 
Signature · 1 line
2.17.1
#2 lore raw
Quoted 7 lines · Guenter Roeck
Quoted 6 lines · Nicolin Chen

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.

Quoted 8 lines
Quoted 2 lines

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.

The penalty here is that the resistor value would be mismatched between here and the ongoing calculation when a race happens to the sysfs accesses. But I feel it might not be really that hurt to allow it happen.

Let's just drop it then: the mutex was used to protect register read/writes any way.

Thanks
Nicolin