[PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Radu Sabau via B4 Relay]

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

ad4695_enter_advanced_sequencer_mode() was called after
spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
flight while the offload was already active. When the offload fires its
completion interrupt concurrently with the regular transfer, the SPI
engine interrupt handler is not designed to handle both at once, leading
to a kernel panic.

Fix this by calling ad4695_enter_advanced_sequencer_mode() before
spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
before the offload becomes active. This is consistent with the same
constraint that already applies to the BUSY_GP_EN write above it.

Update the error unwind labels accordingly: add err_exit_conversion_mode
so that a failure of spi_offload_trigger_enable() correctly exits
conversion mode before clearing BUSY_GP_EN.

Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
Signed-off-by: Radu Sabau <radu.sabau@analog.com>
---
When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
ad4695_enter_advanced_sequencer_mode() was called after
spi_offload_trigger_enable(), resulting in a regular SPI transfer being
in flight while the offload was already active. This caused a kernel
panic in the SPI engine interrupt handler.

This series fixes the call ordering so all SPI bus accesses complete
before the offload becomes active, consistent with the constraint that
was already documented for the BUSY_GP_EN write in the same function.
---
 drivers/iio/adc/ad4695.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index cda419638d9a..c6721f5ac8af 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -866,24 +866,24 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
 		return ret;
 
 	/*
-	 * NB: technically, this is part the SPI offload trigger enable, but it
-	 * doesn't work to call it from the offload trigger enable callback
-	 * because it requires accessing the SPI bus. Calling it from the
-	 * trigger enable callback could cause a deadlock.
+	 * NB: these are technically part of the SPI offload trigger enable, but
+	 * they can't be called from the offload trigger enable callback because
+	 * they require accessing the SPI bus. Calling them from the trigger
+	 * enable callback could cause a deadlock.
 	 */
 	ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE,
 			      AD4695_REG_GP_MODE_BUSY_GP_EN);
 	if (ret)
 		goto err_unoptimize_message;
 
-	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
-					 &config);
+	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
 	if (ret)
 		goto err_disable_busy_output;
 
-	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
+	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
+					 &config);
 	if (ret)
-		goto err_offload_trigger_disable;
+		goto err_exit_conversion_mode;
 
 	mutex_lock(&st->cnv_pwm_lock);
 	pwm_get_state(st->cnv_pwm, &state);
@@ -895,22 +895,19 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
 	ret = pwm_apply_might_sleep(st->cnv_pwm, &state);
 	mutex_unlock(&st->cnv_pwm_lock);
 	if (ret)
-		goto err_offload_exit_conversion_mode;
+		goto err_trigger_disable;
 
 	return 0;
 
-err_offload_exit_conversion_mode:
+err_trigger_disable:
 	/*
-	 * We have to unwind in a different order to avoid triggering offload.
-	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to be
-	 * done after spi_offload_trigger_disable().
+	 * ad4695_exit_conversion_mode() triggers a conversion, so the offload
+	 * must be disabled first to avoid capturing a spurious sample.
 	 */
 	spi_offload_trigger_disable(st->offload, st->offload_trigger);
-	ad4695_exit_conversion_mode(st);
-	goto err_disable_busy_output;
 
-err_offload_trigger_disable:
-	spi_offload_trigger_disable(st->offload, st->offload_trigger);
+err_exit_conversion_mode:
+	ad4695_exit_conversion_mode(st);
 
 err_disable_busy_output:
 	regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE,

---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20260330-ad4696-fix-186955a8c511

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

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[David Lechner]

On 3/30/26 8:34 AM, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@analog.com>
> 
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
> flight while the offload was already active. When the offload fires its
> completion interrupt concurrently with the regular transfer, the SPI
> engine interrupt handler is not designed to handle both at once, leading
> to a kernel panic.
> 
> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
> before the offload becomes active. This is consistent with the same
> constraint that already applies to the BUSY_GP_EN write above it.
> 
> Update the error unwind labels accordingly: add err_exit_conversion_mode
> so that a failure of spi_offload_trigger_enable() correctly exits
> conversion mode before clearing BUSY_GP_EN.
> 
> Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), resulting in a regular SPI transfer being
> in flight while the offload was already active. This caused a kernel
> panic in the SPI engine interrupt handler.

This used to work (I spend many hours testing it to arrive at the current
state). Are we sure there hasn't been a bug introduced in the AXI SPI Engine
instead?

> 
> This series fixes the call ordering so all SPI bus accesses complete
> before the offload becomes active, consistent with the constraint that
> was already documented for the BUSY_GP_EN write in the same function.
> ---
>  drivers/iio/adc/ad4695.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index cda419638d9a..c6721f5ac8af 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -866,24 +866,24 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  		return ret;
>  
>  	/*
> -	 * NB: technically, this is part the SPI offload trigger enable, but it
> -	 * doesn't work to call it from the offload trigger enable callback
> -	 * because it requires accessing the SPI bus. Calling it from the
> -	 * trigger enable callback could cause a deadlock.
> +	 * NB: these are technically part of the SPI offload trigger enable, but
> +	 * they can't be called from the offload trigger enable callback because
> +	 * they require accessing the SPI bus. Calling them from the trigger
> +	 * enable callback could cause a deadlock.
>  	 */
>  	ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE,
>  			      AD4695_REG_GP_MODE_BUSY_GP_EN);
>  	if (ret)
>  		goto err_unoptimize_message;
>  
> -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> -					 &config);
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>  	if (ret)
>  		goto err_disable_busy_output;
>  
> -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);

Swapping the order here introduces a race condition. 

ad4695_enter_advanced_sequencer_mode() starts sampling (triggered from the
internal clock in the ADC) and the first sample could be completed before
the SPI offload is enabled. If the first sample is missed, then all of the
data will be off by one index on the buffer.


>  	if (ret)
> -		goto err_offload_trigger_disable;
> +		goto err_exit_conversion_mode;
>  
>  	mutex_lock(&st->cnv_pwm_lock);
>  	pwm_get_state(st->cnv_pwm, &state);
> @@ -895,22 +895,19 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = pwm_apply_might_sleep(st->cnv_pwm, &state);
>  	mutex_unlock(&st->cnv_pwm_lock);
>  	if (ret)
> -		goto err_offload_exit_conversion_mode;
> +		goto err_trigger_disable;
>  
>  	return 0;
>  
> -err_offload_exit_conversion_mode:
> +err_trigger_disable:
>  	/*
> -	 * We have to unwind in a different order to avoid triggering offload.
> -	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to be
> -	 * done after spi_offload_trigger_disable().
> +	 * ad4695_exit_conversion_mode() triggers a conversion, so the offload
> +	 * must be disabled first to avoid capturing a spurious sample.
>  	 */
>  	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> -	ad4695_exit_conversion_mode(st);
> -	goto err_disable_busy_output;
>  
> -err_offload_trigger_disable:
> -	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +err_exit_conversion_mode:
> +	ad4695_exit_conversion_mode(st);
>  
>  err_disable_busy_output:
>  	regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE,
> 
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260330-ad4696-fix-186955a8c511
> 
> Best regards,

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Nuno Sá]

On Mon, Mar 30, 2026 at 04:34:39PM +0300, Radu Sabau wrote:
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
> flight while the offload was already active. When the offload fires its
> completion interrupt concurrently with the regular transfer, the SPI
> engine interrupt handler is not designed to handle both at once, leading
> to a kernel panic.
> 
> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
> before the offload becomes active. This is consistent with the same
> constraint that already applies to the BUSY_GP_EN write above it.
> 
> Update the error unwind labels accordingly: add err_exit_conversion_mode
> so that a failure of spi_offload_trigger_enable() correctly exits
> conversion mode before clearing BUSY_GP_EN.
> 
> Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), resulting in a regular SPI transfer being
> in flight while the offload was already active. This caused a kernel
> panic in the SPI engine interrupt handler.
> 
> This series fixes the call ordering so all SPI bus accesses complete
> before the offload becomes active, consistent with the constraint that
> was already documented for the BUSY_GP_EN write in the same function.
> ---

LGTM,

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/iio/adc/ad4695.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index cda419638d9a..c6721f5ac8af 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -866,24 +866,24 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  		return ret;
>  
>  	/*
> -	 * NB: technically, this is part the SPI offload trigger enable, but it
> -	 * doesn't work to call it from the offload trigger enable callback
> -	 * because it requires accessing the SPI bus. Calling it from the
> -	 * trigger enable callback could cause a deadlock.
> +	 * NB: these are technically part of the SPI offload trigger enable, but
> +	 * they can't be called from the offload trigger enable callback because
> +	 * they require accessing the SPI bus. Calling them from the trigger
> +	 * enable callback could cause a deadlock.
>  	 */
>  	ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE,
>  			      AD4695_REG_GP_MODE_BUSY_GP_EN);
>  	if (ret)
>  		goto err_unoptimize_message;
>  
> -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> -					 &config);
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>  	if (ret)
>  		goto err_disable_busy_output;
>  
> -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
>  	if (ret)
> -		goto err_offload_trigger_disable;
> +		goto err_exit_conversion_mode;
>  
>  	mutex_lock(&st->cnv_pwm_lock);
>  	pwm_get_state(st->cnv_pwm, &state);
> @@ -895,22 +895,19 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = pwm_apply_might_sleep(st->cnv_pwm, &state);
>  	mutex_unlock(&st->cnv_pwm_lock);
>  	if (ret)
> -		goto err_offload_exit_conversion_mode;
> +		goto err_trigger_disable;
>  
>  	return 0;
>  
> -err_offload_exit_conversion_mode:
> +err_trigger_disable:
>  	/*
> -	 * We have to unwind in a different order to avoid triggering offload.
> -	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to be
> -	 * done after spi_offload_trigger_disable().
> +	 * ad4695_exit_conversion_mode() triggers a conversion, so the offload
> +	 * must be disabled first to avoid capturing a spurious sample.
>  	 */
>  	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> -	ad4695_exit_conversion_mode(st);
> -	goto err_disable_busy_output;
>  
> -err_offload_trigger_disable:
> -	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +err_exit_conversion_mode:
> +	ad4695_exit_conversion_mode(st);
>  
>  err_disable_busy_output:
>  	regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE,
> 
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260330-ad4696-fix-186955a8c511
> 
> Best regards,
> -- 
> Radu Sabau <radu.sabau@analog.com>
> 

RE: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Monday, March 30, 2026 5:11 PM
> To: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron <jic23@kernel.org>;
> Andy Shevchenko <andy@kernel.org>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer
> postenable
> 
> [External]
> 
> On 3/30/26 8:34 AM, Radu Sabau via B4 Relay wrote:
> > From: Radu Sabau <radu.sabau@analog.com>
> >
> > ad4695_enter_advanced_sequencer_mode() was called after
> > spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
> > flight while the offload was already active. When the offload fires its
> > completion interrupt concurrently with the regular transfer, the SPI
> > engine interrupt handler is not designed to handle both at once, leading
> > to a kernel panic.
> >
> > Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> > spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
> > before the offload becomes active. This is consistent with the same
> > constraint that already applies to the BUSY_GP_EN write above it.
> >
> > Update the error unwind labels accordingly: add err_exit_conversion_mode
> > so that a failure of spi_offload_trigger_enable() correctly exits
> > conversion mode before clearing BUSY_GP_EN.
> >
> > Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
> > Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> > ---
> > When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
> > ad4695_enter_advanced_sequencer_mode() was called after
> > spi_offload_trigger_enable(), resulting in a regular SPI transfer being
> > in flight while the offload was already active. This caused a kernel
> > panic in the SPI engine interrupt handler.
> 
> This used to work (I spend many hours testing it to arrive at the current
> state). Are we sure there hasn't been a bug introduced in the AXI SPI Engine
> instead?
> 

Hi David,

I also tried looking in the AXI SPI Engine and find something related first
but couldn't find anything, though I am happy to be corrected.

> >
> > This series fixes the call ordering so all SPI bus accesses complete
> > before the offload becomes active, consistent with the constraint that
> > was already documented for the BUSY_GP_EN write in the same function.

...

> > -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> > -					 &config);
> > +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> >  	if (ret)
> >  		goto err_disable_busy_output;
> >
> > -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> > +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> > +					 &config);
> 
> Swapping the order here introduces a race condition.
> 
> ad4695_enter_advanced_sequencer_mode() starts sampling (triggered from
> the
> internal clock in the ADC) and the first sample could be completed before
> the SPI offload is enabled. If the first sample is missed, then all of the
> data will be off by one index on the buffer.
> 

I thought this too at the first glance, but couldn’t be sure so I tried testing it
anyway using the FMCZ evaluation board and a Zedboard, and everything
worked as expected.
I have plotted buffered data and it showed exactly what the Signal Generator
was outputting and on the right channel, so no index was off, if that's what
you meant.

Best Regards,
Radu

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[David Lechner]

On 3/30/26 9:31 AM, Sabau, Radu bogdan wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Monday, March 30, 2026 5:11 PM
>> To: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen
>> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
>> Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron <jic23@kernel.org>;
>> Andy Shevchenko <andy@kernel.org>
>> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer
>> postenable
>>
>> [External]
>>
>> On 3/30/26 8:34 AM, Radu Sabau via B4 Relay wrote:
>>> From: Radu Sabau <radu.sabau@analog.com>
>>>
>>> ad4695_enter_advanced_sequencer_mode() was called after
>>> spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
>>> flight while the offload was already active. When the offload fires its
>>> completion interrupt concurrently with the regular transfer, the SPI
>>> engine interrupt handler is not designed to handle both at once, leading
>>> to a kernel panic.
>>>
>>> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
>>> spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
>>> before the offload becomes active. This is consistent with the same
>>> constraint that already applies to the BUSY_GP_EN write above it.
>>>
>>> Update the error unwind labels accordingly: add err_exit_conversion_mode
>>> so that a failure of spi_offload_trigger_enable() correctly exits
>>> conversion mode before clearing BUSY_GP_EN.
>>>
>>> Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
>>> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
>>> ---
>>> When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
>>> ad4695_enter_advanced_sequencer_mode() was called after
>>> spi_offload_trigger_enable(), resulting in a regular SPI transfer being
>>> in flight while the offload was already active. This caused a kernel
>>> panic in the SPI engine interrupt handler.
>>
>> This used to work (I spend many hours testing it to arrive at the current
>> state). Are we sure there hasn't been a bug introduced in the AXI SPI Engine
>> instead?
>>
> 
> Hi David,
> 
> I also tried looking in the AXI SPI Engine and find something related first
> but couldn't find anything, though I am happy to be corrected.
> 

Which commit is the HDL being built from? It could be that something
changed in the FPGA implementation rather than the driver.

Sending a regular SPI message while offload is enabled should still
work. If there was a design decision that changed that, then we should
also fix the AXI SPI Engine driver to return an error instead of crashing
in this case. (I hope we can avoid this though.)

Even if this one particular case can be worked around, I expect the same
race condition could be a problem on other chips (or this chip in a
different configuration). So it would be best if we could restore the
AXI SPI Engine to the way it was working before.

>>>
>>> This series fixes the call ordering so all SPI bus accesses complete
>>> before the offload becomes active, consistent with the constraint that
>>> was already documented for the BUSY_GP_EN write in the same function.
> 
> ...
> 
>>> -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
>>> -					 &config);
>>> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>>>  	if (ret)
>>>  		goto err_disable_busy_output;
>>>
>>> -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>>> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
>>> +					 &config);
>>
>> Swapping the order here introduces a race condition.
>>
>> ad4695_enter_advanced_sequencer_mode() starts sampling (triggered from
>> the
>> internal clock in the ADC) and the first sample could be completed before
>> the SPI offload is enabled. If the first sample is missed, then all of the
>> data will be off by one index on the buffer.
>>
> 
> I thought this too at the first glance, but couldn’t be sure so I tried testing it
> anyway using the FMCZ evaluation board and a Zedboard, and everything
> worked as expected.
> I have plotted buffered data and it showed exactly what the Signal Generator
> was outputting and on the right channel, so no index was off, if that's what
> you meant.
> 
> Best Regards,
> Radu
> 

Does it still work correctly if you add a msleep(100); after enabling the
sequencer and before enabling the SPI offload? (to simulate a slow/busy
machine).

It would also help to look at the SPI signals with a logic analyzer to see
what the timing is.

I guess it could still be OK since we currently only support the case of
the PWM acting as the CNV source.

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Andy Shevchenko]

On Mon, Mar 30, 2026 at 04:34:39PM +0300, Radu Sabau via B4 Relay wrote:

> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
> flight while the offload was already active. When the offload fires its
> completion interrupt concurrently with the regular transfer, the SPI
> engine interrupt handler is not designed to handle both at once, leading
> to a kernel panic.

Please, provide ~3-5 (the most important) lines of that panic.

> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
> before the offload becomes active. This is consistent with the same
> constraint that already applies to the BUSY_GP_EN write above it.
> 
> Update the error unwind labels accordingly: add err_exit_conversion_mode
> so that a failure of spi_offload_trigger_enable() correctly exits
> conversion mode before clearing BUSY_GP_EN.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Monday, March 30, 2026 7:46 PM
> To: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen

Hi David, Andy,

> 
> Which commit is the HDL being built from? It could be that something
> changed in the FPGA implementation rather than the driver.
> 

Good call. The bitstream was built from commit 79446d8ee in the ADI HDL
tree — "library/spi_engine: fix spi engine for SDI backpressure",
authored by Carlos Souza, merged February 4 2026.

That commit made two relevant changes to spi_engine_offload.v:

1. cmd_valid was decoupled from spi_active. Previously:

     assign cmd_valid = spi_active;

   Now cmd_valid is driven by a separate offload_cmd_valid register.
   The practical effect is cmd_valid is no longer asserted the instant
   spi_active goes high.

2. spi_active now stays asserted until the DMA has consumed all pending
   SDI data, not just until the last command is accepted. The old
   deassertion condition was:

     cmd_ready && (spi_cmd_rd_addr_next == ctrl_cmd_wr_addr)

   The new condition holds spi_active high while
   offload_disable_pending && sdi_data_valid. Since interconnect_dir =
   spi_enable | spi_active, this means the interconnect stays in offload
   direction for the full duration of the SDI drain phase.

The February commit also changed the execution module's io_ready1 logic
(now gated on pending_sdi_data_valid instead of sdi_data_valid directly),
which shifts when the execution engine signals completion via SYNC relative
to the last bit being clocked. Together these changes alter the timing of
the mode boundary between regular SPI and offload. The panic — as shown
below — is consistent with a race where SPI_ENGINE_INT_CMD_ALMOST_EMPTY
is left armed in int_enable after host->cur_msg has already been cleared.
The exact causal chain needs more instrumentation to confirm, but this
commit is the most significant HDL delta between your test setup and mine.

So your instinct was right — something changed on the FPGA side,
specifically the SDI backpressure fix. The driver ordering fix is still
correct regardless (and I would say is the right thing to do), but
I wanted to close the loop on the root cause.

Regarding the race you raised with the new ordering — I tested with
msleep(100) inserted between ad4695_enter_advanced_sequencer_mode() and
spi_offload_trigger_enable() to simulate a slow machine. Data came back
correctly with no index shift. This confirms the expectation: with PWM
as the CNV source the ADC only converts on an external CNV pulse, and
the PWM isn't running until spi_offload_trigger_enable() enables the
trigger, so there is nothing to miss during that gap.

And for the 5 most relevant panic lines, here they are:

  Unable to handle kernel NULL pointer dereference at virtual address 0000002c when read
  PC is at spi_engine_write_cmd_fifo+0x8/0x74
  LR is at spi_engine_irq+0x10c/0x290
  r1 : 00000000
   spi_engine_write_cmd_fifo from spi_engine_irq+0x10c/0x290

The sequence that leads there with the old call ordering:

1. spi_offload_trigger_enable() completes — offload is now active,
   interconnect_dir switches to 1 (offload direction).

2. ad4695_enter_advanced_sequencer_mode() submits a regular SPI
   transfer. spi_engine_transfer_one_message() sets host->cur_msg,
   writes commands to the CPU CMD FIFO, and arms
   SPI_ENGINE_INT_CMD_ALMOST_EMPTY in int_enable, then waits for
   completion.

3. With interconnect_dir=1, s1_cmd_ready=0 — the execution engine
   cannot consume the CPU commands. The regular transfer stalls.

4. The PWM fires a CNV pulse. The offload starts executing its
   pre-programmed command sequence and sends its SYNC through the
   s0 path (not s1), so the CPU transfer never gets its completion
   signal.

5. The regular SPI transfer times out or its completion path is
   disrupted. host->cur_msg gets cleared (set to NULL), but
   SPI_ENGINE_INT_CMD_ALMOST_EMPTY is still armed in int_enable.

6. SPI_ENGINE_INT_CMD_ALMOST_EMPTY fires. The IRQ handler reads
   host->cur_msg = NULL into msg (r1=0x00000000), then calls
   spi_engine_write_cmd_fifo(spi_engine, NULL). The function
   immediately dereferences msg at offset 0x2c — 8 bytes into
   the function — and the kernel panics.

Best regards,
Radu

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[David Lechner]

On 3/31/26 5:53 AM, Sabau, Radu bogdan wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Monday, March 30, 2026 7:46 PM
>> To: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen
> 
> Hi David, Andy,
> 
>>
>> Which commit is the HDL being built from? It could be that something
>> changed in the FPGA implementation rather than the driver.
>>
> 
> Good call. The bitstream was built from commit 79446d8ee in the ADI HDL
> tree — "library/spi_engine: fix spi engine for SDI backpressure",
> authored by Carlos Souza, merged February 4 2026.
> 
> That commit made two relevant changes to spi_engine_offload.v:
> 
> 1. cmd_valid was decoupled from spi_active. Previously:
> 
>      assign cmd_valid = spi_active;
> 
>    Now cmd_valid is driven by a separate offload_cmd_valid register.
>    The practical effect is cmd_valid is no longer asserted the instant
>    spi_active goes high.
> 
> 2. spi_active now stays asserted until the DMA has consumed all pending
>    SDI data, not just until the last command is accepted. The old
>    deassertion condition was:
> 
>      cmd_ready && (spi_cmd_rd_addr_next == ctrl_cmd_wr_addr)
> 
>    The new condition holds spi_active high while
>    offload_disable_pending && sdi_data_valid. Since interconnect_dir =
>    spi_enable | spi_active, this means the interconnect stays in offload
>    direction for the full duration of the SDI drain phase.
> 
> The February commit also changed the execution module's io_ready1 logic
> (now gated on pending_sdi_data_valid instead of sdi_data_valid directly),
> which shifts when the execution engine signals completion via SYNC relative
> to the last bit being clocked. Together these changes alter the timing of
> the mode boundary between regular SPI and offload. The panic — as shown
> below — is consistent with a race where SPI_ENGINE_INT_CMD_ALMOST_EMPTY
> is left armed in int_enable after host->cur_msg has already been cleared.
> The exact causal chain needs more instrumentation to confirm, but this
> commit is the most significant HDL delta between your test setup and mine.
> 
> So your instinct was right — something changed on the FPGA side,
> specifically the SDI backpressure fix. The driver ordering fix is still
> correct regardless (and I would say is the right thing to do), but
> I wanted to close the loop on the root cause.
> 
> Regarding the race you raised with the new ordering — I tested with
> msleep(100) inserted between ad4695_enter_advanced_sequencer_mode() and
> spi_offload_trigger_enable() to simulate a slow machine. Data came back
> correctly with no index shift. This confirms the expectation: with PWM
> as the CNV source the ADC only converts on an external CNV pulse, and
> the PWM isn't running until spi_offload_trigger_enable() enables the
> trigger, so there is nothing to miss during that gap.

Thanks for clearing this up. I found my notes from when I was working on
this (back in Oct. 2024). It looks like when you aren't using an external
CNV trigger, then ad4695_enter_advanced_sequencer_mode() actually triggers
the first conversion. But it looks like in the end, we ended up only
supporting the case where PWM is connected to CNV.

So the ordering currently in the driver was probably from an earlier
revision that supported more wiring options.

So yes, I'm convinced that changing the ordering for the PWM/CNV case
should be OK.

We'll still want a v2 though with an updated commit message that gives
the reason for the change independent of the AXI SPI Engine changes that
triggered a bug.

Re: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[David Lechner]

On 3/30/26 8:34 AM, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@analog.com>
> 
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), meaning a regular SPI transfer could be in
> flight while the offload was already active. When the offload fires its
> completion interrupt concurrently with the regular transfer, the SPI
> engine interrupt handler is not designed to handle both at once, leading
> to a kernel panic.
> 
> Fix this by calling ad4695_enter_advanced_sequencer_mode() before
> spi_offload_trigger_enable(), ensuring all SPI bus accesses are complete
> before the offload becomes active. This is consistent with the same
> constraint that already applies to the BUSY_GP_EN write above it.
> 
> Update the error unwind labels accordingly: add err_exit_conversion_mode
> so that a failure of spi_offload_trigger_enable() correctly exits
> conversion mode before clearing BUSY_GP_EN.
> 
> Fixes: f09f140e3ea8 ("iio: adc: ad4695: Add support for SPI offload")
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> When enabling the IIO buffer for SPI offload operation on AD4695/AD4696,
> ad4695_enter_advanced_sequencer_mode() was called after
> spi_offload_trigger_enable(), resulting in a regular SPI transfer being
> in flight while the offload was already active. This caused a kernel
> panic in the SPI engine interrupt handler.
> 
> This series fixes the call ordering so all SPI bus accesses complete
> before the offload becomes active, consistent with the constraint that
> was already documented for the BUSY_GP_EN write in the same function.
> ---
>  drivers/iio/adc/ad4695.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index cda419638d9a..c6721f5ac8af 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -866,24 +866,24 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  		return ret;
>  
>  	/*
> -	 * NB: technically, this is part the SPI offload trigger enable, but it
> -	 * doesn't work to call it from the offload trigger enable callback
> -	 * because it requires accessing the SPI bus. Calling it from the
> -	 * trigger enable callback could cause a deadlock.
> +	 * NB: these are technically part of the SPI offload trigger enable, but
> +	 * they can't be called from the offload trigger enable callback because
> +	 * they require accessing the SPI bus. Calling them from the trigger
> +	 * enable callback could cause a deadlock.

I didn't see a explanation for changing the comment here. I think "this" is
still correct instead of "these" because only the BUSY pin is the trigger,
so only the one regmap_set_bits() below would be in the trigger callback
if it was possible.

If we want to improve the comment to make it more clear, I would save that
for a separate patch and specifically mention the BUSY output.

>  	 */
>  	ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE,
>  			      AD4695_REG_GP_MODE_BUSY_GP_EN);
>  	if (ret)
>  		goto err_unoptimize_message;
>  
> -	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> -					 &config);
> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>  	if (ret)
>  		goto err_disable_busy_output;
>  
> -	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
>  	if (ret)
> -		goto err_offload_trigger_disable;
> +		goto err_exit_conversion_mode;
>  
>  	mutex_lock(&st->cnv_pwm_lock);
>  	pwm_get_state(st->cnv_pwm, &state);
> @@ -895,22 +895,19 @@ static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = pwm_apply_might_sleep(st->cnv_pwm, &state);
>  	mutex_unlock(&st->cnv_pwm_lock);
>  	if (ret)
> -		goto err_offload_exit_conversion_mode;
> +		goto err_trigger_disable;
>  
>  	return 0;
>  
> -err_offload_exit_conversion_mode:
> +err_trigger_disable:
>  	/*
> -	 * We have to unwind in a different order to avoid triggering offload.
> -	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to be
> -	 * done after spi_offload_trigger_disable().
> +	 * ad4695_exit_conversion_mode() triggers a conversion, so the offload
> +	 * must be disabled first to avoid capturing a spurious sample.

Since this becomes the natural unwinding order, I don't think we need
the comment at all any more.

There is still a similar comment in ad4695_offload_buffer_predisable()
so we aren't losing this information if we remove it here.

>  	 */
>  	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> -	ad4695_exit_conversion_mode(st);
> -	goto err_disable_busy_output;
>  
> -err_offload_trigger_disable:
> -	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +err_exit_conversion_mode:
> +	ad4695_exit_conversion_mode(st);
>  
>  err_disable_busy_output:
>  	regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE,
> 
> ---
> base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
> change-id: 20260330-ad4696-fix-186955a8c511
> 
> Best regards,

RE: [PATCH] iio: adc: ad4695: Fix call ordering in offload buffer postenable

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Tuesday, March 31, 2026 5:02 PM

...

> >
> >  	/*
> > -	 * NB: technically, this is part the SPI offload trigger enable, but it
> > -	 * doesn't work to call it from the offload trigger enable callback
> > -	 * because it requires accessing the SPI bus. Calling it from the
> > -	 * trigger enable callback could cause a deadlock.
> > +	 * NB: these are technically part of the SPI offload trigger enable, but
> > +	 * they can't be called from the offload trigger enable callback because
> > +	 * they require accessing the SPI bus. Calling them from the trigger
> > +	 * enable callback could cause a deadlock.
> 
> I didn't see a explanation for changing the comment here. I think "this" is
> still correct instead of "these" because only the BUSY pin is the trigger,
> so only the one regmap_set_bits() below would be in the trigger callback
> if it was possible.

I was thinking this comment could be left untouched as well, but couldn't
decide on whether to modify it or not and thought is better to wait for a
second opinion on it. Thanks for clarifying this!

> 
> If we want to improve the comment to make it more clear, I would save that
> for a separate patch and specifically mention the BUSY output.
> 
> >  	 */
> >  	ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE,
> >  			      AD4695_REG_GP_MODE_BUSY_GP_EN);
> >  	if (ret)
> >  		goto err_unoptimize_message;

...

> >
> > -err_offload_exit_conversion_mode:
> > +err_trigger_disable:
> >  	/*
> > -	 * We have to unwind in a different order to avoid triggering offload.
> > -	 * ad4695_exit_conversion_mode() triggers a conversion, so it has to
> be
> > -	 * done after spi_offload_trigger_disable().
> > +	 * ad4695_exit_conversion_mode() triggers a conversion, so the
> offload
> > +	 * must be disabled first to avoid capturing a spurious sample.
> 
> Since this becomes the natural unwinding order, I don't think we need
> the comment at all any more.
> 
> There is still a similar comment in ad4695_offload_buffer_predisable()
> so we aren't losing this information if we remove it here.
> 

Yep, I agree on this, too. The comment in postenable could be removed.