| Message ID | 1410251533-4990-1-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- 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
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
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
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
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 -- 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
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;
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(+)