Re: [PATCH] genirq: Provide IRQCHIP_ONESHOT_EDGE_SAFE

[Benjamin Herrenschmidt]

On Mon, 2017-07-31 at 21:33 +0200, Thomas Gleixner wrote:
> If an interrupt chip is marked IRQCHIP_ONESHOT_SAFE it signals that the
> interrupt chip does not require the ONESHOT mode for threaded
> interrupts. Unfortunately this is applied independent of the interrupt type
> (edge/level).
> 
> The powerpc XPIC wants this functionality only for edge type
> interrupts. Provide a new flag which provides the same functionality
> restricted to edge type interrupts.

Thanks ! I'll test that asap (got pulled into another emergency so it
might take a day or two).

Cheers,
Ben.

> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  include/linux/irq.h  |    2 ++
>  kernel/irq/debugfs.c |    1 +
>  kernel/irq/manage.c  |   34 +++++++++++++++++++++-------------
>  3 files changed, 24 insertions(+), 13 deletions(-)
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -481,6 +481,7 @@ struct irq_chip {
>   * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
>   * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
>   * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
> + * IRQCHIP_ONESHOT_EDGE_SAFE:	Skip the oneshot logic for edge type interrupts
>   */
>  enum {
>  	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
> @@ -490,6 +491,7 @@ enum {
>  	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
>  	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
>  	IRQCHIP_EOI_THREADED		= (1 <<  6),
> +	IRQCHIP_ONESHOT_EDGE_SAFE	= (1 <<  7),
>  };
>  
>  #include <linux/irqdesc.h>
> --- a/kernel/irq/debugfs.c
> +++ b/kernel/irq/debugfs.c
> @@ -56,6 +56,7 @@ static const struct irq_bit_descr irqchi
>  	BIT_MASK_DESCR(IRQCHIP_SKIP_SET_WAKE),
>  	BIT_MASK_DESCR(IRQCHIP_ONESHOT_SAFE),
>  	BIT_MASK_DESCR(IRQCHIP_EOI_THREADED),
> +	BIT_MASK_DESCR(IRQCHIP_ONESHOT_EDGE_SAFE),
>  };
>  
>  static void
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1166,18 +1166,6 @@ static int
>  	}
>  
>  	/*
> -	 * Drivers are often written to work w/o knowledge about the
> -	 * underlying irq chip implementation, so a request for a
> -	 * threaded irq without a primary hard irq context handler
> -	 * requires the ONESHOT flag to be set. Some irq chips like
> -	 * MSI based interrupts are per se one shot safe. Check the
> -	 * chip flags, so we can avoid the unmask dance at the end of
> -	 * the threaded handler for those.
> -	 */
> -	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> -		new->flags &= ~IRQF_ONESHOT;
> -
> -	/*
>  	 * Protects against a concurrent __free_irq() call which might wait
>  	 * for synchronize_irq() to complete without holding the optional
>  	 * chip bus lock and desc->lock.
> @@ -1208,6 +1196,25 @@ static int
>  	 * desc->request_mutex or the optional bus lock.
>  	 */
>  	raw_spin_lock_irqsave(&desc->lock, flags);
> +
> +	/*
> +	 * Drivers are often written to work w/o knowledge about the
> +	 * underlying irq chip implementation, so a request for a
> +	 * threaded irq without a primary hard irq context handler
> +	 * requires the ONESHOT flag to be set. Some irq chips like
> +	 * MSI based interrupts are per se one shot safe. Check the
> +	 * chip flags, so we can avoid the unmask dance at the end of
> +	 * the threaded handler for those.
> +	 */
> +	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> +		new->flags &= ~IRQF_ONESHOT;
> +
> +	/* Same as above, but restricted to edge type interrupts */
> +	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_EDGE_SAFE) {
> +		if (!irq_settings_is_level(desc))
> +			new->flags &= ~IRQF_ONESHOT;
> +	}
> +
>  	old_ptr = &desc->action;
>  	old = *old_ptr;
>  	if (old) {
> @@ -1281,7 +1288,8 @@ static int
>  		new->thread_mask = 1 << ffz(thread_mask);
>  
>  	} else if (new->handler == irq_default_primary_handler &&
> -		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
> +		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) &&
> +		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_EDGE_SAFE)) {
>  		/*
>  		 * The interrupt was requested with handler = NULL, so
>  		 * we use the default primary handler for it. But it