On Thu, 23 May 2024 14:21:40 -0700
Smita Koralahalli <Smita.KoralahalliChannabasappa@xxxxxxx> wrote:
> Hi Alison,
>
> On 5/22/2024 5:03 PM, Alison Schofield wrote:
> > On Wed, May 22, 2024 at 03:08:37PM +0000, 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>
> >> ---
> >> drivers/acpi/apei/ghes.c | 10 +++++
> >> drivers/firmware/efi/cper_cxl.c | 66 +++++++++++++++++++++++++++++++++
> >> include/linux/cxl-event.h | 26 +++++++++++++
> >> 3 files changed, 102 insertions(+)
> >>
> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> >> index 623cc0cb4a65..1a58032770ee 100644
> >> --- a/drivers/acpi/apei/ghes.c
> >> +++ b/drivers/acpi/apei/ghes.c
> >> @@ -717,6 +717,14 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
> >> schedule_work(cxl_cper_work);
> >> }
> >>
> >> +static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
> >> +{
> >> + struct cxl_cper_work_data wd;
> >> +
> >> + if (cxl_cper_handle_prot_err_info(gdata, &wd.p_err))
> >> + return;
> >> +}
> >> +
> >> int cxl_cper_register_work(struct work_struct *work)
> >> {
> >> if (cxl_cper_work)
> >> @@ -791,6 +799,8 @@ static bool ghes_do_proc(struct ghes *ghes,
> >> struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
> >>
> >> cxl_cper_post_event(CXL_CPER_EVENT_MEM_MODULE, rec);
> >> + } else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
> >> + cxl_cper_handle_prot_err(gdata);
> >> } else {
> >> void *err = acpi_hest_get_payload(gdata);
> >>
> >> diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c
> >> index 4fd8d783993e..03b9839f3b73 100644
> >> --- a/drivers/firmware/efi/cper_cxl.c
> >> +++ b/drivers/firmware/efi/cper_cxl.c
> >> @@ -8,6 +8,7 @@
> >> */
> >>
> >> #include <linux/cper.h>
> >> +#include <acpi/ghes.h>
> >> #include "cper_cxl.h"
> >>
> >> #define PROT_ERR_VALID_AGENT_TYPE BIT_ULL(0)
> >> @@ -44,6 +45,17 @@ enum {
> >> USP, /* CXL Upstream Switch Port */
> >> };
> >>
> >> +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;
> >> + }
> >> +}
> >> +
> >> void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_err)
> >> {
> >> if (prot_err->valid_bits & PROT_ERR_VALID_AGENT_TYPE)
> >> @@ -176,3 +188,57 @@ void cper_print_prot_err(const char *pfx, const struct cper_sec_prot_err *prot_e
> >> sizeof(cxl_ras->header_log), 0);
> >> }
> >> }
> >> +
> >> +int cxl_cper_handle_prot_err_info(struct acpi_hest_generic_data *gdata,
> >> + struct cxl_cper_prot_err *p_err)
> >> +{
> >> + struct cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
> >> + u8 *dvsec_start, *cap_start;
> >> +
> >> + if (!(prot_err->valid_bits & PROT_ERR_VALID_DEVICE_ID)) {
> >> + pr_err(FW_WARN "No Device ID\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /*
> >> + * The device ID or agent address is required for CXL RCD, CXL
> >> + * SLD, CXL LD, CXL Fabric Manager Managed LD, CXL Root Port,
> >> + * CXL Downstream Switch Port and CXL Upstream Switch Port.
> >> + */
> >> + if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
> >
> > For this check against agent_type, and the similar one below, would a boolean
> > array indexed by the agent type work? That would avoid the <= 0x7 and > 0x4
> > below. It seems one array would suffice for this case, but naming it isn't obvious
> > to me. Maybe it'll be to you.
> >
> > Something similar to what is done for prot_err_agent_type_strs[]
> >
> > static const bool agent_requires_id_address_serial[] = {
> > true, /* RDC */
> > false, /* RCH_DP */
[RCD] = false,
etc rather than comments would be neater.
Given two similar things already. Maybe time for a little structure.
//with better name than this
struct agent_reqs {
bool sn;
bool sbdf;
};
static const agent_reqs agent_reqs[] = {
[RCD] = { .sn = false, .sbdf = true, },
};
etc.
Maybe just bring the the string in as well
struct agent_info {
const char *string;
bool req_sn;
bool req_sbdf;
};
static const agent_info agent_info[] = {
[RD] = {
.string = "Restricted CXL Device",
.req_sn = false,
.req_sbdf = true,
},
};
Values made up, but hopefully conveys that moving to having
all the data in one place makes it harder to forget stuff
for new entries etc.
> > .
> > .
> > .
> > };
> >
> >
>
> Noted. Will implement it this way!
>
> Thanks
> Smita
>
> >> + p_err->segment = prot_err->agent_addr.segment;
> >> + p_err->bus = prot_err->agent_addr.bus;
> >> + p_err->device = prot_err->agent_addr.device;
> >> + p_err->function = prot_err->agent_addr.function;
> >> + } else {
> >> + pr_err(FW_WARN "Invalid agent type\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (!(prot_err->valid_bits & PROT_ERR_VALID_ERROR_LOG)) {
> >> + pr_err(FW_WARN "Invalid Protocol Error log\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + dvsec_start = (u8 *)(prot_err + 1);
> >> + cap_start = dvsec_start + prot_err->dvsec_len;
> >> + p_err->cxl_ras = *(struct cxl_ras_capability_regs *)cap_start;
> >> +
> >> + /*
> >> + * Set device serial number unconditionally.
> >> + *
> >> + * Print a warning message if it is not valid. The device serial
> >> + * number is required for CXL RCD, CXL SLD, CXL LD and CXL Fabric
> >> + * Manager Managed LD.
> >> + */
> >> + if (!(prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) ||
> >> + prot_err->agent_type > 0x4 || prot_err->agent_type == RCH_DP)
> >
> > then this also can be replaced with
> > agent_requires_id_address_serial[prot_err->agent_type]
> >
> >
> > -- Alison
> >
> >
> >> + pr_warn(FW_WARN "No Device Serial number\n");
> >> +
> >> + p_err->lower_dw = prot_err->dev_serial_num.lower_dw;
> >> + p_err->upper_dw = prot_err->dev_serial_num.upper_dw;
> >> +
> >> + p_err->severity = cper_severity_cxl_aer(gdata->error_severity);
> >> +
> >> + return 0;
> >> +}
> >
> > snip
> >
