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

[James Morse <james.morse@arm.com>]

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