lkml.org 
[lkml]   [2014]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 3/5] acpi, apei, ghes: Introduce more generic mechanism to init/deinit GHES error notifications.
    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


    \
     
     \ /
      Last update: 2014-06-24 11:21    [W:3.799 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site