Message ID | 1288777018-24259-1-git-send-email-b21989@freescale.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Kumar Gala |
Headers | show |
Shaohui Xie <b21989@freescale.com> wrote: > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index a45a63c..2a5fb9d 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -55,6 +55,7 @@ > #endif > #include <asm/kexec.h> > #include <asm/ppc-opcode.h> > +#include <linux/rio.h> > > #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) > int (*__debugger)(struct pt_regs *regs) __read_mostly; > @@ -500,6 +501,13 @@ int machine_check_e500mc(struct pt_regs *regs) > reason & MCSR_MEA ? "Effective" : "Physical", addr); > } > > + if (reason & MCSR_BUS_RBERR) { > + printk("Bus - Read Data Bus Error\n"); > +#ifdef CONFIG_RAPIDIO > + recoverable = fsl_rio_mcheck_exception(regs); > +#endif > + } > + > mtspr(SPRN_MCSR, mcsr); > return mfspr(SPRN_MCSR) == 0 && recoverable; > } > @@ -527,8 +535,12 @@ int machine_check_e500(struct pt_regs *regs) > printk("Bus - Write Address Error\n"); > if (reason & MCSR_BUS_IBERR) > printk("Bus - Instruction Data Error\n"); > - if (reason & MCSR_BUS_RBERR) > + if (reason & MCSR_BUS_RBERR) { > printk("Bus - Read Data Bus Error\n"); > +#ifdef CONFIG_RAPIDIO > + fsl_rio_mcheck_exception(regs); > +#endif > + } > if (reason & MCSR_BUS_WBERR) > printk("Bus - Read Data Bus Error\n"); > if (reason & MCSR_BUS_IPERR) This implementation breaks an intended use of fsl_rio_mcheck_exception(): 1. for e500 it does not check the return value of the rio handler and crashes the system even after RIO Mchk was serviced. Looks like e500mc version handles it better but I have no HW to test it. 2. the RIO Mchk is expected to be handled quietly but here it has many printk's. May be it is better to call the fsl_rio_mcheck_exception() handler in very beginning and simply exit if it returns 1. Alex.
Best Regards, Shaohui Xie > -----Original Message----- > From: Bounine, Alexandre [mailto:Alexandre.Bounine@idt.com] > Sent: Thursday, November 11, 2010 6:55 AM > To: Xie Shaohui-B21989; akpm@linux-foundation.org > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Li Yang- > R58472; Gala Kumar-B11780; Zang Roy-R61911 > Subject: RE: [PATCH 3/4][v2] fsl_rio: move machine_check handler into > machine_check_e500 & machine_check_e500mc > > Shaohui Xie <b21989@freescale.com> wrote: > > > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > > index a45a63c..2a5fb9d 100644 > > --- a/arch/powerpc/kernel/traps.c > > +++ b/arch/powerpc/kernel/traps.c > > @@ -55,6 +55,7 @@ > > #endif > > #include <asm/kexec.h> > > #include <asm/ppc-opcode.h> > > +#include <linux/rio.h> > > > > #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) int > > (*__debugger)(struct pt_regs *regs) __read_mostly; @@ -500,6 +501,13 > > @@ int machine_check_e500mc(struct pt_regs *regs) > > reason & MCSR_MEA ? "Effective" : "Physical", > addr); > > } > > > > + if (reason & MCSR_BUS_RBERR) { > > + printk("Bus - Read Data Bus Error\n"); #ifdef CONFIG_RAPIDIO > > + recoverable = fsl_rio_mcheck_exception(regs); #endif > > + } > > + > > mtspr(SPRN_MCSR, mcsr); > > return mfspr(SPRN_MCSR) == 0 && recoverable; } @@ -527,8 +535,12 > @@ > > int machine_check_e500(struct pt_regs *regs) > > printk("Bus - Write Address Error\n"); > > if (reason & MCSR_BUS_IBERR) > > printk("Bus - Instruction Data Error\n"); > > - if (reason & MCSR_BUS_RBERR) > > + if (reason & MCSR_BUS_RBERR) { > > printk("Bus - Read Data Bus Error\n"); > > +#ifdef CONFIG_RAPIDIO > > + fsl_rio_mcheck_exception(regs); > > +#endif > > + } > > if (reason & MCSR_BUS_WBERR) > > printk("Bus - Read Data Bus Error\n"); > > if (reason & MCSR_BUS_IPERR) > > This implementation breaks an intended use of > fsl_rio_mcheck_exception(): > 1. for e500 it does not check the return value of the rio handler and > crashes the system even after RIO Mchk was serviced. Looks like e500mc > version handles it better but I have no HW to test it. > 2. the RIO Mchk is expected to be handled quietly but here it has many > printk's. May be it is better to call the fsl_rio_mcheck_exception() > handler in very beginning and simply exit if it returns 1. > > Alex. [Xie Shaohui-B21989] Hi Alex, seems your suggestion is some kind of conflict with Kumar, you can have a look at http://patchwork.ozlabs.org/patch/67774/ Thanks Shaohui > >
On Nov 11, 2010, at 4:19 AM, Xie Shaohui-B21989 wrote: > > > > Best Regards, > Shaohui Xie > > >> -----Original Message----- >> From: Bounine, Alexandre [mailto:Alexandre.Bounine@idt.com] >> Sent: Thursday, November 11, 2010 6:55 AM >> To: Xie Shaohui-B21989; akpm@linux-foundation.org >> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Li > Yang- >> R58472; Gala Kumar-B11780; Zang Roy-R61911 >> Subject: RE: [PATCH 3/4][v2] fsl_rio: move machine_check handler into >> machine_check_e500 & machine_check_e500mc >> >> Shaohui Xie <b21989@freescale.com> wrote: >>> >>> diff --git a/arch/powerpc/kernel/traps.c > b/arch/powerpc/kernel/traps.c >>> index a45a63c..2a5fb9d 100644 >>> --- a/arch/powerpc/kernel/traps.c >>> +++ b/arch/powerpc/kernel/traps.c >>> @@ -55,6 +55,7 @@ >>> #endif >>> #include <asm/kexec.h> >>> #include <asm/ppc-opcode.h> >>> +#include <linux/rio.h> >>> >>> #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) int >>> (*__debugger)(struct pt_regs *regs) __read_mostly; @@ -500,6 +501,13 >>> @@ int machine_check_e500mc(struct pt_regs *regs) >>> reason & MCSR_MEA ? "Effective" : "Physical", >> addr); >>> } >>> >>> + if (reason & MCSR_BUS_RBERR) { >>> + printk("Bus - Read Data Bus Error\n"); #ifdef > CONFIG_RAPIDIO >>> + recoverable = fsl_rio_mcheck_exception(regs); #endif >>> + } >>> + >>> mtspr(SPRN_MCSR, mcsr); >>> return mfspr(SPRN_MCSR) == 0 && recoverable; } @@ -527,8 > +535,12 >> @@ >>> int machine_check_e500(struct pt_regs *regs) >>> printk("Bus - Write Address Error\n"); >>> if (reason & MCSR_BUS_IBERR) >>> printk("Bus - Instruction Data Error\n"); >>> - if (reason & MCSR_BUS_RBERR) >>> + if (reason & MCSR_BUS_RBERR) { >>> printk("Bus - Read Data Bus Error\n"); >>> +#ifdef CONFIG_RAPIDIO >>> + fsl_rio_mcheck_exception(regs); >>> +#endif >>> + } >>> if (reason & MCSR_BUS_WBERR) >>> printk("Bus - Read Data Bus Error\n"); >>> if (reason & MCSR_BUS_IPERR) >> >> This implementation breaks an intended use of >> fsl_rio_mcheck_exception(): >> 1. for e500 it does not check the return value of the rio handler and >> crashes the system even after RIO Mchk was serviced. Looks like e500mc >> version handles it better but I have no HW to test it. >> 2. the RIO Mchk is expected to be handled quietly but here it has many >> printk's. May be it is better to call the fsl_rio_mcheck_exception() >> handler in very beginning and simply exit if it returns 1. >> >> Alex. > [Xie Shaohui-B21989] Hi Alex, seems your suggestion is some kind of > conflict with Kumar, you can have a look at > http://patchwork.ozlabs.org/patch/67774/ I think Alex's comment is the fact we ignore the 'return' value in the machine_check_e500 case. - k
On Nov 3, 2010, at 4:36 AM, Shaohui Xie wrote: > Signed-off-by: Shaohui Xie <b21989@freescale.com> > Cc: Li Yang <leoli@freescale.com> > Cc: Kumar Gala <kumar.gala@freescale.com> > Cc: Roy Zang <tie-fei.zang@freescale.com> > Cc: Alexandre Bounine <alexandre.bounine@idt.com> > --- > arch/powerpc/kernel/traps.c | 14 +++++++++++++- > arch/powerpc/sysdev/fsl_rio.c | 15 +++------------ > include/linux/rio.h | 1 + > 3 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index a45a63c..2a5fb9d 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -55,6 +55,7 @@ > #endif > #include <asm/kexec.h> > #include <asm/ppc-opcode.h> > +#include <linux/rio.h> > > #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) > int (*__debugger)(struct pt_regs *regs) __read_mostly; > @@ -500,6 +501,13 @@ int machine_check_e500mc(struct pt_regs *regs) > reason & MCSR_MEA ? "Effective" : "Physical", addr); > } > > + if (reason & MCSR_BUS_RBERR) { > + printk("Bus - Read Data Bus Error\n"); > +#ifdef CONFIG_RAPIDIO > + recoverable = fsl_rio_mcheck_exception(regs); > +#endif > + } > + > mtspr(SPRN_MCSR, mcsr); > return mfspr(SPRN_MCSR) == 0 && recoverable; > } > @@ -527,8 +535,12 @@ int machine_check_e500(struct pt_regs *regs) To deal w/Alex's comment do: machine_check_e500(...) { int ret = 0; > printk("Bus - Write Address Error\n"); > if (reason & MCSR_BUS_IBERR) > printk("Bus - Instruction Data Error\n"); > - if (reason & MCSR_BUS_RBERR) > + if (reason & MCSR_BUS_RBERR) { > printk("Bus - Read Data Bus Error\n"); > +#ifdef CONFIG_RAPIDIO > + fsl_rio_mcheck_exception(regs); > +#endif make this like 'ret = fsl_rio... > + } > if (reason & MCSR_BUS_WBERR) > printk("Bus - Read Data Bus Error\n"); > if (reason & MCSR_BUS_IPERR) return ret > diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c > index 1143c93..a9bc1e8 100644 > --- a/arch/powerpc/sysdev/fsl_rio.c > +++ b/arch/powerpc/sysdev/fsl_rio.c > @@ -256,9 +256,7 @@ struct rio_priv { > static void __iomem *rio_regs_win; > > #ifdef CONFIG_E500 > -static int (*saved_mcheck_exception)(struct pt_regs *regs); > - > -static int fsl_rio_mcheck_exception(struct pt_regs *regs) > +int fsl_rio_mcheck_exception(struct pt_regs *regs) > { > const struct exception_table_entry *entry = NULL; > unsigned long reason = mfspr(SPRN_MCSR); > @@ -280,11 +278,9 @@ static int fsl_rio_mcheck_exception(struct pt_regs *regs) > } > } > > - if (saved_mcheck_exception) > - return saved_mcheck_exception(regs); > - else > - return cur_cpu_spec->machine_check(regs); > + return 0; > } > +EXPORT_SYMBOL_GPL(fsl_rio_mcheck_exception); > #endif > > /** > @@ -1534,11 +1530,6 @@ int fsl_rio_setup(struct platform_device *dev) > fsl_rio_doorbell_init(port); > fsl_rio_port_write_init(port); > > -#ifdef CONFIG_E500 > - saved_mcheck_exception = ppc_md.machine_check_exception; > - ppc_md.machine_check_exception = fsl_rio_mcheck_exception; > -#endif > - > return 0; > err: > iounmap(priv->regs_win); > diff --git a/include/linux/rio.h b/include/linux/rio.h > index bd6eb0e..685b18f 100644 > --- a/include/linux/rio.h > +++ b/include/linux/rio.h > @@ -365,5 +365,6 @@ extern int rio_open_inb_mbox(struct rio_mport *, void *, int, int); > extern void rio_close_inb_mbox(struct rio_mport *, int); > extern int rio_open_outb_mbox(struct rio_mport *, void *, int, int); > extern void rio_close_outb_mbox(struct rio_mport *, int); > +extern int fsl_rio_mcheck_exception(struct pt_regs *); This should be in asm/rio.h not linux/rio.h and: Make it look like: #ifdef CONFIG_RAPIDIO extern int fsl_rio_mcheck_exception(struct pt_regs *); #else static inline int fsl_rio_mcheck_exception(struct pt_regs *) { } #endif > > #endif /* LINUX_RIO_H */ > -- > 1.6.4 > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
Kumar Gala <galak@kernel.crashing.org> wrote: > > [Xie Shaohui-B21989] Hi Alex, seems your suggestion is some kind of > > conflict with Kumar, you can have a look at > > http://patchwork.ozlabs.org/patch/67774/ > > I think Alex's comment is the fact we ignore the 'return' value in the machine_check_e500 case. Yes, this one and plus the fact that Mchk exception messages are printed even if it was handled successfully (by RIO handler). Messages are printed in both: e500 and e500mc handlers. Alex.
Kumar Gala <kumar.gala@freescale.com> wrote: > > @@ -527,8 +535,12 @@ int machine_check_e500(struct pt_regs *regs) > > To deal w/Alex's comment do: > > machine_check_e500(...) { > > int ret = 0; > > > > printk("Bus - Write Address Error\n"); > > if (reason & MCSR_BUS_IBERR) > > printk("Bus - Instruction Data Error\n"); > > - if (reason & MCSR_BUS_RBERR) > > + if (reason & MCSR_BUS_RBERR) { > > printk("Bus - Read Data Bus Error\n"); > > +#ifdef CONFIG_RAPIDIO > > + fsl_rio_mcheck_exception(regs); > > +#endif > > make this like 'ret = fsl_rio... > Please, place it in the beginning of the machine_check_e500[mc](...) as well to avoid any 'printk' if RIO exception was handled properly. Alex.
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index a45a63c..2a5fb9d 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -55,6 +55,7 @@ #endif #include <asm/kexec.h> #include <asm/ppc-opcode.h> +#include <linux/rio.h> #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) int (*__debugger)(struct pt_regs *regs) __read_mostly; @@ -500,6 +501,13 @@ int machine_check_e500mc(struct pt_regs *regs) reason & MCSR_MEA ? "Effective" : "Physical", addr); } + if (reason & MCSR_BUS_RBERR) { + printk("Bus - Read Data Bus Error\n"); +#ifdef CONFIG_RAPIDIO + recoverable = fsl_rio_mcheck_exception(regs); +#endif + } + mtspr(SPRN_MCSR, mcsr); return mfspr(SPRN_MCSR) == 0 && recoverable; } @@ -527,8 +535,12 @@ int machine_check_e500(struct pt_regs *regs) printk("Bus - Write Address Error\n"); if (reason & MCSR_BUS_IBERR) printk("Bus - Instruction Data Error\n"); - if (reason & MCSR_BUS_RBERR) + if (reason & MCSR_BUS_RBERR) { printk("Bus - Read Data Bus Error\n"); +#ifdef CONFIG_RAPIDIO + fsl_rio_mcheck_exception(regs); +#endif + } if (reason & MCSR_BUS_WBERR) printk("Bus - Read Data Bus Error\n"); if (reason & MCSR_BUS_IPERR) diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c index 1143c93..a9bc1e8 100644 --- a/arch/powerpc/sysdev/fsl_rio.c +++ b/arch/powerpc/sysdev/fsl_rio.c @@ -256,9 +256,7 @@ struct rio_priv { static void __iomem *rio_regs_win; #ifdef CONFIG_E500 -static int (*saved_mcheck_exception)(struct pt_regs *regs); - -static int fsl_rio_mcheck_exception(struct pt_regs *regs) +int fsl_rio_mcheck_exception(struct pt_regs *regs) { const struct exception_table_entry *entry = NULL; unsigned long reason = mfspr(SPRN_MCSR); @@ -280,11 +278,9 @@ static int fsl_rio_mcheck_exception(struct pt_regs *regs) } } - if (saved_mcheck_exception) - return saved_mcheck_exception(regs); - else - return cur_cpu_spec->machine_check(regs); + return 0; } +EXPORT_SYMBOL_GPL(fsl_rio_mcheck_exception); #endif /** @@ -1534,11 +1530,6 @@ int fsl_rio_setup(struct platform_device *dev) fsl_rio_doorbell_init(port); fsl_rio_port_write_init(port); -#ifdef CONFIG_E500 - saved_mcheck_exception = ppc_md.machine_check_exception; - ppc_md.machine_check_exception = fsl_rio_mcheck_exception; -#endif - return 0; err: iounmap(priv->regs_win); diff --git a/include/linux/rio.h b/include/linux/rio.h index bd6eb0e..685b18f 100644 --- a/include/linux/rio.h +++ b/include/linux/rio.h @@ -365,5 +365,6 @@ extern int rio_open_inb_mbox(struct rio_mport *, void *, int, int); extern void rio_close_inb_mbox(struct rio_mport *, int); extern int rio_open_outb_mbox(struct rio_mport *, void *, int, int); extern void rio_close_outb_mbox(struct rio_mport *, int); +extern int fsl_rio_mcheck_exception(struct pt_regs *); #endif /* LINUX_RIO_H */
Signed-off-by: Shaohui Xie <b21989@freescale.com> Cc: Li Yang <leoli@freescale.com> Cc: Kumar Gala <kumar.gala@freescale.com> Cc: Roy Zang <tie-fei.zang@freescale.com> Cc: Alexandre Bounine <alexandre.bounine@idt.com> --- arch/powerpc/kernel/traps.c | 14 +++++++++++++- arch/powerpc/sysdev/fsl_rio.c | 15 +++------------ include/linux/rio.h | 1 + 3 files changed, 17 insertions(+), 13 deletions(-)