Re: [PATCH 1/6] iio: adc: versal-sysmon: fix race in interrupt teardown

[Jonathan Cameron]

On Mon, 2 Nov 2026 17:51:07 +0530
Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx> wrote:

> sysmon_disable_interrupts() masks all interrupts, clears masked_temp and
> cancels the unmask worker, but leaves the interrupt handler live. A
> worker that is already running can re-enable an alarm through IER after
> the mask write. If that alarm fires, the handler queues the worker again
> after cancel_delayed_work_sync() has returned. On unbind this action runs
> before devm releases the IRQ, so the queued work can run after the
> devm-allocated iio_dev that embeds it has been freed.
> 
> Disable the IRQ first so that no handler is running or can start, then
> clear masked_temp and cancel the worker, and mask all interrupts last,
> once nothing is left that can unmask them. Record the IRQ number for
> this.

I always look closely at disable_irq() calls as they normally indicate
a state machine issue - or at least a less than ideal state machine.
Interrupts that hit them in disabled state will start the counting
for spurious interrupt detection etc.  So whilst they sometimes can
be used to cover up for really nasty to handle race conditions they
get extra scrutiny...  So generally I'd expect to let the handler
run, but ensure it doesn't do us any harm.

Given sysmon_disable_interrupts clears masked_temp under the
irq_lock can we not use that to decide not reenable anything
in the unmask_worker.  

So in conclusion it's almost what you have here, but drop the disable_irq
and instead check if there are any masked interrupts that might
be unmasked in the unmask_worker. I think that can either be done before
calling sysmon_unmask_temp() or at the start of that function before
we do anything that might set IER.


> 
> Fixes: c7f7eb10638f ("iio: adc: versal-sysmon: add threshold event support")
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
> ---
>  drivers/iio/adc/versal-sysmon-core.c | 5 ++++-
>  drivers/iio/adc/versal-sysmon.h      | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 1b55d343982e..6b9a58e19831 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
> @@ -781,12 +781,14 @@ static void sysmon_disable_interrupts(void *data)
>  {
>  	struct sysmon *sysmon = data;
>  
> -	regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
> +	disable_irq(sysmon->irq);
>  
>  	scoped_guard(spinlock_irq, &sysmon->irq_lock)
>  		sysmon->masked_temp = 0;
>  
>  	cancel_delayed_work_sync(&sysmon->sysmon_unmask_work);
> +
> +	regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
>  }
>  
>  static int sysmon_init_interrupt(struct sysmon *sysmon,
> @@ -807,6 +809,7 @@ static int sysmon_init_interrupt(struct sysmon *sysmon,
>  	if (ret)
>  		return ret;
>  	sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
> +	sysmon->irq = irq;
>  
>  	ret = devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
>  	if (ret)
> diff --git a/drivers/iio/adc/versal-sysmon.h b/drivers/iio/adc/versal-sysmon.h
> index bb9a75bf71c0..701ef9b56263 100644
> --- a/drivers/iio/adc/versal-sysmon.h
> +++ b/drivers/iio/adc/versal-sysmon.h
> @@ -85,6 +85,7 @@ struct regmap;
>   * @regmap: register map for hardware access
>   * @lock: protects read-modify-write sequences on threshold registers
>   *        and cached state that spans multiple regmap calls
> + * @irq: interrupt number, 0 when events are not supported
>   * @irq_lock: protects interrupt mask register updates (MMIO path only)
>   * @masked_temp: currently masked temperature alarm bits
>   * @temp_mask: temperature interrupt configuration mask
> @@ -101,6 +102,7 @@ struct sysmon {
>  	 * that spans multiple regmap calls.
>  	 */
>  	struct mutex lock;
> +	int irq;
>  	/*
>  	 * Protects interrupt mask register updates.  Only used on the
>  	 * MMIO path (fast_io regmap); I2C has no IRQ and never reaches