| Message ID | 1439320409-20084-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/11/2015 03:13 PM, Murali Karicheri wrote: > Currently on some devices, an asynchronous external abort exception > happens during boot up when exception handlers are enabled in kernel > before switching to user space. This patch adds a workaround to handle > this once during boot. Many customers are already using this > with out any issues and is required to workaround the above issue. > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- > arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c > index e2880105..c1d0fe5 100644 > --- a/arch/arm/mach-keystone/keystone.c > +++ b/arch/arm/mach-keystone/keystone.c > @@ -15,6 +15,7 @@ > #include <linux/of_platform.h> > #include <linux/of_address.h> > #include <linux/memblock.h> > +#include <linux/signal.h> > > #include <asm/setup.h> > #include <asm/mach/map.h> > @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { > .notifier_call = keystone_platform_notifier, > }; > > +static bool ignore_first = true; > +static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, > + struct pt_regs *regs) > +{ > + /* > + * if first time, ignore this as this is a asynchronous external abort > + * happening only some devices that couldn't be root caused and we add > + * this work around to handle this first time. > + */ > + if (ignore_first) { > + ignore_first = false; > + return 0; > + } > + > + /* Subsequent ones should be handled as fault */ > + return 1; > +} > + > static void __init keystone_init(void) > { > if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { > @@ -61,6 +80,13 @@ static void __init keystone_init(void) > } > keystone_pm_runtime_init(); > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > + > + /* > + * Add a one time exception handler to catch asynchronous external > + * abort > + */ > + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, > + "async external abort handler"); > } > > static phys_addr_t keystone_virt_to_idmap(unsigned long x) > Can this be applied if it looks good? Murali
On Fri, Aug 14, 2015 at 10:04:41AM -0400, Murali Karicheri wrote: > On 08/11/2015 03:13 PM, Murali Karicheri wrote: > >Currently on some devices, an asynchronous external abort exception > >happens during boot up when exception handlers are enabled in kernel > >before switching to user space. This patch adds a workaround to handle > >this once during boot. Many customers are already using this > >with out any issues and is required to workaround the above issue. > > > >Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > >--- > > arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > >diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c > >index e2880105..c1d0fe5 100644 > >--- a/arch/arm/mach-keystone/keystone.c > >+++ b/arch/arm/mach-keystone/keystone.c > >@@ -15,6 +15,7 @@ > > #include <linux/of_platform.h> > > #include <linux/of_address.h> > > #include <linux/memblock.h> > >+#include <linux/signal.h> > > > > #include <asm/setup.h> > > #include <asm/mach/map.h> > >@@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { > > .notifier_call = keystone_platform_notifier, > > }; > > > >+static bool ignore_first = true; > >+static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, > >+ struct pt_regs *regs) > >+{ > >+ /* > >+ * if first time, ignore this as this is a asynchronous external abort > >+ * happening only some devices that couldn't be root caused and we add > >+ * this work around to handle this first time. > >+ */ > >+ if (ignore_first) { > >+ ignore_first = false; > >+ return 0; > >+ } > >+ > >+ /* Subsequent ones should be handled as fault */ > >+ return 1; > >+} > >+ > > static void __init keystone_init(void) > > { > > if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { > >@@ -61,6 +80,13 @@ static void __init keystone_init(void) > > } > > keystone_pm_runtime_init(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > >+ > >+ /* > >+ * Add a one time exception handler to catch asynchronous external > >+ * abort > >+ */ > >+ hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, > >+ "async external abort handler"); > > } > > > > static phys_addr_t keystone_virt_to_idmap(unsigned long x) > > > Can this be applied if it looks good? What causes the abort? We shouldn't be adding hacks like this to the kernel without having the full picture...
Am Freitag, den 14.08.2015, 10:04 -0400 schrieb Murali Karicheri: > On 08/11/2015 03:13 PM, Murali Karicheri wrote: > > Currently on some devices, an asynchronous external abort exception > > happens during boot up when exception handlers are enabled in kernel > > before switching to user space. This patch adds a workaround to handle > > this once during boot. Many customers are already using this > > with out any issues and is required to workaround the above issue. > > > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > > --- > > arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c > > index e2880105..c1d0fe5 100644 > > --- a/arch/arm/mach-keystone/keystone.c > > +++ b/arch/arm/mach-keystone/keystone.c > > @@ -15,6 +15,7 @@ > > #include <linux/of_platform.h> > > #include <linux/of_address.h> > > #include <linux/memblock.h> > > +#include <linux/signal.h> > > > > #include <asm/setup.h> > > #include <asm/mach/map.h> > > @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { > > .notifier_call = keystone_platform_notifier, > > }; > > > > +static bool ignore_first = true; > > +static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, > > + struct pt_regs *regs) > > +{ > > + /* > > + * if first time, ignore this as this is a asynchronous external abort > > + * happening only some devices that couldn't be root caused and we add > > + * this work around to handle this first time. > > + */ > > + if (ignore_first) { > > + ignore_first = false; > > + return 0; > > + } > > + > > + /* Subsequent ones should be handled as fault */ > > + return 1; > > +} > > + > > static void __init keystone_init(void) > > { > > if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { > > @@ -61,6 +80,13 @@ static void __init keystone_init(void) > > } > > keystone_pm_runtime_init(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > + > > + /* > > + * Add a one time exception handler to catch asynchronous external > > + * abort > > + */ > > + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, > > + "async external abort handler"); > > } > > > > static phys_addr_t keystone_virt_to_idmap(unsigned long x) > > > Can this be applied if it looks good? > The keystone PCIe host driver already hooks the same fault code. Those hooks are no chain, but a simple pointer, so one of those handlers is going to loose out. This likely isn't what you intended. Regards, Lucas
On 8/14/2015 7:09 AM, Russell King - ARM Linux wrote: > On Fri, Aug 14, 2015 at 10:04:41AM -0400, Murali Karicheri wrote: >> On 08/11/2015 03:13 PM, Murali Karicheri wrote: >>> Currently on some devices, an asynchronous external abort exception >>> happens during boot up when exception handlers are enabled in kernel >>> before switching to user space. This patch adds a workaround to handle >>> this once during boot. Many customers are already using this >>> with out any issues and is required to workaround the above issue. >>> >>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>> --- >>> arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c >>> index e2880105..c1d0fe5 100644 >>> --- a/arch/arm/mach-keystone/keystone.c >>> +++ b/arch/arm/mach-keystone/keystone.c >>> @@ -15,6 +15,7 @@ >>> #include <linux/of_platform.h> >>> #include <linux/of_address.h> >>> #include <linux/memblock.h> >>> +#include <linux/signal.h> >>> >>> #include <asm/setup.h> >>> #include <asm/mach/map.h> >>> @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { >>> .notifier_call = keystone_platform_notifier, >>> }; >>> >>> +static bool ignore_first = true; >>> +static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, >>> + struct pt_regs *regs) >>> +{ >>> + /* >>> + * if first time, ignore this as this is a asynchronous external abort >>> + * happening only some devices that couldn't be root caused and we add >>> + * this work around to handle this first time. >>> + */ >>> + if (ignore_first) { >>> + ignore_first = false; >>> + return 0; >>> + } >>> + >>> + /* Subsequent ones should be handled as fault */ >>> + return 1; >>> +} >>> + >>> static void __init keystone_init(void) >>> { >>> if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { >>> @@ -61,6 +80,13 @@ static void __init keystone_init(void) >>> } >>> keystone_pm_runtime_init(); >>> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); >>> + >>> + /* >>> + * Add a one time exception handler to catch asynchronous external >>> + * abort >>> + */ >>> + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, >>> + "async external abort handler"); >>> } >>> >>> static phys_addr_t keystone_virt_to_idmap(unsigned long x) >>> >> Can this be applied if it looks good? > > What causes the abort? We shouldn't be adding hacks like this to the > kernel without having the full picture... > Indeed. These external aborts are notorious and often hides dangerous bugs. On OMAP as well many folks burn their had with it till the interconnect handlers were added to detect those and hunt those bugs. In my experience such aborts happen outside ARM subsystem, either in the interconnect or at the salve targets which are reported over the ARM bus as async external aborts. And often these errors are due to bad accesses/wrong accesses/un-clocked accesses at slaves. Regards, Santosh
On 08/14/2015 11:14 AM, santosh shilimkar wrote: > On 8/14/2015 7:09 AM, Russell King - ARM Linux wrote: >> On Fri, Aug 14, 2015 at 10:04:41AM -0400, Murali Karicheri wrote: >>> On 08/11/2015 03:13 PM, Murali Karicheri wrote: >>>> Currently on some devices, an asynchronous external abort exception >>>> happens during boot up when exception handlers are enabled in kernel >>>> before switching to user space. This patch adds a workaround to handle >>>> this once during boot. Many customers are already using this >>>> with out any issues and is required to workaround the above issue. >>>> >>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>>> --- >>>> arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-keystone/keystone.c >>>> b/arch/arm/mach-keystone/keystone.c >>>> index e2880105..c1d0fe5 100644 >>>> --- a/arch/arm/mach-keystone/keystone.c >>>> +++ b/arch/arm/mach-keystone/keystone.c >>>> @@ -15,6 +15,7 @@ >>>> #include <linux/of_platform.h> >>>> #include <linux/of_address.h> >>>> #include <linux/memblock.h> >>>> +#include <linux/signal.h> >>>> >>>> #include <asm/setup.h> >>>> #include <asm/mach/map.h> >>>> @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { >>>> .notifier_call = keystone_platform_notifier, >>>> }; >>>> >>>> +static bool ignore_first = true; >>>> +static int keystone_async_ext_abort_fault(unsigned long addr, >>>> unsigned int fsr, >>>> + struct pt_regs *regs) >>>> +{ >>>> + /* >>>> + * if first time, ignore this as this is a asynchronous >>>> external abort >>>> + * happening only some devices that couldn't be root caused and >>>> we add >>>> + * this work around to handle this first time. >>>> + */ >>>> + if (ignore_first) { >>>> + ignore_first = false; >>>> + return 0; >>>> + } >>>> + >>>> + /* Subsequent ones should be handled as fault */ >>>> + return 1; >>>> +} >>>> + >>>> static void __init keystone_init(void) >>>> { >>>> if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { >>>> @@ -61,6 +80,13 @@ static void __init keystone_init(void) >>>> } >>>> keystone_pm_runtime_init(); >>>> of_platform_populate(NULL, of_default_bus_match_table, NULL, >>>> NULL); >>>> + >>>> + /* >>>> + * Add a one time exception handler to catch asynchronous external >>>> + * abort >>>> + */ >>>> + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, >>>> + "async external abort handler"); >>>> } >>>> >>>> static phys_addr_t keystone_virt_to_idmap(unsigned long x) >>>> >>> Can this be applied if it looks good? >> >> What causes the abort? We shouldn't be adding hacks like this to the >> kernel without having the full picture... >> > Indeed. These external aborts are notorious and often hides dangerous > bugs. On OMAP as well many folks burn their had with it till the > interconnect handlers were added to detect those and hunt those > bugs. > > In my experience such aborts happen outside ARM subsystem, either in > the interconnect or at the salve targets which are reported over > the ARM bus as async external aborts. And often these errors are > due to bad accesses/wrong accesses/un-clocked accesses at slaves. > We have spend some time already to debug the root cause. Do you have idea on how this was hunted down on OMAP that we can learn from? The bad address is NULL and it seems to happen very rarely and is not easily reproducible. Don't want to put this workaround, but we couldn't track it down either. So any help to debug this will be appreciated. Murali > Regards, > Santosh > > >
On Fri, Aug 14, 2015 at 05:53:00PM -0400, Murali Karicheri wrote: > We have spend some time already to debug the root cause. Do you have idea on > how this was hunted down on OMAP that we can learn from? The bad address is > NULL and it seems to happen very rarely and is not easily reproducible. > Don't want to put this workaround, but we couldn't track it down either. So > any help to debug this will be appreciated. If you try applying Lucas' patch, you should receive the abort earlier in the kernel boot up, which may help narrow down what is provoking it.
On 8/14/15 2:53 PM, Murali Karicheri wrote: > On 08/14/2015 11:14 AM, santosh shilimkar wrote: >> On 8/14/2015 7:09 AM, Russell King - ARM Linux wrote: >>> On Fri, Aug 14, 2015 at 10:04:41AM -0400, Murali Karicheri wrote: >>>> On 08/11/2015 03:13 PM, Murali Karicheri wrote: >>>>> Currently on some devices, an asynchronous external abort exception >>>>> happens during boot up when exception handlers are enabled in kernel >>>>> before switching to user space. This patch adds a workaround to handle >>>>> this once during boot. Many customers are already using this >>>>> with out any issues and is required to workaround the above issue. >>>>> >>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>>>> --- >>>>> arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ >>>>> 1 file changed, 26 insertions(+) [...] >>>>> + >>>>> + /* >>>>> + * Add a one time exception handler to catch asynchronous >>>>> external >>>>> + * abort >>>>> + */ >>>>> + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, >>>>> + "async external abort handler"); >>>>> } >>>>> >>>>> static phys_addr_t keystone_virt_to_idmap(unsigned long x) >>>>> >>>> Can this be applied if it looks good? >>> >>> What causes the abort? We shouldn't be adding hacks like this to the >>> kernel without having the full picture... >>> >> Indeed. These external aborts are notorious and often hides dangerous >> bugs. On OMAP as well many folks burn their had with it till the >> interconnect handlers were added to detect those and hunt those >> bugs. >> >> In my experience such aborts happen outside ARM subsystem, either in >> the interconnect or at the salve targets which are reported over >> the ARM bus as async external aborts. And often these errors are >> due to bad accesses/wrong accesses/un-clocked accesses at slaves. >> > We have spend some time already to debug the root cause. Do you have > idea on how this was hunted down on OMAP that we can learn from? The bad > address is NULL and it seems to happen very rarely and is not easily > reproducible. Don't want to put this workaround, but we couldn't track > it down either. So any help to debug this will be appreciated. > As RMK pointed out, try Lucas patch and see if it gives any useful information to narrow it down. On OMAP, fortunately interconnect has IRQ(s) which are hooked with ARM subsystem. So the bus driver(drivers/bus/omap-l3*) was able to handle those events and report the offenders. Regards, Santosh
On 08/14/2015 10:11 AM, Lucas Stach wrote: > Am Freitag, den 14.08.2015, 10:04 -0400 schrieb Murali Karicheri: >> On 08/11/2015 03:13 PM, Murali Karicheri wrote: >>> Currently on some devices, an asynchronous external abort exception >>> happens during boot up when exception handlers are enabled in kernel >>> before switching to user space. This patch adds a workaround to handle >>> this once during boot. Many customers are already using this >>> with out any issues and is required to workaround the above issue. >>> >>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>> --- >>> arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c >>> index e2880105..c1d0fe5 100644 >>> --- a/arch/arm/mach-keystone/keystone.c >>> +++ b/arch/arm/mach-keystone/keystone.c >>> @@ -15,6 +15,7 @@ >>> #include <linux/of_platform.h> >>> #include <linux/of_address.h> >>> #include <linux/memblock.h> >>> +#include <linux/signal.h> >>> >>> #include <asm/setup.h> >>> #include <asm/mach/map.h> >>> @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { >>> .notifier_call = keystone_platform_notifier, >>> }; >>> >>> +static bool ignore_first = true; >>> +static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, >>> + struct pt_regs *regs) >>> +{ >>> + /* >>> + * if first time, ignore this as this is a asynchronous external abort >>> + * happening only some devices that couldn't be root caused and we add >>> + * this work around to handle this first time. >>> + */ >>> + if (ignore_first) { >>> + ignore_first = false; >>> + return 0; >>> + } >>> + >>> + /* Subsequent ones should be handled as fault */ >>> + return 1; >>> +} >>> + >>> static void __init keystone_init(void) >>> { >>> if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { >>> @@ -61,6 +80,13 @@ static void __init keystone_init(void) >>> } >>> keystone_pm_runtime_init(); >>> of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); >>> + >>> + /* >>> + * Add a one time exception handler to catch asynchronous external >>> + * abort >>> + */ >>> + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, >>> + "async external abort handler"); >>> } >>> >>> static phys_addr_t keystone_virt_to_idmap(unsigned long x) >>> >> Can this be applied if it looks good? >> > The keystone PCIe host driver already hooks the same fault code. Those > hooks are no chain, but a simple pointer, so one of those handlers is > going to loose out. > > This likely isn't what you intended. You are right. I will try to debug this further based on your patch and as per RMK's suggestion. Thanks and regards, Murali > > Regards, > Lucas >
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c index e2880105..c1d0fe5 100644 --- a/arch/arm/mach-keystone/keystone.c +++ b/arch/arm/mach-keystone/keystone.c @@ -15,6 +15,7 @@ #include <linux/of_platform.h> #include <linux/of_address.h> #include <linux/memblock.h> +#include <linux/signal.h> #include <asm/setup.h> #include <asm/mach/map.h> @@ -52,6 +53,24 @@ static struct notifier_block platform_nb = { .notifier_call = keystone_platform_notifier, }; +static bool ignore_first = true; +static int keystone_async_ext_abort_fault(unsigned long addr, unsigned int fsr, + struct pt_regs *regs) +{ + /* + * if first time, ignore this as this is a asynchronous external abort + * happening only some devices that couldn't be root caused and we add + * this work around to handle this first time. + */ + if (ignore_first) { + ignore_first = false; + return 0; + } + + /* Subsequent ones should be handled as fault */ + return 1; +} + static void __init keystone_init(void) { if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { @@ -61,6 +80,13 @@ static void __init keystone_init(void) } keystone_pm_runtime_init(); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + /* + * Add a one time exception handler to catch asynchronous external + * abort + */ + hook_fault_code(17, keystone_async_ext_abort_fault, SIGBUS, 0, + "async external abort handler"); } static phys_addr_t keystone_virt_to_idmap(unsigned long x)
Currently on some devices, an asynchronous external abort exception happens during boot up when exception handlers are enabled in kernel before switching to user space. This patch adds a workaround to handle this once during boot. Many customers are already using this with out any issues and is required to workaround the above issue. Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> --- arch/arm/mach-keystone/keystone.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)