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

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