| Message ID | CAMPhdO8uQR4km9szkuGDjuym-hViE9+zxtie6D2WGJmf1_9rUg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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?
On Mon, Jul 11, 2011 at 3:41 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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.
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 > <linux@arm.linux.org.uk> 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 ?
On Mon, Jul 11, 2011 at 3:46 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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 >> <linux@arm.linux.org.uk> 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.
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 > <linux@arm.linux.org.uk> 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 > >> <linux@arm.linux.org.uk> 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: [<c002406c>] (asm_do_IRQ+0x6c/0x8c) from [<c0024b84>] (__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.
On Mon, Jul 11, 2011 at 3:52 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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 >> <linux@arm.linux.org.uk> 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 >> >> <linux@arm.linux.org.uk> 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: > > [<c002406c>] (asm_do_IRQ+0x6c/0x8c) from [<c0024b84>] (__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?
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 > <linux@arm.linux.org.uk> 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 > >> <linux@arm.linux.org.uk> 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 > >> >> <linux@arm.linux.org.uk> 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: > > > > [<c002406c>] (asm_do_IRQ+0x6c/0x8c) from [<c0024b84>] (__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.
On Mon, Jul 11, 2011 at 4:40 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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 >> <linux@arm.linux.org.uk> 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 >> >> <linux@arm.linux.org.uk> 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 >> >> >> <linux@arm.linux.org.uk> 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: >> > >> > [<c002406c>] (asm_do_IRQ+0x6c/0x8c) from [<c0024b84>] (__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?
On Mon, Jul 11, 2011 at 06:07:56PM +0800, Eric Miao wrote: > 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? Or we could rename asm_do_IRQ() to handle_arch_irq() without the __exception tag, and recreate asm_do_IRQ() with the tag, which just calls handle_arch_irq().
On Monday 11 July 2011, Eric Miao wrote: > The following changes since commit fe0d42203cb5616eeff68b14576a0f7e2dd56625: > > Linux 3.0-rc6 (2011-07-04 15:56:24 -0700) > > are available in the git repository at: > git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git devel I'll wait for the next version on this one, until you fixed the problem pointed out by Russell, ok? Arnd
The following changes since commit fe0d42203cb5616eeff68b14576a0f7e2dd56625: Linux 3.0-rc6 (2011-07-04 15:56:24 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ycmiao/pxa-linux-2.6.git devel Eric Miao (6): ARM: pxa: enable AUTO_ZRELADDR ARM: pxa: add common header file for pxa3xx ARM: pxa: avoid accessing interrupt registers directly ARM: pxa: introduce {icip,ichp}_handle_irq() to prepare MULTI_IRQ_HANDLER ARM: pxa: move declarations from generic.h to <soc>.h ARM: pxa: enable MULTI_IRQ_HANDLER for all boards Haojian Zhuang (2): ARM: pxa: add clk_set_rate() ARM: mmp/dkb: enable max7312 gpio expander Tanmay Upadhyay (3): ARM: pxa168: Add support for UART3 ARM: pxa168: Add support for Ethernet ARM: pxa168: Add board support for gplugD Vasily Khoruzhick (1): ARM: pxa/z2: add poweroff function arch/arm/Kconfig | 2 + arch/arm/mach-mmp/Kconfig | 7 + arch/arm/mach-mmp/Makefile | 1 + arch/arm/mach-mmp/clock.c | 15 ++ arch/arm/mach-mmp/clock.h | 1 + arch/arm/mach-mmp/gplugd.c | 189 +++++++++++++++++++++++++++ arch/arm/mach-mmp/include/mach/mfp-gplugd.h | 52 ++++++++ arch/arm/mach-mmp/include/mach/mfp-pxa168.h | 19 +++ arch/arm/mach-mmp/include/mach/pxa168.h | 8 + arch/arm/mach-mmp/include/mach/regs-apmu.h | 1 + arch/arm/mach-mmp/pxa168.c | 6 + arch/arm/mach-mmp/ttc_dkb.c | 31 +++++- arch/arm/mach-pxa/balloon3.c | 1 + arch/arm/mach-pxa/capc7117.c | 1 + arch/arm/mach-pxa/clock.c | 15 ++ arch/arm/mach-pxa/clock.h | 1 + arch/arm/mach-pxa/cm-x2xx.c | 5 +- arch/arm/mach-pxa/cm-x300.c | 1 + arch/arm/mach-pxa/colibri-pxa270.c | 2 + arch/arm/mach-pxa/colibri-pxa300.c | 1 + arch/arm/mach-pxa/colibri-pxa320.c | 4 +- arch/arm/mach-pxa/corgi.c | 3 + arch/arm/mach-pxa/csb726.c | 4 +- arch/arm/mach-pxa/em-x270.c | 2 + arch/arm/mach-pxa/eseries.c | 6 + arch/arm/mach-pxa/ezx.c | 6 + arch/arm/mach-pxa/generic.h | 13 -- arch/arm/mach-pxa/gumstix.c | 1 + arch/arm/mach-pxa/h5000.c | 2 + arch/arm/mach-pxa/himalaya.c | 4 +- arch/arm/mach-pxa/hx4700.c | 1 + arch/arm/mach-pxa/icontrol.c | 1 + arch/arm/mach-pxa/idp.c | 1 + arch/arm/mach-pxa/include/mach/irqs.h | 12 ++ arch/arm/mach-pxa/include/mach/pxa25x.h | 9 ++ arch/arm/mach-pxa/include/mach/pxa27x.h | 5 + arch/arm/mach-pxa/include/mach/pxa300.h | 3 +- arch/arm/mach-pxa/include/mach/pxa320.h | 3 +- arch/arm/mach-pxa/include/mach/pxa3xx.h | 14 ++ arch/arm/mach-pxa/include/mach/pxa930.h | 3 +- arch/arm/mach-pxa/include/mach/regs-intc.h | 30 ----- arch/arm/mach-pxa/irq.c | 36 +++++- arch/arm/mach-pxa/littleton.c | 1 + arch/arm/mach-pxa/lpd270.c | 1 + arch/arm/mach-pxa/lubbock.c | 1 + arch/arm/mach-pxa/magician.c | 1 + arch/arm/mach-pxa/mainstone.c | 1 + arch/arm/mach-pxa/mioa701.c | 1 + arch/arm/mach-pxa/mp900.c | 1 + arch/arm/mach-pxa/palmld.c | 1 + arch/arm/mach-pxa/palmt5.c | 1 + arch/arm/mach-pxa/palmtc.c | 4 +- arch/arm/mach-pxa/palmte2.c | 3 +- arch/arm/mach-pxa/palmtreo.c | 2 + arch/arm/mach-pxa/palmtx.c | 1 + arch/arm/mach-pxa/palmz72.c | 1 + arch/arm/mach-pxa/pcm027.c | 1 + arch/arm/mach-pxa/poodle.c | 1 + arch/arm/mach-pxa/pxa3xx.c | 5 +- arch/arm/mach-pxa/pxa95x.c | 1 - arch/arm/mach-pxa/raumfeld.c | 8 +- arch/arm/mach-pxa/saar.c | 1 + arch/arm/mach-pxa/saarb.c | 1 + arch/arm/mach-pxa/spitz.c | 3 + arch/arm/mach-pxa/stargate2.c | 2 + arch/arm/mach-pxa/tavorevb.c | 1 + arch/arm/mach-pxa/tavorevb3.c | 1 + arch/arm/mach-pxa/tosa.c | 1 + arch/arm/mach-pxa/trizeps4.c | 2 + arch/arm/mach-pxa/viper.c | 1 + arch/arm/mach-pxa/vpac270.c | 1 + arch/arm/mach-pxa/xcep.c | 4 +- arch/arm/mach-pxa/z2.c | 18 +++ arch/arm/mach-pxa/zeus.c | 4 +- arch/arm/mach-pxa/zylonite.c | 3 +- 75 files changed, 526 insertions(+), 75 deletions(-) create mode 100644 arch/arm/mach-mmp/gplugd.c create mode 100644 arch/arm/mach-mmp/include/mach/mfp-gplugd.h create mode 100644 arch/arm/mach-pxa/include/mach/pxa3xx.h delete mode 100644 arch/arm/mach-pxa/include/mach/regs-intc.h