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

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

[Lothar Waßmann]

When using e.g. the matrix_keymap driver with the gpio-pca953x driver,
the following warning may be issued when a keypress is detected:
WARNING: CPU: 0 PID: 3 at kernel/irq/manage.c:677 irq_nested_primary_handler+0x18/0x2c()
Primary handler called for nested irq 245
Modules linked in: evbug ci_hdrc_imx usbmisc_imx ci_hdrc udc_core ehci_hcd phy_mxs_usb
CPU: 0 PID: 3 Comm: ksoftirqd/0 Tainted: G        W     3.16.0-karo+ #118
[<c00142cc>] (unwind_backtrace) from [<c00118f8>] (show_stack+0x10/0x14)
[<c00118f8>] (show_stack) from [<c001bf10>] (warn_slowpath_common+0x64/0x84)
[<c001bf10>] (warn_slowpath_common) from [<c001bfc4>] (warn_slowpath_fmt+0x30/0x40)
[<c001bfc4>] (warn_slowpath_fmt) from [<c004ce08>] (irq_nested_primary_handler+0x18/0x2c)
[<c004ce08>] (irq_nested_primary_handler) from [<c004cb80>] (handle_irq_event_percpu+0x50/0x168)
[<c004cb80>] (handle_irq_event_percpu) from [<c004ccf8>] (handle_irq_event+0x60/0x7c)
[<c004ccf8>] (handle_irq_event) from [<c004f02c>] (handle_simple_irq+0x78/0xdc)
[<c004f02c>] (handle_simple_irq) from [<c004ed14>] (resend_irqs+0x4c/0x78)
[<c004ed14>] (resend_irqs) from [<c001f3ec>] (tasklet_action+0x74/0xd8)
[<c001f3ec>] (tasklet_action) from [<c001f760>] (__do_softirq+0xd0/0x1f4)
[<c001f760>] (__do_softirq) from [<c001f8c4>] (run_ksoftirqd+0x40/0x64)
[<c001f8c4>] (run_ksoftirqd) from [<c003d464>] (smpboot_thread_fn+0x174/0x234)
[<c003d464>] (smpboot_thread_fn) from [<c0036dd4>] (kthread+0xb4/0xd0)
[<c0036dd4>] (kthread) from [<c000f0f0>] (ret_from_fork+0x14/0x24)
---[ end trace a68cf7bc5348c4f7 ]---

This happens when an IRQ is detected by the GPIO driver while the GPIO
client driver (matrix_keypad in this case) has disabled the irq for
the GPIO it has acquired. When the HW IRQ is being rescheduled by the
softirq thread, the primary IRQ handler is called for the nested IRQ
registered by the client driver rather than for the parent IRQ from
the GPIO driver.

This patch makes sure, that the parent_irq (gpio-pca953x) is
rescheduled rather than the nested irq registered by the matrix_keypad
driver.
Similar patches are most probably required for a bunch of other
drivers too.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/gpio/gpio-pca953x.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index e2da64a..770ef6b 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -518,6 +518,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 			     int irq_base)
 {
 	struct i2c_client *client = chip->client;
+	struct gpio_chip *gpio_chip = &chip->gpio_chip;
 	int ret, i, offset = 0;
 
 	if (client->irq && irq_base != -1
@@ -567,6 +568,17 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 				"could not connect irqchip to gpiochip\n");
 			return ret;
 		}
+
+		for (i = 0; i < NBANK(chip); i++) {
+			int j;
+
+			for (j = 0; j < BANK_SZ; j++) {
+				int gpio = gpio_chip->base + i * BANK_SZ + j;
+				int irq = gpio_to_irq(gpio);
+
+				irq_set_parent(irq, client->irq);
+			}
+		}
 	}
 
 	return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

On Tue, Sep 9, 2014 at 10:32 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:

> When using e.g. the matrix_keymap driver with the gpio-pca953x driver,
> the following warning may be issued when a keypress is detected:
> WARNING: CPU: 0 PID: 3 at kernel/irq/manage.c:677 irq_nested_primary_handler+0x18/0x2c()
> Primary handler called for nested irq 245
> Modules linked in: evbug ci_hdrc_imx usbmisc_imx ci_hdrc udc_core ehci_hcd phy_mxs_usb
> CPU: 0 PID: 3 Comm: ksoftirqd/0 Tainted: G        W     3.16.0-karo+ #118
> [<c00142cc>] (unwind_backtrace) from [<c00118f8>] (show_stack+0x10/0x14)
> [<c00118f8>] (show_stack) from [<c001bf10>] (warn_slowpath_common+0x64/0x84)
> [<c001bf10>] (warn_slowpath_common) from [<c001bfc4>] (warn_slowpath_fmt+0x30/0x40)
> [<c001bfc4>] (warn_slowpath_fmt) from [<c004ce08>] (irq_nested_primary_handler+0x18/0x2c)
> [<c004ce08>] (irq_nested_primary_handler) from [<c004cb80>] (handle_irq_event_percpu+0x50/0x168)
> [<c004cb80>] (handle_irq_event_percpu) from [<c004ccf8>] (handle_irq_event+0x60/0x7c)
> [<c004ccf8>] (handle_irq_event) from [<c004f02c>] (handle_simple_irq+0x78/0xdc)
> [<c004f02c>] (handle_simple_irq) from [<c004ed14>] (resend_irqs+0x4c/0x78)
> [<c004ed14>] (resend_irqs) from [<c001f3ec>] (tasklet_action+0x74/0xd8)
> [<c001f3ec>] (tasklet_action) from [<c001f760>] (__do_softirq+0xd0/0x1f4)
> [<c001f760>] (__do_softirq) from [<c001f8c4>] (run_ksoftirqd+0x40/0x64)
> [<c001f8c4>] (run_ksoftirqd) from [<c003d464>] (smpboot_thread_fn+0x174/0x234)
> [<c003d464>] (smpboot_thread_fn) from [<c0036dd4>] (kthread+0xb4/0xd0)
> [<c0036dd4>] (kthread) from [<c000f0f0>] (ret_from_fork+0x14/0x24)
> ---[ end trace a68cf7bc5348c4f7 ]---
>
> This happens when an IRQ is detected by the GPIO driver while the GPIO
> client driver (matrix_keypad in this case) has disabled the irq for
> the GPIO it has acquired. When the HW IRQ is being rescheduled by the
> softirq thread, the primary IRQ handler is called for the nested IRQ
> registered by the client driver rather than for the parent IRQ from
> the GPIO driver.
>
> This patch makes sure, that the parent_irq (gpio-pca953x) is
> rescheduled rather than the nested irq registered by the matrix_keypad
> driver.
> Similar patches are most probably required for a bunch of other
> drivers too.
>
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/gpio/gpio-pca953x.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index e2da64a..770ef6b 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -518,6 +518,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>                              int irq_base)
>  {
>         struct i2c_client *client = chip->client;
> +       struct gpio_chip *gpio_chip = &chip->gpio_chip;
>         int ret, i, offset = 0;
>
>         if (client->irq && irq_base != -1
> @@ -567,6 +568,17 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>                                 "could not connect irqchip to gpiochip\n");
>                         return ret;
>                 }
> +
> +               for (i = 0; i < NBANK(chip); i++) {
> +                       int j;
> +
> +                       for (j = 0; j < BANK_SZ; j++) {
> +                               int gpio = gpio_chip->base + i * BANK_SZ + j;
> +                               int irq = gpio_to_irq(gpio);
> +
> +                               irq_set_parent(irq, client->irq);
> +                       }
> +               }

While this is fixing the problem, but isn't the right fix to patch
the function gpiochip_irq_map() in gpiolib.c to call
irq_set_parent() for each IRQ as it gets mapped?

This driver is using the gpiolib irqchip helpers...

Then you fix not just this driver but all drivers, plus the complex
loop and calls to gpio_to_irq() etc goes away.

Yours,
Linus Walleij

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

[Grygorii Strashko]

On 09/23/2014 03:04 PM, Linus Walleij wrote:
> On Tue, Sep 9, 2014 at 10:32 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> 
>> When using e.g. the matrix_keymap driver with the gpio-pca953x driver,
>> the following warning may be issued when a keypress is detected:
>> WARNING: CPU: 0 PID: 3 at kernel/irq/manage.c:677 irq_nested_primary_handler+0x18/0x2c()
>> Primary handler called for nested irq 245
>> Modules linked in: evbug ci_hdrc_imx usbmisc_imx ci_hdrc udc_core ehci_hcd phy_mxs_usb
>> CPU: 0 PID: 3 Comm: ksoftirqd/0 Tainted: G        W     3.16.0-karo+ #118
>> [<c00142cc>] (unwind_backtrace) from [<c00118f8>] (show_stack+0x10/0x14)
>> [<c00118f8>] (show_stack) from [<c001bf10>] (warn_slowpath_common+0x64/0x84)
>> [<c001bf10>] (warn_slowpath_common) from [<c001bfc4>] (warn_slowpath_fmt+0x30/0x40)
>> [<c001bfc4>] (warn_slowpath_fmt) from [<c004ce08>] (irq_nested_primary_handler+0x18/0x2c)
>> [<c004ce08>] (irq_nested_primary_handler) from [<c004cb80>] (handle_irq_event_percpu+0x50/0x168)
>> [<c004cb80>] (handle_irq_event_percpu) from [<c004ccf8>] (handle_irq_event+0x60/0x7c)
>> [<c004ccf8>] (handle_irq_event) from [<c004f02c>] (handle_simple_irq+0x78/0xdc)
>> [<c004f02c>] (handle_simple_irq) from [<c004ed14>] (resend_irqs+0x4c/0x78)
>> [<c004ed14>] (resend_irqs) from [<c001f3ec>] (tasklet_action+0x74/0xd8)
>> [<c001f3ec>] (tasklet_action) from [<c001f760>] (__do_softirq+0xd0/0x1f4)
>> [<c001f760>] (__do_softirq) from [<c001f8c4>] (run_ksoftirqd+0x40/0x64)
>> [<c001f8c4>] (run_ksoftirqd) from [<c003d464>] (smpboot_thread_fn+0x174/0x234)
>> [<c003d464>] (smpboot_thread_fn) from [<c0036dd4>] (kthread+0xb4/0xd0)
>> [<c0036dd4>] (kthread) from [<c000f0f0>] (ret_from_fork+0x14/0x24)
>> ---[ end trace a68cf7bc5348c4f7 ]---
>>
>> This happens when an IRQ is detected by the GPIO driver while the GPIO
>> client driver (matrix_keypad in this case) has disabled the irq for
>> the GPIO it has acquired. When the HW IRQ is being rescheduled by the
>> softirq thread, the primary IRQ handler is called for the nested IRQ
>> registered by the client driver rather than for the parent IRQ from
>> the GPIO driver.
>>
>> This patch makes sure, that the parent_irq (gpio-pca953x) is
>> rescheduled rather than the nested irq registered by the matrix_keypad
>> driver.
>> Similar patches are most probably required for a bunch of other
>> drivers too.
>>
>> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> ---
>>   drivers/gpio/gpio-pca953x.c |   12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index e2da64a..770ef6b 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -518,6 +518,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>>                               int irq_base)
>>   {
>>          struct i2c_client *client = chip->client;
>> +       struct gpio_chip *gpio_chip = &chip->gpio_chip;
>>          int ret, i, offset = 0;
>>
>>          if (client->irq && irq_base != -1
>> @@ -567,6 +568,17 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>>                                  "could not connect irqchip to gpiochip\n");
>>                          return ret;
>>                  }
>> +
>> +               for (i = 0; i < NBANK(chip); i++) {
>> +                       int j;
>> +
>> +                       for (j = 0; j < BANK_SZ; j++) {
>> +                               int gpio = gpio_chip->base + i * BANK_SZ + j;
>> +                               int irq = gpio_to_irq(gpio);
>> +
>> +                               irq_set_parent(irq, client->irq);
>> +                       }
>> +               }
> 
> While this is fixing the problem, but isn't the right fix to patch
> the function gpiochip_irq_map() in gpiolib.c to call
> irq_set_parent() for each IRQ as it gets mapped?
> 
> This driver is using the gpiolib irqchip helpers...
> 
> Then you fix not just this driver but all drivers, plus the complex
> loop and calls to gpio_to_irq() etc goes away.

The problem here is that:
- we don't know parent IRQ number inside gpiolib irqchip;
- there can be more then one Parent IRQ per GPIO chip

in other words, the knowledge about Parent IRQs is specific to drivers :(

Might be custom .irq_map() callback can be used to fix this issue? 

Regards,
-grygorii

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

On Tue, Sep 23, 2014 at 2:26 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 09/23/2014 03:04 PM, Linus Walleij wrote:
>> On Tue, Sep 9, 2014 at 10:32 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:

>>> @@ -567,6 +568,17 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>>>                                  "could not connect irqchip to gpiochip\n");
>>>                          return ret;
>>>                  }
>>> +
>>> +               for (i = 0; i < NBANK(chip); i++) {
>>> +                       int j;
>>> +
>>> +                       for (j = 0; j < BANK_SZ; j++) {
>>> +                               int gpio = gpio_chip->base + i * BANK_SZ + j;
>>> +                               int irq = gpio_to_irq(gpio);
>>> +
>>> +                               irq_set_parent(irq, client->irq);
>>> +                       }
>>> +               }
>>
>> While this is fixing the problem, but isn't the right fix to patch
>> the function gpiochip_irq_map() in gpiolib.c to call
>> irq_set_parent() for each IRQ as it gets mapped?
>>
>> This driver is using the gpiolib irqchip helpers...
>>
>> Then you fix not just this driver but all drivers, plus the complex
>> loop and calls to gpio_to_irq() etc goes away.
>
> The problem here is that:
> - we don't know parent IRQ number inside gpiolib irqchip;
> - there can be more then one Parent IRQ per GPIO chip

We could for the simple case:

void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
                  struct irq_chip *irqchip,
                  int parent_irq,
                  irq_flow_handler_t parent_handler)

Note parent_irq.

Just add a field for parent_irq in struct gpio_chip so the
mapping function has this around.

But the second case with multiple parents is a valid
counterargument, gpiolib irqchip helpers
is just for the simple case of a cascaded IRQ off a single
parent, not for complex scenarios.

So PCA cannot use gpiochip_set_chained_irqchip()?

Anyway I feel I should fix this as per above for all
chips using that function, right?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 09/24/2014 02:17 PM, Linus Walleij wrote:
> On Tue, Sep 23, 2014 at 2:26 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 09/23/2014 03:04 PM, Linus Walleij wrote:
>>> On Tue, Sep 9, 2014 at 10:32 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> 
>>>> @@ -567,6 +568,17 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>>>>                                   "could not connect irqchip to gpiochip\n");
>>>>                           return ret;
>>>>                   }
>>>> +
>>>> +               for (i = 0; i < NBANK(chip); i++) {
>>>> +                       int j;
>>>> +
>>>> +                       for (j = 0; j < BANK_SZ; j++) {
>>>> +                               int gpio = gpio_chip->base + i * BANK_SZ + j;
>>>> +                               int irq = gpio_to_irq(gpio);
>>>> +
>>>> +                               irq_set_parent(irq, client->irq);
>>>> +                       }
>>>> +               }
>>>
>>> While this is fixing the problem, but isn't the right fix to patch
>>> the function gpiochip_irq_map() in gpiolib.c to call
>>> irq_set_parent() for each IRQ as it gets mapped?
>>>
>>> This driver is using the gpiolib irqchip helpers...
>>>
>>> Then you fix not just this driver but all drivers, plus the complex
>>> loop and calls to gpio_to_irq() etc goes away.
>>
>> The problem here is that:
>> - we don't know parent IRQ number inside gpiolib irqchip;
>> - there can be more then one Parent IRQ per GPIO chip
> 
> We could for the simple case:
> 
> void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
>                    struct irq_chip *irqchip,
>                    int parent_irq,
>                    irq_flow_handler_t parent_handler)
> 
> Note parent_irq.
> 
> Just add a field for parent_irq in struct gpio_chip so the
> mapping function has this around.
> 
> But the second case with multiple parents is a valid
> counterargument, gpiolib irqchip helpers
> is just for the simple case of a cascaded IRQ off a single
> parent, not for complex scenarios.
> 
> So PCA cannot use gpiochip_set_chained_irqchip()?

Yes. It can't - pca is i2c device.

> 
> 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.

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.

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 :).

regards,
-grygorii
 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

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.

>> 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.

> 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.

> 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?

Yours,
Linus Walleij

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

[Grygorii Strashko]

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

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

[Linus Walleij]

On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> 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()

Ah yeah you're right of course. I considered adding a separate
registration function for the sleeping handlers, like
gpiochip_set_threaded_irqchip() that would handle
setting up the nested case.

>> 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().

Ah yeah you're right, I was just wrong here :-/

>> 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.

Yeah. Well what I'm thinking as subsystem maintainer, is that
when I added the generic GPIOlib irqchip helpers we managed to
smoke out a large:ish set of subtle bugs that different drivers
had created independently of each other.

So centralizing code is very important if at all possible to bring
down the maintainer burden.

So for that reason I'm looking a second and a third time at these
things before going ahead with custom code in drivers...

> === 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);
> +       }

Yeah I need to think of something like this...

Yours,
Linus Walleij

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

[Christian Gmeiner]

Hi all

2014-09-26 11:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> 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()
>
> Ah yeah you're right of course. I considered adding a separate
> registration function for the sleeping handlers, like
> gpiochip_set_threaded_irqchip() that would handle
> setting up the nested case.
>
>>> 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().
>
> Ah yeah you're right, I was just wrong here :-/
>
>>> 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.
>
> Yeah. Well what I'm thinking as subsystem maintainer, is that
> when I added the generic GPIOlib irqchip helpers we managed to
> smoke out a large:ish set of subtle bugs that different drivers
> had created independently of each other.
>
> So centralizing code is very important if at all possible to bring
> down the maintainer burden.
>
> So for that reason I'm looking a second and a third time at these
> things before going ahead with custom code in drivers...
>
>> === 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);
>> +       }
>
> Yeah I need to think of something like this...
>

I did run into exact the same situation with a 4.1.1 kernel :)

[  312.863047] ------------[ cut here ]------------
[  312.867681] WARNING: CPU: 1 PID: 12 at kernel/irq/manage.c:696
irq_nested_primary_handler+0x30/0x38()
[  312.876901] Primary handler called for nested irq 301
[  312.881953] Modules linked in: uinput ipv6 smsc95xx usbnet mii
imx2_wdt etnaviv(C) matrix_keypad matrix_keymap ar1021_i2c
[  312.893067] CPU: 1 PID: 12 Comm: ksoftirqd/1 Tainted: G        WC
   4.1.1 #9
[  312.900378] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  312.906906] Backtrace:
[  312.909377] [<c0013298>] (dump_backtrace) from [<c0013488>]
(show_stack+0x20/0x24)
[  312.916947]  r6:00000009 r5:ffffffff r4:00000000 r3:dc8ba301
[  312.922673] [<c0013468>] (show_stack) from [<c05743c4>]
(dump_stack+0x70/0xc0)
[  312.929924] [<c0574354>] (dump_stack) from [<c002b7b8>]
(warn_slowpath_common+0x88/0xc0)
[  312.938029]  r5:000002b8 r4:00000000
[  312.941678] [<c002b730>] (warn_slowpath_common) from [<c002b8ac>]
(warn_slowpath_fmt+0x40/0x48)
[  312.950391]  r8:00000000 r7:c07b1314 r6:0000012d r5:eea07720 r4:c3048840
[  312.957233] [<c002b870>] (warn_slowpath_fmt) from [<c0075798>]
(irq_nested_primary_handler+0x30/0x38)
[  312.966467]  r3:0000012d r2:c06b9128
[  312.970113] [<c0075768>] (irq_nested_primary_handler) from
[<c0075200>] (handle_irq_event_percpu+0x70/0x2d0)
[  312.979967] [<c0075190>] (handle_irq_event_percpu) from
[<c00754ac>] (handle_irq_event+0x4c/0x6c)
[  312.988854]  r10:00000002 r9:00000018 r8:00000000 r7:c07b1314
r6:c3048840 r5:eea07720
[  312.996812]  r4:eea076c0
[  312.999395] [<c0075460>] (handle_irq_event) from [<c0078204>]
(handle_simple_irq+0xa4/0xc8)
[  313.007760]  r6:c07c5404 r5:eea07720 r4:eea076c0 r3:00000003
[  313.013536] [<c0078160>] (handle_simple_irq) from [<c0077cd4>]
(resend_irqs+0x50/0x7c)
[  313.021468]  r5:c07c5404 r4:0000012d
[  313.025119] [<c0077c84>] (resend_irqs) from [<c002f99c>]
(tasklet_action+0x94/0x140)
[  313.032877]  r6:00000000 r5:c07c5444 r4:c07c5440 r3:c0077c84
[  313.038655] [<c002f908>] (tasklet_action) from [<c002eea8>]
(__do_softirq+0xa0/0x3c8)
[  313.046500]  r8:c07c1908 r7:00000006 r6:00000001 r5:00000040
r4:c07ba098 r3:c002f908
[  313.054387] [<c002ee08>] (__do_softirq) from [<c002f208>]
(run_ksoftirqd+0x38/0x54)
[  313.062058]  r10:00000002 r9:00000000 r8:c07c1908 r7:00000000
r6:00000001 r5:00000001
[  313.070017]  r4:ee893980
[  313.072605] [<c002f1d0>] (run_ksoftirqd) from [<c004b1e4>]
(smpboot_thread_fn+0x1f8/0x2f0)
[  313.080906] [<c004afec>] (smpboot_thread_fn) from [<c0047744>]
(kthread+0xe8/0x104)
[  313.088578]  r10:00000000 r8:00000000 r7:c004afec r6:ee893980
r5:00000000 r4:ee893a40
[  313.096558] [<c004765c>] (kthread) from [<c000fac8>]
(ret_from_fork+0x14/0x2c)
[  313.103796]  r7:00000000 r6:00000000 r5:c004765c r4:ee893a40
[  313.109560] ---[ end trace 96052cda48865769 ]---


Linus, what is the state of the your last thinking about this topic?

greets
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner

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

[Grygorii Strashko]

Hi Christian,

On 07/07/2015 09:29 AM, Christian Gmeiner wrote:
> Hi all
> 
> 2014-09-26 11:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>> 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()
>>
>> Ah yeah you're right of course. I considered adding a separate
>> registration function for the sleeping handlers, like
>> gpiochip_set_threaded_irqchip() that would handle
>> setting up the nested case.
>>
>>>> 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().
>>
>> Ah yeah you're right, I was just wrong here :-/
>>
>>>> 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.
>>
>> Yeah. Well what I'm thinking as subsystem maintainer, is that
>> when I added the generic GPIOlib irqchip helpers we managed to
>> smoke out a large:ish set of subtle bugs that different drivers
>> had created independently of each other.
>>
>> So centralizing code is very important if at all possible to bring
>> down the maintainer burden.
>>
>> So for that reason I'm looking a second and a third time at these
>> things before going ahead with custom code in drivers...
>>
>>> === 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);
>>> +       }
>>
>> Yeah I need to think of something like this...
>>
> 
> I did run into exact the same situation with a 4.1.1 kernel :)
> 
> [  312.863047] ------------[ cut here ]------------
> [  312.867681] WARNING: CPU: 1 PID: 12 at kernel/irq/manage.c:696
> irq_nested_primary_handler+0x30/0x38()
> [  312.876901] Primary handler called for nested irq 301
> [  312.881953] Modules linked in: uinput ipv6 smsc95xx usbnet mii
> imx2_wdt etnaviv(C) matrix_keypad matrix_keymap ar1021_i2c
> [  312.893067] CPU: 1 PID: 12 Comm: ksoftirqd/1 Tainted: G        WC
>     4.1.1 #9
> [  312.900378] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [  312.906906] Backtrace:
> [  312.909377] [<c0013298>] (dump_backtrace) from [<c0013488>]
> (show_stack+0x20/0x24)
> [  312.916947]  r6:00000009 r5:ffffffff r4:00000000 r3:dc8ba301
> [  312.922673] [<c0013468>] (show_stack) from [<c05743c4>]
> (dump_stack+0x70/0xc0)
> [  312.929924] [<c0574354>] (dump_stack) from [<c002b7b8>]
> (warn_slowpath_common+0x88/0xc0)
> [  312.938029]  r5:000002b8 r4:00000000
> [  312.941678] [<c002b730>] (warn_slowpath_common) from [<c002b8ac>]
> (warn_slowpath_fmt+0x40/0x48)
> [  312.950391]  r8:00000000 r7:c07b1314 r6:0000012d r5:eea07720 r4:c3048840
> [  312.957233] [<c002b870>] (warn_slowpath_fmt) from [<c0075798>]
> (irq_nested_primary_handler+0x30/0x38)
> [  312.966467]  r3:0000012d r2:c06b9128
> [  312.970113] [<c0075768>] (irq_nested_primary_handler) from
> [<c0075200>] (handle_irq_event_percpu+0x70/0x2d0)
> [  312.979967] [<c0075190>] (handle_irq_event_percpu) from
> [<c00754ac>] (handle_irq_event+0x4c/0x6c)
> [  312.988854]  r10:00000002 r9:00000018 r8:00000000 r7:c07b1314
> r6:c3048840 r5:eea07720
> [  312.996812]  r4:eea076c0
> [  312.999395] [<c0075460>] (handle_irq_event) from [<c0078204>]
> (handle_simple_irq+0xa4/0xc8)
> [  313.007760]  r6:c07c5404 r5:eea07720 r4:eea076c0 r3:00000003
> [  313.013536] [<c0078160>] (handle_simple_irq) from [<c0077cd4>]
> (resend_irqs+0x50/0x7c)
> [  313.021468]  r5:c07c5404 r4:0000012d
> [  313.025119] [<c0077c84>] (resend_irqs) from [<c002f99c>]
> (tasklet_action+0x94/0x140)
> [  313.032877]  r6:00000000 r5:c07c5444 r4:c07c5440 r3:c0077c84
> [  313.038655] [<c002f908>] (tasklet_action) from [<c002eea8>]
> (__do_softirq+0xa0/0x3c8)
> [  313.046500]  r8:c07c1908 r7:00000006 r6:00000001 r5:00000040
> r4:c07ba098 r3:c002f908
> [  313.054387] [<c002ee08>] (__do_softirq) from [<c002f208>]
> (run_ksoftirqd+0x38/0x54)
> [  313.062058]  r10:00000002 r9:00000000 r8:c07c1908 r7:00000000
> r6:00000001 r5:00000001
> [  313.070017]  r4:ee893980
> [  313.072605] [<c002f1d0>] (run_ksoftirqd) from [<c004b1e4>]
> (smpboot_thread_fn+0x1f8/0x2f0)
> [  313.080906] [<c004afec>] (smpboot_thread_fn) from [<c0047744>]
> (kthread+0xe8/0x104)
> [  313.088578]  r10:00000000 r8:00000000 r7:c004afec r6:ee893980
> r5:00000000 r4:ee893a40
> [  313.096558] [<c004765c>] (kthread) from [<c000fac8>]
> (ret_from_fork+0x14/0x2c)
> [  313.103796]  r7:00000000 r6:00000000 r5:c004765c r4:ee893a40
> [  313.109560] ---[ end trace 96052cda48865769 ]---
> 
> 
> Linus, what is the state of the your last thinking about this topic?

Could you try if below change works for you, pls (not tested):

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d233eb3..ac29308 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -570,6 +570,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
                                "could not connect irqchip to gpiochip\n");
                        return ret;
                }
+
+               gpiochip_set_chained_irqchip(&chip->gpio_chip, &pca953x_irq_chip,
+                                            client->irq, NULL);
        }
 
        return 0;
-- 
regards,
-grygorii

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

[Christian Gmeiner]

Hi Grygorii


2015-07-07 12:00 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
> Hi Christian,
>
> On 07/07/2015 09:29 AM, Christian Gmeiner wrote:
>> Hi all
>>
>> 2014-09-26 11:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>>> On Thu, Sep 25, 2014 at 6:26 PM, Grygorii Strashko
>>> <grygorii.strashko@ti.com> wrote:
>>>> 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()
>>>
>>> Ah yeah you're right of course. I considered adding a separate
>>> registration function for the sleeping handlers, like
>>> gpiochip_set_threaded_irqchip() that would handle
>>> setting up the nested case.
>>>
>>>>> 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().
>>>
>>> Ah yeah you're right, I was just wrong here :-/
>>>
>>>>> 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.
>>>
>>> Yeah. Well what I'm thinking as subsystem maintainer, is that
>>> when I added the generic GPIOlib irqchip helpers we managed to
>>> smoke out a large:ish set of subtle bugs that different drivers
>>> had created independently of each other.
>>>
>>> So centralizing code is very important if at all possible to bring
>>> down the maintainer burden.
>>>
>>> So for that reason I'm looking a second and a third time at these
>>> things before going ahead with custom code in drivers...
>>>
>>>> === 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);
>>>> +       }
>>>
>>> Yeah I need to think of something like this...
>>>
>>
>> I did run into exact the same situation with a 4.1.1 kernel :)
>>
>> [  312.863047] ------------[ cut here ]------------
>> [  312.867681] WARNING: CPU: 1 PID: 12 at kernel/irq/manage.c:696
>> irq_nested_primary_handler+0x30/0x38()
>> [  312.876901] Primary handler called for nested irq 301
>> [  312.881953] Modules linked in: uinput ipv6 smsc95xx usbnet mii
>> imx2_wdt etnaviv(C) matrix_keypad matrix_keymap ar1021_i2c
>> [  312.893067] CPU: 1 PID: 12 Comm: ksoftirqd/1 Tainted: G        WC
>>     4.1.1 #9
>> [  312.900378] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>> [  312.906906] Backtrace:
>> [  312.909377] [<c0013298>] (dump_backtrace) from [<c0013488>]
>> (show_stack+0x20/0x24)
>> [  312.916947]  r6:00000009 r5:ffffffff r4:00000000 r3:dc8ba301
>> [  312.922673] [<c0013468>] (show_stack) from [<c05743c4>]
>> (dump_stack+0x70/0xc0)
>> [  312.929924] [<c0574354>] (dump_stack) from [<c002b7b8>]
>> (warn_slowpath_common+0x88/0xc0)
>> [  312.938029]  r5:000002b8 r4:00000000
>> [  312.941678] [<c002b730>] (warn_slowpath_common) from [<c002b8ac>]
>> (warn_slowpath_fmt+0x40/0x48)
>> [  312.950391]  r8:00000000 r7:c07b1314 r6:0000012d r5:eea07720 r4:c3048840
>> [  312.957233] [<c002b870>] (warn_slowpath_fmt) from [<c0075798>]
>> (irq_nested_primary_handler+0x30/0x38)
>> [  312.966467]  r3:0000012d r2:c06b9128
>> [  312.970113] [<c0075768>] (irq_nested_primary_handler) from
>> [<c0075200>] (handle_irq_event_percpu+0x70/0x2d0)
>> [  312.979967] [<c0075190>] (handle_irq_event_percpu) from
>> [<c00754ac>] (handle_irq_event+0x4c/0x6c)
>> [  312.988854]  r10:00000002 r9:00000018 r8:00000000 r7:c07b1314
>> r6:c3048840 r5:eea07720
>> [  312.996812]  r4:eea076c0
>> [  312.999395] [<c0075460>] (handle_irq_event) from [<c0078204>]
>> (handle_simple_irq+0xa4/0xc8)
>> [  313.007760]  r6:c07c5404 r5:eea07720 r4:eea076c0 r3:00000003
>> [  313.013536] [<c0078160>] (handle_simple_irq) from [<c0077cd4>]
>> (resend_irqs+0x50/0x7c)
>> [  313.021468]  r5:c07c5404 r4:0000012d
>> [  313.025119] [<c0077c84>] (resend_irqs) from [<c002f99c>]
>> (tasklet_action+0x94/0x140)
>> [  313.032877]  r6:00000000 r5:c07c5444 r4:c07c5440 r3:c0077c84
>> [  313.038655] [<c002f908>] (tasklet_action) from [<c002eea8>]
>> (__do_softirq+0xa0/0x3c8)
>> [  313.046500]  r8:c07c1908 r7:00000006 r6:00000001 r5:00000040
>> r4:c07ba098 r3:c002f908
>> [  313.054387] [<c002ee08>] (__do_softirq) from [<c002f208>]
>> (run_ksoftirqd+0x38/0x54)
>> [  313.062058]  r10:00000002 r9:00000000 r8:c07c1908 r7:00000000
>> r6:00000001 r5:00000001
>> [  313.070017]  r4:ee893980
>> [  313.072605] [<c002f1d0>] (run_ksoftirqd) from [<c004b1e4>]
>> (smpboot_thread_fn+0x1f8/0x2f0)
>> [  313.080906] [<c004afec>] (smpboot_thread_fn) from [<c0047744>]
>> (kthread+0xe8/0x104)
>> [  313.088578]  r10:00000000 r8:00000000 r7:c004afec r6:ee893980
>> r5:00000000 r4:ee893a40
>> [  313.096558] [<c004765c>] (kthread) from [<c000fac8>]
>> (ret_from_fork+0x14/0x2c)
>> [  313.103796]  r7:00000000 r6:00000000 r5:c004765c r4:ee893a40
>> [  313.109560] ---[ end trace 96052cda48865769 ]---
>>
>>
>> Linus, what is the state of the your last thinking about this topic?
>
> Could you try if below change works for you, pls (not tested):
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index d233eb3..ac29308 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -570,6 +570,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>                                 "could not connect irqchip to gpiochip\n");
>                         return ret;
>                 }
> +
> +               gpiochip_set_chained_irqchip(&chip->gpio_chip, &pca953x_irq_chip,
> +                                            client->irq, NULL);
>         }
>
>         return 0;

Nice - I see no warnings from the kernel in dmesg in my tests.

Tested-by: Christian Gmeiner <christian.gmeiner@gmail.com>

greets
--
Christian Gmeiner, MSc

https://soundcloud.com/christian-gmeiner

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

[Linus Walleij]

On Tue, Jul 7, 2015 at 12:00 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 07/07/2015 09:29 AM, Christian Gmeiner wrote:

>> Linus, what is the state of the your last thinking about this topic?
>
> Could you try if below change works for you, pls (not tested):
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index d233eb3..ac29308 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -570,6 +570,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>                                 "could not connect irqchip to gpiochip\n");
>                         return ret;
>                 }
> +
> +               gpiochip_set_chained_irqchip(&chip->gpio_chip, &pca953x_irq_chip,
> +                                            client->irq, NULL);

Looks like it works, can you send a proper patch?

(Maybe there is one in my inbox, whatdoIknow...)

Yours,
Linus Walleij