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@xxxxxxxxxxxxxx> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
