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, ®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.
>
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
