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: > > -- 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
