| Message ID | 20240522150839.27578-1-Smita.KoralahalliChannabasappa@amd.com |
|---|---|
| Headers | show |
| Series | acpi/ghes, cper, cxl: Trace FW-First CXL Protocol Errors | expand |
On 5/22/24 8:08 AM, Smita Koralahalli wrote: > In preparation to add tracepoint support, move protocol error UUID > definition to a common location and make CXL RAS capability struct > global for use across different modules. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/firmware/efi/cper_cxl.c | 11 ----------- > drivers/firmware/efi/cper_cxl.h | 7 ++----- > include/linux/cper.h | 4 ++++ > include/linux/cxl-event.h | 11 +++++++++++ > 4 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/drivers/firmware/efi/cper_cxl.c b/drivers/firmware/efi/cper_cxl.c > index a55771b99a97..4fd8d783993e 100644 > --- a/drivers/firmware/efi/cper_cxl.c > +++ b/drivers/firmware/efi/cper_cxl.c > @@ -18,17 +18,6 @@ > #define PROT_ERR_VALID_DVSEC BIT_ULL(5) > #define PROT_ERR_VALID_ERROR_LOG BIT_ULL(6) > > -/* CXL RAS Capability Structure, CXL v3.0 sec 8.2.4.16 */ > -struct cxl_ras_capability_regs { > - u32 uncor_status; > - u32 uncor_mask; > - u32 uncor_severity; > - u32 cor_status; > - u32 cor_mask; > - u32 cap_control; > - u32 header_log[16]; > -}; > - > static const char * const prot_err_agent_type_strs[] = { > "Restricted CXL Device", > "Restricted CXL Host Downstream Port", > diff --git a/drivers/firmware/efi/cper_cxl.h b/drivers/firmware/efi/cper_cxl.h > index 86bfcf7909ec..6f8c00495708 100644 > --- a/drivers/firmware/efi/cper_cxl.h > +++ b/drivers/firmware/efi/cper_cxl.h > @@ -7,14 +7,11 @@ > * Author: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > */ > > +#include <linux/cxl-event.h> > + > #ifndef LINUX_CPER_CXL_H > #define LINUX_CPER_CXL_H > > -/* CXL Protocol Error Section */ > -#define CPER_SEC_CXL_PROT_ERR \ > - GUID_INIT(0x80B9EFB4, 0x52B5, 0x4DE3, 0xA7, 0x77, 0x68, 0x78, \ > - 0x4B, 0x77, 0x10, 0x48) > - > #pragma pack(1) > > /* Compute Express Link Protocol Error Section, UEFI v2.10 sec N.2.13 */ > diff --git a/include/linux/cper.h b/include/linux/cper.h > index 265b0f8fc0b3..5c6d4d5b9975 100644 > --- a/include/linux/cper.h > +++ b/include/linux/cper.h > @@ -89,6 +89,10 @@ enum { > #define CPER_NOTIFY_DMAR \ > GUID_INIT(0x667DD791, 0xC6B3, 0x4c27, 0x8A, 0x6B, 0x0F, 0x8E, \ > 0x72, 0x2D, 0xEB, 0x41) > +/* CXL Protocol Error Section */ > +#define CPER_SEC_CXL_PROT_ERR \ > + GUID_INIT(0x80B9EFB4, 0x52B5, 0x4DE3, 0xA7, 0x77, 0x68, 0x78, \ > + 0x4B, 0x77, 0x10, 0x48) > > /* CXL Event record UUIDs are formatted as GUIDs and reported in section type */ > /* > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h > index 60b25020281f..f11e52ff565a 100644 > --- a/include/linux/cxl-event.h > +++ b/include/linux/cxl-event.h > @@ -154,6 +154,17 @@ struct cxl_cper_event_rec { > union cxl_event event; > } __packed; > > +/* CXL RAS Capability Structure, CXL v3.0 sec 8.2.4.16 */ > +struct cxl_ras_capability_regs { > + u32 uncor_status; > + u32 uncor_mask; > + u32 uncor_severity; > + u32 cor_status; > + u32 cor_mask; > + u32 cap_control; > + u32 header_log[16]; > +}; > + > struct cxl_cper_work_data { > enum cxl_event_type event_type; > struct cxl_cper_event_rec rec;
On Wed, May 22, 2024 at 03:08:36PM +0000, Smita Koralahalli wrote: > In preparation to add tracepoint support, move protocol error UUID > definition to a common location and make CXL RAS capability struct > global for use across different modules. Reviewed-by: Alison Schofield <alison.schofield@intel.com> > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > --- > drivers/firmware/efi/cper_cxl.c | 11 ----------- > drivers/firmware/efi/cper_cxl.h | 7 ++----- > include/linux/cper.h | 4 ++++ > include/linux/cxl-event.h | 11 +++++++++++ > 4 files changed, 17 insertions(+), 16 deletions(-) > snip to end
Hi Dave, On 5/22/2024 10:59 AM, Dave Jiang wrote: > > > On 5/22/24 8:08 AM, 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@amd.com> >> --- >> drivers/acpi/apei/ghes.c | 10 +++++ >> drivers/firmware/efi/cper_cxl.c | 66 +++++++++++++++++++++++++++++++++ >> include/linux/cxl-event.h | 26 +++++++++++++ >> 3 files changed, 102 insertions(+) >> [snip] >> + * 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) { > > Perhaps define an enum CXL_AGENT_TYPE_MAX instead of 0x7 magic number? Otherwise if a new type is introduced, it would break this code. Agreed. I will define a boolean array indexed by agent type as suggested by Alison. That would avoid all these comparisons and not worry about breaking code in future. > >> + 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; >> + } > > Up to you if you want to do this or not, but maybe: > > if (prot_err->agent_type >= CXL_AGENT_TYPE_MAX || prot_err->agent_type == RCH_DP) { > pr_warn(...); > return -EINVAL; > } > > p_err->segment = ...; > p_err->bus = ...; Noted. > ... > > Although perhaps a helper function cxl_cper_valid_agent_type() that checks invalid agent type by checking the valid_bits, the agent_type boundary, and if agent_type != RCH_DP? Okay. >> + >> + 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) > > prot_err->agent_type > FM_LD? Although maybe it would be a clearer read if a helper function is defined to identify the agent types such as cxl_cper_prot_err_serial_needed() or cxl_cper_prot_agent_type_device() and with it a switch statement to explicitly identify all the agent types that require serial number. If a future device is defined, the > 0x4 logic may break. Probably helper function is not required if boolean array is defined? What do you think? Thanks, Smita [snip]
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@amd.com> >> --- >> 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 */ > . > . > . > }; > > 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 >
On 5/22/2024 12:42 PM, Dave Jiang wrote: > > > On 5/22/24 8:08 AM, Smita Koralahalli wrote: >> Refactor computation of cxlds to a common function get_cxl_dev() and reuse >> the function in both cxl_handle_cper_event() and cxl_handle_prot_err(). > > I think just introduce the function where you open coded it instead of adding code and then deleting them in the same series. Okay refactor first, then add trace support. Will change. >> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> >> --- >> drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------ >> 1 file changed, 26 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >> index 3e3c36983686..26e65e5b68cb 100644 >> --- a/drivers/cxl/pci.c >> +++ b/drivers/cxl/pci.c >> @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = { >> }, >> }; >> >> +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device, >> + u8 function) > > get_cxlds() or get_cxl_device_state() would be better. Okay will change > > DJ > Thanks Smita >> +{ >> + struct pci_dev *pdev __free(pci_dev_put) = NULL; >> + struct cxl_dev_state *cxlds; >> + unsigned int devfn; >> + >> + devfn = PCI_DEVFN(device, function); >> + pdev = pci_get_domain_bus_and_slot(segment, bus, devfn); >> + >> + if (!pdev) >> + return NULL; >> + >> + guard(device)(&pdev->dev); >> + if (pdev->driver != &cxl_pci_driver) >> + return NULL; >> + >> + cxlds = pci_get_drvdata(pdev); >> + >> + return cxlds; >> +} >> + >> #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0) >> static void cxl_handle_cper_event(enum cxl_event_type ev_type, >> struct cxl_cper_event_rec *rec) >> { >> struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; >> - struct pci_dev *pdev __free(pci_dev_put) = NULL; >> enum cxl_event_log_type log_type; >> struct cxl_dev_state *cxlds; >> - unsigned int devfn; >> u32 hdr_flags; >> >> pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type, >> device_id->segment_num, device_id->bus_num, >> device_id->device_num, device_id->func_num); >> >> - devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); >> - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, >> - device_id->bus_num, devfn); >> - if (!pdev) >> - return; >> - >> - guard(device)(&pdev->dev); >> - if (pdev->driver != &cxl_pci_driver) >> - return; >> - >> - cxlds = pci_get_drvdata(pdev); >> + cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num, >> + device_id->device_num, device_id->func_num); >> if (!cxlds) >> return; >> >> @@ -1013,21 +1024,10 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type, >> >> static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err) >> { >> - struct pci_dev *pdev __free(pci_dev_put) = NULL; >> struct cxl_dev_state *cxlds; >> - unsigned int devfn; >> >> - devfn = PCI_DEVFN(p_err->device, p_err->function); >> - pdev = pci_get_domain_bus_and_slot(p_err->segment, >> - p_err->bus, devfn); >> - if (!pdev) >> - return; >> - >> - guard(device)(&pdev->dev); >> - if (pdev->driver != &cxl_pci_driver) >> - return; >> - >> - cxlds = pci_get_drvdata(pdev); >> + cxlds = get_cxl_dev(p_err->segment, p_err->bus, >> + p_err->device, p_err->function); >> if (!cxlds) >> return; >>
On 5/23/24 2:19 PM, Smita Koralahalli wrote: > Hi Dave, > > On 5/22/2024 10:59 AM, Dave Jiang wrote: >> >> >> On 5/22/24 8:08 AM, 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@amd.com> >>> --- >>> drivers/acpi/apei/ghes.c | 10 +++++ >>> drivers/firmware/efi/cper_cxl.c | 66 +++++++++++++++++++++++++++++++++ >>> include/linux/cxl-event.h | 26 +++++++++++++ >>> 3 files changed, 102 insertions(+) >>> > > [snip] > > >>> + * 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) { >> >> Perhaps define an enum CXL_AGENT_TYPE_MAX instead of 0x7 magic number? Otherwise if a new type is introduced, it would break this code. > > Agreed. I will define a boolean array indexed by agent type as suggested by Alison. That would avoid all these comparisons and not worry about breaking code in future. > >> >>> + 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; >>> + } >> >> Up to you if you want to do this or not, but maybe: >> >> if (prot_err->agent_type >= CXL_AGENT_TYPE_MAX || prot_err->agent_type == RCH_DP) { >> pr_warn(...); >> return -EINVAL; >> } >> >> p_err->segment = ...; >> p_err->bus = ...; > > Noted. > >> ... >> >> Although perhaps a helper function cxl_cper_valid_agent_type() that checks invalid agent type by checking the valid_bits, the agent_type boundary, and if agent_type != RCH_DP? > > Okay. > >>> + >>> + 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) >> >> prot_err->agent_type > FM_LD? Although maybe it would be a clearer read if a helper function is defined to identify the agent types such as cxl_cper_prot_err_serial_needed() or cxl_cper_prot_agent_type_device() and with it a switch statement to explicitly identify all the agent types that require serial number. If a future device is defined, the > 0x4 logic may break. > > Probably helper function is not required if boolean array is defined? What do you think? That works for me. My main concern is to clarify the code and remove possibility of breakage from future changes. > > Thanks, > Smita > > [snip]
On Wed, 22 May 2024 15:08:36 +0000 Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote: > In preparation to add tracepoint support, move protocol error UUID > definition to a common location and make CXL RAS capability struct > global for use across different modules. > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> FWIW given already well reviewed. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Thu, 23 May 2024 14:21:40 -0700 Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> 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@amd.com> > >> --- > >> 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 > >
On Wed, 22 May 2024 15:08:39 +0000 Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote: > Refactor computation of cxlds to a common function get_cxl_dev() and reuse > the function in both cxl_handle_cper_event() and cxl_handle_prot_err(). > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > --- > drivers/cxl/pci.c | 52 +++++++++++++++++++++++------------------------ > 1 file changed, 26 insertions(+), 26 deletions(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 3e3c36983686..26e65e5b68cb 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -974,32 +974,43 @@ static struct pci_driver cxl_pci_driver = { > }, > }; > > +static struct cxl_dev_state *get_cxl_dev(u16 segment, u8 bus, u8 device, > + u8 function) I'd not expect a function with this name to return anything other than a struct device * get_cxl_devstate() maybe? > +{ > + struct pci_dev *pdev __free(pci_dev_put) = NULL; > + struct cxl_dev_state *cxlds; > + unsigned int devfn; > + > + devfn = PCI_DEVFN(device, function); Might as well do unsigned int devfn = PCI_DEVFN(device, function); > + pdev = pci_get_domain_bus_and_slot(segment, bus, devfn); struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(segment, bus, devfn); see fwctl thread for a reference to Linus expressing a preference for keeping construct and destructor definitions together even when that means relaxing c conventions that we are so used to! Obviously this is copied from below, but might as well tidy it up whilst here. However, do the devfn change above and it is in the definitions block... > + > + if (!pdev) > + return NULL; > + > + guard(device)(&pdev->dev); > + if (pdev->driver != &cxl_pci_driver) > + return NULL; > + > + cxlds = pci_get_drvdata(pdev); > + > + return cxlds; return pci_get_drvdata(pdev); I think the function is small enough having cxlds just so it's obvious what is being returned is unnecessary. > +} > + > #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0) > static void cxl_handle_cper_event(enum cxl_event_type ev_type, > struct cxl_cper_event_rec *rec) > { > struct cper_cxl_event_devid *device_id = &rec->hdr.device_id; > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > enum cxl_event_log_type log_type; > struct cxl_dev_state *cxlds; > - unsigned int devfn; > u32 hdr_flags; > > pr_debug("CPER event %d for device %u:%u:%u.%u\n", ev_type, > device_id->segment_num, device_id->bus_num, > device_id->device_num, device_id->func_num); > > - devfn = PCI_DEVFN(device_id->device_num, device_id->func_num); > - pdev = pci_get_domain_bus_and_slot(device_id->segment_num, > - device_id->bus_num, devfn); > - if (!pdev) > - return; > - > - guard(device)(&pdev->dev); > - if (pdev->driver != &cxl_pci_driver) > - return; > - > - cxlds = pci_get_drvdata(pdev); > + cxlds = get_cxl_dev(device_id->segment_num, device_id->bus_num, > + device_id->device_num, device_id->func_num); > if (!cxlds) > return; > > @@ -1013,21 +1024,10 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type, > > static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err) > { > - struct pci_dev *pdev __free(pci_dev_put) = NULL; > struct cxl_dev_state *cxlds; > - unsigned int devfn; > > - devfn = PCI_DEVFN(p_err->device, p_err->function); > - pdev = pci_get_domain_bus_and_slot(p_err->segment, > - p_err->bus, devfn); > - if (!pdev) > - return; > - > - guard(device)(&pdev->dev); > - if (pdev->driver != &cxl_pci_driver) > - return; > - > - cxlds = pci_get_drvdata(pdev); > + cxlds = get_cxl_dev(p_err->segment, p_err->bus, > + p_err->device, p_err->function); > if (!cxlds) > return; >
On 6/7/2024 8:14 AM, Jonathan Cameron wrote: > On Wed, 22 May 2024 15:08:36 +0000 > Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote: > >> In preparation to add tracepoint support, move protocol error UUID >> definition to a common location and make CXL RAS capability struct >> global for use across different modules. >> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> > FWIW given already well reviewed. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Sorry I missed your reviewed by tag. Thanks for reviewing my patches! Thanks Smita
On 6/7/2024 8:26 AM, Jonathan Cameron wrote: > On Thu, 23 May 2024 14:21:40 -0700 > Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> 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@amd.com> [snip] >>>> + * 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. > Okay. I was considering 2D array without naming before. Something like: static const bool agent_requires_id_address[][] = { /* Device_ID, Serial_num */ {true, true}, /* RCD */ {false, false}, /* RCH_DP */ Let me change to array of structures if naming helps. Thanks, Smita [snip]