Re: [Linaro-acpi] [PATCH 4/7] acpi, apei, ghes: Factor out NMI error notification context.

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!