RE: {RFC PATCH v1 v4.11-rc1 1/1} acpi: apei: common handler in ghes for HW errors notified via hed(PNP0C33) driver
From: Shiju Jose
Date: Mon Mar 13 2017 - 05:41:34 EST
Hi Peter,
Thanks for the comments.
> -----Original Message-----
> From: James Morse [mailto:james.morse@xxxxxxx]
> Sent: 10 March 2017 17:16
> To: Shiju Jose
> Cc: catalin.marinas@xxxxxxx; will.deacon@xxxxxxx;
> tbaicar@xxxxxxxxxxxxxx; zjzhang@xxxxxxxxxxxxxx; marc.zyngier@xxxxxxx;
> xuwei (O); Gabriele Paoloni; John Garry; Guohanjun (Hanjun Guo);
> Zhengqiang (turing); Xiexiuqi; fu.wei@xxxxxxxxxx; wangxiongfeng (C);
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-acpi@xxxxxxxxxxxxxxx
> Subject: Re: {RFC PATCH v1 v4.11-rc1 1/1} acpi: apei: common handler in
> ghes for HW errors notified via hed(PNP0C33) driver
>
> Hi Shiju,
>
> On 07/03/17 16:07, Shiju Jose wrote:
> > Add common handler in ghes for HW errors notified via hed(PNP0C33)
> driver.
> > 1. Rename ghes_notify_sci() to ghes_notify_hed().
> > 2. Rename struct notifier_block ghes_notifier_sci to
> > struct notifier_block ghes_notifier_hed.
> > 3. Rename ghes_sci list to ghes_hed.
> > 4. Make ghes_notify_hed as common handler for
> > notification types SCI, GSIV and GPIO.
> >
>
> I think the code here is fine, but we need to put this in front of the
> ACPI maintainers, and if we can, make their job easy.
>
> How did you come up with the CC list for this patch?
> scripts/get_maintainer.pl
> lists:
> > "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > (supporter:ACPI,commit_signer:2/4=50%)
> > Len Brown <lenb@xxxxxxxxxx> (supporter:ACPI)
>
> as the maintainers for the file changed by this patch, but they weren't
> CCd.
Ok. I will post the patch CC ACPI maintainers.
>
> Did you use git format-patch to create this? All the other patches on
> the list have subjects of the form "[PATCH] acpi: apei....", the {}s
> confuse 'git am'
> meaning whoever applies this would have to edit your patch before
> applying it.
Ok. I got it. I will correct in next patch. I used git format-patch to create the patch.
>
>
> Your commit message doesn't add anything that wasn't in the subject-
> line. It should describe the reason for the change. Based on Hanjun's
> explanation I can
> offer:
> ---
> 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.
sure. I will correct the commit message.
>
> ---
>
> If you're confident you solved this the right way, (which I think we
> are), you should drop the 'RFC' from the subject. RFC indicates you
> don't think this should be merged you just want feedback.
This patch was posted for feedback. I will post the next version of the patch with RFC removed.
>
>
>
> Thanks,
>
> James
>
>
> > 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,
> > * so they need to be linked and checked one by one. This is
> applied
> > * to NMI 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) {
> > 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;
> > 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;
> > 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;
> > 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;
> > case ACPI_HEST_NOTIFY_NMI:
> >