[PATCH 02/11] ACPI / APEI: Generalise the estatus queue's add/remove and notify code

James Morse
2018-02-23T18:21:31Z

Hi Punit,

On 20/02/18 18:26, Punit Agrawal wrote:
> James Morse <james.morse@arm.com> writes:
>
>> To support asynchronous NMI-like notifications on arm64 we need to use
>> the estatus-queue. These patches refactor it to allow multiple APEI
>> notification types to use it.
>>
>> Refactor the estatus queue's pool grow/shrink code and notification
>> routine from NOTIFY_NMI's handlers. This will allow another notification
>> method to use the estatus queue without duplicating this code.
>>
>> This patch adds rcu_read_lock()/rcu_read_unlock() around the list
>> list_for_each_entry_rcu() walker. These aren't strictly necessary as
>> the whole nmi_enter/nmi_exit() window is a spooky RCU read-side
>> critical section.
>>
>> Keep the oops_begin() call for x86, arm64 doesn't have one of these,
>> and APEI is the only thing outside arch code calling this..
>>
>> The existing ghes_estatus_pool_shrink() is folded into the new
>> ghes_estatus_queue_shrink_pool() as only the queue uses it.
>>
>> _in_nmi_notify_one() is separate from the rcu-list walker for a later
>> caller that doesn't need to walk a list.

>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index e42b587c509b..d3cc5bd5b496 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -749,6 +749,54 @@ static void __process_error(struct ghes *ghes)
>> #endif
>> }
>>
>> +static int _in_nmi_notify_one(struct ghes *ghes)
>> +{
>> + int sev;
>> + int ret = -ENOENT;
>
> If ret is initialised to 0 ...
>
>> +
>> + if (ghes_read_estatus(ghes, 1)) {
>> + ghes_clear_estatus(ghes);
>> + return ret;
>
> and return -ENOENT here...
>
>> + } else {
>> + ret = 0;
>> + }
>
> ... then the else block can be dropped.

Good point, this happened because I was trying to keep the same shape as the
existing notify_nmi() code as far as possible.

>> +
>> + sev = ghes_severity(ghes->estatus->error_severity);
>> + if (sev >= GHES_SEV_PANIC) {
>> +#ifdef CONFIG_X86
>> + oops_begin();
>> +#endif
>
> Can you use IS_ENABLED() here as well?

I didn't think that would build without an empty declaration for arm64, I
assumed it would generate an implicit-declaration-of warning. But, I've tried
it, and evidently today's toolchain does dead-code elimination before generating
implicit-declaration-of warnings...
I'd prefer to leave this (ugly as it is), to avoid warnings on a different
version of the compiler.

Thanks,

James