| Message ID | 87shs1sb1b.fsf@e105922-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Punit, On 10/12/2016 12:00 PM, Punit Agrawal wrote: > Tyler Baicar <tbaicar@codeaurora.org> writes: > >> ARM APEI extension proposal added SEA (Synchrounous External >> Abort) notification type for ARMv8. >> Add a new GHES error source handling function for SEA. If an error >> source's notification type is SEA, then this function can be registered >> into the SEA exception handler. That way GHES will parse and report >> SEA exceptions when they occur. >> >> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> >> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org> >> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org> > This patch fails to apply for me on v4.8. Is there a different tree this > is based on? This patch was giving me some grief as well. I'm not sure why that is because this patchset was based on the 4.8 kernel with the dependent patch for initial APEI support. > One comment below. > > [...] > >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index c8488f1..28d5a09 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -50,6 +50,10 @@ >> #include <acpi/apei.h> >> #include <asm/tlbflush.h> >> >> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >> +#include <asm/system_misc.h> >> +#endif >> + >> #include "apei-internal.h" >> >> #define GHES_PFX "GHES: " >> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = { >> .notifier_call = ghes_notify_sci, >> }; >> >> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >> +static LIST_HEAD(ghes_sea); >> + >> +static int ghes_notify_sea(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_sea, list) { >> + if (!ghes_proc(ghes)) >> + ret = NOTIFY_OK; > Not something you've introduced but it looks like ghes_proc erroneously > never returns anything other than 0. I plan to post the below fix to > address it. > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 60746ef..caea575 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes) > ghes_do_proc(ghes, ghes->estatus); > out: > ghes_clear_estatus(ghes); > - return 0; > + return rc; > } Yes, this definitely should be fixed :) Thanks, Tyler >> + } >> + rcu_read_unlock(); >> + >> + return ret; >> +} >> + > [...] >
"Baicar, Tyler" <tbaicar@codeaurora.org> writes: > Hello Punit, > > > On 10/12/2016 12:00 PM, Punit Agrawal wrote: >> Tyler Baicar <tbaicar@codeaurora.org> writes: >> >>> ARM APEI extension proposal added SEA (Synchrounous External >>> Abort) notification type for ARMv8. >>> Add a new GHES error source handling function for SEA. If an error >>> source's notification type is SEA, then this function can be registered >>> into the SEA exception handler. That way GHES will parse and report >>> SEA exceptions when they occur. >>> >>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> >>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org> >>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org> >> This patch fails to apply for me on v4.8. Is there a different tree this >> is based on? > This patch was giving me some grief as well. I'm not sure why that is > because this patchset was based on the 4.8 kernel with the dependent > patch for initial APEI support. That explains it!. I've missed out the dependency called out in the cover letter. >> One comment below. >> >> [...] >> >>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >>> index c8488f1..28d5a09 100644 >>> --- a/drivers/acpi/apei/ghes.c >>> +++ b/drivers/acpi/apei/ghes.c >>> @@ -50,6 +50,10 @@ >>> #include <acpi/apei.h> >>> #include <asm/tlbflush.h> >>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >>> +#include <asm/system_misc.h> >>> +#endif >>> + >>> #include "apei-internal.h" >>> #define GHES_PFX "GHES: " >>> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = { >>> .notifier_call = ghes_notify_sci, >>> }; >>> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA >>> +static LIST_HEAD(ghes_sea); >>> + >>> +static int ghes_notify_sea(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_sea, list) { >>> + if (!ghes_proc(ghes)) >>> + ret = NOTIFY_OK; >> Not something you've introduced but it looks like ghes_proc erroneously >> never returns anything other than 0. I plan to post the below fix to >> address it. >> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 60746ef..caea575 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes) >> ghes_do_proc(ghes, ghes->estatus); >> out: >> ghes_clear_estatus(ghes); >> - return 0; >> + return rc; >> } > Yes, this definitely should be fixed :) > > Thanks, > Tyler >>> + } >>> + rcu_read_unlock(); >>> + >>> + return ret; >>> +} >>> + >> [...] >>
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 60746ef..caea575 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes) ghes_do_proc(ghes, ghes->estatus); out: ghes_clear_estatus(ghes); - return 0; + return rc; } > + }