Re: [PATCH v2 3/4] acpi/ghes, efi/cper: Recognize and process CXL Protocol Errors.

[Dan Williams]

Smita Koralahalli wrote:
> On 10/2/2024 4:47 PM, Dan Williams wrote:
> > Smita Koralahalli wrote:
> >> UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
> >>
> >> Add GHES support to detect CXL CPER Protocol Error Record and Cache Error
> >> Severity, Device ID, Device Serial number and CXL RAS capability struct in
> >> struct cxl_cper_prot_err. Include this struct as a member of struct
> >> cxl_cper_work_data.
> >>
> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@xxxxxxx>
> >> ---
> >> v2:
> >> 	Defined array of structures for Device ID and Serial number
> >> 	comparison.
> >> 	p_err -> rec/p_rec.
> >> ---
> >>   drivers/acpi/apei/ghes.c        |  10 +++
> >>   drivers/firmware/efi/cper_cxl.c | 115 ++++++++++++++++++++++++++++++++
> >>   include/cxl/event.h             |  26 ++++++++
> >>   3 files changed, 151 insertions(+)
> >>
[..]
> >> +static enum cxl_aer_err_type cper_severity_cxl_aer(int cper_severity)
> >> +{
> >> +	switch (cper_severity) {
> >> +	case CPER_SEV_RECOVERABLE:
> >> +	case CPER_SEV_FATAL:
> >> +		return CXL_AER_UNCORRECTABLE;
> >> +	default:
> >> +		return CXL_AER_CORRECTABLE;
> >> +	}
> > 
> > Why does the CPER severity need to be converted to a new CXL specific
> > enum value?
> 
> I was just following up the same convention here as done for AER..
> cper_severity_to_aer(). I can change if there is no value in doing it.

I think because PCIe and CXL are both using AER as the transport they
can both use cper_severity_to_aer(), or at a minimum do not introduce
'enum cxl_aer_err_type' and instead use the existing AER_ values.

[..]
> > All CPER records without a device-id have already been dropped above, so
> > why reject agent-types that do not require a device-id here?
> > 
> > I think this agent_info[] scheme makes the code more difficult to read
> > especially since agent_info() is only consulted a couple times. Just put
> > a "switch (prot_err->agent_type)" in the code directly and skip the
> > indirection.
> 
> Hmm, I initially thought I would do switch case and then changed it to 
> if-else thinking that would look cleaner.
> 
> What would you suggest? Just incorporate switch case similar to 
> cper_print_prot_err() here as well or clean up switch case in 
> cper_print_prot_err() and reuse the agent_info[] there?

Even though it is more lines, I find cper_print_prot_err() easier to
read because I do not need to switch back and forth to the agent_info
definition.

> This agent_info[] would include 3 fields then, two as above and another 
> for valid_cap. Maybe unify sbdf with valid_cap..

agent_info[] ends up being code-logic masquerading as a data-structure.
Keep the logic all in one coherent readable block.

[..]
> > Hmm, 'struct cxl_cper_event_rec' follows the raw format of the record
> > from the specification, and 'struct cxl_cper_sec_prot_err' (formerly
> > cper_sec_prot_err) already exists, so why is this new intermediate data
> > structure needed?
> 
> Yeah, the intention was to extract only necessary info to be consumed by 
> cxl_pci driver and to be passed to cxl_cper_fifo.
> 
> Going forward, while handling protocol errors separately, I can send the 
> entire cxl_cper_sec_prot_err.

I think it defensible to send all of it. Recall that the real consumer
is not cxl_pci it is rasdaemon in userspace. The kernel is not in a good
position to filter what rasdaemon expects to find in the record based on
the EFI specification.