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

[Fabien Dessenne]

Hi,


I have a fix for that issue, but I need to spend more time testing some 
additional corner cases.

As suggested by Alexandre, I have moved the EXTI MUX validity from the 
.activate() to the .alloc() ops.

gpiod_to_irq() calls (indirectly) irq_create_fwspec_mapping() which then 
calls .activate(). So the invalid IRQ can be detected earlier avoiding 
to break the irq mapping.

Thank you Martin for having raised that issue.
I'll post the fix as soon as I have passe more tests.

BR

Fabien



On 24/02/2021 5:37 pm, Alexandre TORGUE wrote:
> 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