Re: [PATCH] gpio: pca953x: Fix warning when HW interrupts are rescheduled by the softirq tasklet

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 09/25/2014 11:07 AM, Linus Walleij wrote:
> On Wed, Sep 24, 2014 at 2:28 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 09/24/2014 02:17 PM, Linus Walleij wrote:
> 
>>> So PCA cannot use gpiochip_set_chained_irqchip()?
>>
>> Yes. It can't - pca is i2c device.
> 
> ? I don't get this statement.
> 
> Why does the fact that it is an I2C device matter?
> We have several devices that are in fact on top of
> I2C (albeit as MFD cells) like gpio-tc3589x.c.

gpio-tc3589x.c doesn't call gpiochip_set_chained_irqchip()

Chained IRQ handler replaces High level HW IRQ handler
(in case of ARM+GIC it replaces handle_fasteoi_irq() with custom handler)

The I2C devices require their IRQ handler to be interruptible and
allow to sleep, so Threaded IRQ handler should be used there.

By the way, gpiochip_set_chained_irqchip() has a blocking check inside:
	if (gpiochip->can_sleep) {
		chip_err(gpiochip, "you cannot have chained interrupts on a chip that may sleep\n");
		return;
	}

Actually, this issue can occur only with Nested IRQs (not chained). 

> 
>>> Anyway I feel I should fix this as per above for all
>>> chips using that function, right?
>>
>> Pls note, the gpiochip_irqchip_add() is called before
>> gpiochip_set_chained_irqchip() in all places now.
> 
> Yes, but the .map function isn't called until a client
> wants to use an IRQ. And that won't happen until after
> we exit the whole .probe() function.

.map function is called from irq_create_mapping() which,
in turn, can be called as from irq_domain_add_simple() -
as result .map will be always called from gpiochip_irqchip_add().

> 
>> Also seems, we should assume that
>> gpiochip_set_chained_irqchip() can be called
>> few times and it could be unsafe to use it for storing parent_irq.
> 
> Well it happens in one single driver, and was done by me.
> Maybe I should either disallow that, as that means we're adding
> multiple parents (which is what you want, right?) or actually
> implement it in a way so that multiple parents can be handled
> by the helpers, by adding the parents to a list or something.


Sorry, but It seems the simplest way is to allow drivers to manage
parent IRQs for the complex cases. So, I vote for custom .map()
callback, but It could be not too simple to implement it, because
private driver data need to be passed as parameter to this callback
somehow.

Of course, final decision is up to you and may be it would be ok
to fix just simple case (one parent IRQ) in short term perspective. 

=== Simple one - driver need to set parent_irq before adding gpiochip ===

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8aa84d5..292840b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -447,8 +447,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
        irq_set_lockdep_class(irq, &gpiochip_irq_lock_class);
        irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler);
        /* Chips that can sleep need nested thread handlers */
-       if (chip->can_sleep)
+       if (chip->can_sleep) {
                irq_set_nested_thread(irq, 1);
+               if (chip->parent_irq)
+                       irq_set_parent(irq, chip->parent_irq);
+       }
 #ifdef CONFIG_ARM
        set_irq_flags(irq, IRQF_VALID);
 #else
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 976276a..88486c7 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -112,6 +112,7 @@ struct gpio_chip {
        struct irq_domain       *irqdomain;
        irq_flow_handler_t      irq_handler;
        unsigned int            irq_default_type;
+       int                     parent_irq;
 #endif
 
 #if defined(CONFIG_OF_GPIO)


> 
>> So maybe it would be enough to just adding field for parent_irq
>> in struct gpio_chip + fix for gpiochip_irq_map() to re-use it
>> (to fix simple cases :).
> 
> Yes for the simple case. But maybe it's better to patch
> gpiochip_set_chained_irqchip() to handle also the complex
> case per above?

As I mentioned above gpiochip_set_chained_irqchip() can't be
used for GPIO chips which set .can_sleep = true.

Regards,
-grygorii