From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pb0-f51.google.com (mail-pb0-f51.google.com [209.85.160.51]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 351CBB6FA9 for ; Thu, 3 May 2012 12:32:24 +1000 (EST) Received: by pbbrp16 with SMTP id rp16so2336677pbb.38 for ; Wed, 02 May 2012 19:32:23 -0700 (PDT) Message-ID: <4FA1EE2C.6050201@gmail.com> Date: Thu, 03 May 2012 10:32:12 +0800 From: Wang Sheng-Hui MIME-Version: 1.0 To: Benjamin Herrenschmidt Subject: Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay References: <4FA1E527.1090807@gmail.com> <1336011306.2653.3.camel@pasglop> In-Reply-To: <1336011306.2653.3.camel@pasglop> Content-Type: text/plain; charset=UTF-8 Cc: Stephen Rothwell , linux-kernel@vger.kernel.org, Milton Miller , Anton Blanchard , linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 2012年05月03日 10:15, Benjamin Herrenschmidt wrote: > 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 ? Since __check_irq_replay() should always be called with interrupts hard disabled, I think it's harmless to use local var here. > > Cheers, > Ben. > >> Signed-off-by: Wang Sheng-Hui >> --- >> 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) > >