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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5421670D.10200@ti.com>
Date:	Tue, 23 Sep 2014 15:26:53 +0300
From:	Grygorii Strashko <grygorii.strashko@...com>
To:	Linus Walleij <linus.walleij@...aro.org>,
	Lothar Waßmann 
	<LW@...o-electronics.de>
CC:	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Alexandre Courbot <gnurou@...il.com>
Subject: Re: [PATCH] gpio: pca953x: Fix warning when HW interrupts are rescheduled
 by the softirq tasklet

On 09/23/2014 03:04 PM, Linus Walleij wrote:
> On Tue, Sep 9, 2014 at 10:32 AM, Lothar Waßmann <LW@...o-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@...O-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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/