Signed-off-by: Shaohui Xie <[email protected]>
Cc: Li Yang <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Roy Zang <[email protected]>
Cc: Alexandre Bounine <[email protected]>
---
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)
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 */
--
1.6.4
Shaohui Xie <[email protected]> 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:[email protected]]
> Sent: Thursday, November 11, 2010 6:55 AM
> To: Xie Shaohui-B21989; [email protected]
> Cc: [email protected]; [email protected]; 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 <[email protected]> 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:[email protected]]
>> Sent: Thursday, November 11, 2010 6:55 AM
>> To: Xie Shaohui-B21989; [email protected]
>> Cc: [email protected]; [email protected]; 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 <[email protected]> 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 <[email protected]>
> Cc: Li Yang <[email protected]>
> Cc: Kumar Gala <[email protected]>
> Cc: Roy Zang <[email protected]>
> Cc: Alexandre Bounine <[email protected]>
> ---
> 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
> [email protected]
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Kumar Gala <[email protected]> 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 <[email protected]> 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.