Re: [PATCH v2 4/4] acpi/ghes, cxl/pci: Trace FW-First CXL Protocol Errors

[Smita Koralahalli]

On 10/1/2024 8:52 AM, Ira Weiny wrote:
> Smita Koralahalli wrote:
> > When PCIe AER is in FW-First, OS should process CXL Protocol errors from
> > CPER records.
> >
> > Reuse the existing work queue cxl_cper_work registered with GHES to notify
> > the CXL subsystem on a Protocol error.
> >
> > The defined trace events cxl_aer_uncorrectable_error and
> > cxl_aer_correctable_error currently trace native CXL AER errors. Reuse
> > them to trace FW-First Protocol Errors.
> >
> > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@xxxxxxx>
> > ---
> > v2:
> > 	Removed pr_warn for serial number.
> > 	p_err -> rec/p_rec.
> > ---
> >   drivers/acpi/apei/ghes.c | 14 ++++++++++++++
> >   drivers/cxl/core/pci.c   | 24 ++++++++++++++++++++++++
> >   drivers/cxl/cxlpci.h     |  3 +++
> >   drivers/cxl/pci.c        | 20 ++++++++++++++++++--
> >   include/cxl/event.h      |  1 +
> >   5 files changed, 60 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index 9dcf0f78458f..5082885e1f2c 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
> >   
> >   	if (cxl_cper_handle_prot_err_info(gdata, &wd.p_rec))
> >   		return;
> > +
> > +	guard(spinlock_irqsave)(&cxl_cper_work_lock);
> > +
> > +	if (!cxl_cper_work)
> > +		return;
> > +
> > +	wd.event_type = CXL_CPER_EVENT_PROT_ERR;
> > +
> > +	if (!kfifo_put(&cxl_cper_fifo, wd)) {
> > +		pr_err_ratelimited("CXL CPER kfifo overflow\n");
> > +		return;
> > +	}
> > +
> > +	schedule_work(cxl_cper_work);
> >   }
> >   
> >   int cxl_cper_register_work(struct work_struct *work)
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 5b46bc46aaa9..39ef24c8991f 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -650,6 +650,30 @@ void read_cdat_data(struct cxl_port *port)
> >   }
> >   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> >   
> > +void cxl_trace_prot_err(struct cxl_dev_state *cxlds,
> > +			struct cxl_cper_prot_err *rec)
> > +{
> > +	u32 status, fe;
> > +
> > +	if (rec->severity == CXL_AER_CORRECTABLE) {
> > +		status = rec->cxl_ras.cor_status & ~rec->cxl_ras.cor_mask;
> > +
> > +		trace_cxl_aer_correctable_error(cxlds->cxlmd, status);
> > +	} else {
> > +		status = rec->cxl_ras.uncor_status & ~rec->cxl_ras.uncor_mask;
> > +
> > +		if (hweight32(status) > 1)
> > +			fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
> > +					   rec->cxl_ras.cap_control));
> > +		else
> > +			fe = status;
> > +
> > +		trace_cxl_aer_uncorrectable_error(cxlds->cxlmd, status, fe,
> > +						  rec->cxl_ras.header_log);
> > +	}
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_trace_prot_err, CXL);
> > +
> >   static void __cxl_handle_cor_ras(struct cxl_dev_state *cxlds,
> >   				 void __iomem *ras_base)
> >   {
> > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > index 4da07727ab9c..8acd8f2c39c9 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -129,4 +129,7 @@ void read_cdat_data(struct cxl_port *port);
> >   void cxl_cor_error_detected(struct pci_dev *pdev);
> >   pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> >   				    pci_channel_state_t state);
> > +struct cxl_cper_prot_err;
> > +void cxl_trace_prot_err(struct cxl_dev_state *cxlds,
> > +			struct cxl_cper_prot_err *rec);
> >   #endif /* __CXL_PCI_H__ */
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 915102f5113f..0a29321921a0 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1064,12 +1064,28 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
> >   			       &uuid_null, &rec->event);
> >   }
> >   
> > +static void cxl_handle_prot_err(struct cxl_cper_prot_err *rec)
> > +{
> > +	struct cxl_dev_state *cxlds;
> > +
> > +	cxlds = get_cxl_devstate(rec->segment, rec->bus,
> > +				 rec->device, rec->function);
>
> As mentioned in 2/4 I don't think this is going to work correctly.
>
> > +	if (!cxlds)
> > +		return;
> > +
> > +	cxl_trace_prot_err(cxlds, rec);
> > +}
> > +
> >   static void cxl_cper_work_fn(struct work_struct *work)
> >   {
> >   	struct cxl_cper_work_data wd;
> >   
> > -	while (cxl_cper_kfifo_get(&wd))
> > -		cxl_handle_cper_event(wd.event_type, &wd.rec);
> > +	while (cxl_cper_kfifo_get(&wd)) {
> > +		if (wd.event_type == CXL_CPER_EVENT_PROT_ERR)
> > +			cxl_handle_prot_err(&wd.p_rec);
> > +		else
> > +			cxl_handle_cper_event(wd.event_type, &wd.rec);
>
> Why does this need to change?
>
> Have cxl_handle_cper_event() decode the BDF as it does now and call
> cxl_handle_prot_err() there?  I think you could drop patch 2/4 entirely
> and not have to duplicate the logic.
>
> Ira

Yeah works for me. My intention was to not disturb the 
cxl_handle_cper_event() as that was focussed on handling device errors.

Thanks
Smita
> > +	}
> >   }
> >   static DECLARE_WORK(cxl_cper_work, cxl_cper_work_fn);
> >   
> > diff --git a/include/cxl/event.h b/include/cxl/event.h
> > index 5b316150556a..d854d8c435db 100644
> > --- a/include/cxl/event.h
> > +++ b/include/cxl/event.h
> > @@ -115,6 +115,7 @@ struct cxl_event_record_raw {
> >   } __packed;
> >   
> >   enum cxl_event_type {
> > +	CXL_CPER_EVENT_PROT_ERR,
> >   	CXL_CPER_EVENT_GENERIC,
> >   	CXL_CPER_EVENT_GEN_MEDIA,
> >   	CXL_CPER_EVENT_DRAM,
> > --
> > 2.17.1