[PATCH v4 4/6] iio: dac: ad5504: introduce local lock to protect state and spi transfers

Taha Ed-Dafili <[email protected]>
Newsgroups org.kernel.vger.linux-iio,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
The AD5504 driver currently lacks locking, exposing it to several
multi-threading race conditions:
1. The shared DMA-safe SPI transfer buffers (st->data) can be
   corrupted if multiple threads trigger read_raw or write_raw
   simultaneously.
2. The ad5504_write_dac_powerdown() routine executes a sequence of
   back-to-back SPI writes (a CTRL register update followed by a
   mandatory NOOP). This entire sequence must be atomic.
3. Internal state variables like pwr_down_mask and pwr_down_mode
   can be read and modified concurrently.

Introduce a mutex in the ad5504_state structure and initialize it via
devm_mutex_init() in probe. Use the modern scoped guard(mutex) macro
at the top-level public IIO callbacks (read_raw, write_raw, and the
powerdown attributes) to safely serialize access to the device state
and the SPI bus.

In ad5504_read_raw() and ad5504_write_raw(), guard(mutex) is scoped to
the IIO_CHAN_INFO_RAW case only, since IIO_CHAN_INFO_SCALE merely reads
vref_mv, which is fixed at probe time and never modified afterward and
therefore needs no serialization. Because guard(mutex) declares a
cleanup-scoped variable, it cannot appear directly after a case label;
wrap the case body in a compound statement (case IIO_CHAN_INFO_RAW: {
... }) to give it the block scope it requires.

Signed-off-by: Taha Ed-Dafili <[email protected]>
---
Note: The concurrency race conditions addressed in this patch were 
originally reported by the Sashiko bot:
https://sashiko.dev/#/patchset/[email protected]

 drivers/iio/dac/ad5504.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 55ce7e49e0e0..87946cdf69e7 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -7,12 +7,14 @@
 
 #include <linux/array_size.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/kstrtox.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
@@ -49,10 +51,12 @@
  * @pwr_down_mask:	power down mask
  * @pwr_down_mode:	current power down mode
  * @data:		transfer buffer
+ * @lock:		lock to protect state and spi transfers
  */
 struct ad5504_state {
 	struct spi_device		*spi;
 	struct regulator		*reg;
+	struct mutex			lock;
 	unsigned short			vref_mv;
 	unsigned			pwr_down_mask;
 	unsigned			pwr_down_mode;
@@ -103,7 +107,8 @@ static int ad5504_read_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (m) {
-	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&st->lock);
 		ret = ad5504_spi_read(st, chan->address);
 		if (ret < 0)
 			return ret;
@@ -111,6 +116,7 @@ static int ad5504_read_raw(struct iio_dev *indio_dev,
 		*val = ret;
 
 		return IIO_VAL_INT;
+	}
 	case IIO_CHAN_INFO_SCALE:
 		*val = st->vref_mv;
 		*val2 = chan->scan_type.realbits;
@@ -128,11 +134,13 @@ static int ad5504_write_raw(struct iio_dev *indio_dev,
 	struct ad5504_state *st = iio_priv(indio_dev);
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&st->lock);
 		if (val >= (1 << chan->scan_type.realbits) || val < 0)
 			return -EINVAL;
 
 		return ad5504_spi_write(st, chan->address, val);
+	}
 	default:
 		return -EINVAL;
 	}
@@ -148,6 +156,7 @@ static int ad5504_get_powerdown_mode(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	return st->pwr_down_mode;
 }
 
@@ -156,6 +165,7 @@ static int ad5504_set_powerdown_mode(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	st->pwr_down_mode = mode;
 
 	return 0;
@@ -173,6 +183,7 @@ static ssize_t ad5504_read_dac_powerdown(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	return sysfs_emit(buf, "%d\n",
 			  !(st->pwr_down_mask & (1 << chan->channel)));
 }
@@ -185,6 +196,7 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev,
 	int ret;
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	ret = kstrtobool(buf, &pwr_down);
 	if (ret)
 		return ret;
@@ -284,6 +296,10 @@ static int ad5504_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
 	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
 	if (ret < 0)
 		return ret;
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.