iio: resolver: ad2s1210: notify trigger and clear state on fault read error

[PATCH] iio: resolver: ad2s1210: notify trigger and clear state on fault read error

Posted by Stepan Ionichev 3 weeks, 6 days ago

ad2s1210_trigger_handler() walks several scan-mask branches and uses
"goto error_ret" to land on the iio_trigger_notify_done() teardown at
the bottom of the function for every I/O error -- except the
MOD_CONFIG fault-register read, which uses a bare "return ret":

	if (st->fixed_mode == MOD_CONFIG) {
		unsigned int reg_val;

		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
		if (ret < 0)
			return ret;
		...
	}

Two problems on that path:

  - the handler returns a negative errno where the prototype expects
    an irqreturn_t (IRQ_HANDLED / IRQ_NONE), so the caller in the
    IIO core sees a value outside the enum;
  - iio_trigger_notify_done() is skipped, leaving the trigger
    busy-flag set. A single transient SPI/regmap error on the fault
    read then wedges the trigger so subsequent samples are dropped
    until the consumer is detached.

Convert the error path to "goto error_ret" so the failure path goes
through the same notify_done() teardown as every other error in the
handler.

Fixes: f9b9ff95be8c ("iio: resolver: ad2s1210: add support for adi,fixed-mode")
Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/iio/resolver/ad2s1210.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
index 774728c80..1be19fe8a 100644
--- a/drivers/iio/resolver/ad2s1210.c
+++ b/drivers/iio/resolver/ad2s1210.c
@@ -1334,7 +1334,7 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
 
 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
 		if (ret < 0)
-			return ret;
+			goto error_ret;
 
 		st->sample.fault = reg_val;
 	}
-- 
2.43.0

Re: [PATCH] iio: resolver: ad2s1210: notify trigger and clear state on fault read error

Posted by Jonathan Cameron 3 weeks, 5 days ago

On Fri, 15 May 2026 18:31:38 +0500
Stepan Ionichev <sozdayvek@gmail.com> wrote:

> ad2s1210_trigger_handler() walks several scan-mask branches and uses
> "goto error_ret" to land on the iio_trigger_notify_done() teardown at
> the bottom of the function for every I/O error -- except the
> MOD_CONFIG fault-register read, which uses a bare "return ret":
> 
> 	if (st->fixed_mode == MOD_CONFIG) {
> 		unsigned int reg_val;
> 
> 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
> 		if (ret < 0)
> 			return ret;
> 		...
> 	}
> 
> Two problems on that path:
> 
>   - the handler returns a negative errno where the prototype expects
>     an irqreturn_t (IRQ_HANDLED / IRQ_NONE), so the caller in the
>     IIO core sees a value outside the enum;
>   - iio_trigger_notify_done() is skipped, leaving the trigger
>     busy-flag set. A single transient SPI/regmap error on the fault
>     read then wedges the trigger so subsequent samples are dropped
>     until the consumer is detached.
> 
> Convert the error path to "goto error_ret" so the failure path goes
> through the same notify_done() teardown as every other error in the
> handler.
> 
> Fixes: f9b9ff95be8c ("iio: resolver: ad2s1210: add support for adi,fixed-mode")
> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>

Not related to this as the fix is good, but I was obviously half asleep
when guard() got added to this function which is full of gotos :(

If you have time could you look at factoring out helper that basically lifts
everything before trigger_notify_done() which should not be done with
the guard held - at least in this case it's not a deadlock source
as the device doesn't have any triggers!

That helper can use guard() and do direct returns to simplify the code flow.

Anyhow, this fix is good so applied and marked for stable.

> ---
>  drivers/iio/resolver/ad2s1210.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
> index 774728c80..1be19fe8a 100644
> --- a/drivers/iio/resolver/ad2s1210.c
> +++ b/drivers/iio/resolver/ad2s1210.c
> @@ -1334,7 +1334,7 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>  
>  		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
>  		if (ret < 0)
> -			return ret;
> +			goto error_ret;
>  
>  		st->sample.fault = reg_val;
>  	}

Re: [PATCH] iio: resolver: ad2s1210: notify trigger and clear state on fault read error

Posted by David Lechner 3 weeks, 5 days ago

On 5/16/26 6:28 AM, Jonathan Cameron wrote:
> On Fri, 15 May 2026 18:31:38 +0500
> Stepan Ionichev <sozdayvek@gmail.com> wrote:
> 
>> ad2s1210_trigger_handler() walks several scan-mask branches and uses
>> "goto error_ret" to land on the iio_trigger_notify_done() teardown at
>> the bottom of the function for every I/O error -- except the
>> MOD_CONFIG fault-register read, which uses a bare "return ret":
>>
>> 	if (st->fixed_mode == MOD_CONFIG) {
>> 		unsigned int reg_val;
>>
>> 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
>> 		if (ret < 0)
>> 			return ret;
>> 		...
>> 	}
>>
>> Two problems on that path:
>>
>>   - the handler returns a negative errno where the prototype expects
>>     an irqreturn_t (IRQ_HANDLED / IRQ_NONE), so the caller in the
>>     IIO core sees a value outside the enum;
>>   - iio_trigger_notify_done() is skipped, leaving the trigger
>>     busy-flag set. A single transient SPI/regmap error on the fault
>>     read then wedges the trigger so subsequent samples are dropped
>>     until the consumer is detached.
>>
>> Convert the error path to "goto error_ret" so the failure path goes
>> through the same notify_done() teardown as every other error in the
>> handler.
>>
>> Fixes: f9b9ff95be8c ("iio: resolver: ad2s1210: add support for adi,fixed-mode")
>> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
> 
> Not related to this as the fix is good, but I was obviously half asleep
> when guard() got added to this function which is full of gotos :(
> 
> If you have time could you look at factoring out helper that basically lifts
> everything before trigger_notify_done() which should not be done with
> the guard held - at least in this case it's not a deadlock source
> as the device doesn't have any triggers!
> 
> That helper can use guard() and do direct returns to simplify the code flow.

I can do that since my name is all over the driver.

> 
> Anyhow, this fix is good so applied and marked for stable.

Reviewed-by: David Lechner <dlechner@baylibre.com>