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

[Borislav Petkov <bp@alien8.de>]

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!

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--