hwmon: (ads7871) Fix concurrency and modernize API

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

Posted by Tabrez Ahmed 4 days, 18 hours ago

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

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

Posted by Guenter Roeck 4 days, 9 hours ago

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

Posted by Tabrez Ahmed 4 days, 7 hours ago

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