Re: [PATCH] PCI/AER: Consolidate CXL and native AER reporting paths

[Karolina Stolarek <karolina.stolarek@xxxxxxxxxx>]

On 19/03/2025 23:21, Bjorn Helgaas wrote:
> [+cc ACPI APEI reviewers and more CXL folks]

Thanks, it's good to get more eyeballs on this patch :)

> On Mon, Mar 17, 2025 at 10:14:46AM +0000, Karolina Stolarek wrote:
> > Make CXL devices use aer_print_error() when reporting AER errors.
> > Add a helper function to populate aer_err_info struct before logging
> > an error. Move struct aer_err_info definition to the aer.h header
> > to make it visible to CXL.
>
> Previously, pci_print_aer() was used by both CXL (via
> cxl_handle_rdport_errors()) and by ACPI GHES via aer_recover_queue()
> and aer_recover_work_func(), right?
>
> And after this patch, they would use aer_print_error() like native
> AER, native DPC, and the ACPI EDR DPC path?

That is correct.

> If it changes the GHES path, I think we should mention that in the
> subject and commit log as well.

OK, I'll make it clearer in v2.

> I think this consolidation is a good thing, because I don't think we
> should log errors differently just because we learned about them via a
> different path.
>
> But I think this also changes the text we put in dmesg, which is
> potentially disruptive to users and scripts that consume it, so I
> think we should include a comparison of the previous and new text in
> the commit log.

Like I said in a comment to the patch, I tested CXL error reporting in 
QEMU with and without my patch, and the output is the same:

[  152.090666] pcieport 0000:0c:00.0: aer_inject: Injecting errors 
00004000/00000000 into device 0000:0c:00.0
[  152.095743] pcieport 0000:0c:00.0: AER: Correctable error message 
received from 0000:0c:00.0
[  152.098676] pcieport 0000:0c:00.0: CXL Bus Error: 
severity=Correctable, type=Transaction Layer, (Receiver ID)
[  152.101576] pcieport 0000:0c:00.0:   device [8086:7075] error 
status/mask=00004000/0000a000
[  152.104012] pcieport 0000:0c:00.0:    [14] CorrIntErr

Please mind that this is coming from a kernel with Terry's patches 
applied, as I wasn't sure if and how I can inject CXL errors without them.

> Eventually would like an ack from the CXL and GHES folks before
> merging, but I have a couple more questions below.

Absolutely, many thanks for taking a look at the patch.

> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 013b869b66cb..217f13c30bde 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -885,7 +886,9 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
> >   	if (!cxl_rch_get_aer_severity(&aer_regs, &severity))
> >   		return;
> >   
> > -	pci_print_aer(pdev, severity, &aer_regs);
> > +	memset(&info, 0, sizeof(info));
> > +	populate_aer_err_info(&info, severity, &aer_regs);
>
> Maybe the memset() should go inside populate_aer_err_info() to avoid
> potential error in the callers?  I'm guessing we don't envision a case
> where "info" already contains data that needs to be preserved?

That's a good call, I can move it there. We always want to start with a 
blank slate when reporting errors.

> Both GHES and CXL start with struct aer_capability_regs from
> include/linux/aer.h, so instead of also exposing struct aer_err_info
> to the world, maybe we should have a logging interface like
> aer_recover_queue() that takes a struct aer_capability_regs pointer
> that CXL could use?  Or maybe it could use aer_recover_queue()
> directly?

Hmm, sounds good, I can try implementing it this way.

All the best,
Karolina

> > +	aer_print_error(pdev, &info);
> >   
> >   	if (severity == AER_CORRECTABLE)
> >   		cxl_handle_rdport_cor_ras(cxlds, dport);
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 9e0378fa30ac..b799c2ff7b85 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -561,30 +561,7 @@ static inline bool pci_dev_test_and_set_removed(struct pci_dev *dev)
> >   #ifdef CONFIG_PCIEAER
> >   #include <linux/aer.h>
> >   
> > -#define AER_MAX_MULTI_ERR_DEVICES	5	/* Not likely to have more */
> > -
> > -struct aer_err_info {
> > -	struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
> > -	int error_dev_num;
> > -
> > -	unsigned int id:16;
> > -
> > -	unsigned int severity:2;	/* 0:NONFATAL | 1:FATAL | 2:COR */
> > -	unsigned int __pad1:5;
> > -	unsigned int multi_error_valid:1;
> > -
> > -	unsigned int first_error:5;
> > -	unsigned int __pad2:2;
> > -	unsigned int tlp_header_valid:1;
> > -
> > -	unsigned int status;		/* COR/UNCOR Error Status */
> > -	unsigned int mask;		/* COR/UNCOR Error Mask */
> > -	struct pcie_tlp_log tlp;	/* TLP Header */
> > -};
> > -
> >   int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
> > -void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> > -
> >   int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
> >   		      unsigned int tlp_len, bool flit,
> >   		      struct pcie_tlp_log *log);
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index a1cf8c7ef628..411450ff981e 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -760,47 +760,33 @@ int cper_severity_to_aer(int cper_severity)
> >   EXPORT_SYMBOL_GPL(cper_severity_to_aer);
> >   #endif
> >   
> > -void pci_print_aer(struct pci_dev *dev, int aer_severity,
> > -		   struct aer_capability_regs *aer)
> > +void populate_aer_err_info(struct aer_err_info *info, int aer_severity,
> > +			   struct aer_capability_regs *regs)
> >   {
> > -	int layer, agent, tlp_header_valid = 0;
> > -	u32 status, mask;
> > -	struct aer_err_info info;
> > +	int tlp_header_valid;
> > +
> > +	info->severity = aer_severity;
> > +	info->first_error = PCI_ERR_CAP_FEP(regs->cap_control);
> >   
> >   	if (aer_severity == AER_CORRECTABLE) {
> > -		status = aer->cor_status;
> > -		mask = aer->cor_mask;
> > +		info->id = regs->cor_err_source;
> > +		info->status = regs->cor_status;
> > +		info->mask = regs->cor_mask;
> >   	} else {
> > -		status = aer->uncor_status;
> > -		mask = aer->uncor_mask;
> > -		tlp_header_valid = status & AER_LOG_TLP_MASKS;
> > +		info->id = regs->uncor_err_source;
> > +		info->status = regs->uncor_status;
> > +		info->mask = regs->uncor_mask;
> > +		tlp_header_valid = info->status & AER_LOG_TLP_MASKS;
> > +
> > +		if (tlp_header_valid) {
> > +			info->tlp_header_valid = tlp_header_valid;
> > +			info->tlp = regs->header_log;
> > +		}
> >   	}
> > +}
> > +EXPORT_SYMBOL_NS_GPL(populate_aer_err_info, "CXL");
> >   
> > -	layer = AER_GET_LAYER_ERROR(aer_severity, status);
> > -	agent = AER_GET_AGENT(aer_severity, status);
> > -
> > -	memset(&info, 0, sizeof(info));
> > -	info.severity = aer_severity;
> > -	info.status = status;
> > -	info.mask = mask;
> > -	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
> > -
> > -	pci_err(dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
> > -	__aer_print_error(dev, &info);
> > -	pci_err(dev, "aer_layer=%s, aer_agent=%s\n",
> > -		aer_error_layer[layer], aer_agent_string[agent]);
> > -
> > -	if (aer_severity != AER_CORRECTABLE)
> > -		pci_err(dev, "aer_uncor_severity: 0x%08x\n",
> > -			aer->uncor_severity);
> > -
> > -	if (tlp_header_valid)
> > -		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
> >   
> > -	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> > -			aer_severity, tlp_header_valid, &aer->header_log);
> > -}
> > -EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
> >   
> >   /**
> >    * add_error_device - list device to be handled
> > @@ -1136,6 +1122,7 @@ static void aer_recover_work_func(struct work_struct *work)
> >   {
> >   	struct aer_recover_entry entry;
> >   	struct pci_dev *pdev;
> > +	struct aer_err_info info;
> >   
> >   	while (kfifo_get(&aer_recover_ring, &entry)) {
> >   		pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus,
> > @@ -1146,7 +1133,10 @@ static void aer_recover_work_func(struct work_struct *work)
> >   			       PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));
> >   			continue;
> >   		}
> > -		pci_print_aer(pdev, entry.severity, entry.regs);
> > +
> > +		memset(&info, 0, sizeof(info));
> > +		populate_aer_err_info(&info, entry.severity, entry.regs);
> > +		aer_print_error(pdev, &info);
> >   
> >   		/*
> >   		 * Memory for aer_capability_regs(entry.regs) is being
> > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > index 02940be66324..ab408ec18e85 100644
> > --- a/include/linux/aer.h
> > +++ b/include/linux/aer.h
> > @@ -53,6 +53,26 @@ struct aer_capability_regs {
> >   	u16 uncor_err_source;
> >   };
> >   
> > +#define AER_MAX_MULTI_ERR_DEVICES	5	/* Not likely to have more */
> > +struct aer_err_info {
> > +	struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
> > +	int error_dev_num;
> > +
> > +	unsigned int id:16;
> > +
> > +	unsigned int severity:2;	/* 0:NONFATAL | 1:FATAL | 2:COR */
> > +	unsigned int __pad1:5;
> > +	unsigned int multi_error_valid:1;
> > +
> > +	unsigned int first_error:5;
> > +	unsigned int __pad2:2;
> > +	unsigned int tlp_header_valid:1;
> > +
> > +	unsigned int status;		/* COR/UNCOR Error Status */
> > +	unsigned int mask;		/* COR/UNCOR Error Mask */
> > +	struct pcie_tlp_log tlp;	/* TLP Header */
> > +};
> > +
> >   #if defined(CONFIG_PCIEAER)
> >   int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
> >   int pcie_aer_is_native(struct pci_dev *dev);
> > @@ -64,8 +84,9 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev)
> >   static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
> >   #endif
> >   
> > -void pci_print_aer(struct pci_dev *dev, int aer_severity,
> > -		    struct aer_capability_regs *aer);
> > +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> > +void populate_aer_err_info(struct aer_err_info *info, int aer_severity,
> > +			   struct aer_capability_regs *regs);
> >   int cper_severity_to_aer(int cper_severity);
> >   void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
> >   		       int severity, struct aer_capability_regs *aer_regs);
> > --
> > 2.43.5