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

[Bowman, Terry]

On 3/28/2025 12:02 PM, Bjorn Helgaas wrote:
> 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.

This base commit is from cxl/next and can be found here:
https://web.git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=next

Terry

> 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