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

Tomasz Nowicki
2014-06-24T09:06:21Z

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