Re: [PATCH 44/60] ACPI, APEI, Add ERST record ID cache

[Ingo Molnar <mingo@elte.hu>]

* Len Brown <lenb@kernel.org> wrote:

> From: Huang Ying <ying.huang@intel.com>
> 
> APEI ERST firmware interface and implementation has no multiple users
> in mind. For example, there is four records in storage with ID: 1, 2,
> 3 and 4, if two ERST readers enumerate the records via
> GET_NEXT_RECORD_ID as follow,
> 
> reader 1		reader 2
> 1
> 			2
> 3
> 			4
> -1
> 			-1
> 
> where -1 signals there is no more record ID.
> 
> Reader 1 has no chance to check record 2 and 4, while reader 2 has no
> chance to check record 1 and 3. And any other GET_NEXT_RECORD_ID will
> return -1, that is, other readers will has no chance to check any
> record even they are not cleared by anyone.
> 
> This makes raw GET_NEXT_RECORD_ID not suitable for usage of multiple
> users.
> 
> To solve the issue, an in memory ERST record ID cache is designed and
> implemented. When enumerating record ID, the ID returned by
> GET_NEXT_RECORD_ID is added into cache in addition to be returned to
> caller. So other readers can check the cache to get all record ID
> available.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce-apei.c |   42 ++++---
>  drivers/acpi/apei/erst-dbg.c          |   24 +++-
>  drivers/acpi/apei/erst.c              |  241 +++++++++++++++++++++++++++------
>  include/acpi/apei.h                   |    5 +-
>  4 files changed, 247 insertions(+), 65 deletions(-)

NAK on similar grounds.

The code here is exporting all sorts of lowlevel hw error event details to 
user-space, in a vendor specific form, without _ANY_ effort whatsoever at 
generalizing it, merging it with the existing EDAC code, extending EDAC so that both 
can offer similar functionality, etc. etc.

And before you say that "it's for debug only": the direction of these patches 
suggests that this exported data will be used by user-space as an ad-hoc event 
interface, resulting in a de-facto ABI later on.

(Or if it will not be used by user-space then it should not be merged upstream.)

There's EDAC code that tries to do all this correctly, there was even an RFC 
user-space EDAC tool posted recently that uses the perf syscall to receive error 
events, etc. This code goes into debugfs and adds Yet Another Random ABI instead of 
using (or extending) existing facilities of structured event logging.

NAKed-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo