Re: [Linux-stm32] [PATCH 2/2] pinctrl: stm32: fix GPIO level interrupts

[Antonio Borneo <antonio.borneo@xxxxxxxxxxx>]

On Mon, 2023-12-04 at 15:33 -0500, Ben Wolsieffer wrote:
> The STM32 doesn't support GPIO level interrupts in hardware, so the
> driver tries to emulate them using edge interrupts, by retriggering the
> interrupt if necessary based on the pin state after the handler
> finishes.
> 
> Currently, this functionality does not work because the irqchip uses
> handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask()
> callbacks after handling the interrupt. This patch fixes this by using
> handle_level_irq() for level interrupts, which causes irq_unmask() to be
> called to retrigger the interrupt.
> 
> Signed-off-by: Ben Wolsieffer <ben.wolsieffer@xxxxxxxxxxx>
> ---
>  drivers/pinctrl/stm32/pinctrl-stm32.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 603f900e88c1..fb9532601cbb 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -348,12 +348,15 @@ static int stm32_gpio_set_type(struct irq_data *d, unsigned int type)
>         case IRQ_TYPE_EDGE_RISING:
>         case IRQ_TYPE_EDGE_FALLING:
>         case IRQ_TYPE_EDGE_BOTH:
> +               irq_set_handler_locked(d, handle_edge_irq);

Hi,
this patch causes a NULL pointer dereference and crashes the kernel boot on STM32 MPU's,
either STM32MP13x, STM32MP15x and the new STM32MP25x.

Please do not merge it as is.

This pinctrl-stm32 driver is shared between STM32 MCUs and MPUs.
In both cases the EXTI is the parent interrupt controller of this pinctrl, but despite
the fact that there is a single file irq-stm32-exti.c, it contains two independent
drivers, one for MCUs and the other for MPUs.
Swapping in this function the irq_desc::handle_irq between handle_edge_irq() and
handle_level_irq() is probably fine for MCU (I have not tested it).
But on MPUs the default handler is handle_fasteoi_irq(); should not be changed here.

Checking quickly ... this function calls irq_chip_set_type_parent() at the very end.
It will in turn call EXTI's irq_set_type(), which has different implementations for MCU
and MPU.
By moving this handler swap in the MCU specific stm32_irq_set_type() it will not impact
MPUs.

Best Regards,
Antonio


>                 parent_type = type;
>                 break;
>         case IRQ_TYPE_LEVEL_HIGH:
> +               irq_set_handler_locked(d, handle_level_irq);
>                 parent_type = IRQ_TYPE_EDGE_RISING;
>                 break;
>         case IRQ_TYPE_LEVEL_LOW:
> +               irq_set_handler_locked(d, handle_level_irq);
>                 parent_type = IRQ_TYPE_EDGE_FALLING;
>                 break;
>         default: