Hi Boris, Thanks for your comments. I will address the comments in the next version. > -----Original Message----- > From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi- > owner@xxxxxxxxxxxxxxx] On Behalf Of Borislav Petkov > Sent: 18 April 2017 17:35 > To: Shiju Jose > Cc: rjw@xxxxxxxxxxxxx; lenb@xxxxxxxxxx; mingo@xxxxxxxxxx; > prarit@xxxxxxxxxx; tbaicar@xxxxxxxxxxxxxx; punit.agrawal@xxxxxxx; > linux-acpi@xxxxxxxxxxxxxxx; james.morse@xxxxxxx; fu.wei@xxxxxxxxxx; > Guohanjun (Hanjun Guo); Gabriele Paoloni; John Garry; xuwei (O); > Zhengqiang (turing) > Subject: Re: [PATCH V2] acpi:apei:handle GSIV and GPIO notification > types > > Just nitpicks below. > > On Thu, Mar 16, 2017 at 04:08:17PM +0000, Shiju Jose wrote: > > System Controller Interrupts are received by ACPI's error device, > > which in turn notifies the GHES code. The same is true of APEI's GSIV > > and GPIO notification types. > > Add support for GSIV and GPIO sharing the SCI > > register/unregister/notifier code.Rename the list and notifier to > show > > this is no longer just SCI, but anything from the Hardware Error > > Device. > > > > Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx> > > [james.morse@xxxxxxx: rewrote commit log] > > Signed-off-by: James Morse <james.morse@xxxxxxx> > > CC: James Morse <james.morse@xxxxxxx> > > CC: Hanjun Guo <guohanjun@xxxxxxxxxx> > > Reviewed-by: James Morse <james.morse@xxxxxxx> > > --- > > drivers/acpi/apei/ghes.c | 28 +++++++++++++++++----------- > > 1 file changed, 17 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index > > b192b42..fd39929 100644 > > --- a/drivers/acpi/apei/ghes.c > > +++ b/drivers/acpi/apei/ghes.c > > @@ -89,14 +89,14 @@ > > module_param_named(disable, ghes_disable, bool, 0); > > > > /* > > - * All error sources notified with SCI shares one notifier function, > > + * All error sources notified with HED shares one notifier function, > > s/shares one notifier function/share a single notifier callback/ > > Also, let's write out here what HED stands for. Ok I will make these changes. > > > * so they need to be linked and checked one by one. This is > applied > > * to NMI too. > > This sentence wants to say "This holds true for NMIs too." I presume? Ok. I will change this sentence to "This holds true for NMIs too." > > > * > > * RCU is used for these lists, so ghes_list_mutex is only used for > > * list changing, not for traversing. > > */ > > -static LIST_HEAD(ghes_sci); > > +static LIST_HEAD(ghes_hed); > > static DEFINE_MUTEX(ghes_list_mutex); > > > > /* > > @@ -702,14 +702,14 @@ static irqreturn_t ghes_irq_func(int irq, void > *data) > > return IRQ_HANDLED; > > } > > > > -static int ghes_notify_sci(struct notifier_block *this, > > +static int ghes_notify_hed(struct notifier_block *this, > > unsigned long event, void *data) > > Align args at opening brace. Ok. I will align the args. > > > { > > struct ghes *ghes; > > int ret = NOTIFY_DONE; > > > > rcu_read_lock(); > > - list_for_each_entry_rcu(ghes, &ghes_sci, list) { > > + list_for_each_entry_rcu(ghes, &ghes_hed, list) { > > if (!ghes_proc(ghes)) > > ret = NOTIFY_OK; > > } > > @@ -718,8 +718,8 @@ static int ghes_notify_sci(struct notifier_block > *this, > > return ret; > > } > > > > -static struct notifier_block ghes_notifier_sci = { > > - .notifier_call = ghes_notify_sci, > > +static struct notifier_block ghes_notifier_hed = { > > + .notifier_call = ghes_notify_hed, > > }; > > > > #ifdef CONFIG_HAVE_ACPI_APEI_NMI > > @@ -966,6 +966,8 @@ static int ghes_probe(struct platform_device > *ghes_dev) > > case ACPI_HEST_NOTIFY_POLLED: > > case ACPI_HEST_NOTIFY_EXTERNAL: > > case ACPI_HEST_NOTIFY_SCI: > > + case ACPI_HEST_NOTIFY_GSIV: > > + case ACPI_HEST_NOTIFY_GPIO: > > break; > > <-- \n here for readability. Ok I will add new line here. > > > case ACPI_HEST_NOTIFY_NMI: > > if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) { @@ -1026,10 > +1028,12 > > @@ static int ghes_probe(struct platform_device *ghes_dev) > > } > > break; > > ditto. Ok I will add new line here. > > case ACPI_HEST_NOTIFY_SCI: > > + case ACPI_HEST_NOTIFY_GSIV: > > + case ACPI_HEST_NOTIFY_GPIO: > > mutex_lock(&ghes_list_mutex); > > - if (list_empty(&ghes_sci)) > > - register_acpi_hed_notifier(&ghes_notifier_sci); > > - list_add_rcu(&ghes->list, &ghes_sci); > > + if (list_empty(&ghes_hed)) > > + register_acpi_hed_notifier(&ghes_notifier_hed); > > + list_add_rcu(&ghes->list, &ghes_hed); > > mutex_unlock(&ghes_list_mutex); > > break; > > ditto. Ok I will add new line here. > > > case ACPI_HEST_NOTIFY_NMI: > > @@ -1068,10 +1072,12 @@ static int ghes_remove(struct platform_device > *ghes_dev) > > free_irq(ghes->irq, ghes); > > break; > > case ACPI_HEST_NOTIFY_SCI: > > + case ACPI_HEST_NOTIFY_GSIV: > > + case ACPI_HEST_NOTIFY_GPIO: > > mutex_lock(&ghes_list_mutex); > > list_del_rcu(&ghes->list); > > - if (list_empty(&ghes_sci)) > > - unregister_acpi_hed_notifier(&ghes_notifier_sci); > > + if (list_empty(&ghes_hed)) > > + unregister_acpi_hed_notifier(&ghes_notifier_hed); > > mutex_unlock(&ghes_list_mutex); > > break; > > ditto. Ok I will add new line here. > > -- > Regards/Gruss, > Boris. > Thanks, Shiju > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, > HRB 21284 (AG Nürnberg) > -- > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
