| Message ID | 538340EE.7070408@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, May 26, 2014 at 03:26:06PM +0200, Tomasz Nowicki wrote: > Now I do follow :) Nicely done, I have applied your patch and indeed > there are more arch dependencies for !X86. Not nicely enough, I guess :-) > diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h > index 86f9301..0f03ab6 100644 > --- a/arch/x86/include/asm/nmi.h > +++ b/arch/x86/include/asm/nmi.h > @@ -54,7 +54,18 @@ struct nmiaction { > > int __register_nmi_handler(unsigned int, struct nmiaction *); > > -void unregister_nmi_handler(unsigned int, const char *); > +void unregister_nmi_handler(unsigned int type, const char *name); > + > +static inline int arch_apei_register_nmi(nmi_handler_t fn, > + const char *name) > +{ > + return register_nmi_handler(NMI_LOCAL, fn, 0, name); > +} > + > +static inline void arch_apei_unregister_nmi(const char *name) > +{ > + unregister_nmi_handler(NMI_LOCAL, name); > +} I'm guessing you've added those wrappers so that you don't have to export NMI_LOCAL? > void stop_nmi(void); > void restart_nmi(void); > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 35a44d9..84c79af 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -47,13 +47,11 @@ > #include <linux/genalloc.h> > #include <linux/pci.h> > #include <linux/aer.h> > +#include <linux/nmi.h> > > #include <acpi/ghes.h> > #include <asm/apei.h> > #include <asm/tlbflush.h> > -#ifdef CONFIG_ACPI_APEI_NMI > -#include <asm/nmi.h> > -#endif > > #include "apei-internal.h" > > @@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this, > return ret; > } > > -#ifdef CONFIG_ACPI_APEI_NMI > /* > * printk is not safe in NMI context. So in NMI handler, we allocate > * required memory from lock-less memory allocator > @@ -817,7 +814,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct > pt_regs *regs) > { > struct ghes *ghes, *ghes_global = NULL; > int sev, sev_global = -1; > - int ret = NMI_DONE; > + int ret = APEI_NMI_DONE; > > BUG_ON(!IS_ENABLED(CONFIG_ACPI_APEI_NMI)); > > @@ -832,14 +829,14 @@ static int ghes_notify_nmi(unsigned int cmd, struct > pt_regs *regs) > sev_global = sev; > ghes_global = ghes; > } > - ret = NMI_HANDLED; > + ret = APEI_NMI_HANDLED; > } > > - if (ret == NMI_DONE) > + if (ret == APEI_NMI_DONE) > goto out; > > if (sev_global >= GHES_SEV_PANIC) { > - oops_begin(); > + arch_apei_nmi_oops_begin(); > ghes_print_queued_estatus(); > __ghes_print_estatus(KERN_EMERG, ghes_global->generic, > ghes_global->estatus); > @@ -914,7 +911,7 @@ static int ghes_notify_init_nmi(struct ghes *ghes) > ghes_estatus_pool_expand(len); > mutex_lock(&ghes_list_mutex); > if (list_empty(&ghes_nmi)) > - status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes"); > + status = arch_apei_register_nmi(ghes_notify_nmi, "ghes"); > list_add_rcu(&ghes->list, &ghes_nmi); > mutex_unlock(&ghes_list_mutex); > > @@ -928,7 +925,7 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) > mutex_lock(&ghes_list_mutex); > list_del_rcu(&ghes->list); > if (list_empty(&ghes_nmi)) > - unregister_nmi_handler(NMI_LOCAL, "ghes"); > + arch_apei_unregister_nmi("ghes"); > mutex_unlock(&ghes_list_mutex); > /* > * To synchronize with NMI handler, ghes can only be > @@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) > > static void ghes_init_nmi(void) > { > + if (!IS_ENABLED(CONFIG_ACPI_APEI_NMI)) > + return; > + > init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq); > ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi; > ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call = > ghes_notify_remove_nmi; > } > -#else > -static inline void ghes_init_nmi(void) > -{ > - > -} > -#endif > > static int ghes_notify_init_polled(struct ghes *ghes) > { > diff --git a/include/linux/nmi.h b/include/linux/nmi.h > index 084b4c5..1aa351c 100644 > --- a/include/linux/nmi.h > +++ b/include/linux/nmi.h > @@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int , > void __user *, size_t *, loff_t *); > #endif > > +#define APEI_NMI_DONE 0 > +#define APEI_NMI_HANDLED 1 > + > +#ifdef CONFIG_ACPI_APEI_NMI > +#include <asm/nmi.h> > +#define arch_apei_nmi_oops_begin() oops_begin() > +#else > +#define arch_apei_register_nmi(fn, n) ({ \ > + void __attribute__((unused)) *dummy = fn; \ Do we really need this dummy assignment? Wouldn't it be just fine to simply have: #define arch_apei_register_nmi(fn, n) ({ (-ENOSYS); }) Just a nitpick, though; otherwise, this looks nicely abstracted. Thanks!
On 26.05.2014 15:45, Borislav Petkov wrote: > On Mon, May 26, 2014 at 03:26:06PM +0200, Tomasz Nowicki wrote: >> Now I do follow :) Nicely done, I have applied your patch and indeed >> there are more arch dependencies for !X86. > > Not nicely enough, I guess :-) > >> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h >> index 86f9301..0f03ab6 100644 >> --- a/arch/x86/include/asm/nmi.h >> +++ b/arch/x86/include/asm/nmi.h >> @@ -54,7 +54,18 @@ struct nmiaction { >> >> int __register_nmi_handler(unsigned int, struct nmiaction *); >> >> -void unregister_nmi_handler(unsigned int, const char *); >> +void unregister_nmi_handler(unsigned int type, const char *name); >> + >> +static inline int arch_apei_register_nmi(nmi_handler_t fn, >> + const char *name) >> +{ >> + return register_nmi_handler(NMI_LOCAL, fn, 0, name); >> +} >> + >> +static inline void arch_apei_unregister_nmi(const char *name) >> +{ >> + unregister_nmi_handler(NMI_LOCAL, name); >> +} > > I'm guessing you've added those wrappers so that you don't have to > export NMI_LOCAL? Yep. > >> void stop_nmi(void); >> void restart_nmi(void); >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 35a44d9..84c79af 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -47,13 +47,11 @@ >> #include <linux/genalloc.h> >> #include <linux/pci.h> >> #include <linux/aer.h> >> +#include <linux/nmi.h> >> >> #include <acpi/ghes.h> >> #include <asm/apei.h> >> #include <asm/tlbflush.h> >> -#ifdef CONFIG_ACPI_APEI_NMI >> -#include <asm/nmi.h> >> -#endif >> >> #include "apei-internal.h" >> >> @@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this, >> return ret; >> } >> >> -#ifdef CONFIG_ACPI_APEI_NMI >> /* >> * printk is not safe in NMI context. So in NMI handler, we allocate >> * required memory from lock-less memory allocator >> @@ -817,7 +814,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct >> pt_regs *regs) >> { >> struct ghes *ghes, *ghes_global = NULL; >> int sev, sev_global = -1; >> - int ret = NMI_DONE; >> + int ret = APEI_NMI_DONE; >> >> BUG_ON(!IS_ENABLED(CONFIG_ACPI_APEI_NMI)); >> >> @@ -832,14 +829,14 @@ static int ghes_notify_nmi(unsigned int cmd, struct >> pt_regs *regs) >> sev_global = sev; >> ghes_global = ghes; >> } >> - ret = NMI_HANDLED; >> + ret = APEI_NMI_HANDLED; >> } >> >> - if (ret == NMI_DONE) >> + if (ret == APEI_NMI_DONE) >> goto out; >> >> if (sev_global >= GHES_SEV_PANIC) { >> - oops_begin(); >> + arch_apei_nmi_oops_begin(); >> ghes_print_queued_estatus(); >> __ghes_print_estatus(KERN_EMERG, ghes_global->generic, >> ghes_global->estatus); >> @@ -914,7 +911,7 @@ static int ghes_notify_init_nmi(struct ghes *ghes) >> ghes_estatus_pool_expand(len); >> mutex_lock(&ghes_list_mutex); >> if (list_empty(&ghes_nmi)) >> - status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes"); >> + status = arch_apei_register_nmi(ghes_notify_nmi, "ghes"); >> list_add_rcu(&ghes->list, &ghes_nmi); >> mutex_unlock(&ghes_list_mutex); >> >> @@ -928,7 +925,7 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) >> mutex_lock(&ghes_list_mutex); >> list_del_rcu(&ghes->list); >> if (list_empty(&ghes_nmi)) >> - unregister_nmi_handler(NMI_LOCAL, "ghes"); >> + arch_apei_unregister_nmi("ghes"); >> mutex_unlock(&ghes_list_mutex); >> /* >> * To synchronize with NMI handler, ghes can only be >> @@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) >> >> static void ghes_init_nmi(void) >> { >> + if (!IS_ENABLED(CONFIG_ACPI_APEI_NMI)) >> + return; >> + >> init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq); >> ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi; >> ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call = >> ghes_notify_remove_nmi; >> } >> -#else >> -static inline void ghes_init_nmi(void) >> -{ >> - >> -} >> -#endif >> >> static int ghes_notify_init_polled(struct ghes *ghes) >> { >> diff --git a/include/linux/nmi.h b/include/linux/nmi.h >> index 084b4c5..1aa351c 100644 >> --- a/include/linux/nmi.h >> +++ b/include/linux/nmi.h >> @@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int , >> void __user *, size_t *, loff_t *); >> #endif >> >> +#define APEI_NMI_DONE 0 >> +#define APEI_NMI_HANDLED 1 >> + >> +#ifdef CONFIG_ACPI_APEI_NMI >> +#include <asm/nmi.h> >> +#define arch_apei_nmi_oops_begin() oops_begin() >> +#else >> +#define arch_apei_register_nmi(fn, n) ({ \ >> + void __attribute__((unused)) *dummy = fn; \ > > Do we really need this dummy assignment? Wouldn't it be just fine to > simply have: > > #define arch_apei_register_nmi(fn, n) ({ (-ENOSYS); }) For !X86 I've got compilator warning that ghes_notify_nmi is defined but not used. I just want to make console a bit cleaner :) > > Just a nitpick, though; otherwise, this looks nicely abstracted. Thanks for your help! Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h index 86f9301..0f03ab6 100644 --- a/arch/x86/include/asm/nmi.h +++ b/arch/x86/include/asm/nmi.h @@ -54,7 +54,18 @@ struct nmiaction { int __register_nmi_handler(unsigned int, struct nmiaction *); -void unregister_nmi_handler(unsigned int, const char *); +void unregister_nmi_handler(unsigned int type, const char *name); + +static inline int arch_apei_register_nmi(nmi_handler_t fn, + const char *name) +{ + return register_nmi_handler(NMI_LOCAL, fn, 0, name); +} + +static inline void arch_apei_unregister_nmi(const char *name) +{ + unregister_nmi_handler(NMI_LOCAL, name); +} void stop_nmi(void); void restart_nmi(void); diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 35a44d9..84c79af 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -47,13 +47,11 @@ #include <linux/genalloc.h> #include <linux/pci.h> #include <linux/aer.h> +#include <linux/nmi.h> #include <acpi/ghes.h> #include <asm/apei.h> #include <asm/tlbflush.h> -#ifdef CONFIG_ACPI_APEI_NMI -#include <asm/nmi.h> -#endif #include "apei-internal.h" @@ -718,7 +716,6 @@ static int ghes_notify_sci(struct notifier_block *this, return ret; } -#ifdef CONFIG_ACPI_APEI_NMI /* * printk is not safe in NMI context. So in NMI handler, we allocate * required memory from lock-less memory allocator @@ -817,7 +814,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) { struct ghes *ghes, *ghes_global = NULL; int sev, sev_global = -1; - int ret = NMI_DONE; + int ret = APEI_NMI_DONE; BUG_ON(!IS_ENABLED(CONFIG_ACPI_APEI_NMI)); @@ -832,14 +829,14 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) sev_global = sev; ghes_global = ghes; } - ret = NMI_HANDLED; + ret = APEI_NMI_HANDLED; } - if (ret == NMI_DONE) + if (ret == APEI_NMI_DONE) goto out; if (sev_global >= GHES_SEV_PANIC) { - oops_begin(); + arch_apei_nmi_oops_begin(); ghes_print_queued_estatus(); __ghes_print_estatus(KERN_EMERG, ghes_global->generic, ghes_global->estatus); @@ -914,7 +911,7 @@ static int ghes_notify_init_nmi(struct ghes *ghes) ghes_estatus_pool_expand(len); mutex_lock(&ghes_list_mutex); if (list_empty(&ghes_nmi)) - status = register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes"); + status = arch_apei_register_nmi(ghes_notify_nmi, "ghes"); list_add_rcu(&ghes->list, &ghes_nmi); mutex_unlock(&ghes_list_mutex); @@ -928,7 +925,7 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) mutex_lock(&ghes_list_mutex); list_del_rcu(&ghes->list); if (list_empty(&ghes_nmi)) - unregister_nmi_handler(NMI_LOCAL, "ghes"); + arch_apei_unregister_nmi("ghes"); mutex_unlock(&ghes_list_mutex); /* * To synchronize with NMI handler, ghes can only be @@ -941,17 +938,14 @@ static void ghes_notify_remove_nmi(struct ghes *ghes) static void ghes_init_nmi(void) { + if (!IS_ENABLED(CONFIG_ACPI_APEI_NMI)) + return; + init_irq_work(&ghes_proc_irq_work, ghes_proc_in_irq); ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].init_call = ghes_notify_init_nmi; ghes_notify_tab[ACPI_HEST_NOTIFY_NMI].remove_call = ghes_notify_remove_nmi; } -#else -static inline void ghes_init_nmi(void) -{ - -} -#endif static int ghes_notify_init_polled(struct ghes *ghes) { diff --git a/include/linux/nmi.h b/include/linux/nmi.h index 084b4c5..1aa351c 100644 --- a/include/linux/nmi.h +++ b/include/linux/nmi.h @@ -56,4 +56,19 @@ extern int proc_dowatchdog(struct ctl_table *, int , void __user *, size_t *, loff_t *); #endif +#define APEI_NMI_DONE 0 +#define APEI_NMI_HANDLED 1 + +#ifdef CONFIG_ACPI_APEI_NMI +#include <asm/nmi.h> +#define arch_apei_nmi_oops_begin() oops_begin() +#else +#define arch_apei_register_nmi(fn, n) ({ \ + void __attribute__((unused)) *dummy = fn; \ + (-ENOSYS); \ +}); +#define arch_apei_unregister_nmi(n) +#define arch_apei_nmi_oops_begin() +#endif /* CONFIG_ACPI_APEI_NMI */ + #endif