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

[Ben Cheatham <benjamin.cheatham@amd.com>]

Hi Bjorn, thanks for the comments! Responses inline.

On 2/15/24 3:13 PM, Bjorn Helgaas wrote:
> 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.
> 

Yeah a lot of the spec references in cxl.h are out of date, I'll update them
where I touch them. And I didn't realize 3.1 was out yet, I'll update the references
to that revision as well.

>> +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.
> 

Will do.

> Rewrap to fit in 78 columns.
> 
> s/PCIE/PCIe/ (also below)
> 

Will do both above.

>> + * 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?
> 

12.3.2 is the CXL.mem portion of the section, which is the only section implemented
in the patch set. That being said, I don't mind changing it to 12.3 instead.

>> +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.
> 

Will do.

>> +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.
> 

Will do to all 4 above.

>> +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)
> 

I wasn't aware of that, I'll change it.

>> +	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