Follow drivers/pci/ subject line convention. Suggest
PCI/CXL: Add CXL Timeout & Isolation service driver
On Thu, Feb 15, 2024 at 01:40:44PM -0600, Ben Cheatham wrote:
> Add a CXL Timeout & Isolation (CXL 3.0 12.3) service driver to the
> PCIe port bus driver for CXL root ports. The service will support
> enabling/programming CXL.mem transaction timeout, error isolation,
> and interrupt handling.
> ...
> /* CXL 3.0 8.2.4.23 CXL Timeout and Isolation Capability Structure */
> #define CXL_TIMEOUT_CAPABILITY_OFFSET 0x0
> +#define CXL_TIMEOUT_CAP_MEM_TIMEOUT_SUPP BIT(4)
> +#define CXL_TIMEOUT_CONTROL_OFFSET 0x8
> +#define CXL_TIMEOUT_CONTROL_MEM_TIMEOUT_ENABLE BIT(4)
> #define CXL_TIMEOUT_CAPABILITY_LENGTH 0x10
Weird lack of alignment makes these #defines hard to read, but kudos
for following the local style ;)
> /* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
Update to current CXL spec, please. In r3.0, it looks like this is
sec 8.2.4.16. Updating everything to r3.1 references would be even
better.
> +config PCIE_CXL_TIMEOUT
> + bool "PCI Express CXL.mem Timeout & Isolation Interrupt support"
> + depends on PCIEPORTBUS
> + depends on CXL_BUS=PCIEPORTBUS && CXL_PORT
> + help
> + Enables the CXL.mem Timeout & Isolation PCIE port service driver. This
> + driver, in combination with the CXL driver core, is responsible for
> + handling CXL capable PCIE root ports that undergo CXL.mem error isolation
> + due to either a CXL.mem transaction timeout or uncorrectable device error.
When running menuconfig, it seems like both PCIEAER_CXL and
PCIE_CXL_TIMEOUT would fit more logically in the drivers/cxl/Kconfig
menu along with the rest of the CXL options.
Rewrap to fit in 78 columns.
s/PCIE/PCIe/ (also below)
> + * Implements CXL Timeout & Isolation (CXL 3.0 12.3.2) interrupt support as a
> + * PCIE port service driver. The driver is set up such that near all of the
> + * work for setting up and handling interrupts are in this file, while the
> + * CXL core enables the interrupts during port enumeration.
Seems like sec 12.3.2 is slightly too specific here. Maybe 12.3?
> +struct pcie_cxlt_data {
> + struct cxl_timeout *cxlt;
> + struct cxl_dport *dport;
"dport" is not used here. Would be better to add it in the patch that
needs it.
> +static int cxl_map_timeout_regs(struct pci_dev *port,
> + struct cxl_register_map *map,
> + struct cxl_component_regs *regs)
> +{
> + int rc = 0;
Unnecessary initialization.
> + rc = cxl_find_regblock(port, CXL_REGLOC_RBI_COMPONENT, map);
> ...
> +int cxl_find_timeout_cap(struct pci_dev *dev, u32 *cap)
> +{
> + struct cxl_component_regs regs;
> + struct cxl_register_map map;
> + int rc = 0;
Unnecessary initialization.
> + rc = cxl_map_timeout_regs(dev, &map, ®s);
> + if (rc)
> + return rc;
> +
> + *cap = readl(regs.timeout + CXL_TIMEOUT_CAPABILITY_OFFSET);
> + cxl_unmap_timeout_regs(dev, &map, ®s);
> +
> + return rc;
Since we already know the value:
return 0;
> +}
Move cxl_find_timeout_cap() to the patch that needs it. It's unused
in this one.
> +static int cxl_timeout_probe(struct pcie_device *dev)
> +{
> + struct pci_dev *port = dev->port;
> + struct pcie_cxlt_data *pdata;
> + struct cxl_timeout *cxlt;
> + int rc = 0;
Unnecessary initialization.
> + /* Limit to CXL root ports */
> + if (!pci_find_dvsec_capability(port, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DVSEC_PORT_EXTENSIONS))
> + return -ENODEV;
> +
> + pdata = cxlt_create_pdata(dev);
> + if (IS_ERR_OR_NULL(pdata))
> + return PTR_ERR(pdata);
> +
> + set_service_data(dev, pdata);
> + cxlt = pdata->cxlt;
> +
> + rc = cxl_enable_timeout(dev, cxlt);
> + if (rc)
> + pci_dbg(dev->port, "Failed to enable CXL.mem timeout: %d\n",
> + rc);
"(%pe)" (similar in subsequent patches)
> + return rc;
> +}
> +
> @@ -829,6 +829,7 @@ static void __init pcie_init_services(void)
> pcie_pme_init();
> pcie_dpc_init();
> pcie_hp_init();
> + pcie_cxlt_init();
"cxlt" seems slightly too generic outside cxl_timeout.c, since there
might be other CXL things that start with "t" someday.
> +#define PCIE_PORT_SERVICE_CXLT_SHIFT 5 /* CXL Timeout & Isolation */
> +#define PCIE_PORT_SERVICE_CXLT (1 << PCIE_PORT_SERVICE_CXLT_SHIFT)
I hate having to add this to the portdrv, but I see why you need to
(so we can deal with the weirdness of PCIe feature interrupts).
Bjorn
