On 13.06.2014 15:10, Robert Richter wrote:
On 13.06.14 13:02:58, Tomasz Nowicki wrote:
@@ -811,6 +819,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) int sev, sev_global = -1; int ret = NMI_DONE;
- BUG_ON(!IS_ENABLED(ARCH_HAS_ACPI_APEI_NMI));
Now that we have the ARCH_HAS_ACPI_APEI_NMI option, group nmi code, put it in an #ifdef ... and make function stubs for the !nmi case where necessary. That code should moved to patch #2. If an arch does not support nmi code, we don't want to compile it into the kernel.
Also this patch is quit a bit large and should further split into moving functional code into separate functions and the introduction of the notifier setup. This makes review much easier.
I did not yet took a deep look into your notifier framework, but I don't really see a reason for the dynamic collection of function pointers in ghes_notify_tab. See below.
raw_spin_lock(&ghes_nmi_lock); list_for_each_entry_rcu(ghes, &ghes_nmi, list) { if (ghes_read_estatus(ghes, 1)) { @@ -875,10 +885,6 @@ out: return ret; }
+static int ghes_notify_init_nmi(struct ghes *ghes) +{
- unsigned long len;
- int status = 0;
- len = ghes_esource_prealloc_size(ghes->generic);
- 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");- list_add_rcu(&ghes->list, &ghes_nmi);
- mutex_unlock(&ghes_list_mutex);
- return status;
+}
+static void ghes_notify_remove_nmi(struct ghes *ghes) +{
- unsigned long len;
- mutex_lock(&ghes_list_mutex);
- list_del_rcu(&ghes->list);
- if (list_empty(&ghes_nmi))
unregister_nmi_handler(NMI_LOCAL, "ghes");- mutex_unlock(&ghes_list_mutex);
- /*
* To synchronize with NMI handler, ghes can only be* freed after NMI handler finishes.*/- synchronize_rcu();
- len = ghes_esource_prealloc_size(ghes->generic);
- ghes_estatus_pool_shrink(len);
+}
+static void ghes_init_nmi(void) +{
- if (!IS_ENABLED(ARCH_HAS_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;+}
So this is the only code of your whole patch set that actually changes an entry, and just one time only during nmi init. Thus, there is no need at all for ghes_notify_tab. Just create function stubs for ghes_notify_{init,remove}_nmi for the !nmi case with the error message in it and call the functions directly in the switch/cases.
+static struct ghes_notify_setup
- ghes_notify_tab[ACPI_HEST_NOTIFY_RESERVED] = {
[ACPI_HEST_NOTIFY_POLLED] = {"POLLED",ghes_notify_init_polled,ghes_notify_remove_polled},[ACPI_HEST_NOTIFY_EXTERNAL] = {"EXT_IRQ",ghes_notify_init_external,ghes_notify_remove_external},[ACPI_HEST_NOTIFY_LOCAL] = {"LOCAL_IRQ", NULL, NULL},[ACPI_HEST_NOTIFY_SCI] = {"SCI",ghes_notify_init_sci,ghes_notify_remove_sci},[ACPI_HEST_NOTIFY_NMI] = {"NMI", NULL, NULL},[ACPI_HEST_NOTIFY_CMCI] = {"CMCI", NULL, NULL},[ACPI_HEST_NOTIFY_MCE] = {"MCE", NULL, NULL},+};
Again, just keep the switch/case statements in the probe and removal function and call the init/remove functions directly in them. This is much easier.
If we need dynamic registration of handlers (which I don't see yet) for the error sources above we could do this with an acpi notify handler or so.
Without abstraction, notify handler registration seems to be overhead. I will modify code as you suggested. Thanks.
Tomasz