Re: [PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.

[Robert Richter <rric@kernel.org>]

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.

-Robert