Re: [PATCH v8 03/16] CXL/AER: Introduce Kfifo for forwarding CXL errors

[Bjorn Helgaas <helgaas@kernel.org>]

What does this series apply to?  I default to the current -rc1
(v6.14-rc1), but this doesn't apply there, and I don't have the
base-commit: aae0594a7053c60b82621136257c8b648c67b512 mentioned in the
cover letter.

Sometimes things make more sense when I can see everything as applied.

On Thu, Mar 27, 2025 at 01:12:30PM -0500, Bowman, Terry wrote:
> On 3/27/2025 12:08 PM, Bjorn Helgaas wrote:
> > On Wed, Mar 26, 2025 at 08:47:04PM -0500, Terry Bowman wrote:
> >> CXL error handling will soon be moved from the AER driver into the CXL
> >> driver. This requires a notification mechanism for the AER driver to share
> >> the AER interrupt details with CXL driver. The notification is required for
> >> the CXL drivers to then handle CXL RAS errors.
> >>
> >> Add a kfifo work queue to be used by the AER driver and CXL driver. The AER
> >> driver will be the sole kfifo producer adding work. The cxl_core will be
> >> the sole kfifo consumer removing work. Add the boilerplate kfifo support.
> >>
> >> Add CXL work queue handler registration functions in the AER driver. Export
> >> the functions allowing CXL driver to access. Implement the registration
> >> functions for the CXL driver to assign or clear the work handler function.
> >>
> >> Create a work queue handler function, cxl_prot_err_work_fn(), as a stub for
> >> now. The CXL specific handling will be added in future patch.
> >>
> >> Introduce 'struct cxl_prot_err_info'. This structure caches CXL error
> >> details used in completing error handling. This avoid duplicating some
> >> function calls and allows the error to be treated generically when
> >> possible.

> >> +int cxl_create_prot_err_info(struct pci_dev *_pdev, int severity,
> >> +			     struct cxl_prot_error_info *err_info)
> >> +{
> ...

> >> +	if ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT) &&
> >> +	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)) {
> >> +		pci_warn_once(pdev, "Error: Unsupported device type (%X)", pci_pcie_type(pdev));
> >> +		return -ENODEV;
> >
> > Similar.  A pci_warn_once() here seems like a debugging aid during
> > development, not necessarily a production kind of thing.
> >
> > Thanks for printing the type.  I would use "%#x" to make it clear that
> > it's hex.  There are about 1900 %X uses compared with 33K
> > %x uses, but maybe you have a reason to capitalize it?
>
> Got it "%x". Would you recommend the pci_warn_once() is removed?

The dependency on pdev being an endpoint is not clear here, so I would
just remove the check altogether or move it to the place that breaks
if pdev is not an endpoint.

> >> +#if defined(CONFIG_PCIEAER_CXL)
> >> +int cxl_register_prot_err_work(struct work_struct *work,
> >> +			       int (*_cxl_create_prot_err_info)(struct pci_dev*, int,
> >> +								struct cxl_prot_error_info*))
> >
> > Ditto.  Rewrap to fit in 80 columns, unindent this function
> > pointer decl to make it fit.  Same below in aer.h.
>
> Ok, got it. Without using typedefs, right ?

A typedef would be fine with me.

> >> +struct cxl_prot_error_info {
> >> +	struct pci_dev *pdev;
> >> +	struct device *dev;
> >> +	void __iomem *ras_base;
> >> +	int severity;
> >
> > What does the "prot" in "cxl_prot_error_info" refer to?
>
> Protocol. As in CXL Protocol Error Information. I suppose it needs
> to be renamed if it wasn't obvious.

Unless there are CXL non-protocol errors that need to be
distinguished, I would just omit "prot" altogether.

> > There's basically no error info here other than "severity".
>
> Correct. It's more accurately "CXL Protocol Error Context" but I didn't
> want to re-use 'context' because 'context' is used for thread/process
> statefulness. Also, I followed the existing CPER parallel work that uses
> a similar kfifo etc. Thoughts on rename?

What's the name of the corresponding CPER struct?

> > I guess "dev" and "pdev" are separate devices (otherwise you would
> > just use "&pdev->dev"), but I don't have any intuition about how they
> > might be related, which is a little disconcerting.
>
> "pdev" represents a PCIe device: RP, USP, DSP, or EP.  "dev" is the
> same device as "pdev" but "dev" is found in CXL topology. "dev" is
> discovered through ACPI/platform enumeration and interconnected with
> other CXL "devs" using upstream and downstream links. Moving back
> and forth between "pdev" and its CXL "dev" requires a search unique
> to the device type and point beginning the search.

It seems weird to me to have two device pointers here.  Seems like we
should use a single pointer to identify the device, and if we need to
get from PCI to CXL or vice versa, there should be a pointer somewhere
so we don't have to search all the time.

> > I would have thought that "ras_base" would be a property of "dev"
> > (the CXL device) and wouldn't need to be separate.
>
> "ras_base" is a common member of the CXL Port, CXL Downstream Port,
> CXL Upstream Port, and CXL EP. If one wants the "ras_base" for a
> given CXL "dev" then the "dev" must be converted to CXL Port,
> Downstream Port, or Upstream Port.

Passing around ras_base seems dodgy to me.  I think it's better to
pass around a real entity like a pci_dev or cxl_port or cxl_dport or
whatever.  Code that needs to deal with ras_base presumably should
know about the internals of the device ras_base belongs to.

Bjorn