Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
> local_paca->irq_happened may be changed asychronously. 
> 
> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
> LTP test suite.
> 
> In a short while, the system would crash. Seems that oprofile may change
> the irq_happened.

 .../...

> Use local var instead of local_paca->irq_happened directly in this function here.
> 
> Please check this patch. Any comments are welcome.

It should not as __check_irq_replay() should always be called
with interrupts hard disabled... Do you see any code path
where that is not the case ?

Cheers,
Ben.

> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> ---
>  arch/powerpc/kernel/irq.c |   46 +++++++++++++++++++++++++++++---------------
>  1 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5ec1b23..3d48b23 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>   */
>  notrace unsigned int __check_irq_replay(void)
>  {
> +	unsigned int ret_val;
>  	/*
>  	 * We use local_paca rather than get_paca() to avoid all
>  	 * the debug_smp_processor_id() business in this low level
>  	 * function
>  	 */
> -	unsigned char happened = local_paca->irq_happened;
> +	unsigned char happened, irq_happened;
> +	happened = irq_happened = local_paca->irq_happened;
>  
>  	/* Clear bit 0 which we wouldn't clear otherwise */
> -	local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> +	irq_happened &= ~PACA_IRQ_HARD_DIS;
>  
>  	/*
>  	 * Force the delivery of pending soft-disabled interrupts on PS3.
> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>  	 * decrementer itself rather than the paca irq_happened field
>  	 * in case we also had a rollover while hard disabled
>  	 */
> -	local_paca->irq_happened &= ~PACA_IRQ_DEC;
> -	if (decrementer_check_overflow())
> -		return 0x900;
> +	irq_happened &= ~PACA_IRQ_DEC;
> +	if (decrementer_check_overflow()) {
> +		ret_val = 0x900;
> +		goto replay;
> +	}
>  
>  	/* Finally check if an external interrupt happened */
> -	local_paca->irq_happened &= ~PACA_IRQ_EE;
> -	if (happened & PACA_IRQ_EE)
> -		return 0x500;
> +	irq_happened &= ~PACA_IRQ_EE;
> +	if (happened & PACA_IRQ_EE) {
> +		ret_val = 0x500;
> +		goto replay;
> +	}
>  
>  #ifdef CONFIG_PPC_BOOK3E
>  	/* Finally check if an EPR external interrupt happened
>  	 * this bit is typically set if we need to handle another
>  	 * "edge" interrupt from within the MPIC "EPR" handler
>  	 */
> -	local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> -	if (happened & PACA_IRQ_EE_EDGE)
> -		return 0x500;
> +	irq_happened &= ~PACA_IRQ_EE_EDGE;
> +	if (happened & PACA_IRQ_EE_EDGE) {
> +		ret_val = 0x500;
> +		goto replay;
> +	}
>  
> -	local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> -	if (happened & PACA_IRQ_DBELL)
> -		return 0x280;
> +	irq_happened &= ~PACA_IRQ_DBELL;
> +	if (happened & PACA_IRQ_DBELL) {
> +		ret_val = 0x280;
> +		goto replay;
> +	}
>  #endif /* CONFIG_PPC_BOOK3E */
>  
>  	/* There should be nothing left ! */
> -	BUG_ON(local_paca->irq_happened != 0);
> +	BUG_ON(irq_happened != 0);
> +	ret_val = 0;
>  
> -	return 0;
> +replay:
> +	local_paca->irq_happened = irq_happened;
> +
> +	return ret_val;
>  }
>  
>  notrace void arch_local_irq_restore(unsigned long en)