diff mbox series

[RFC,4/6] pcie/cxl_timeout: Add CXL.mem error isolation support

Message ID 20240215194048.141411-5-Benjamin.Cheatham@amd.com
State New, archived
Headers show
Series Implement initial CXL Timeout & Isolation support | expand

Commit Message

Ben Cheatham Feb. 15, 2024, 7:40 p.m. UTC
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(-)

Comments

Bjorn Helgaas Feb. 15, 2024, 9:49 p.m. UTC | #1
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.
Ben Cheatham Feb. 15, 2024, 10:21 p.m. UTC | #2
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 mbox series

Patch

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;
 }