Re: [RFC PATCH 2/6] pcie/cxl_timeout: Add CXL Timeout & Isolation service driver

[Bjorn Helgaas <helgaas@kernel.org>]

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, &regs);
> +	if (rc)
> +		return rc;
> +
> +	*cap = readl(regs.timeout + CXL_TIMEOUT_CAPABILITY_OFFSET);
> +	cxl_unmap_timeout_regs(dev, &map, &regs);
> +
> +	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