| Message ID | 20200804091603.541-1-a.fatoum@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: siox: indicate exclusive support of threaded IRQs | expand |
Hello, [adding tglx for irq expertise to Cc] On Tue, Aug 04, 2020 at 11:16:03AM +0200, Ahmad Fatoum wrote: > Generic GPIO consumers like gpio-keys use request_any_context_irq() > to request a threaded handler if irq_settings_is_nested_thread() == > true or a hardirq handler otherwise. > > Drivers using handle_nested_irq() must be sure that the nested > IRQs were requested with threaded handlers, because the IRQ > is handled by calling action->thread_fn(). > > The gpio-siox driver dispatches IRQs via handle_nested_irq, > but has irq_settings_is_nested_thread() == false. > > Set gpio_irq_chip::threaded to remedy this. Sounds reasonable, but I have to keep this for others to judge if this is indeed how the irq stuff works. > --- > I am writing a driver similar to gpio-siox and I ran into a null pointer > dereference, because ->threaded wasn't set. I didn't test this on actual > SIOX hardware. > > This patch doesn't fix the case were are driver explicitly calls > request_irq and is combined with a driver that does handle_nested_irq. > > Is there a flag, such drivers should additionally set or should we > check action->thread_fn before calling it inside handle_nested_irq? If gpio_irq_chip::threaded being set means that this problem happens IMHO the driver is fine and something in the generic gpio code should do the right thing here. Best regards and thanks for caring, Uwe > --- > drivers/gpio/gpio-siox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c > index 26e1fe092304..f8c5e9fc4bac 100644 > --- a/drivers/gpio/gpio-siox.c > +++ b/drivers/gpio/gpio-siox.c > @@ -245,6 +245,7 @@ static int gpio_siox_probe(struct siox_device *sdevice) > girq->chip = &ddata->ichip; > girq->default_type = IRQ_TYPE_NONE; > girq->handler = handle_level_irq; > + girq->threaded = true; > > ret = devm_gpiochip_add_data(dev, &ddata->gchip, NULL); > if (ret) > -- > 2.28.0 > > >
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > On Tue, Aug 04, 2020 at 11:16:03AM +0200, Ahmad Fatoum wrote: >> Generic GPIO consumers like gpio-keys use request_any_context_irq() >> to request a threaded handler if irq_settings_is_nested_thread() == >> true or a hardirq handler otherwise. >> >> Drivers using handle_nested_irq() must be sure that the nested >> IRQs were requested with threaded handlers, because the IRQ >> is handled by calling action->thread_fn(). >> >> The gpio-siox driver dispatches IRQs via handle_nested_irq, >> but has irq_settings_is_nested_thread() == false. >> >> Set gpio_irq_chip::threaded to remedy this. > > Sounds reasonable, but I have to keep this for others to judge if this > is indeed how the irq stuff works. handle_nested_irq() documentation clearly says: "Handle a nested irq from a irq thread". As a consequence the caller of this function must run in an interrupt thread. This is an optimization to spare tons of interrupt threads and context switches. So the solution for this driver is either to make the dispatch handler threaded or use the hard interrupt variant of dispatching the demultiplexed GPIO interrupts. Thanks, tglx
On Thu, Aug 6, 2020 at 12:20 PM Thomas Gleixner <tglx@linutronix.de> wrote: > So the solution for this driver is either to make the dispatch handler > threaded or use the hard interrupt variant of dispatching the > demultiplexed GPIO interrupts. The struct gpio_irq_chip .threaded bool that the patch sets just instructs the gpio core to issue irq_set_nested_thread(irq, 1) on the child IRQ. This is a driver of type "struct siox_driver" handling the IRQ through the special .get_data callback supplied in the driver struct and it calls handle_nested_irq(irq) so with this fix it percolated up to the parent as intended. So far so good. So I think the patch should be applied. But what is behind this .get_data() callback for siox drivers? The siox driver framework in drivers/siox dispatches calls to .get_data() from a polling thread which is just some ordinary kthread. It looks like this because the SIOX (I think) needs to do polled I/O. (drivers/siox/siox-core.c) So this is a thread but it is not an irq thread from the irq core, however it is treated like such by the driver, and in a way what happens is events, just polled by a thread. So when we call handle_nested_irq() ... we are not really calling that from an irq handler. I am just very confused :D But Uwe must have designed this thread to mimic IRQs specifically? (Uwe?) I don't know if the IRQ core even sees a difference between which thread it gets interfaced with. I suppose it does? :/ Yours, Linus Walleij
On Thu, Aug 06, 2020 at 12:20:52PM +0200, Thomas Gleixner wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > On Tue, Aug 04, 2020 at 11:16:03AM +0200, Ahmad Fatoum wrote: > >> Generic GPIO consumers like gpio-keys use request_any_context_irq() > >> to request a threaded handler if irq_settings_is_nested_thread() == > >> true or a hardirq handler otherwise. > >> > >> Drivers using handle_nested_irq() must be sure that the nested > >> IRQs were requested with threaded handlers, because the IRQ > >> is handled by calling action->thread_fn(). > >> > >> The gpio-siox driver dispatches IRQs via handle_nested_irq, > >> but has irq_settings_is_nested_thread() == false. > >> > >> Set gpio_irq_chip::threaded to remedy this. > > > > Sounds reasonable, but I have to keep this for others to judge if this > > is indeed how the irq stuff works. > > handle_nested_irq() documentation clearly says: "Handle a nested irq > from a irq thread". As a consequence the caller of this function must > run in an interrupt thread. This is an optimization to spare tons of > interrupt threads and context switches. > > So the solution for this driver is either to make the dispatch handler > threaded or use the hard interrupt variant of dispatching the > demultiplexed GPIO interrupts. The action item isn't entirely clear for me. There is no "parent" irq, I have for siox a kthread that does some IO and looks that the resulting data which effectively reports the current state of the GPIO line. And if this GPIO is configured to trigger an irq and the matching transition (or state) is seen, I want to trigger the irq action. So the caller has neither hard nor threaded irq context. Best regards Uwe
Linus Walleij <linus.walleij@linaro.org> writes: > On Thu, Aug 6, 2020 at 12:20 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> So the solution for this driver is either to make the dispatch handler >> threaded or use the hard interrupt variant of dispatching the >> demultiplexed GPIO interrupts. > > The struct gpio_irq_chip .threaded bool that the patch > sets just instructs the gpio core to issue > irq_set_nested_thread(irq, 1) on the child IRQ. > > This is a driver of type "struct siox_driver" handling the > IRQ through the special .get_data callback supplied in the > driver struct and it calls handle_nested_irq(irq) so with > this fix it percolated up to the parent as intended. > > So far so good. So I think the patch should be applied. > > But what is behind this .get_data() callback for siox drivers? > > The siox driver framework in drivers/siox dispatches calls > to .get_data() from a polling thread which is just some ordinary > kthread. It looks like this because the SIOX (I think) needs > to do polled I/O. (drivers/siox/siox-core.c) > > So this is a thread but it is not an irq thread from the irq core, > however it is treated like such by the driver, and in a way what > happens is events, just polled by a thread. As Uwe just explained. > So when we call handle_nested_irq() ... we are not really > calling that from an irq handler. > > I don't know if the IRQ core even sees a difference between which > thread it gets interfaced with. I suppose it does? :/ handle_nested_irq() does not care. It cares about thread context, external reentrancy protection for the same nested interrupt and that the nested interrupt has a thread handler. The latter is what goes belly up because w/o that threaded bit set the GPIO core fails to set nested thread. So if a consumer requests an interrupt with request_any_context_irq() then that fails to select thread mode which means the threaded handler is not set causing handle_nested_irq() to fail. The polling kthread is a slight but clever abomination, but it just works because it provides thread context and cannot run concurrently. So Ahmad's patch is correct, just the changelog needs polishing. Thanks, tglx
On Thu, Aug 06, 2020 at 08:50:45PM +0200, Thomas Gleixner wrote: > Linus Walleij <linus.walleij@linaro.org> writes: > > On Thu, Aug 6, 2020 at 12:20 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > > >> So the solution for this driver is either to make the dispatch handler > >> threaded or use the hard interrupt variant of dispatching the > >> demultiplexed GPIO interrupts. > > > > The struct gpio_irq_chip .threaded bool that the patch > > sets just instructs the gpio core to issue > > irq_set_nested_thread(irq, 1) on the child IRQ. > > > > This is a driver of type "struct siox_driver" handling the > > IRQ through the special .get_data callback supplied in the > > driver struct and it calls handle_nested_irq(irq) so with > > this fix it percolated up to the parent as intended. > > > > So far so good. So I think the patch should be applied. > > > > But what is behind this .get_data() callback for siox drivers? > > > > The siox driver framework in drivers/siox dispatches calls > > to .get_data() from a polling thread which is just some ordinary > > kthread. It looks like this because the SIOX (I think) needs > > to do polled I/O. (drivers/siox/siox-core.c) > > > > So this is a thread but it is not an irq thread from the irq core, > > however it is treated like such by the driver, and in a way what > > happens is events, just polled by a thread. > > As Uwe just explained. > > > So when we call handle_nested_irq() ... we are not really > > calling that from an irq handler. > > > > I don't know if the IRQ core even sees a difference between which > > thread it gets interfaced with. I suppose it does? :/ > > handle_nested_irq() does not care. It cares about thread context, > external reentrancy protection for the same nested interrupt and that > the nested interrupt has a thread handler. > > The latter is what goes belly up because w/o that threaded bit set the > GPIO core fails to set nested thread. So if a consumer requests an > interrupt with request_any_context_irq() then that fails to select > thread mode which means the threaded handler is not set causing > handle_nested_irq() to fail. For a caller of request_threaded_irq() that passes a relevant hardirq handler the hardirq handler is never called but request_threaded_irq() doesn't fail. The handler is just replaced by irq_nested_primary_handler in __setup_irq(). Is that a bug? (I didn't test, just read the code, so I might have missed something.) > The polling kthread is a slight but clever abomination, but it just > works because it provides thread context and cannot run concurrently. I think this is the first time you called any of my code "clever" :-) > So Ahmad's patch is correct, just the changelog needs polishing. Trying to be constructive, here is my suggested changelog: gpio: siox: explicitly only support threaded irqs The gpio-siox driver uses handle_nested_irq() to implement its interrupt support. This is only capable to handle threaded irq actions. For a hardirq action it triggers a NULL pointer oops. (It calls action->thread_fn which is NULL then.) So prevent registration of a hardirq action by setting gpio_irq_chip::threaded to true. Does this address all your concerns? Is this bad enough to justify sending this patch to stable? Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > On Thu, Aug 06, 2020 at 08:50:45PM +0200, Thomas Gleixner wrote: >> handle_nested_irq() does not care. It cares about thread context, >> external reentrancy protection for the same nested interrupt and that >> the nested interrupt has a thread handler. >> >> The latter is what goes belly up because w/o that threaded bit set the >> GPIO core fails to set nested thread. So if a consumer requests an >> interrupt with request_any_context_irq() then that fails to select >> thread mode which means the threaded handler is not set causing >> handle_nested_irq() to fail. > > For a caller of request_threaded_irq() that passes a relevant hardirq > handler the hardirq handler is never called but request_threaded_irq() > doesn't fail. The handler is just replaced by irq_nested_primary_handler > in __setup_irq(). Is that a bug? (I didn't test, just read the code, so I > might have missed something.) Depends on what the threaded handler expects what the primary handler has done. It might just work or not :) > Trying to be constructive, here is my suggested changelog: > > gpio: siox: explicitly only support threaded irqs > > The gpio-siox driver uses handle_nested_irq() to implement its > interrupt support. This is only capable to handle threaded irq > actions. For a hardirq action it triggers a NULL pointer oops. > (It calls action->thread_fn which is NULL then.) > > So prevent registration of a hardirq action by setting > gpio_irq_chip::threaded to true. > > Does this address all your concerns? LGTM > Is this bad enough to justify sending this patch to stable? Yes, a Cc: stable and a Fixes: tag is justified. Thanks, tglx
Hello Thomas, On Thu, Aug 06, 2020 at 10:33:06PM +0200, Thomas Gleixner wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > On Thu, Aug 06, 2020 at 08:50:45PM +0200, Thomas Gleixner wrote: > >> handle_nested_irq() does not care. It cares about thread context, > >> external reentrancy protection for the same nested interrupt and that > >> the nested interrupt has a thread handler. > >> > >> The latter is what goes belly up because w/o that threaded bit set the > >> GPIO core fails to set nested thread. So if a consumer requests an > >> interrupt with request_any_context_irq() then that fails to select > >> thread mode which means the threaded handler is not set causing > >> handle_nested_irq() to fail. > > > > For a caller of request_threaded_irq() that passes a relevant hardirq > > handler the hardirq handler is never called but request_threaded_irq() > > doesn't fail. The handler is just replaced by irq_nested_primary_handler > > in __setup_irq(). Is that a bug? (I didn't test, just read the code, so I > > might have missed something.) > > Depends on what the threaded handler expects what the primary handler > has done. It might just work or not :) So we need something like: diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 48c38e09c673..31777a0b79df 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1393,12 +1393,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) ret = -EINVAL; goto out_mput; } - /* - * Replace the primary handler which was provided from - * the driver for non nested interrupt handling by the - * dummy function which warns when called. - */ - new->handler = irq_nested_primary_handler; + + if (new->handler == NULL) { + /* Scream loud if the primary handler gets called */ + new->handler = irq_nested_primary_handler; + } else { + /* + * The handler won't be called as the requestor expects, + * so refuse to install the handler + */ + ret = -EINVAL; + goto out_mput; + } } else { if (irq_settings_can_thread(desc)) { ret = irq_setup_forced_threading(new); ? Do we need to care for other allowed values of new->handler? Maybe irq_default_primary_handler? > > Is this bad enough to justify sending this patch to stable? > > Yes, a Cc: stable and a Fixes: tag is justified. That would be Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox") Best regards Uwe
On Thu, Aug 06, 2020 at 11:07:09PM +0200, Uwe Kleine-König wrote: > Hello Thomas, > > On Thu, Aug 06, 2020 at 10:33:06PM +0200, Thomas Gleixner wrote: > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > > > On Thu, Aug 06, 2020 at 08:50:45PM +0200, Thomas Gleixner wrote: > > >> handle_nested_irq() does not care. It cares about thread context, > > >> external reentrancy protection for the same nested interrupt and that > > >> the nested interrupt has a thread handler. > > >> > > >> The latter is what goes belly up because w/o that threaded bit set the > > >> GPIO core fails to set nested thread. So if a consumer requests an > > >> interrupt with request_any_context_irq() then that fails to select > > >> thread mode which means the threaded handler is not set causing > > >> handle_nested_irq() to fail. > > > > > > For a caller of request_threaded_irq() that passes a relevant hardirq > > > handler the hardirq handler is never called but request_threaded_irq() > > > doesn't fail. The handler is just replaced by irq_nested_primary_handler > > > in __setup_irq(). Is that a bug? (I didn't test, just read the code, so I > > > might have missed something.) > > > > Depends on what the threaded handler expects what the primary handler > > has done. It might just work or not :) > > So we need something like: > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 48c38e09c673..31777a0b79df 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1393,12 +1393,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > ret = -EINVAL; > goto out_mput; > } > - /* > - * Replace the primary handler which was provided from > - * the driver for non nested interrupt handling by the > - * dummy function which warns when called. > - */ > - new->handler = irq_nested_primary_handler; > + > + if (new->handler == NULL) { > + /* Scream loud if the primary handler gets called */ > + new->handler = irq_nested_primary_handler; > + } else { > + /* > + * The handler won't be called as the requestor expects, > + * so refuse to install the handler > + */ > + ret = -EINVAL; > + goto out_mput; > + } > } else { > if (irq_settings_can_thread(desc)) { > ret = irq_setup_forced_threading(new); > The siox stuff is used at Eckelmann (i.e. probably the only siox user) via /dev/gpiochip%d. The code providing this device uses request_threaded_irq(), so that's why we didn't run into the oops. That the primary handler might not run was noticed already and cared for in commit 1033be58992f ("gpiolib: fix line event timestamps for nested irqs"). I grepped around a bit and I think most other drivers depend on their primary handler being called. (Some primary handlers disable and/or mask the irq[1], this is wrong, isn't it?) So I really think request_threaded_irq should not silently rop the primary handler on the floor. Best regards Uwe [1] I saw: - arch/mips/alchemy/devboards/db1200.c - drivers/crypto/hisilicon/sec/sec_drv.c - drivers/crypto/stm32/stm32-hash.c - drivers/dma/idxd/init.c
Hi Ahmad, On Tue, Aug 4, 2020 at 11:18 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > Generic GPIO consumers like gpio-keys use request_any_context_irq() > to request a threaded handler if irq_settings_is_nested_thread() == > true or a hardirq handler otherwise. > > Drivers using handle_nested_irq() must be sure that the nested > IRQs were requested with threaded handlers, because the IRQ > is handled by calling action->thread_fn(). > > The gpio-siox driver dispatches IRQs via handle_nested_irq, > but has irq_settings_is_nested_thread() == false. > > Set gpio_irq_chip::threaded to remedy this. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> I think we concluded we want to apply this patch, but with a fixed commit text, can you send a V2? (Or ask Uwe if he wants to pick it up and write the text.) Yours, Linus Walleij
Hello Linus, On 8/28/20 12:55 AM, Linus Walleij wrote: > Hi Ahmad, > > On Tue, Aug 4, 2020 at 11:18 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote: > >> Generic GPIO consumers like gpio-keys use request_any_context_irq() >> to request a threaded handler if irq_settings_is_nested_thread() == >> true or a hardirq handler otherwise. >> >> Drivers using handle_nested_irq() must be sure that the nested >> IRQs were requested with threaded handlers, because the IRQ >> is handled by calling action->thread_fn(). >> >> The gpio-siox driver dispatches IRQs via handle_nested_irq, >> but has irq_settings_is_nested_thread() == false. >> >> Set gpio_irq_chip::threaded to remedy this. >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > I think we concluded we want to apply this patch, but with > a fixed commit text, can you send a V2? (Or ask Uwe if he wants > to pick it up and write the text.) Thanks for the reminder. This slipped through. I just sent out v2 (<20200907153135.3307-1-a.fatoum@pengutronix.de>) Cheers, Ahmad > > Yours, > Linus Walleij >
diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c index 26e1fe092304..f8c5e9fc4bac 100644 --- a/drivers/gpio/gpio-siox.c +++ b/drivers/gpio/gpio-siox.c @@ -245,6 +245,7 @@ static int gpio_siox_probe(struct siox_device *sdevice) girq->chip = &ddata->ichip; girq->default_type = IRQ_TYPE_NONE; girq->handler = handle_level_irq; + girq->threaded = true; ret = devm_gpiochip_add_data(dev, &ddata->gchip, NULL); if (ret)
Generic GPIO consumers like gpio-keys use request_any_context_irq() to request a threaded handler if irq_settings_is_nested_thread() == true or a hardirq handler otherwise. Drivers using handle_nested_irq() must be sure that the nested IRQs were requested with threaded handlers, because the IRQ is handled by calling action->thread_fn(). The gpio-siox driver dispatches IRQs via handle_nested_irq, but has irq_settings_is_nested_thread() == false. Set gpio_irq_chip::threaded to remedy this. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- I am writing a driver similar to gpio-siox and I ran into a null pointer dereference, because ->threaded wasn't set. I didn't test this on actual SIOX hardware. This patch doesn't fix the case were are driver explicitly calls request_irq and is combined with a driver that does handle_nested_irq. Is there a flag, such drivers should additionally set or should we check action->thread_fn before calling it inside handle_nested_irq? --- drivers/gpio/gpio-siox.c | 1 + 1 file changed, 1 insertion(+)