From patchwork Mon Jul 11 10:17:25 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Miao X-Patchwork-Id: 963902 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p6BAI5ON012785 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 11 Jul 2011 10:18:26 GMT Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QgDYp-00075Q-CW; Mon, 11 Jul 2011 10:17:55 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QgDYo-00012W-VX; Mon, 11 Jul 2011 10:17:55 +0000 Received: from mail-vw0-f49.google.com ([209.85.212.49]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QgDYk-00012D-L1 for linux-arm-kernel@lists.infradead.org; Mon, 11 Jul 2011 10:17:51 +0000 Received: by vws8 with SMTP id 8so3247045vws.36 for ; Mon, 11 Jul 2011 03:17:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; bh=GKlAG31KQmoWqCN+hR1+CSY4c1UqQVUxwTz51COfydk=; b=tKU2oLbc5QkYTiqYrO/Y1rvWcXw7s6E/2m9TpfJUUjpwqLrqPh+lQlB9po+Dwm0p3w l6kvJKiBu/GNYXSykyOmQw3+yftlPfaHZJLDLILlvuaJ5DM27aRhY7MCjRE+2pp9g6vH PFvufi5/emSu9lgIYGiJs8L8YWCHOeuBwYY70= Received: by 10.52.175.129 with SMTP id ca1mr493084vdc.168.1310379465116; Mon, 11 Jul 2011 03:17:45 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.181.65 with HTTP; Mon, 11 Jul 2011 03:17:25 -0700 (PDT) In-Reply-To: References: <20110711074113.GK4812@n2100.arm.linux.org.uk> <20110711074600.GL4812@n2100.arm.linux.org.uk> <20110711075243.GM4812@n2100.arm.linux.org.uk> <20110711084026.GN4812@n2100.arm.linux.org.uk> From: Eric Miao Date: Mon, 11 Jul 2011 18:17:25 +0800 Message-ID: Subject: Re: [GIT PULL] pxa: features for next To: Russell King - ARM Linux X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110711_061750_920744_94C99851 X-CRM114-Status: GOOD ( 33.35 ) X-Spam-Score: -0.8 (/) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (-0.8 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.212.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (eric.y.miao[at]gmail.com) -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Arnd Bergmann , linux-arm-kernel X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Mon, 11 Jul 2011 10:18:43 +0000 (UTC) X-MIME-Autoconverted: from base64 to 8bit by demeter1.kernel.org id p6BAI5ON012785 On Mon, Jul 11, 2011 at 6:07 PM, Eric Miao wrote: > On Mon, Jul 11, 2011 at 4:40 PM, Russell King - ARM Linux > wrote: >> On Mon, Jul 11, 2011 at 04:31:04PM +0800, Eric Miao wrote: >>> On Mon, Jul 11, 2011 at 3:52 PM, Russell King - ARM Linux >>> wrote: >>> > On Mon, Jul 11, 2011 at 03:47:27PM +0800, Eric Miao wrote: >>> >> On Mon, Jul 11, 2011 at 3:46 PM, Russell King - ARM Linux >>> >> wrote: >>> >> > On Mon, Jul 11, 2011 at 03:44:54PM +0800, Eric Miao wrote: >>> >> >> On Mon, Jul 11, 2011 at 3:41 PM, Russell King - ARM Linux >>> >> >> wrote: >>> >> >> > On Mon, Jul 11, 2011 at 03:27:15PM +0800, Eric Miao wrote: >>> >> >> >>       ARM: pxa: avoid accessing interrupt registers directly >>> >> >> >>       ARM: pxa: introduce {icip,ichp}_handle_irq() to prepare MULTI_IRQ_HANDLER >>> >> >> > >>> >> >> > What happened about the __exception issue with asm_do_IRQ? >>> >> >> > >>> >> >> >>> >> >> I just removed the __exception from the C handler. >>> >> > >>> >> > From asm_do_IRQ ? >>> >> > >>> >> >>> >> No. From icip_handle_irq() and ichp_handle_irq(). Thought the ability to >>> >> unwind asm_do_IRQ() is more important. >>> > >>> > Which means you didn't understand my objection when I reviewed your patch. >>> > >>> > The __exception annotation on a function causes this to happen: >>> > >>> > [] (asm_do_IRQ+0x6c/0x8c) from [] (__irq_svc+0x44/0xcc) >>> > Exception stack(0xc3897c78 to 0xc3897cc0) >>> > 7c60:                                                       4022d320 4022e000 >>> > 7c80: 08000075 00001000 c32273c0 c03ce1c0 c2b49b78 4022d000 c2b420b4 00000001 >>> > 7ca0: 00000000 c3897cfc 00000000 c3897cc0 c00afc54 c002edd8 00000013 ffffffff >>> > >>> > Where that stack dump represents the pt_regs for the exception which >>> > happened.  Any function found in while unwinding will cause this to >>> > be printed. >>> > >>> > If you insert a C function between the IRQ assembly and asm_do_IRQ, the >>> > dump you get from asm_do_IRQ will be the stack for your function, not >>> > the pt_regs.  That makes the feature useless. >>> > >>> >>> Sorry for my stupidity, but I think I still don't get it quite correctly. >>> When both functions are prefixed with __exception_irq_entry, if >>> unwind works correctly, both stacks will be dumped, how would >>> the asm_do_IRQ will be stack for the function inserted? >> >> When __irq_svc - or any of the other exception handling assembly code - >> calls the C code, the stack pointer will be pointing at the pt_regs >> structure. >> >> All the entry points into C code from the exception handling code are >> marked with __exception or __exception_irq_enter to indicate that they >> are one of the functions which has pt_regs above them. >> >> Normally, when you've entered asm_do_IRQ() you will have this stack >> layout (higher address towards top): >> >>        pt_regs >>        asm_do_IRQ frame >> >> If you insert a C function between the exception assembly code and >> asm_do_IRQ, you end up with this stack layout instead: >> >>        pt_regs >>        your function frame >>        asm_do_IRQ frame >> >> This means when we unwind, we'll get to asm_do_IRQ, and rather than >> dumping out the pt_regs, we'll dump out your functions stack frame >> instead, because that's what is above the asm_do_IRQ stack frame >> rather than the expected pt_regs structure. >> > > Ah now I see the problem. So it's actually in dump_backtrace_entry(), > where the if (in_exception_text(where)) dump_mem(...) has this > assumption. > > One way to solve this from my humble opinion is to mandate > __exception for the C function (stack version) of handle_arch_irq, > and when MULTI_IRQ defined, remove __exception from asm_do_IRQ(), > so the assumption in dump_backtrace_entry() still holds true. > > Or do we have a gcc extension to tell the compiler a simple function > doesn't need to use the stack? > Hi Russell, How about something like below, it's not clever at all though: diff --git a/arch/arm/include/asm/mach/irq.h b/arch/arm/include/asm/mach/irq.h index febe495..09a4856 100644 --- a/arch/arm/include/asm/mach/irq.h +++ b/arch/arm/include/asm/mach/irq.h @@ -25,6 +25,16 @@ extern void (*handle_arch_irq)(struct pt_regs *); #endif /* + * do not dump exception stack when MULTI_IRQ_HANDLER is defined, as + * the stack will be dumped for function (*handle_arch_irq)(). + */ +#ifdef CONFIG_MULTI_IRQ_HANDLER +#define __multi_irq_entry +#else +#define __multi_irq_entry __exception_irq_entry +#endif + +/* * This is for easy migration, but should be changed in the source */ #define do_bad_IRQ(irq,desc) \ diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c index 83bbad0..7d32fd5 100644 --- a/arch/arm/kernel/irq.c +++ b/arch/arm/kernel/irq.c @@ -71,7 +71,7 @@ int arch_show_interrupts(struct seq_file *p, int prec) * come via this function. Instead, they should provide their * own 'handler' */ -asmlinkage void __exception_irq_entry +asmlinkage void __multi_irq_entry asm_do_IRQ(unsigned int irq, struct pt_regs *regs) { struct pt_regs *old_regs = set_irq_regs(regs); diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c index 2d079f4..d9cab3b 100644 --- a/arch/arm/mach-pxa/irq.c +++ b/arch/arm/mach-pxa/irq.c @@ -129,7 +129,7 @@ static struct irq_chip pxa_low_gpio_chip = { .irq_set_type = pxa_set_low_gpio_type, }; -asmlinkage void icip_handle_irq(struct pt_regs *regs) +asmlinkage void __exception_irq_entry icip_handle_irq(struct pt_regs *regs) { uint32_t icip, icmr, mask; @@ -145,7 +145,7 @@ asmlinkage void icip_handle_irq(struct pt_regs *regs) } while (1); } -asmlinkage void ichp_handle_irq(struct pt_regs *regs) +asmlinkage void __exception_irq_entry ichp_handle_irq(struct pt_regs *regs) { uint32_t ichp;