[PATCH 0/2] hwmon: (ads7871) Fix concurrency and modernize API

[PATCH 0/2] hwmon: (ads7871) Fix concurrency and modernize API

[Tabrez Ahmed]

This patch series addresses a concurrency issue in the ads7871 driver and 
modernizes it to use the current hwmon API.

Patch 1: Adds a mutex to the driver's private data to serialize SPI 
transactions. Previously, if userspace read multiple 
channels concurrently, the driver could overwrite ongoing REG_GAIN_MUX 
writes before the conversion completed.

Patch 2: Converts the driver to use hwmon_device_register_with_info(). 
This architectural shift removes boilerplate code and delegates sysfs 
management to the hwmon core:
 - Removes manual file creators (SENSOR_DEVICE_ATTR_RO and ads7871_attrs).
 - Implements ads7871_is_visible() for dynamic permission control.
 - Replaces the sysfs-specific voltage_show() with a 
standard ads7871_read() callback.
 - Defines supported channels via hwmon_channel_info.

Tabrez Ahmed (2):
  hwmon: (ads7871) Add mutex to serialize SPI transactions
  hwmon: (ads7871) Convert to hwmon_device_register_with_info

 drivers/hwmon/ads7871.c | 91 +++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 39 deletions(-)

-- 
2.43.0

[PATCH 1/2] hwmon: (ads7871) Add mutex to serialize SPI transactions

[Tabrez Ahmed]

If userspace reads multiple channels concurrently, the driver can
overwrite ongoing REG_GAIN_MUX writes before the conversion completes.

Add a mutex to the driver's private data structure to serialize
hardware access and prevent concurrent userspace reads from corrupting
the ADC multiplexer state. Update error paths to ensure the mutex
is properly unlocked.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
---
 drivers/hwmon/ads7871.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
index 9bfdf9e6bcd77..02e69bef2ca12 100644
--- a/drivers/hwmon/ads7871.c
+++ b/drivers/hwmon/ads7871.c
@@ -59,11 +59,13 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/mutex.h>
 
 #define DEVICE_NAME	"ads7871"
 
 struct ads7871_data {
 	struct spi_device *spi;
+	struct mutex lock;
 };
 
 static int ads7871_read_reg8(struct spi_device *spi, int reg)
@@ -98,6 +100,8 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 	uint8_t channel, mux_cnv;
 
 	channel = attr->index;
+
+	mutex_lock(&pdata->lock);
 	/*
 	 * TODO: add support for conversions
 	 * other than single ended with a gain of 1
@@ -107,11 +111,11 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 	ret = ads7871_write_reg8(spi, REG_GAIN_MUX,
 				 (MUX_CNV_BM | MUX_M3_BM | channel));
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 
 	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
 	if (ret < 0)
-		return ret;
+		goto out_unlock;
 	mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
 	/*
 	 * on 400MHz arm9 platform the conversion
@@ -121,21 +125,27 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 		i++;
 		ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
 		if (ret < 0)
-			return ret;
+			goto out_unlock;
 		mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
 		msleep_interruptible(1);
 	}
 
 	if (mux_cnv == 0) {
 		val = ads7871_read_reg16(spi, REG_LS_BYTE);
-		if (val < 0)
-			return val;
+		if (val < 0) {
+			ret = val;
+			goto out_unlock;
+		}
 		/*result in volts*10000 = (val/8192)*2.5*10000*/
 		val = ((val >> 2) * 25000) / 8192;
-		return sysfs_emit(buf, "%d\n", val);
+		ret = sysfs_emit(buf, "%d\n", val);
+		goto out_unlock;
 	}
 
-	return -ETIMEDOUT;
+	ret = -ETIMEDOUT;
+out_unlock:
+	mutex_unlock(&pdata->lock);
+	return ret;
 }
 
 static SENSOR_DEVICE_ATTR_RO(in0_input, voltage, 0);
@@ -194,7 +204,7 @@ static int ads7871_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	pdata->spi = spi;
-
+	mutex_init(&pdata->lock);
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, spi->modalias,
 							   pdata,
 							   ads7871_groups);
-- 
2.43.0

[PATCH 2/2] hwmon: (ads7871) Convert to hwmon_device_register_with_info

[Tabrez Ahmed]

Convert the driver to use the modern hwmon_device_register_with_info()
API. This removes the need for manual sysfs attribute creation and
simplifies the driver by utilizing the hwmon core's standard callbacks.

Remove the SENSOR_DEVICE_ATTR_RO macros and the attribute group arrays.
Replace the voltage_show() function with an ads7871_read() callback
and define the supported channels via hwmon_channel_info.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
---
 drivers/hwmon/ads7871.c | 71 +++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
index 02e69bef2ca12..8e93e58e9298c 100644
--- a/drivers/hwmon/ads7871.c
+++ b/drivers/hwmon/ads7871.c
@@ -56,7 +56,6 @@
 #include <linux/init.h>
 #include <linux/spi/spi.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
@@ -67,6 +66,15 @@ struct ads7871_data {
 	struct spi_device *spi;
 	struct mutex lock;
 };
+static umode_t ads7871_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	if (type == hwmon_in && attr == hwmon_in_input)
+		return 0444;
+
+	return 0;
+}
 
 static int ads7871_read_reg8(struct spi_device *spi, int reg)
 {
@@ -90,16 +98,16 @@ static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
 	return spi_write(spi, tmp, sizeof(tmp));
 }
 
-static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
-			    char *buf)
+static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
 {
 	struct ads7871_data *pdata = dev_get_drvdata(dev);
 	struct spi_device *spi = pdata->spi;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	int ret, val, i = 0;
-	uint8_t channel, mux_cnv;
+	int ret, raw_val, i = 0;
+	uint8_t mux_cnv;
 
-	channel = attr->index;
+	if (type != hwmon_in || attr != hwmon_in_input)
+		return -EOPNOTSUPP;
 
 	mutex_lock(&pdata->lock);
 	/*
@@ -131,14 +139,14 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 	}
 
 	if (mux_cnv == 0) {
-		val = ads7871_read_reg16(spi, REG_LS_BYTE);
-		if (val < 0) {
-			ret = val;
+		raw_val = ads7871_read_reg16(spi, REG_LS_BYTE);
+		if (raw_val < 0) {
+			ret = raw_val;
 			goto out_unlock;
 		}
 		/*result in volts*10000 = (val/8192)*2.5*10000*/
-		val = ((val >> 2) * 25000) / 8192;
-		ret = sysfs_emit(buf, "%d\n", val);
+		*val = ((raw_val >> 2) * 25000) / 8192;
+		ret = 0;
 		goto out_unlock;
 	}
 
@@ -148,28 +156,22 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 	return ret;
 }
 
-static SENSOR_DEVICE_ATTR_RO(in0_input, voltage, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_input, voltage, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_input, voltage, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_input, voltage, 3);
-static SENSOR_DEVICE_ATTR_RO(in4_input, voltage, 4);
-static SENSOR_DEVICE_ATTR_RO(in5_input, voltage, 5);
-static SENSOR_DEVICE_ATTR_RO(in6_input, voltage, 6);
-static SENSOR_DEVICE_ATTR_RO(in7_input, voltage, 7);
-
-static struct attribute *ads7871_attrs[] = {
-	&sensor_dev_attr_in0_input.dev_attr.attr,
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
-	&sensor_dev_attr_in5_input.dev_attr.attr,
-	&sensor_dev_attr_in6_input.dev_attr.attr,
-	&sensor_dev_attr_in7_input.dev_attr.attr,
+static const struct hwmon_channel_info * const ads7871_info[] = {
+	HWMON_CHANNEL_INFO(in,
+		HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT,
+		HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT),
 	NULL
 };
 
-ATTRIBUTE_GROUPS(ads7871);
+static const struct hwmon_ops ads7871_hwmon_ops = {
+	.is_visible = ads7871_is_visible,
+	.read = ads7871_read,
+};
+
+static const struct hwmon_chip_info ads7871_chip_info = {
+	.ops = &ads7871_hwmon_ops,
+	.info = ads7871_info,
+};
 
 static int ads7871_probe(struct spi_device *spi)
 {
@@ -205,9 +207,10 @@ static int ads7871_probe(struct spi_device *spi)
 
 	pdata->spi = spi;
 	mutex_init(&pdata->lock);
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, spi->modalias,
-							   pdata,
-							   ads7871_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, spi->modalias,
+							 pdata,
+							 &ads7871_chip_info,
+							 NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.43.0

Re: [PATCH 1/2] hwmon: (ads7871) Add mutex to serialize SPI transactions

[Guenter Roeck]

On 3/28/26 11:20, Tabrez Ahmed wrote:
> If userspace reads multiple channels concurrently, the driver can
> overwrite ongoing REG_GAIN_MUX writes before the conversion completes.
> 
> Add a mutex to the driver's private data structure to serialize
> hardware access and prevent concurrent userspace reads from corrupting
> the ADC multiplexer state. Update error paths to ensure the mutex
> is properly unlocked.
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>

When using the with_info API to register the hwmon device, the subsystem already
serializes sysfs accesses. The next patch converts the driver to use the
with_info API. Pleas explain why the extra protection is necessary.

Sashiko has the same feedback:

https://sashiko.dev/#/patchset/20260328182015.220298-1-tabreztalks%40gmail.com

Guenter

> ---
>   drivers/hwmon/ads7871.c | 26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> index 9bfdf9e6bcd77..02e69bef2ca12 100644
> --- a/drivers/hwmon/ads7871.c
> +++ b/drivers/hwmon/ads7871.c
> @@ -59,11 +59,13 @@
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/err.h>
>   #include <linux/delay.h>
> +#include <linux/mutex.h>
>   
>   #define DEVICE_NAME	"ads7871"
>   
>   struct ads7871_data {
>   	struct spi_device *spi;
> +	struct mutex lock;
>   };
>   
>   static int ads7871_read_reg8(struct spi_device *spi, int reg)
> @@ -98,6 +100,8 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
>   	uint8_t channel, mux_cnv;
>   
>   	channel = attr->index;
> +
> +	mutex_lock(&pdata->lock);
>   	/*
>   	 * TODO: add support for conversions
>   	 * other than single ended with a gain of 1
> @@ -107,11 +111,11 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
>   	ret = ads7871_write_reg8(spi, REG_GAIN_MUX,
>   				 (MUX_CNV_BM | MUX_M3_BM | channel));
>   	if (ret < 0)
> -		return ret;
> +		goto out_unlock;
>   
>   	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
>   	if (ret < 0)
> -		return ret;
> +		goto out_unlock;
>   	mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
>   	/*
>   	 * on 400MHz arm9 platform the conversion
> @@ -121,21 +125,27 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
>   		i++;
>   		ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
>   		if (ret < 0)
> -			return ret;
> +			goto out_unlock;
>   		mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
>   		msleep_interruptible(1);
>   	}
>   
>   	if (mux_cnv == 0) {
>   		val = ads7871_read_reg16(spi, REG_LS_BYTE);
> -		if (val < 0)
> -			return val;
> +		if (val < 0) {
> +			ret = val;
> +			goto out_unlock;
> +		}
>   		/*result in volts*10000 = (val/8192)*2.5*10000*/
>   		val = ((val >> 2) * 25000) / 8192;
> -		return sysfs_emit(buf, "%d\n", val);
> +		ret = sysfs_emit(buf, "%d\n", val);
> +		goto out_unlock;
>   	}
>   
> -	return -ETIMEDOUT;
> +	ret = -ETIMEDOUT;
> +out_unlock:
> +	mutex_unlock(&pdata->lock);
> +	return ret;
>   }
>   
>   static SENSOR_DEVICE_ATTR_RO(in0_input, voltage, 0);
> @@ -194,7 +204,7 @@ static int ads7871_probe(struct spi_device *spi)
>   		return -ENOMEM;
>   
>   	pdata->spi = spi;
> -
> +	mutex_init(&pdata->lock);
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, spi->modalias,
>   							   pdata,
>   							   ads7871_groups);

Re: [PATCH 1/2] hwmon: (ads7871) Add mutex to serialize SPI transactions

[Tabrez Ahmed]

On 3/29/26 08:46, Guenter Roeck wrote:
> When using the with_info API to register the hwmon device, the subsystem 
> already
> serializes sysfs accesses. The next patch converts the driver to use the
> with_info API. Pleas explain why the extra protection is necessary.

Ah yes. I originally wrote the mutex patch to fix the race condition in 
the legacy sysfs code, but totally missed that migrating to with_info 
handles the serialization for us natively.

> Sashiko has the same feedback:
> 
> https://sashiko.dev/#/patchset/20260328182015.220298-1- 
> tabreztalks%40gmail.com

Separately, Sashiko flagged a pre-existing bug where 
ads7871_write_reg8() passes a stack-allocated array (u8 tmp[2]) to 
spi_write(), breaking DMA.

To fix this properly, I need to move the buffer into the device's 
private data structure. However, doing so requires serialization to 
prevent concurrent reads from corrupting the shared buffer. Since moving 
to with_info provides that serialization, my plan for the v2 series is:

- Patch 1: Convert to hwmon_device_register_with_info (which natively 
serializes the driver).
- Patch 2: Fix the SPI DMA bug by introducing a shared tx_buf (now 
safely protected by the hwmon core).

I will drop the custom mutex entirely and send out v2 with this structure.

Thanks,
Tabrez