[PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Mohammad Amin Hosseini]

From: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>

The ad7816 driver was accessing SPI and GPIO lines without
synchronization, which could lead to race conditions when accessed
concurrently from multiple contexts. This might result in corrupted
readings or inconsistent GPIO states.

Introduce an io_lock mutex in the driver structure to serialize:
- SPI transactions in ad7816_spi_read() and ad7816_spi_write()
- GPIO pin toggling sequences
- Updates to device state via sysfs store functions (mode, channel, oti)

The mutex ensures proper mutual exclusion and prevents race
conditions under concurrent access.

Changes in v2:
- Fixed mismatch between From: and Signed-off-by lines

Signed-off-by: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 4774df778de9..06567d048a6d 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -50,6 +50,7 @@ struct ad7816_chip_info {
 	u8  oti_data[AD7816_CS_MAX + 1];
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
+	struct mutex io_lock;	/* Protects SPI transactions and GPIO toggling */
 };
 
 enum ad7816_type {
@@ -67,13 +68,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	int ret;
 	__be16 buf;
 
+	mutex_lock(&chip->io_lock);
+
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
 	ret = spi_write(spi_dev, &chip->channel_id, sizeof(chip->channel_id));
-	if (ret < 0) {
-		dev_err(&spi_dev->dev, "SPI channel setting error\n");
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 	gpiod_set_value(chip->rdwr_pin, 1);
 
 	if (chip->mode == AD7816_PD) { /* operating mode 2 */
@@ -92,13 +93,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	gpiod_set_value(chip->rdwr_pin, 0);
 	gpiod_set_value(chip->rdwr_pin, 1);
 	ret = spi_read(spi_dev, &buf, sizeof(*data));
-	if (ret < 0) {
-		dev_err(&spi_dev->dev, "SPI data read error\n");
-		return ret;
-	}
+	if (ret < 0)
+		goto unlock;
 
 	*data = be16_to_cpu(buf);
 
+unlock:
+	mutex_unlock(&chip->io_lock);
 	return ret;
 }
 
@@ -107,12 +108,13 @@ static int ad7816_spi_write(struct ad7816_chip_info *chip, u8 data)
 	struct spi_device *spi_dev = chip->spi_dev;
 	int ret;
 
+	mutex_lock(&chip->io_lock);
+
 	gpiod_set_value(chip->rdwr_pin, 1);
 	gpiod_set_value(chip->rdwr_pin, 0);
 	ret = spi_write(spi_dev, &data, sizeof(data));
-	if (ret < 0)
-		dev_err(&spi_dev->dev, "SPI oti data write error\n");
 
+	mutex_unlock(&chip->io_lock);
 	return ret;
 }
 
@@ -136,6 +138,7 @@ static ssize_t ad7816_store_mode(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7816_chip_info *chip = iio_priv(indio_dev);
 
+	mutex_lock(&chip->io_lock);
 	if (strcmp(buf, "full") == 0) {
 		gpiod_set_value(chip->rdwr_pin, 1);
 		chip->mode = AD7816_FULL;
@@ -143,6 +146,7 @@ static ssize_t ad7816_store_mode(struct device *dev,
 		gpiod_set_value(chip->rdwr_pin, 0);
 		chip->mode = AD7816_PD;
 	}
+	mutex_unlock(&chip->io_lock);
 
 	return len;
 }
@@ -200,7 +204,9 @@ static ssize_t ad7816_store_channel(struct device *dev,
 		return -EINVAL;
 	}
 
+	mutex_lock(&chip->io_lock);
 	chip->channel_id = data;
+	mutex_unlock(&chip->io_lock);
 
 	return len;
 }
@@ -322,7 +328,9 @@ static inline ssize_t ad7816_set_oti(struct device *dev,
 	if (ret)
 		return -EIO;
 
+	mutex_lock(&chip->io_lock);
 	chip->oti_data[chip->channel_id] = data;
+	mutex_unlock(&chip->io_lock);
 
 	return len;
 }
@@ -363,6 +371,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
 	dev_set_drvdata(&spi_dev->dev, indio_dev);
 
 	chip->spi_dev = spi_dev;
+	mutex_init(&chip->io_lock);
 	for (i = 0; i <= AD7816_CS_MAX; i++)
 		chip->oti_data[i] = 203;
 
-- 
2.43.0

Re: [PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Greg KH]

On Mon, Sep 01, 2025 at 10:24:45AM +0330, Mohammad Amin Hosseini wrote:
> From: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>
> 
> The ad7816 driver was accessing SPI and GPIO lines without
> synchronization, which could lead to race conditions when accessed
> concurrently from multiple contexts. This might result in corrupted
> readings or inconsistent GPIO states.
> 
> Introduce an io_lock mutex in the driver structure to serialize:
> - SPI transactions in ad7816_spi_read() and ad7816_spi_write()
> - GPIO pin toggling sequences
> - Updates to device state via sysfs store functions (mode, channel, oti)
> 
> The mutex ensures proper mutual exclusion and prevents race
> conditions under concurrent access.
> 
> Changes in v2:
> - Fixed mismatch between From: and Signed-off-by lines
> 
> Signed-off-by: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>

I think your old "From:" line was correct, as capital letters are
proper, right?

Also, the "v2" information goes below the --- line.

thanks,

greg k-h

Re: [PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Dan Carpenter]

On Mon, Sep 01, 2025 at 10:24:45AM +0330, Mohammad Amin Hosseini wrote:
> From: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>
> 
> The ad7816 driver was accessing SPI and GPIO lines without
> synchronization, which could lead to race conditions when accessed
> concurrently from multiple contexts. This might result in corrupted
> readings or inconsistent GPIO states.
> 
> Introduce an io_lock mutex in the driver structure to serialize:
> - SPI transactions in ad7816_spi_read() and ad7816_spi_write()
> - GPIO pin toggling sequences
> - Updates to device state via sysfs store functions (mode, channel, oti)
> 
> The mutex ensures proper mutual exclusion and prevents race
> conditions under concurrent access.
> 

The commit message says "might result", which basically implies this
is from reviewing the code and that we don't have a stack trace?  Is
that correct?

Are you using some kind of static checker to find racy code?  What
kind of heuristic is it using to find buggy code?

> Changes in v2:
> - Fixed mismatch between From: and Signed-off-by lines
> 
> Signed-off-by: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>

Please add a Fixes tag.

> @@ -200,7 +204,9 @@ static ssize_t ad7816_store_channel(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&chip->io_lock);
>  	chip->channel_id = data;
> +	mutex_unlock(&chip->io_lock);
>  
>  	return len;
>  }
> @@ -322,7 +328,9 @@ static inline ssize_t ad7816_set_oti(struct device *dev,
>  	if (ret)
>  		return -EIO;
>  
> +	mutex_lock(&chip->io_lock);
>  	chip->oti_data[chip->channel_id] = data;
> +	mutex_unlock(&chip->io_lock);
>  
>  	return len;
>  }

I'm not really knowledgeable to review the others, if they are
required or how the locking is supposed to work.  But these aren't
correct because we're only locking around the writers and not the
readers so it could still race.

regards,
dan carpenter

Re: [PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 01:23:40PM +0300, Dan Carpenter wrote:
> On Mon, Sep 01, 2025 at 10:24:45AM +0330, Mohammad Amin Hosseini wrote:
> > From: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>
> > 
> > The ad7816 driver was accessing SPI and GPIO lines without
> > synchronization, which could lead to race conditions when accessed
> > concurrently from multiple contexts. This might result in corrupted
> > readings or inconsistent GPIO states.
> > 
> > Introduce an io_lock mutex in the driver structure to serialize:
> > - SPI transactions in ad7816_spi_read() and ad7816_spi_write()
> > - GPIO pin toggling sequences
> > - Updates to device state via sysfs store functions (mode, channel, oti)
> > 
> > The mutex ensures proper mutual exclusion and prevents race
> > conditions under concurrent access.

...

> > +	mutex_lock(&chip->io_lock);
> >  	chip->channel_id = data;
> > +	mutex_unlock(&chip->io_lock);

> > +	mutex_lock(&chip->io_lock);
> >  	chip->oti_data[chip->channel_id] = data;
> > +	mutex_unlock(&chip->io_lock);

> I'm not really knowledgeable to review the others, if they are
> required or how the locking is supposed to work.  But these aren't
> correct because we're only locking around the writers and not the
> readers so it could still race.

Readers are in spi_write(), or what do you imply by this comment?
I.o.w. I do not see the issue with the idea of locking and how it's
done (I haven't checked all of the details, though).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Dan Carpenter]

On Mon, Sep 01, 2025 at 03:40:13PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 01, 2025 at 01:23:40PM +0300, Dan Carpenter wrote:
> > On Mon, Sep 01, 2025 at 10:24:45AM +0330, Mohammad Amin Hosseini wrote:
> > > From: mohammad amin hosseini <moahmmad.hosseinii@gmail.com>
> > > 
> > > The ad7816 driver was accessing SPI and GPIO lines without
> > > synchronization, which could lead to race conditions when accessed
> > > concurrently from multiple contexts. This might result in corrupted
> > > readings or inconsistent GPIO states.
> > > 
> > > Introduce an io_lock mutex in the driver structure to serialize:
> > > - SPI transactions in ad7816_spi_read() and ad7816_spi_write()
> > > - GPIO pin toggling sequences
> > > - Updates to device state via sysfs store functions (mode, channel, oti)
> > > 
> > > The mutex ensures proper mutual exclusion and prevents race
> > > conditions under concurrent access.
> 
> ...
> 
> > > +	mutex_lock(&chip->io_lock);
> > >  	chip->channel_id = data;
> > > +	mutex_unlock(&chip->io_lock);
> 
> > > +	mutex_lock(&chip->io_lock);
> > >  	chip->oti_data[chip->channel_id] = data;
> > > +	mutex_unlock(&chip->io_lock);
> 
> > I'm not really knowledgeable to review the others, if they are
> > required or how the locking is supposed to work.  But these aren't
> > correct because we're only locking around the writers and not the
> > readers so it could still race.
> 
> Readers are in spi_write(), or what do you imply by this comment?
> I.o.w. I do not see the issue with the idea of locking and how it's
> done (I haven't checked all of the details, though).

Sorry, I meant we don't have locking in ad7816_show_oti(), for example.

regards,
dan carpenter

Re: [PATCH v2] staging: iio: adc: ad7816: add mutex to serialize SPI/GPIO operations

[Dan Carpenter]

On Mon, Sep 01, 2025 at 10:24:45AM +0330, Mohammad Amin Hosseini wrote:
> @@ -200,7 +204,9 @@ static ssize_t ad7816_store_channel(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&chip->io_lock);
>  	chip->channel_id = data;
> +	mutex_unlock(&chip->io_lock);
>  
>  	return len;
>  }

Unrelated, but what is the point of setting chip->channel_id to
AD7816_CS_MASK?  The only time where that is used is in
ad7816_spi_read() when we do:

	ret = spi_write(spi_dev, &chip->channel_id, sizeof(chip->channel_id));

So it's something in the hardware spec, I guess, but it isn't
documented.

regards,
dan carpenter