Re: [RFC] cxl: Device protocol AER injection

> > 
> > Why is this an RFC rather than a final proposal?  There should always
> > be something to give the reviewer that info in the patch description.
> > I'd actually be tempted to throw a cover letter in to have somewhere
> > out of the way to put that information.
> > 
> > Is it simply because it only makes sense once the other seris lands. 
> >   
> 
> The immediate priority was providing a testing procedure for the v18 series.
> I wasn't sure how the design would be received. For instance, the to_einj_ras_base() 
> could be moved into cxl-test as a to_ras_base() mock implemented function or 
> it could remain in ras.c (or maybe even ras_einj.c) outside of cxl-mock. I'm 
> looking forward to Alison's review and comments. 
> 
> My preference is to introduce core/ras_einj,c and add these changes. This 
> would be to isolate all the changes except for: cxl_ras_einj_init(), cxl_ras_einj_exit(), 
> and to_einj_ras_base(). I've started moving forward with making these changes 
> knowing the direction could change once this receives more reviews.
> 
> Also worth discussing is the commandline takes multiple parameters for
> a single sysfs file which I know isn't acceptable by everyone. I personally 
> like the interface as-is because its simpler to use in comparison to multiple 
> files that must be set individually.

It is debugfs so rules are much more flexible than sysfs.




> >> ---
> >>  drivers/cxl/Kconfig           |  13 +++
> >>  drivers/cxl/core/core.h       |  21 ++++
> >>  drivers/cxl/core/port.c       |   2 +-
> >>  drivers/cxl/core/ras.c        | 208 ++++++++++++++++++++++++++++++++++
> >>  drivers/cxl/core/ras_rch.c    |  12 ++
> >>  drivers/pci/pcie/aer_inject.c |  29 ++---
> >>  include/linux/aer.h           |  15 +++
> >>  7 files changed, 281 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> >> index 80aeb0d556bd7..ef449228b2549 100644
> >> --- a/drivers/cxl/Kconfig
> >> +++ b/drivers/cxl/Kconfig
> >> @@ -238,6 +238,19 @@ config CXL_RAS
> >>  	def_bool y
> >>  	depends on ACPI_APEI_GHES && PCIEAER && CXL_BUS
> >>  
> >> +config CXL_PROTO_AER_EINJ
> >> +	bool "CXL: RAS Protocol Error Injection using AER EINJ"
> >> +	depends on CXL_RAS
> >> +	depends on PCIEAER_INJECT  
> > 
> > Do we think anyone who has CXL and PCIEAER_INJECT support will want
> > to carefully not build this?  I'm just wondering if we can avoid asking
> > the question and base the built or not on the combination of those.
> >   
> 
> Good question: How do we incorporate this with the existing CXL EINJ 
> functionality making them complementary and consistant? The existing 
> EINJ is true ACPI injection but only supports RPs. The ACPI EINJ callouts 
> are currrently in core/port.c. We should consider moving it into a common file 
> such as core/ras_einj.c. With that move it will help force us merge the interfaces
> where/if possible and at least give central location for error RAS injection.
> I think folding the AER injection into PCIEAER_INJECT kernel config is 
> reasonable but looking for others feedback.
> 

I hadn't really thought about the injection method.  Indeed raises
interesting questions of how it should be done.  Add an ABI doc
in Documentation/ABI for next version.

> >   
> >> +static DEFINE_MUTEX(cxl_aer_einj_mutex);  
> > 
> > Needs a comment for what data it is protecting.
> >   
> >> +
> >> +struct cxl_aer_einj cxl_aer_einj = {
> >> +	.lock = &cxl_aer_einj_mutex,
> >> +};
> >> +
> >> +static const char cxl_aer_einj_usage[] =
> >> +	"ssss:bb:dd.f [UCE|CE] AER_STATUS RAS_STATUS [RCH]\n";
> >> +
> >> +static int cxl_aer_inject_error(struct pci_dev *pdev, bool correctable,
> >> +				u32 aer_status, u32 ras_status)
> >> +{
> >> +	/* RCD errors are signaled as internal errors on the associated RCEC */
> >> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) {
> >> +		if (!pdev->rcec)
> >> +			return -ENODEV;
> >> +		pdev = pdev->rcec;
> >> +	}
> >> +
> >> +	struct aer_error_inj einj = {
> >> +		.bus = pdev->bus->number,
> >> +		.dev = PCI_SLOT(pdev->devfn),
> >> +		.fn = PCI_FUNC(pdev->devfn),
> >> +		.domain = pci_domain_nr(pdev->bus),
> >> +	};
> >> +	int ret;
> >> +	int aer_offset;
> >> +	int ras_offset;
> >> +
> >> +	if (correctable) {
> >> +		einj.cor_status = aer_status | PCI_ERR_COR_INTERNAL;
> >> +		aer_offset = PCI_ERR_COR_STATUS / sizeof(u32);
> >> +		ras_offset = CXL_RAS_CORRECTABLE_STATUS_OFFSET / sizeof(u32);  
> > 
> > Given these are offsets into cxl_aer_einj.aer_registers / ras_registers
> > can we use sizeof(*cxl_aer_einj.aer_registers) etc
> >   
> We could but that assumes the CEs are book ending the register blocks. And this would 
> be inconsistent with UCE case below, right? Tell me if I misunderstaood the question.

I just meant to replace those sizeof(u32) with something that indicates
where the size is coming from.  Applies equally below.


> >   
> >> +
> >> +static void __iomem *to_einj_ras_base(struct cxl_port *port, struct cxl_dport *dport)
> >> +{
> >> +	if (dport) {
> >> +		if (cxl_aer_einj.is_rch) {
> >> +			if (cxl_aer_einj.dev == dport->dport_dev) {
> >> +				cxl_aer_einj.dev = NULL;
> >> +				return (__force void __iomem *)cxl_aer_einj.ras_registers;  
> > 
> > Given the output of this is always force cast, maybe move that force up to the caller?
> >   
> 
> I think that works if it remains an internal helper and isn't made a mock function to to_ras_base(). 
> If to_einj_ras_base() is used as a mock than it would require changing the to_ras_base() 
> as well. 
> 
> >> +			}
> >> +		} else {
> >> +			if (cxl_aer_einj.dev == dport->dport_dev) {
> >> +				pci_dev_put(to_pci_dev(cxl_aer_einj.dev));  
> > 
> > Not locally obvious why a thing called to_einj_ras_base should put anything it didn't
> > get.  I think this needs a restructure to more obviously be tidying up references
> > that were held over the queue. At very leads needs a comment.
> > /* Reference held from X no longer needed so drop */
> >   
> 
> The ref was incremented in cxl_aer_einj_write() on invoking injection. The ref is 
> decremented here after its usage. RCHs are excluded because they dont have a SBDF.

Just add minimal reference for that.

> 
> >> +				cxl_aer_einj.dev = NULL;
> >> +				return (__force void __iomem *)cxl_aer_einj.ras_registers;
> >> +			}
>
> >> +	if (cxl_aer_einj.dev) {
> >> +		void __iomem *einj = to_einj_ras_base(port, dport);
> >> +		if (einj)
> >> +			return einj;
> >> +	}
> >> +#endif
> >> +
> >>  	if (dport)
> >>  		return dport->regs.ras;
> >>  
> >> @@ -458,10 +656,20 @@ void cxl_ras_init(void)
> >>  	cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> >>  	cxl_register_proto_err_work(&cxl_proto_err_work,
> >>  				   cxl_proto_err_do_flush);
> >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ)
> >> +	cxl_ras_create_debugfs(cxl_debugfs);  
> > stub that in a header.
> >   
> 
> Actually, during rework today I moved it to cxl_ras_einj_init(),
> a new function. Thoughts?

Sounds good.

> >> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
> >> index 09bfc7194ef31..b313adef680ae 100644
> >> --- a/drivers/pci/pcie/aer_inject.c
> >> +++ b/drivers/pci/pcie/aer_inject.c
> >> @@ -14,6 +14,7 @@
> >>  
> >>  #define dev_fmt(fmt) "aer_inject: " fmt
> >>  
> >> +#include <linux/aer.h>
> >>  #include <linux/module.h>
> >>  #include <linux/init.h>
> >>  #include <linux/interrupt.h>
> >> @@ -31,19 +32,6 @@
> >>  static bool aer_mask_override;
> >>  module_param(aer_mask_override, bool, 0);
> >>  
> >> -struct aer_error_inj {
> >> -	u8 bus;
> >> -	u8 dev;
> >> -	u8 fn;
> >> -	u32 uncor_status;
> >> -	u32 cor_status;
> >> -	u32 header_log0;
> >> -	u32 header_log1;
> >> -	u32 header_log2;
> >> -	u32 header_log3;
> >> -	u32 domain;
> >> -};
> >> -
> >>  struct aer_error {
> >>  	struct list_head list;
> >>  	u32 domain;
> >> @@ -316,7 +304,7 @@ static int pci_bus_set_aer_ops(struct pci_bus *bus)
> >>  	return 0;
> >>  }
> >>  
> >> -static int aer_inject(struct aer_error_inj *einj)
> >> +int aer_inject(struct aer_error_inj *einj)
> >>  {
> >>  	struct aer_error *err, *rperr;
> >>  	struct aer_error *err_alloc = NULL, *rperr_alloc = NULL;
> >> @@ -332,10 +320,14 @@ static int aer_inject(struct aer_error_inj *einj)
> >>  	dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn);
> >>  	if (!dev)
> >>  		return -ENODEV;
> >> -	rpdev = pcie_find_root_port(dev);
> >> -	/* If Root Port not found, try to find an RCEC */
> >> -	if (!rpdev)
> >> -		rpdev = dev->rcec;
> >> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)   
> > 
> > { }
> > as the else is multiline (see coding standard)
> >   
> 
> Ok
> 
> > Maybe need a comment for why it might be an RCEC for injection.
> > Is this an RCH specific path where there is nothing else to target?
> >   
> >> +		rpdev = dev;
> >> +	else {
> >> +		rpdev = pcie_find_root_port(dev);
> >> +		/* If Root Port not found, try to find an RCEC */
> >> +		if (!rpdev)
> >> +			rpdev = dev->rcec;
> >> +	}
> >>  	if (!rpdev) {
> >>  		pci_err(dev, "Neither Root Port nor RCEC found\n");
> >>  		ret = -ENODEV;
> >> @@ -482,6 +474,7 @@ static int aer_inject(struct aer_error_inj *einj)
> >>  	pci_dev_put(dev);
> >>  	return ret;
> >>  }
> >> +EXPORT_SYMBOL_GPL(aer_inject);  
> > I wonder if we want to restrict this to specific modules?
> > 
> > One for Bjorn probably.
> >   
> Because right now it injects AER to any PCI device. 
> 
> Also, worth discussing is the commandline currently uses multiple parameters for
> a single sysfs file. I mentioned this at the top.
> 
> 
> Thanks for reviewing Jonathan.
> 
> -Terry

>