Re: [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8

[Punit Agrawal <punit.agrawal@arm.com>]

"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;
>>> +}
>>> +
>> [...]
>>