Re: [PATCH] gpio: siox: indicate exclusive support of threaded IRQs

[Uwe Kleine-König]

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