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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
> 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,