Fix for race in PINT_PinInterruptConfig (seen on rt685 SDK)

[ArnaudMouiche]

Hello, we just found that false interruptions may be triggered during IO irq configuration by PINT_PinInterruptConfig due to a race that may happened because of bad register setting order.

here is the patch:

commit 14598565a30544edbb466c551e339ddc9cb67ae7
Author: Arnaud Mouiche <arnaud.mouiche@invoxia.com>
Date: Tue Jan 24 17:25:00 2023 +0100

fsl_pint: fix race during PINT_PinInterruptConfig triggering false interrupts

- when a Level irq is configured, the level (high vs. low) is configured
after the level mode is selected. Depending on the ARM speed, an interruption
can be triggered before the IENF is configured correctly

- when changing from edge to level, IENR & IENF must be set before configuring
ISEL or a interruption may also be triggered depending on the way
edge detection was configured.

Signed-off-by: Arnaud Mouiche <arnaud.mouiche@invoxia.com>

diff --git a/devices/MIMXRT685S/drivers/fsl_pint.c b/devices/MIMXRT685S/drivers/fsl_pint.c
index b713a21..68630f9 100644
--- a/devices/MIMXRT685S/drivers/fsl_pint.c
+++ b/devices/MIMXRT685S/drivers/fsl_pint.c
@@ -182,28 +182,35 @@ void PINT_PinInterruptConfig(PINT_Type *base, pint_pin_int_t intr, pint_pin_enab
#endif
}

+
+ /* Starts by disabling the possibility of interruption if the edge vs. level is changed */
+ base->CIENR = 1UL << (uint32_t)intr;
+ base->CIENF = 1UL << (uint32_t)intr;
+
/* select level or edge sensitive */
base->ISEL = (base->ISEL & ~(1UL << (uint32_t)intr)) |
((((uint32_t)enable & PINT_PIN_INT_LEVEL) != 0U) ? (1UL << (uint32_t)intr) : 0U);

- /* enable rising or level interrupt */
- if (((unsigned)enable & (PINT_PIN_INT_LEVEL | PINT_PIN_INT_RISE)) != 0U)
+ /* Edge mode: Enable falling
+ * Level mode: select first the level triggering the interrupt before enabling the level sensing
+ */
+ if (((unsigned)enable & PINT_PIN_INT_FALL_OR_HIGH_LEVEL) != 0U)
{
- base->SIENR = 1UL << (uint32_t)intr;
+ base->SIENF = 1UL << (uint32_t)intr;
}
else
{
- base->CIENR = 1UL << (uint32_t)intr;
+ base->CIENF = 1UL << (uint32_t)intr;
}

- /* Enable falling or select high level */
- if (((unsigned)enable & PINT_PIN_INT_FALL_OR_HIGH_LEVEL) != 0U)
+ /* then, enable rising or level interrupt */
+ if (((unsigned)enable & (PINT_PIN_INT_LEVEL | PINT_PIN_INT_RISE)) != 0U)
{
- base->SIENF = 1UL << (uint32_t)intr;
+ base->SIENR = 1UL << (uint32_t)intr;
}
else
{
- base->CIENF = 1UL << (uint32_t)intr;
+ base->CIENR = 1UL << (uint32_t)intr;
}
}

[ArnaudMouiche]

Simply configure a High level interrupt on any gpio, and add a small delay between the "SIENR |= 1 << intr" and the "SIENF |= 1<<intr" of the actual code.

An interruption will trigger during this small delay.

In fact, this is perfectly logical since when you configure such interrupt with current code:

- you start with ISEL=0, IENR=0 and IENF=0

- you set ISEL=1 <<intr

- you set IENR=1 << intr

At this point, since IENF is still 0, you have configured an level irq on low level, which is the current idle state of the gpio line.  => the interruption is triggered.

That's why IENF must be configured BEFORE IENR.

For most of drivers, spurious interrupts are managed and are silent. But in some other drivers, this is a real issue.

[diego_charles]

Thanks for your sharing.
Diego

[diego_charles]

Hi @ArnaudMouiche 

Many thanks for your report! We appreciate it.

Are you able to share with us your test code to replicate the issue?

All the best, 

Diego