[PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Radu Sabau via B4 Relay]

From: Radu Sabau <radu.sabau@analog.com>

Commit 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous
ready signals") introduced a new out_unlock: label for an early-exit
path and moved sigma_delta->keep_cs_asserted = false there.
Unfortunately it left ad_sigma_delta_disable_one() in the out: block
above that label, so disable_one()'s SPI write is issued while
keep_cs_asserted is still true.  keep_cs_asserted feeds directly into
the cs_change field of the spi_transfer, so the final write to the
device carries cs_change=1.  The SPI framework only calls spi_set_cs()
to deassert CS as part of message teardown when cs_change is not set on
the last transfer; with cs_change=1, that teardown is skipped and CS
stays physically asserted.  No further transfer to the device is made,
so the driver never deasserts CS.  The framework provides no automatic
CS deassert on bus unlock or when switching to another device, so CS
remains stuck low until the next SPI operation to that same device.

On a shared SPI bus this is problematic: with CS stuck low the ADC
remains selected and subsequent SPI traffic intended for another device
is interpreted as commands by the ADC.  Devices like the AD7124 that
multiplex /RDY onto the MISO line will start spurious conversions and
pull MISO low, corrupting reads from the other device and causing a
deadlock.

Fix by moving ad_sigma_delta_disable_one() into the out_unlock: block,
after keep_cs_asserted is cleared.  The final SPI write now carries
cs_change=0, so the SPI framework deasserts CS as part of normal
message teardown.

As a side effect this also calls disable_one() on the early-exit path
when ad_sigma_delta_clear_pending_event() fails.  That path already had
ad_sigma_delta_set_channel() called, so calling disable_one() to undo
it is the right thing to do and is consistent with ad_sd_calibrate().

Fixes: 132d44dc6966 ("iio: adc: ad_sigma_delta: Check for previous ready signals")
Signed-off-by: Radu Sabau <radu.sabau@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index a955556f9ec8..43aa296922c6 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -442,10 +442,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 	ad_sd_disable_irq(sigma_delta);
 
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
-	ad_sigma_delta_disable_one(sigma_delta, chan->address);
 
 out_unlock:
 	sigma_delta->keep_cs_asserted = false;
+	ad_sigma_delta_disable_one(sigma_delta, chan->address);
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 out_release:

---
base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0

Best regards,
-- 
Radu Sabau <radu.sabau@analog.com>

Re: [PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

Hello,

On Tue, Apr 28, 2026 at 04:45:46PM +0300, Radu Sabau via B4 Relay wrote:
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index a955556f9ec8..43aa296922c6 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -442,10 +442,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  	ad_sd_disable_irq(sigma_delta);
>  
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> -	ad_sigma_delta_disable_one(sigma_delta, chan->address);
>  
>  out_unlock:
>  	sigma_delta->keep_cs_asserted = false;
> +	ad_sigma_delta_disable_one(sigma_delta, chan->address);
>  	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  out_release:

LGTM,

Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Thanks for fixing my patch
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Sabau, Radu bogdan]

> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -442,10 +442,10 @@ int ad_sigma_delta_single_conversion(struct
> iio_dev *indio_dev,
>  	ad_sd_disable_irq(sigma_delta);
> 
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> -	ad_sigma_delta_disable_one(sigma_delta, chan->address);
> 
>  out_unlock:
>  	sigma_delta->keep_cs_asserted = false;
> +	ad_sigma_delta_disable_one(sigma_delta, chan->address);
>  	sigma_delta->bus_locked = false;
>  	spi_bus_unlock(sigma_delta->spi->controller);
>  out_release:
> 
> ---
> base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
> change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0
> 
> Best regards,
> --
> Radu Sabau <radu.sabau@analog.com>
> 

Hi everyone,

Uhmm..., why was this fix marked as Changes Requested? Is it something I am
missing?

Best Regards,
Radu

RE: [PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Sabau, Radu bogdan <Radu.Sabau@analog.com>
> Sent: Thursday, May 7, 2026 3:17 PM

...

> >
> 
> Hi everyone,
> 
> Uhmm..., why was this fix marked as Changes Requested? Is it something I am
> missing?
> 
> Best Regards,
> Radu

I have also had a look here :
https://sashiko.dev/#/patchset/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2%40analog.com

Therefore here is the reply to Sashiko's review:

> For devices that lack the optional disable_one callback,
> ad_sigma_delta_disable_one() is a no-op. In that scenario,
> ad_sigma_delta_set_mode() executes the final SPI transfer for the
> conversion.
>
> Because sigma_delta->keep_cs_asserted is still true when set_mode is
> called, its transfer will carry cs_change = 1. Since disable_one
> does not issue a subsequent transfer with cs_change = 0, will the SPI
> framework skip teardown and leave CS permanently stuck low?
>
> Could sigma_delta->keep_cs_asserted be cleared before calling
> ad_sigma_delta_set_mode() to ensure proper teardown, or should an
> explicit dummy transfer be added?

Good catch, thank you. The previous version only moved disable_one()
after clearing keep_cs_asserted, but left set_mode(AD_SD_MODE_IDLE) in
the out: block where keep_cs_asserted is still true. For devices that
do not implement disable_one, set_mode() issues the last SPI transfer,
so it carries cs_change=1 and CS stays stuck — the same bug reproduced
on a different code path.

Fixed by moving set_mode(IDLE) into the out_unlock: block as well, so
both calls execute after keep_cs_asserted is cleared. This matches the
pattern already used correctly in ad_sd_calibrate(), which clears
keep_cs_asserted before calling set_mode(IDLE) and disable_one() in its
single cleanup block.

...

> I noticed an additional state tracking issue in the error path of
> ad_sd_buffer_postenable() that wasn't introduced by this patch.
> When operations like ad_sigma_delta_clear_pending_event() fail, the error
> path calls spi_bus_unlock() but leaves sigma_delta->bus_locked = true and
> sigma_delta->keep_cs_asserted = true. Since the IIO core does not invoke
> predisable on a postenable failure, this state persists.
>
> Subsequent SPI operations like direct mode reads will observe
> bus_locked == true and incorrectly invoke spi_sync_locked(). As the bus
> is not actually locked at the controller level, this bypasses the
> SPI framework's bus_lock_mutex.
>
> Does this allow concurrent access to the SPI controller from multiple
> threads, potentially corrupting SPI bus data?

Yes, the concern is valid. With bus_locked == true but the controller's
bus_lock_mutex not held, any concurrent spi_sync() from another driver
can proceed unobstructed while our driver issues spi_sync_locked() —
exactly the race bus_lock_mutex is meant to prevent.

Fixed in the same patch by resetting both keep_cs_asserted and
bus_locked to false in err_unlock: before calling spi_bus_unlock(),
consistent with the teardown sequence in predisable().

Re: [PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Jonathan Cameron]

On Thu, 7 May 2026 12:17:25 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -442,10 +442,10 @@ int ad_sigma_delta_single_conversion(struct
> > iio_dev *indio_dev,
> >  	ad_sd_disable_irq(sigma_delta);
> > 
> >  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
> > -	ad_sigma_delta_disable_one(sigma_delta, chan->address);
> > 
> >  out_unlock:
> >  	sigma_delta->keep_cs_asserted = false;
> > +	ad_sigma_delta_disable_one(sigma_delta, chan->address);
> >  	sigma_delta->bus_locked = false;
> >  	spi_bus_unlock(sigma_delta->spi->controller);
> >  out_release:
> > 
> > ---
> > base-commit: 3b3bea6d4b9c162f9e555905d96b8c1da67ecd5b
> > change-id: 20260428-ad_sigma_delta-fix-bb65d56ccbb0
> > 
> > Best regards,
> > --
> > Radu Sabau <radu.sabau@analog.com>
> >   
> 
> Hi everyone,
> 
> Uhmm..., why was this fix marked as Changes Requested? Is it something I am
> missing?
Looks like I missed in patchwork. It has been applied.
> 
> Best Regards,
> Radu

Re: [PATCH] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Jonathan Cameron]

On Thu, 7 May 2026 13:52:20 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> > -----Original Message-----
> > From: Sabau, Radu bogdan <Radu.Sabau@analog.com>
> > Sent: Thursday, May 7, 2026 3:17 PM  
> 
> ...
> 
> > >  
> > 
> > Hi everyone,
> > 
> > Uhmm..., why was this fix marked as Changes Requested? Is it something I am
> > missing?
> > 
> > Best Regards,
> > Radu  
> 
> I have also had a look here :
> https://sashiko.dev/#/patchset/20260428-ad_sigma_delta-fix-v1-1-8e3f925ee8d2%40analog.com
> 
> Therefore here is the reply to Sashiko's review:
> 
> > For devices that lack the optional disable_one callback,
> > ad_sigma_delta_disable_one() is a no-op. In that scenario,
> > ad_sigma_delta_set_mode() executes the final SPI transfer for the
> > conversion.
> >
> > Because sigma_delta->keep_cs_asserted is still true when set_mode is
> > called, its transfer will carry cs_change = 1. Since disable_one
> > does not issue a subsequent transfer with cs_change = 0, will the SPI
> > framework skip teardown and leave CS permanently stuck low?
> >
> > Could sigma_delta->keep_cs_asserted be cleared before calling
> > ad_sigma_delta_set_mode() to ensure proper teardown, or should an
> > explicit dummy transfer be added?  
> 
> Good catch, thank you. The previous version only moved disable_one()
> after clearing keep_cs_asserted, but left set_mode(AD_SD_MODE_IDLE) in
> the out: block where keep_cs_asserted is still true. For devices that
> do not implement disable_one, set_mode() issues the last SPI transfer,
> so it carries cs_change=1 and CS stays stuck — the same bug reproduced
> on a different code path.
> 
> Fixed by moving set_mode(IDLE) into the out_unlock: block as well, so
> both calls execute after keep_cs_asserted is cleared. This matches the
> pattern already used correctly in ad_sd_calibrate(), which clears
> keep_cs_asserted before calling set_mode(IDLE) and disable_one() in its
> single cleanup block.
> 
> ...
> 
> > I noticed an additional state tracking issue in the error path of
> > ad_sd_buffer_postenable() that wasn't introduced by this patch.
> > When operations like ad_sigma_delta_clear_pending_event() fail, the error
> > path calls spi_bus_unlock() but leaves sigma_delta->bus_locked = true and
> > sigma_delta->keep_cs_asserted = true. Since the IIO core does not invoke
> > predisable on a postenable failure, this state persists.
> >
> > Subsequent SPI operations like direct mode reads will observe
> > bus_locked == true and incorrectly invoke spi_sync_locked(). As the bus
> > is not actually locked at the controller level, this bypasses the
> > SPI framework's bus_lock_mutex.
> >
> > Does this allow concurrent access to the SPI controller from multiple
> > threads, potentially corrupting SPI bus data?  
> 
> Yes, the concern is valid. With bus_locked == true but the controller's
> bus_lock_mutex not held, any concurrent spi_sync() from another driver
> can proceed unobstructed while our driver issues spi_sync_locked() —
> exactly the race bus_lock_mutex is meant to prevent.
> 
> Fixed in the same patch by resetting both keep_cs_asserted and
> bus_locked to false in err_unlock: before calling spi_bus_unlock(),
> consistent with the teardown sequence in predisable().
With that lot in mind I'll drop this version.

thanks,

Jonathan