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

[Tomasz Nowicki]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/