Re: [tip:irq/urgent] genirq: Prevent oneshot irq thread race

Re: [tip:irq/urgent] genirq: Prevent oneshot irq thread race

[Yinghai Lu]

On 03/10/2010 08:57 AM, tip-bot for Thomas Gleixner wrote:
> Commit-ID:  0b1adaa031a55e44f5dd942f234bf09d28e8a0d6
> Gitweb:     http://git.kernel.org/tip/0b1adaa031a55e44f5dd942f234bf09d28e8a0d6
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Tue, 9 Mar 2010 19:45:54 +0100
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Wed, 10 Mar 2010 17:45:14 +0100
> 
> genirq: Prevent oneshot irq thread race
> 
> Lars-Peter pointed out that the oneshot threaded interrupt handler
> code has the following race:
> 
>  CPU0                            CPU1
>  hande_level_irq(irq X)
>    mask_ack_irq(irq X)
>    handle_IRQ_event(irq X)
>      wake_up(thread_handler)
>                                  thread handler(irq X) runs
>                                  finalize_oneshot(irq X)
> 				  does not unmask due to 
> 				  !(desc->status & IRQ_MASKED)
> 
>  return from irq
>  does not unmask due to
>  (desc->status & IRQ_ONESHOT)
>   				  
> This leaves the interrupt line masked forever. 
> 
> The reason for this is the inconsistent handling of the IRQ_MASKED
> flag. Instead of setting it in the mask function the oneshot support
> sets the flag after waking up the irq thread.
> 
> The solution for this is to set/clear the IRQ_MASKED status whenever
> we mask/unmask an interrupt line. That's the easy part, but that
> cleanup opens another race:
> 
>  CPU0                            CPU1
>  hande_level_irq(irq)
>    mask_ack_irq(irq)
>    handle_IRQ_event(irq)
>      wake_up(thread_handler)
>                                  thread handler(irq) runs
>                                  finalize_oneshot_irq(irq)
> 				  unmask(irq)
>      irq triggers again
>      handle_level_irq(irq)
>        mask_ack_irq(irq)
>      return from irq due to IRQ_INPROGRESS				  
> 
>  return from irq
>  does not unmask due to
>  (desc->status & IRQ_ONESHOT)
> 
> This requires that we synchronize finalize_oneshot_irq() with the
> primary handler. If IRQ_INPROGESS is set we wait until the primary
> handler on the other CPU has returned before unmasking the interrupt
> line again.
> 
> We probably have never seen that problem because it does not happen on
> UP and on SMP the irqbalancer protects us by pinning the primary
> handler and the thread to the same CPU.
> 
> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@kernel.org
> ---
>  kernel/irq/chip.c   |   31 ++++++++++++++++++++++---------
>  kernel/irq/manage.c |   18 ++++++++++++++++++
>  2 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index d70394f..71eba24 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -359,6 +359,23 @@ static inline void mask_ack_irq(struct irq_desc *desc, int irq)
>  		if (desc->chip->ack)
>  			desc->chip->ack(irq);
>  	}
> +	desc->status |= IRQ_MASKED;
> +}
> +
> +static inline void mask_irq(struct irq_desc *desc, int irq)
> +{
> +	if (desc->chip->mask) {
> +		desc->chip->mask(irq);
> +		desc->status |= IRQ_MASKED;
> +	}
> +}
> +
> +static inline void unmask_irq(struct irq_desc *desc, int irq)
> +{
> +	if (desc->chip->unmask) {
> +		desc->chip->unmask(irq);
> +		desc->status &= ~IRQ_MASKED;
> +	}
>  }

this one will cause cris compiling error...

arch/cris/arch-v10/kernel/irq.c:#define mask_irq(irq_nr) (*R_VECT_MASK_CLR = 1 << (irq_nr));
arch/cris/include/arch-v32/arch/irq.h:void mask_irq(int irq);
arch/mips/nxp/pnx8550/common/int.c:static inline void mask_irq(unsigned int irq_nr)
kernel/irq/chip.c:static inline void mask_irq(struct irq_desc *desc)

because arch/irq.h will be included via linux/irq.h

YH