| Message ID | 20240215194048.141411-5-Benjamin.Cheatham@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement initial CXL Timeout & Isolation support | expand |
On Thu, Feb 15, 2024 at 01:40:46PM -0600, Ben Cheatham wrote: > Add and enable CXL.mem error isolation support (CXL 3.0 12.3.2) > to the CXL Timeout & Isolation service driver. > @@ -341,7 +366,8 @@ 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; > + bool timeout_enabled; > + int rc; > > /* Limit to CXL root ports */ > if (!pci_find_dvsec_capability(port, PCI_DVSEC_VENDOR_ID_CXL, > @@ -360,6 +386,18 @@ static int cxl_timeout_probe(struct pcie_device *dev) > pci_dbg(dev->port, "Failed to enable CXL.mem timeout: %d\n", > rc); > > + timeout_enabled = !rc; This ends up being a little weird: mixing int and bool, negating rc here and then negating timeout_enabled later, several messages. Maybe could just keep rc1 and rc2, drop the pci_dbg messages and enhance the "enabled" message to be something like: "enabled %s%s with IRQ", rc1 ? "" : "timeout", rc2 ? "" : "isolation" ("&" left for your imagination). Or something like #define FLAG(x) ((x) ? '-' : '+') "CXL.mem timeout%c isolation%c enabled with IRQ %d", FLAG(rc1), FLAG(rc2) > + rc = cxl_enable_isolation(dev, cxlt); > + if (rc) > + pci_dbg(dev->port, "Failed to enable CXL.mem isolation: %d\n", > + rc); "(%pe)" > + if (rc && !timeout_enabled) { > + pci_info(dev->port, > + "Failed to enable CXL.mem timeout and isolation.\n"); Most messages don't include a period at end. It just adds the chance for line wrapping unnecessarily.
On 2/15/24 3:49 PM, Bjorn Helgaas wrote: > On Thu, Feb 15, 2024 at 01:40:46PM -0600, Ben Cheatham wrote: >> Add and enable CXL.mem error isolation support (CXL 3.0 12.3.2) >> to the CXL Timeout & Isolation service driver. > >> @@ -341,7 +366,8 @@ 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; >> + bool timeout_enabled; >> + int rc; >> >> /* Limit to CXL root ports */ >> if (!pci_find_dvsec_capability(port, PCI_DVSEC_VENDOR_ID_CXL, >> @@ -360,6 +386,18 @@ static int cxl_timeout_probe(struct pcie_device *dev) >> pci_dbg(dev->port, "Failed to enable CXL.mem timeout: %d\n", >> rc); >> >> + timeout_enabled = !rc; > > This ends up being a little weird: mixing int and bool, negating rc > here and then negating timeout_enabled later, several messages. > > Maybe could just keep rc1 and rc2, drop the pci_dbg messages and > enhance the "enabled" message to be something like: > "enabled %s%s with IRQ", rc1 ? "" : "timeout", rc2 ? "" : "isolation" > ("&" left for your imagination). > > Or something like > #define FLAG(x) ((x) ? '-' : '+') > "CXL.mem timeout%c isolation%c enabled with IRQ %d", FLAG(rc1), FLAG(rc2) > I thought it was janky as well. I tried something really quick using ternaries and it looked weirder :/. But I agree with you here, I'll take another stab at it. >> + rc = cxl_enable_isolation(dev, cxlt); > >> + if (rc) >> + pci_dbg(dev->port, "Failed to enable CXL.mem isolation: %d\n", >> + rc); > > "(%pe)" > >> + if (rc && !timeout_enabled) { >> + pci_info(dev->port, >> + "Failed to enable CXL.mem timeout and isolation.\n"); > > Most messages don't include a period at end. It just adds the chance > for line wrapping unnecessarily. > I'll remove them.
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 4aa5fecc43bd..b1d5232a0127 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -131,9 +131,11 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw) #define CXL_TIMEOUT_CAPABILITY_OFFSET 0x0 #define CXL_TIMEOUT_CAP_MEM_TIMEOUT_MASK GENMASK(3, 0) #define CXL_TIMEOUT_CAP_MEM_TIMEOUT_SUPP BIT(4) +#define CXL_TIMEOUT_CAP_MEM_ISO_SUPP BIT(16) #define CXL_TIMEOUT_CONTROL_OFFSET 0x8 #define CXL_TIMEOUT_CONTROL_MEM_TIMEOUT_MASK GENMASK(3, 0) #define CXL_TIMEOUT_CONTROL_MEM_TIMEOUT_ENABLE BIT(4) +#define CXL_TIMEOUT_CONTROL_MEM_ISO_ENABLE BIT(16) #define CXL_TIMEOUT_CAPABILITY_LENGTH 0x10 /* CXL 3.0 8.2.4.23.2 CXL Timeout and Isolation Control Register, bits 3:0 */ diff --git a/drivers/pci/pcie/cxl_timeout.c b/drivers/pci/pcie/cxl_timeout.c index 916dbaf2bb58..5900239e5bbf 100644 --- a/drivers/pci/pcie/cxl_timeout.c +++ b/drivers/pci/pcie/cxl_timeout.c @@ -207,6 +207,31 @@ static int cxl_enable_timeout(struct pcie_device *dev, struct cxl_timeout *cxlt) cxlt); } +static void cxl_disable_isolation(void *data) +{ + struct cxl_timeout *cxlt = data; + u32 cntrl = readl(cxlt->regs + CXL_TIMEOUT_CONTROL_OFFSET); + + cntrl &= ~CXL_TIMEOUT_CONTROL_MEM_ISO_ENABLE; + writel(cntrl, cxlt->regs + CXL_TIMEOUT_CONTROL_OFFSET); +} + +static int cxl_enable_isolation(struct pcie_device *dev, + struct cxl_timeout *cxlt) +{ + u32 cntrl; + + if (!cxlt || !FIELD_GET(CXL_TIMEOUT_CAP_MEM_ISO_SUPP, cxlt->cap)) + return -ENXIO; + + cntrl = readl(cxlt->regs + CXL_TIMEOUT_CONTROL_OFFSET); + cntrl |= CXL_TIMEOUT_CONTROL_MEM_ISO_ENABLE; + writel(cntrl, cxlt->regs + CXL_TIMEOUT_CONTROL_OFFSET); + + return devm_add_action_or_reset(&dev->device, cxl_disable_isolation, + cxlt); +} + static ssize_t timeout_range_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -341,7 +366,8 @@ 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; + bool timeout_enabled; + int rc; /* Limit to CXL root ports */ if (!pci_find_dvsec_capability(port, PCI_DVSEC_VENDOR_ID_CXL, @@ -360,6 +386,18 @@ static int cxl_timeout_probe(struct pcie_device *dev) pci_dbg(dev->port, "Failed to enable CXL.mem timeout: %d\n", rc); + timeout_enabled = !rc; + + rc = cxl_enable_isolation(dev, cxlt); + if (rc) + pci_dbg(dev->port, "Failed to enable CXL.mem isolation: %d\n", + rc); + + if (rc && !timeout_enabled) { + pci_info(dev->port, + "Failed to enable CXL.mem timeout and isolation.\n"); + } + return rc; }
Add and enable CXL.mem error isolation support (CXL 3.0 12.3.2) to the CXL Timeout & Isolation service driver. Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com> --- drivers/cxl/cxl.h | 2 ++ drivers/pci/pcie/cxl_timeout.c | 40 +++++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-)