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
