Re: [PATCH v4 23/23] PCI/AER: Unmask RCEC internal errors to enable RCH downstream port error handling

[Robert Richter]

On 24.05.23 16:45:06, Bjorn Helgaas wrote:
> On Tue, May 23, 2023 at 06:22:14PM -0500, Terry Bowman wrote:
> > From: Robert Richter <rrichter@xxxxxxx>
> > 
> > AER corrected and uncorrectable internal errors (CIE/UIE) are masked
> > in their corresponding mask registers per default once in power-up
> > state. [1][2] Enable internal errors for RCECs to receive CXL
> > downstream port errors of Restricted CXL Hosts (RCHs).
> > 
> > [1] CXL 3.0 Spec, 12.2.1.1 - RCH Downstream Port Detected Errors
> > [2] PCIe Base Spec 6.0, 7.8.4.3 Uncorrectable Error Mask Register,
> >     7.8.4.6 Correctable Error Mask Register
> 
> I use "r6.0" to make sure it isn't mistaken for a section number.
> 
> > Co-developed-by: Terry Bowman <terry.bowman@xxxxxxx>
> > Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> > Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> > ---
> >  drivers/pci/pcie/aer.c | 64 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index 2e3f00b6a5bd..c5076ae4eb58 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -948,6 +948,32 @@ static bool find_source_device(struct pci_dev *parent,
> >  
> >  #ifdef CONFIG_PCIEAER_CXL
> >  
> > +static int pci_aer_unmask_internal_errors(struct pci_dev *dev)
> > +{
> > +	int aer, rc;
> > +	u32 mask;
> > +
> > +	if (!pcie_aer_is_native(dev))
> > +		return -EIO;
> > +
> > +	aer = dev->aer_cap;
> > +	rc = pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
> > +	if (rc)
> > +		return rc;
> 
> I don't think there's much value in checking all these config accesses
> for failure.  A failure return really just means you called it with
> invalid parameters; it doesn't tell you whether it was successful on
> PCI.
> 
> > +	mask &= ~PCI_ERR_UNC_INTN;
> > +	rc = pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
> > +	if (rc)
> > +		return rc;
> > +	mask &= ~PCI_ERR_COR_INTERNAL;
> > +	rc = pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
> > +
> > +	return rc;
> > +}
> > +
> >  static bool is_cxl_mem_dev(struct pci_dev *dev)
> >  {
> >  	/*
> > @@ -1031,7 +1057,44 @@ static void cxl_rch_handle_error(struct pci_dev *dev, struct aer_err_info *info)
> >  		pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
> >  }
> >  
> > +static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
> > +{
> > +	int *handles_cxl = data;
> > +
> > +	*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
> 
> This effectively only looks at the *last* RCiEP associated with this
> RCEC.  I would expect a logical OR of all of them.
> 
> I see this is another use of is_cxl_mem_dev() and
> cxl_error_is_native() that really requires them to be in this file.
> 
> > +	return *handles_cxl;

If this is non-zero, the iteration stops. So as soon we find a cxl
device we can stop the loop. Else, all devices are non-cxl devs and
the last return is zero too.

Now checking the code, pci_walk_bus() works that way, but walk_rcec()
does not break in all cases. I think this function not working as
expected. We would need to check if pci_walk_bus() stopped the
iteration, e.g. with a return code.

Alternatively we could add this check:

	if (!*handles_cxl)
		*handles_cxl = ...

> > +}
> > +
> > +static bool handles_cxl_errors(struct pci_dev *rcec)
> > +{
> > +	int handles_cxl = 0;
> > +
> > +	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
> > +	    pcie_aer_is_native(rcec))
> > +		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
> > +
> > +	return !!handles_cxl;
> > +}
> > +
> > +static void cxl_rch_enable_rcec(struct pci_dev *rcec)
> > +{
> > +	if (!handles_cxl_errors(rcec))
> > +		return;
> > +
> > +	/*
> > +	 * Internal errors are masked by default, unmask RCEC's here
> > +	 * PCI6.0 7.8.4.3 Uncorrectable Error Mask Register (Offset 08h)
> > +	 * PCI6.0 7.8.4.6 Correctable Error Mask Register (Offset 14h)
> 
> The spec references seem superfluous here.  The PCI_ERR_UNCOR_MASK and
> PCI_ERR_COR_MASK in pci_aer_unmask_internal_errors() are pretty good
> pointers.
> 
> > +	 */
> > +	if (pci_aer_unmask_internal_errors(rcec))
> > +		pci_err(rcec, "CXL: Failed to unmask internal errors");
> > +	else
> > +		pci_info(rcec, "CXL: Internal errors unmasked");
> > +}
> > +
> >  #else
> > +static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
> >  static inline void cxl_rch_handle_error(struct pci_dev *dev,
> >  					struct aer_err_info *info) { }
> >  #endif
> > @@ -1432,6 +1495,7 @@ static int aer_probe(struct pcie_device *dev)
> >  		return status;
> >  	}
> >  
> > +	cxl_rch_enable_rcec(port);
> 
> Could this be done by the driver that claims the CXL RCiEP?  There's
> no point in unmasking the errors before there's a driver with
> pci_error_handlers that can do something with them anyway.

This sounds reasonable at the first glance. The problem is there could
be many devices associated with the RCEC. Not all of them will be
bound to a driver and handler at the same time. We would need to
refcount it or maintain a list of enabled devices. But there is
already something similar by checking dev->driver. But right, AER
errros could be seen and handled then at least on PCI level. I tent to
permanently enable RCEC AER, but that could cause side-effects. What
do you think?

Thanks,

-Robert

> 
> >  	aer_enable_rootport(rpc);
> >  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
> >  	return 0;
> > -- 
> > 2.34.1
> >