[Linux-stm32] [BUG] stm32mp1 gpiod_to_irq() can cause "interrupt stealing"

[Alexandre TORGUE]

Hi Martin

Due to my mailer my answer was not clear.

> -----Original Message-----
> From: Alexandre TORGUE <alexandre.torgue at st.com>
> Sent: mercredi 24 février 2021 16:59
> To: Fuzzey, Martin <martin.fuzzey at flowbird.group>; linux-stm32 at st-md-
> mailman.stormreply.com
> Cc: Fabien DESSENNE - foss <fabien.dessenne at foss.st.com>
> Subject: Re: [BUG] stm32mp1 gpiod_to_irq() can cause "interrupt stealing"
> 
> Hi Martin
> 
> 
> From: Fuzzey, Martin <martin.fuzzey at flowbird.group>
> Sent: Wednesday, February 24, 2021 3:15 PM
> To: linux-stm32 at st-md-mailman.stormreply.com <linux-stm32 at st-md-
> mailman.stormreply.com>
> Cc: Alexandre TORGUE <alexandre.torgue at st.com>
> Subject: [BUG] stm32mp1 gpiod_to_irq() can cause "interrupt stealing"
> 
> Hi,
> 
> I have run into a rather nasty problem on STM32MP1 and I'm not quite sure
> of the right way to fix it (otherwise I would have sent a patch).
> 
> It is possible for one driver to break another driver's GPIO interrupts.
> In fact it is even possible for userspace to break a driver's interrupts.
> 
> On STM32MP1 the external GPIO interrupts go through the EXTI module
> which has 16 interrupt lines connected to the ARM GIC.
> The EXTI maps the GPIO interrupts by line number (so GPIOA_0, GPIOB_0
> ...) all share one GIC interrupt line and only one can be used at any time
> (selected by a multiplexer register in the EXTI)
> 
> The existing stm32mp1 pinctrl driver already handles this, both the setting of
> the EXTI multiplexer and mutual exclusion.
> 
> However consider a driver which does something like
> 
>     int irq = gpiod_to_irq(gpiod);
>     int ret = devm_request_irq(dev, irq, ...)
> 
> Suppose there are 2 such drivers using a gpiod configured from DT that
> happen to both have the same line number (eg GPIOA0 and GPIOB0)
> 
> In this case everything works fine initially for the first driver to probe.
> When the second driver probes gpiod_to_irq() obtains a linux virq number
> but devm_request_irq() returns -EBUSY.
> So far so good, the DT configuration was not compatible with the hardware
> constraints so it is expected to fail.
> 
> However, after the second driver calls gpiod_to_irq(), the interrupt already
> successfully obtained by the first driver no longer receives any interrupts.
> 
> The reason is that gpiod_to_irq() calls the .alloc function of the EXTI interrupt
> controller which, in turn calls .alloc of the parent GIC interrupt controller.
> 
> That not only allocates a linux virq number but also changes the mapping of
> the GIC interrupt line to the new VIRQ.
> 
> So when an interrupt occurs on the GPIO used by the first driver it will now
> be mapped to the VIRQ allocated by the second driver and ignored...


 
Even if this second interrupt is freed ? We have to check if we need to add a
link between .alloc and .activate callbacks or move the protection.


 
> When using the legacy sysfs gpio interface it is even worse because it is
> possible to break the interrupts of a kernel driver without even explicitly
> requesting an interrupt from userspace.
> 
> drivers/gpio/gpiolib-sysfs.c contains this function
> 
> static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
>                                int n)
> {
> ...
>         if (attr == &dev_attr_direction.attr) {
>                 if (!show_direction)
>                         mode = 0;
>         } else if (attr == &dev_attr_edge.attr) {
>                 if (gpiod_to_irq(desc) < 0)
>                         mode = 0;
>                 if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
>                         mode = 0;
>         }
> 
>         return mode;
> }
> 
> The purpose of this is to hide the "edge" attribute if the GPIO does not
> support interrupts (even if no interrupt has yet been requested for that
> GPIO) It does this using the call to gpiod_to_irq() and hiding the attribute if it
> fails.
> 
> So, merely exporting to userspace via sysfs a GPIO (even not as an
> interrupt) having the same line number as another GPIO used by a kernel
> driver will break interrupts for that driver...
> 
> Regards,
> 
> Martin