Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755701Ab2ECCcY (ORCPT ); Wed, 2 May 2012 22:32:24 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:50874 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752640Ab2ECCcX (ORCPT ); Wed, 2 May 2012 22:32:23 -0400 Message-ID: <4FA1EE2C.6050201@gmail.com> Date: Thu, 03 May 2012 10:32:12 +0800 From: Wang Sheng-Hui User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.18) Gecko/20110617 Thunderbird/3.1.11 MIME-Version: 1.0 To: Benjamin Herrenschmidt CC: Milton Miller , Grant Likely , Stephen Rothwell , Anton Blanchard , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org 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 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4056 Lines: 125 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) > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/