Message ID | 86258A5CC0A3704780874CF6004BA8A62DCB6D39@lhreml502-mbx (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
On 13/03/17 13:16, 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. Reviewed-by: James Morse <james.morse@arm.com> ... what looks like an existing bug: > 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 > @@ -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); Suspiciously, there is no synchronize_rcu() between this list_del_rcu() and the kfree(ghes) at the bottom of the function. It looks like 81e88fdc432a lifted it into the NOTIFY_NMI path. I will send a separate fix. > - 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: > Thanks, James -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi James, > -----Original Message----- > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- > owner@vger.kernel.org] On Behalf Of James Morse > Sent: 16 March 2017 14:26 > To: Shiju Jose > Cc: rjw@rjwysocki.net; lenb@kernel.org; bp@suse.de; mingo@kernel.org; > prarit@redhat.com; tbaicar@codeaurora.org; punit.agrawal@arm.com; > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > fu.wei@linaro.org; Guohanjun (Hanjun Guo); Gabriele Paoloni; John Garry; > xuwei (O); Zhengqiang (turing) > Subject: Re: [PATCH 1/1] acpi:apei:handle GSIV and GPIO notification > types > > On 13/03/17 13:16, 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. > > Reviewed-by: James Morse <james.morse@arm.com> > > > ... what looks like an existing bug: > > > 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 > > @@ -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); > > Suspiciously, there is no synchronize_rcu() between this list_del_rcu() > and the > kfree(ghes) at the bottom of the function. It looks like 81e88fdc432a > lifted it into the NOTIFY_NMI path. I will send a separate fix. Thanks. Got it. > > > > - 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: > > > > > Thanks, > > James Thanks, Shiju > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" > in the body of a message to majordomo@vger.kernel.org More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: