[PATCH v3 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[PATCH v3 0/2] staging: iio: ad7816: Fix DMA from stack and race conditions

[Abdelnasser Hussein]

This patch series addresses two issues in the ad7816 driver:
1. Adds a mutex to prevent race conditions during SPI reads.
2. Moves the DMA buffer from the stack to the device structure and
   aligns it properly using IIO_DMA_MINALIGN.

Changes in v3:
- Fixed a formatting issue in the v2 cover letter that caused it to be sent empty. No code changes.

Changes in v2:
- Added a mutex to protect the SPI read sequence.
- Used __aligned(IIO_DMA_MINALIGN) for the DMA buffer.
- Corrected the sizeof() argument in spi_read().
- Added the missing Fixes: tag.

Abdelnasser Hussein (2):
  staging: iio: ad7816: serialize ad7816_spi_read() with a mutex
  staging: iio: ad7816: avoid DMA from stack in spi_read

 drivers/staging/iio/adc/ad7816.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-- 
2.54.0

[PATCH v3 1/2] staging: greybus: audio_codec: fix sscanf return value check

[Abdelnasser Hussein]

Smatch static checker warns:
drivers/staging/greybus/audio_codec.c:335 gbaudio_module_update()
warn: sscanf doesn't return error codes

The sscanf() function returns the number of successfully matched input
items, not a negative error code. Compare the return value directly
with the expected number of conversions (3) instead of storing it in
'ret' and returning it as an error code, which leads to returning
a positive value on failure.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Closes: https://lore.kernel.org/all/YoOLnDkHgVltyXK7@kili/

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
Changes in v3:
- Split from the previous v2 patch into a separate patch.
- Updated tags to properly credit Dan Carpenter for the smatch warning.

 drivers/staging/greybus/audio_codec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 720aa752e17e..295222ec0f1a 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -311,8 +311,7 @@ int gbaudio_module_update(struct gbaudio_codec_info *codec,
 	}
 
 	/* parse dai_id from AIF widget's stream_name */
-	ret = sscanf(w->sname, "%s %d %s", intf_name, &dai_id, dir);
-	if (ret < 3) {
+	if (sscanf(w->sname, "%s %d %s", intf_name, &dai_id, dir) != 3) {
 		dev_err(codec->dev, "Error while parsing dai_id for %s\n", w->name);
 		return -EINVAL;
 	}
-- 
2.54.0

Re: [PATCH v3 1/2] staging: greybus: audio_codec: fix sscanf return value check

[Greg KH]

On Mon, Aug 03, 2026 at 04:43:43PM +0300, Abdelnasser Hussein wrote:
> Smatch static checker warns:
> drivers/staging/greybus/audio_codec.c:335 gbaudio_module_update()
> warn: sscanf doesn't return error codes
> 
> The sscanf() function returns the number of successfully matched input
> items, not a negative error code. Compare the return value directly
> with the expected number of conversions (3) instead of storing it in
> 'ret' and returning it as an error code, which leads to returning
> a positive value on failure.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Closes: https://lore.kernel.org/all/YoOLnDkHgVltyXK7@kili/
> 
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
> Changes in v3:
> - Split from the previous v2 patch into a separate patch.
> - Updated tags to properly credit Dan Carpenter for the smatch warning.
> 
>  drivers/staging/greybus/audio_codec.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index 720aa752e17e..295222ec0f1a 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -311,8 +311,7 @@ int gbaudio_module_update(struct gbaudio_codec_info *codec,
>  	}
>  
>  	/* parse dai_id from AIF widget's stream_name */
> -	ret = sscanf(w->sname, "%s %d %s", intf_name, &dai_id, dir);
> -	if (ret < 3) {
> +	if (sscanf(w->sname, "%s %d %s", intf_name, &dai_id, dir) != 3) {
>  		dev_err(codec->dev, "Error while parsing dai_id for %s\n", w->name);
>  		return -EINVAL;
>  	}
> -- 
> 2.54.0
> 

You didn't cc: the staging list?  And the blank line above your
signed-off-by isn't going to work well.  Please, when you split patches
up, be mindful of what tree they are going to.

thanks,

greg k-h

[PATCH v3 1/2] staging: iio: ad7816: serialize ad7816_spi_read() with a mutex

[Abdelnasser Hussein]

The ad7816_spi_read() path performs a sequence of SPI transfers and GPIO
state changes that must not be interleaved with another read operation.

Without serialization, concurrent callers can interfere with each other,
leading to inconsistent device state and incorrect data being returned.

Add a mutex to struct ad7816_chip_info and hold it across the entire read
sequence to ensure exclusive access to the device.

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 0e32a2295990..b5a0c2871e00 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 lock; /* protect device state during SPI transfers */
 };
 
 enum ad7816_type {
@@ -67,11 +68,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	int ret;
 	__be16 buf;
 
+	mutex_lock(&chip->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");
+		mutex_unlock(&chip->lock);
 		return ret;
 	}
 	gpiod_set_value(chip->rdwr_pin, 1);
@@ -94,11 +98,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 	ret = spi_read(spi_dev, &buf, sizeof(*data));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
+		mutex_unlock(&chip->lock);
+
 		return ret;
 	}
 
 	*data = be16_to_cpu(buf);
-
+	mutex_unlock(&chip->lock);
 	return ret;
 }
 
@@ -359,7 +365,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
 	if (!indio_dev)
 		return -ENOMEM;
 	chip = iio_priv(indio_dev);
-
+	mutex_init(&chip->lock);
 	chip->spi_dev = spi_dev;
 	for (i = 0; i <= AD7816_CS_MAX; i++)
 		chip->oti_data[i] = 203;
-- 
2.54.0

Re: [PATCH v3 1/2] staging: iio: ad7816: serialize ad7816_spi_read() with a mutex

[Jonathan Cameron]

On Mon,  3 Aug 2026 16:43:44 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> The ad7816_spi_read() path performs a sequence of SPI transfers and GPIO
> state changes that must not be interleaved with another read operation.
> 
> Without serialization, concurrent callers can interfere with each other,
> leading to inconsistent device state and incorrect data being returned.
> 
> Add a mutex to struct ad7816_chip_info and hold it across the entire read
> sequence to ensure exclusive access to the device.
> 
> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>

Split the two ad7816 out as a series on their own.

> ---
>  drivers/staging/iio/adc/ad7816.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0e32a2295990..b5a0c2871e00 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 lock; /* protect device state during SPI transfers */
>  };
>  
>  enum ad7816_type {
> @@ -67,11 +68,14 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  	int ret;
>  	__be16 buf;
>  
> +	mutex_lock(&chip->lock);

Use guard(mutex)(&chip->lock);
and adjust the rest as appropriate, including adding cleanup.h if it
isn't already there.




> +
>  	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");
> +		mutex_unlock(&chip->lock);
>  		return ret;
>  	}
>  	gpiod_set_value(chip->rdwr_pin, 1);
> @@ -94,11 +98,13 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  	ret = spi_read(spi_dev, &buf, sizeof(*data));
>  	if (ret < 0) {
>  		dev_err(&spi_dev->dev, "SPI data read error\n");
> +		mutex_unlock(&chip->lock);
> +
>  		return ret;
>  	}
>  
>  	*data = be16_to_cpu(buf);
> -
> +	mutex_unlock(&chip->lock);
>  	return ret;
>  }
>  
> @@ -359,7 +365,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	chip = iio_priv(indio_dev);
> -
> +	mutex_init(&chip->lock);
	
	ret = devm_mutex_init(&chip->lock);
	if (ret)
		return ret;

>  	chip->spi_dev = spi_dev;
>  	for (i = 0; i <= AD7816_CS_MAX; i++)
>  		chip->oti_data[i] = 203;

Re: [PATCH v3 1/2] staging: iio: ad7816: serialize ad7816_spi_read() with a mutex

[nasser]

Thanks for the review. I will update the code to use guard(mutex) and
devm_mutex_init() in v4

[PATCH v3 2/2] staging: greybus: audio_codec: remove redundant else-if check

[Abdelnasser Hussein]

In gbaudio_module_update(), the widget id is validated earlier in the
function to ensure it is either snd_soc_dapm_aif_in or
snd_soc_dapm_aif_out.

Remove the redundant else-if check for snd_soc_dapm_aif_out. The
remaining branch can only handle snd_soc_dapm_aif_out, which avoids
a compiler warning about a potentially uninitialized variable.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202606140347.gGVWDnbi-lkp@intel.com/

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
Changes in v3:
- Split from the previous v2 patch to address the uninitialized variable
  warning separately.

 drivers/staging/greybus/audio_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 295222ec0f1a..6daa4e706792 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -322,7 +322,7 @@ int gbaudio_module_update(struct gbaudio_codec_info *codec,
 			ret = gbaudio_module_enable_tx(codec, module, dai_id);
 		else
 			ret = gbaudio_module_disable_tx(module, dai_id);
-	} else if (w->id == snd_soc_dapm_aif_out) {
+	} else {
 		if (enable)
 			ret = gbaudio_module_enable_rx(codec, module, dai_id);
 		else
-- 
2.54.0

[PATCH v3 2/2] staging: iio: ad7816: avoid DMA from stack in spi_read

[Abdelnasser Hussein]

The SPI transfer buffer is allocated on the stack, which is unsafe when
the SPI core performs DMA transfers. With VMAP_STACK enabled, this can
lead to DMA mapping failures because the stack is not guaranteed to be
DMA-accessible.

Move the buffer into struct ad7816_chip_info to provide storage with an
appropriate lifetime for DMA, align it with
__aligned(IIO_DMA_MINALIGN), and update the spi_read() sizeof() argument
to reference the relocated buffer.

Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")

Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
---
 drivers/staging/iio/adc/ad7816.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index b5a0c2871e00..c58a6bf77020 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -51,6 +51,7 @@ struct ad7816_chip_info {
 	u8  channel_id;	/* 0 always be temperature */
 	u8  mode;
 	struct mutex lock; /* protect device state during SPI transfers */
+	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
 };
 
 enum ad7816_type {
@@ -66,7 +67,6 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 {
 	struct spi_device *spi_dev = chip->spi_dev;
 	int ret;
-	__be16 buf;
 
 	mutex_lock(&chip->lock);
 
@@ -95,7 +95,7 @@ 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));
+	ret = spi_read(spi_dev, &chip->rx_buf, sizeof(chip->rx_buf));
 	if (ret < 0) {
 		dev_err(&spi_dev->dev, "SPI data read error\n");
 		mutex_unlock(&chip->lock);
@@ -103,7 +103,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 		return ret;
 	}
 
-	*data = be16_to_cpu(buf);
+	*data = be16_to_cpu(chip->rx_buf);
 	mutex_unlock(&chip->lock);
 	return ret;
 }
-- 
2.54.0

Re: [PATCH v3 2/2] staging: iio: ad7816: avoid DMA from stack in spi_read

[Jonathan Cameron]

On Mon,  3 Aug 2026 16:43:46 +0300
Abdelnasser Hussein <abdelnasserhussein11@gmail.com> wrote:

> The SPI transfer buffer is allocated on the stack, which is unsafe when
> the SPI core performs DMA transfers. With VMAP_STACK enabled, this can
> lead to DMA mapping failures because the stack is not guaranteed to be
> DMA-accessible.

Hmm. That's a new one for me. I was just thinking it was the cache coherency
problems that meant we shouldn't DMA to/from the stack because we can't
guarantee what is going to be in the cacheline.  Looking briefly into
it it seems this is another good reason not to do it.

> 
> Move the buffer into struct ad7816_chip_info to provide storage with an
> appropriate lifetime for DMA, align it with

Why is lifetime relevant here?  The on stack data was fine lifetime wise.

> __aligned(IIO_DMA_MINALIGN), and update the spi_read() sizeof() argument
> to reference the relocated buffer.
> 
> Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
> 
No blank lines in commit block. Sometimes I just fix these up when picking
patches up, but sometimes I get grumpy and bounce them back to submitter
to fix up in a new version.

> Signed-off-by: Abdelnasser Hussein <abdelnasserhussein11@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7816.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index b5a0c2871e00..c58a6bf77020 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -51,6 +51,7 @@ struct ad7816_chip_info {
>  	u8  channel_id;	/* 0 always be temperature */
>  	u8  mode;
>  	struct mutex lock; /* protect device state during SPI transfers */
> +	__be16 rx_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  enum ad7816_type {
> @@ -66,7 +67,6 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  {
>  	struct spi_device *spi_dev = chip->spi_dev;
>  	int ret;
> -	__be16 buf;
>  
>  	mutex_lock(&chip->lock);
>  
> @@ -95,7 +95,7 @@ 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));
> +	ret = spi_read(spi_dev, &chip->rx_buf, sizeof(chip->rx_buf));
>  	if (ret < 0) {
>  		dev_err(&spi_dev->dev, "SPI data read error\n");
>  		mutex_unlock(&chip->lock);
> @@ -103,7 +103,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  		return ret;
>  	}
>  
> -	*data = be16_to_cpu(buf);
> +	*data = be16_to_cpu(chip->rx_buf);
>  	mutex_unlock(&chip->lock);
>  	return ret;
>  }

Re: [PATCH v3 2/2] staging: iio: ad7816: avoid DMA from stack in spi_read

[nasser]

Hi  Jonathan


Thanks for the review.

> The ad7816_spi_read() path performs a sequence of SPI transfers and GPIO
> state changes that must not be interleaved with another read operation.
>
> Split the two ad7816 out as a series on their own.

Sure, I will split these two patches into a separate series in v4.

> +     mutex_lock(&chip->lock);
>
> Use guard(mutex)(&chip->lock);
> and adjust the rest as appropriate, including adding cleanup.h if it
> isn't already there.

Thanks for pointing this out. I will update the code to use guard(mutex)
and devm_mutex_init() to handle the cleanup automatically.

> Move the buffer into struct ad7816_chip_info to provide storage with an
> appropriate lifetime for DMA, align it with
>
> Why is lifetime relevant here?  The on stack data was fine lifetime wise.

Right, I'll update the commit message to focus on cache coherency .

> Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
>
> No blank lines in commit block. Sometimes I just fix these up when picking
> patches up, but sometimes I get grumpy and bounce them back to submitter
> to fix up in a new version.

Noted. I will make sure to remove the blank line between the tags.

I will send v4 shortly.

Thanks,
Abdelnasser