(no subject)

[Fabio M. De Francesco]

Subject: [PATCH 0/2] PCI/CXL: Recover CXL Downstream Ports from PM Init failure

CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which a
Secondary Bus Reset, a Link Down, or Downstream Port Containment on a
CXL Downstream Port prevents Port PM Init from completing when ACS
Source Validation is enabled on the Downstream Port. The spec states
that another SBR alone does not recover the port and describes a
software recovery sequence.  

Patch 1 extends cxl_reset_bus_function(), the helper backing the cxl_bus
PCI/CXL reset method exposed to userspace via sysfs. It saves, clears,
and restores ACS Source Validation and Bus Master Enable on the CXL
Downstream Port around the SBR it issues. This keeps the userspace
cxl_bus reset path from leaving the port unable to complete PM Init.

Patch 2 adds a recovery pass during CXL enumeration. For each CXL
Downstream Port in a memdev's ancestry, the CXL core checks whether PM
Init has completed. If it has not, regardless of what caused the
failure, it invokes cxl_reset_bus_function() on the child below the port
in the hope of restoring the port to a usable state. CXL enumeration
re-runs after events that tear down and re-probe the memdev, including
DPC, AER, and Link Down, so those paths reach this recovery.

This small series is developed from an old RFC v3:
https://lore.kernel.org/linux-cxl/20260330193347.25072-1-fabio.m.de.francesco@linux.intel.com/

Fabio M. De Francesco (2):
  PCI/CXL: Allow PM Init to complete on cxl_bus reset if ACS SV enabled
  cxl/core: Recover from PM Init failure via cxl_reset_bus_function()

drivers/cxl/core/pci.c        | 30 ++++++++++++++++++++
 drivers/cxl/core/port.c       | 22 +++++++++++++++
 drivers/cxl/cxlpci.h          |  3 ++
 drivers/pci/pci.c             | 52 ++++++++++++++++++++++++++++++++++-
 include/linux/pci.h           |  1 + 
 include/uapi/linux/pci_regs.h |  2 ++
 6 files changed, 109 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH 1/2] PCI/CXL: Allow PM Init to complete on cxl_bus reset if ACS SV enabled

[Fabio M. De Francesco]

CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which
issuing a Secondary Bus Reset on a CXL Downstream Port leaves the
Port Power Management Initialization Complete bit unset when the PCIe
Access Control Service (ACS) Source Validation bit (SV) is enabled on
the Downstream Port. The spec states that another SBR alone will not
facilitate recovery and shows a software recovery sequence.

Implement the sequence by extending cxl_reset_bus_function() to save,
clear, and restore ACS SV and Bus Master Enable (BME) on the Downstream
Port around the SBR with the use of helpers.

The wait inside pci_bridge_secondary_bus_reset() covers the 100 ms
referenced by the spec. The helpers return when ACS SV is not enabled on
the Downstream Port.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/pci/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8f7cfcc00090..047d3b4508a5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4930,10 +4930,55 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
 	return rc;
 }
 
+static void cxl_disable_acs_sv_bme(struct pci_dev *bridge, u16 *saved_cmd,
+				   u16 *saved_acs_ctrl)
+{
+	if (!bridge->acs_cap)
+		return;
+
+	pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
+			     saved_acs_ctrl);
+	if (!(*saved_acs_ctrl & PCI_ACS_SV))
+		return;
+
+	pci_read_config_word(bridge, PCI_COMMAND, saved_cmd);
+	if (*saved_cmd & PCI_COMMAND_MASTER)
+		pci_clear_master(bridge);
+
+	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
+			      *saved_acs_ctrl & ~PCI_ACS_SV);
+}
+
+static void cxl_restore_acs_sv_bme(struct pci_dev *bridge, u16 saved_cmd,
+				   u16 saved_acs_ctrl)
+{
+	if (!bridge->acs_cap || !(saved_acs_ctrl & PCI_ACS_SV))
+		return;
+
+	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
+			      saved_acs_ctrl);
+	if (saved_cmd & PCI_COMMAND_MASTER)
+		pci_set_master(bridge);
+}
+
+/**
+ * cxl_reset_bus_function - SBR for a child of a CXL downstream port
+ * @dev: child device whose upstream bridge is a CXL downstream port
+ * @probe: if true, only check whether the reset is supported
+ *
+ * Issues an SBR on @dev's parent bus. Temporarily sets the CXL Port
+ * DVSEC Unmask SBR bit across the reset. When ACS Source Validation
+ * is enabled on the bridge, also temporarily clears Bus Master Enable
+ * and ACS Source Validation, per CXL r4.0 sec 8.1.5.1.
+ *
+ * Return: 0 on success, -ENOTTY if the reset cannot be issued, or an
+ * errno from the reset path.
+ */
 static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 {
 	struct pci_dev *bridge;
 	u16 dvsec, reg, val;
+	u16 saved_cmd = 0, saved_acs_ctrl = 0;
 	int rc;
 
 	bridge = pci_upstream_bridge(dev);
@@ -4957,6 +5002,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 		return rc;
 	}
 
+	cxl_disable_acs_sv_bme(bridge, &saved_cmd, &saved_acs_ctrl);
+
 	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) {
 		val = reg;
 	} else {
@@ -4971,6 +5018,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
 				      reg);
 
+	cxl_restore_acs_sv_bme(bridge, saved_cmd, saved_acs_ctrl);
+
 	pci_dev_reset_iommu_done(dev);
 	return rc;
 }
-- 
2.53.0

Re: [PATCH 1/2] PCI/CXL: Allow PM Init to complete on cxl_bus reset if ACS SV enabled

[Dave Jiang]

On 4/28/26 11:24 AM, Fabio M. De Francesco wrote:
> CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which
> issuing a Secondary Bus Reset on a CXL Downstream Port leaves the
> Port Power Management Initialization Complete bit unset when the PCIe
> Access Control Service (ACS) Source Validation bit (SV) is enabled on
> the Downstream Port. The spec states that another SBR alone will not
> facilitate recovery and shows a software recovery sequence.
> 
> Implement the sequence by extending cxl_reset_bus_function() to save,
> clear, and restore ACS SV and Bus Master Enable (BME) on the Downstream
> Port around the SBR with the use of helpers.
> 
> The wait inside pci_bridge_secondary_bus_reset() covers the 100 ms
> referenced by the spec. The helpers return when ACS SV is not enabled on
> the Downstream Port.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/pci/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc00090..047d3b4508a5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4930,10 +4930,55 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>  	return rc;
>  }
>  
> +static void cxl_disable_acs_sv_bme(struct pci_dev *bridge, u16 *saved_cmd,
> +				   u16 *saved_acs_ctrl)

Maybe you can call this 'cxl_bus_reset_prep' and the restore one 'cxl_bus_reset_done'?

> +{

Should you return int and check if the two ptr passed in are valid?

> +	if (!bridge->acs_cap)
> +		return;

Returning here and the output parameters are not set. That is a problem when you try to restore with invalid values. Maybe -EOPNOTSUPP needs to be returned. Errno from this function should prevent the restore from being called.

> +
> +	pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			     saved_acs_ctrl);
> +	if (!(*saved_acs_ctrl & PCI_ACS_SV))
> +		return;

What is the caller suppose to do if PCI_ACS_SV is not set? Should you skip restore later?

> +
> +	pci_read_config_word(bridge, PCI_COMMAND, saved_cmd);
> +	if (*saved_cmd & PCI_COMMAND_MASTER)

If all you care is master bit enabled, maybe just write back a bool 'master_en'.

> +		pci_clear_master(bridge);
> +
> +	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			      *saved_acs_ctrl & ~PCI_ACS_SV);
> +}
> +
> +static void cxl_restore_acs_sv_bme(struct pci_dev *bridge, u16 saved_cmd,
> +				   u16 saved_acs_ctrl)

static void cxl_bus_reset_done(struct pci_dev *bridge, bool master_en, u16 acs_ctrl)

saved_ has no relevance to the function itself.

Come to think of it, why not create a local struct and pass that in as parameter for prep and done functions? That way in the future if there are other stuff that needs to be done, this can be more versatile.

struct cxl_reset_ctx {
	u16 acs_ctrl;
	bool master_en;
	bool acs_restore;
};

This way you can also set 'acs_restore' on completion of the prep function. And in the done function, you can check and return early if it's not set. That takes away the messiness of returning errno in the prep function and then needing to determine if you need to call the prep function or not.

DJ

> +{
> +	if (!bridge->acs_cap || !(saved_acs_ctrl & PCI_ACS_SV))
> +		return;
> +
> +	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			      saved_acs_ctrl);
> +	if (saved_cmd & PCI_COMMAND_MASTER)
> +		pci_set_master(bridge);
> +}
> +
> +/**
> + * cxl_reset_bus_function - SBR for a child of a CXL downstream port
> + * @dev: child device whose upstream bridge is a CXL downstream port
> + * @probe: if true, only check whether the reset is supported
> + *
> + * Issues an SBR on @dev's parent bus. Temporarily sets the CXL Port
> + * DVSEC Unmask SBR bit across the reset. When ACS Source Validation
> + * is enabled on the bridge, also temporarily clears Bus Master Enable
> + * and ACS Source Validation, per CXL r4.0 sec 8.1.5.1.
> + *
> + * Return: 0 on success, -ENOTTY if the reset cannot be issued, or an
> + * errno from the reset path.
> + */
>  static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *bridge;
>  	u16 dvsec, reg, val;
> +	u16 saved_cmd = 0, saved_acs_ctrl = 0;
>  	int rc;
>  
>  	bridge = pci_upstream_bridge(dev);
> @@ -4957,6 +5002,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  		return rc;
>  	}
>  
> +	cxl_disable_acs_sv_bme(bridge, &saved_cmd, &saved_acs_ctrl);
> +
>  	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) {
>  		val = reg;
>  	} else {
> @@ -4971,6 +5018,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
>  				      reg);
>  
> +	cxl_restore_acs_sv_bme(bridge, saved_cmd, saved_acs_ctrl);
> +
>  	pci_dev_reset_iommu_done(dev);
>  	return rc;
>  }

[PATCH 2/2] cxl/core: Recover from PM Init failure via cxl_reset_bus_function()

[Fabio M. De Francesco]

CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which a
Secondary Bus Reset, a Link Down, or Downstream Port Containment on a
CXL Downstream Port prevents Port PM Init from completing when ACS
Source Validation is enabled.

During CXL enumeration, for each CXL Downstream Port in a memdev's
ancestry, check whether PM Init has completed. If it has not, invoke
cxl_reset_bus_function() which is exported for use by CXL.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/pci.c        | 30 ++++++++++++++++++++++++++++++
 drivers/cxl/core/port.c       | 22 ++++++++++++++++++++++
 drivers/cxl/cxlpci.h          |  3 +++
 drivers/pci/pci.c             |  3 ++-
 include/linux/pci.h           |  1 +
 include/uapi/linux/pci_regs.h |  2 ++
 6 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index d1f487b3d809..de6a317df650 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -926,3 +926,33 @@ int cxl_port_get_possible_dports(struct cxl_port *port)
 
 	return ctx.count;
 }
+
+/**
+ * cxl_port_pm_init_is_complete - check the downstream port's PM Init Complete
+ * @pdev: downstream port
+ *
+ * Read the Port Power Management Initialization Complete bit in the
+ * Downstream Port's CXL DVSEC Port Extended Status register.
+ *
+ * Return: false only when the bit is observably clear. Return true when PM
+ * init is complete, when @pdev is not a CXL port (no Port DVSEC), or when
+ * the status register cannot be read.
+ */
+bool cxl_port_pm_init_is_complete(struct pci_dev *pdev)
+{
+	u16 status;
+	u16 dvsec;
+	int rc;
+
+	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					  PCI_DVSEC_CXL_PORT);
+	if (!dvsec)
+		return true;
+
+	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
+				  &status);
+	if (rc || PCI_POSSIBLE_ERROR(status))
+		return true;
+
+	return !!FIELD_GET(PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP, status);
+}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index c5aacd7054f1..a91841855d3b 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1825,6 +1825,28 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 		if (is_cxl_host_bridge(dport_dev))
 			return 0;
 
+		/*
+		 * Check the downstream port's PM init status, and if it has
+		 * failed retry PM init according to CXL Spec. 4.0 Sect. 8.1.5.1
+		 * - Implementation Note
+		 */
+		if (dev_is_pci(dport_dev) && dev_is_pci(iter->parent)) {
+			struct pci_dev *dport_pdev = to_pci_dev(dport_dev);
+
+			if (!cxl_port_pm_init_is_complete(dport_pdev)) {
+				dev_dbg(&cxlmd->dev,
+					"PM init failed for %s, retrying PM init\n",
+					dev_name(dport_dev));
+
+				cxl_reset_bus_function(to_pci_dev(iter->parent), false);
+
+				if (!cxl_port_pm_init_is_complete(dport_pdev))
+					dev_dbg(&cxlmd->dev,
+						"PM init failed retry for %s\n",
+						dev_name(dport_dev));
+			}
+		}
+
 		uport_dev = dport_dev->parent;
 		if (!uport_dev) {
 			dev_warn(dev, "at %s no parent for dport: %s\n",
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index b826eb53cf7b..c66ff2ce82a3 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -114,4 +114,7 @@ static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
 
 int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 		       struct cxl_register_map *map);
+
+bool cxl_port_pm_init_is_complete(struct pci_dev *pdev);
+
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 047d3b4508a5..ae30da22daf4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4974,7 +4974,7 @@ static void cxl_restore_acs_sv_bme(struct pci_dev *bridge, u16 saved_cmd,
  * Return: 0 on success, -ENOTTY if the reset cannot be issued, or an
  * errno from the reset path.
  */
-static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
+int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 {
 	struct pci_dev *bridge;
 	u16 dvsec, reg, val;
@@ -5023,6 +5023,7 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 	pci_dev_reset_iommu_done(dev);
 	return rc;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_reset_bus_function, "CXL");
 
 void pci_dev_lock(struct pci_dev *dev)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2c4454583c11..1fb1360d41e8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1477,6 +1477,7 @@ int pci_probe_reset_slot(struct pci_slot *slot);
 int pci_probe_reset_bus(struct pci_bus *bus);
 int pci_reset_bus(struct pci_dev *dev);
 void pci_reset_secondary_bus(struct pci_dev *dev);
+int cxl_reset_bus_function(struct pci_dev *dev, bool probe);
 void pcibios_reset_secondary_bus(struct pci_dev *dev);
 void pci_update_resource(struct pci_dev *dev, int resno);
 int __must_check pci_assign_resource(struct pci_dev *dev, int i);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 14f634ab9350..7e2579f89041 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1369,6 +1369,8 @@
 
 /* CXL r4.0, 8.1.5: Extensions DVSEC for Ports */
 #define PCI_DVSEC_CXL_PORT				3
+#define  PCI_DVSEC_CXL_PORT_EXT_STATUS			0x0A
+#define   PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP	_BITUL(0)
 #define  PCI_DVSEC_CXL_PORT_CTL				0x0c
 #define   PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
 
-- 
2.53.0

Re: [PATCH 2/2] cxl/core: Recover from PM Init failure via cxl_reset_bus_function()

[Dave Jiang]

On 4/28/26 11:24 AM, Fabio M. De Francesco wrote:
> CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which a
> Secondary Bus Reset, a Link Down, or Downstream Port Containment on a
> CXL Downstream Port prevents Port PM Init from completing when ACS
> Source Validation is enabled.
> 
> During CXL enumeration, for each CXL Downstream Port in a memdev's
> ancestry, check whether PM Init has completed. If it has not, invoke
> cxl_reset_bus_function() which is exported for use by CXL.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/cxl/core/pci.c        | 30 ++++++++++++++++++++++++++++++
>  drivers/cxl/core/port.c       | 22 ++++++++++++++++++++++
>  drivers/cxl/cxlpci.h          |  3 +++
>  drivers/pci/pci.c             |  3 ++-
>  include/linux/pci.h           |  1 +
>  include/uapi/linux/pci_regs.h |  2 ++
>  6 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index d1f487b3d809..de6a317df650 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -926,3 +926,33 @@ int cxl_port_get_possible_dports(struct cxl_port *port)
>  
>  	return ctx.count;
>  }
> +
> +/**
> + * cxl_port_pm_init_is_complete - check the downstream port's PM Init Complete
> + * @pdev: downstream port
> + *
> + * Read the Port Power Management Initialization Complete bit in the
> + * Downstream Port's CXL DVSEC Port Extended Status register.
> + *
> + * Return: false only when the bit is observably clear. Return true when PM
> + * init is complete, when @pdev is not a CXL port (no Port DVSEC), or when
> + * the status register cannot be read.
> + */
> +bool cxl_port_pm_init_is_complete(struct pci_dev *pdev)
> +{
> +	u16 status;
> +	u16 dvsec;
> +	int rc;
> +
> +	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> +					  PCI_DVSEC_CXL_PORT);
> +	if (!dvsec)
> +		return true;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
> +				  &status);
> +	if (rc || PCI_POSSIBLE_ERROR(status))
> +		return true;
> +
> +	return !!FIELD_GET(PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP, status);
> +}
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index c5aacd7054f1..a91841855d3b 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1825,6 +1825,28 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  		if (is_cxl_host_bridge(dport_dev))
>  			return 0;
>  
> +		/*
> +		 * Check the downstream port's PM init status, and if it has
> +		 * failed retry PM init according to CXL Spec. 4.0 Sect. 8.1.5.1
> +		 * - Implementation Note
> +		 */
> +		if (dev_is_pci(dport_dev) && dev_is_pci(iter->parent)) {
> +			struct pci_dev *dport_pdev = to_pci_dev(dport_dev);
> +
> +			if (!cxl_port_pm_init_is_complete(dport_pdev)) {
> +				dev_dbg(&cxlmd->dev,
> +					"PM init failed for %s, retrying PM init\n",
> +					dev_name(dport_dev));
> +
> +				cxl_reset_bus_function(to_pci_dev(iter->parent), false);
> +
> +				if (!cxl_port_pm_init_is_complete(dport_pdev))
> +					dev_dbg(&cxlmd->dev,
> +						"PM init failed retry for %s\n",
> +						dev_name(dport_dev));
> +			}
> +		}

Make this a helper function and move it to core/pci.c. Also pass 'struct device' and not 'struct pci_dev' to that function.

I do wonder if you reset it here, maybe you need to restart enumeration from the endpoint instead of just continuing as if nothing happened.

DJ

> +
>  		uport_dev = dport_dev->parent;
>  		if (!uport_dev) {
>  			dev_warn(dev, "at %s no parent for dport: %s\n",
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index b826eb53cf7b..c66ff2ce82a3 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -114,4 +114,7 @@ static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
>  
>  int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  		       struct cxl_register_map *map);
> +
> +bool cxl_port_pm_init_is_complete(struct pci_dev *pdev);
> +
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 047d3b4508a5..ae30da22daf4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4974,7 +4974,7 @@ static void cxl_restore_acs_sv_bme(struct pci_dev *bridge, u16 saved_cmd,
>   * Return: 0 on success, -ENOTTY if the reset cannot be issued, or an
>   * errno from the reset path.
>   */
> -static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
> +int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *bridge;
>  	u16 dvsec, reg, val;
> @@ -5023,6 +5023,7 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  	pci_dev_reset_iommu_done(dev);
>  	return rc;
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_reset_bus_function, "CXL");
>  
>  void pci_dev_lock(struct pci_dev *dev)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2c4454583c11..1fb1360d41e8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1477,6 +1477,7 @@ int pci_probe_reset_slot(struct pci_slot *slot);
>  int pci_probe_reset_bus(struct pci_bus *bus);
>  int pci_reset_bus(struct pci_dev *dev);
>  void pci_reset_secondary_bus(struct pci_dev *dev);
> +int cxl_reset_bus_function(struct pci_dev *dev, bool probe);
>  void pcibios_reset_secondary_bus(struct pci_dev *dev);
>  void pci_update_resource(struct pci_dev *dev, int resno);
>  int __must_check pci_assign_resource(struct pci_dev *dev, int i);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 14f634ab9350..7e2579f89041 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1369,6 +1369,8 @@
>  
>  /* CXL r4.0, 8.1.5: Extensions DVSEC for Ports */
>  #define PCI_DVSEC_CXL_PORT				3
> +#define  PCI_DVSEC_CXL_PORT_EXT_STATUS			0x0A
> +#define   PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP	_BITUL(0)
>  #define  PCI_DVSEC_CXL_PORT_CTL				0x0c
>  #define   PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
>  

Re: [PATCH 2/2] cxl/core: Recover from PM Init failure via cxl_reset_bus_function()

[Alison Schofield]

On Tue, Apr 28, 2026 at 08:24:35PM +0200, Fabio M. De Francesco wrote:
> CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which a
> Secondary Bus Reset, a Link Down, or Downstream Port Containment on a
> CXL Downstream Port prevents Port PM Init from completing when ACS
> Source Validation is enabled.
> 
> During CXL enumeration, for each CXL Downstream Port in a memdev's
> ancestry, check whether PM Init has completed. If it has not, invoke
> cxl_reset_bus_function() which is exported for use by CXL.

Hi Fabio, Not a full review yet, but that !! caught my eye, so...

> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/cxl/core/pci.c        | 30 ++++++++++++++++++++++++++++++
>  drivers/cxl/core/port.c       | 22 ++++++++++++++++++++++
>  drivers/cxl/cxlpci.h          |  3 +++
>  drivers/pci/pci.c             |  3 ++-
>  include/linux/pci.h           |  1 +
>  include/uapi/linux/pci_regs.h |  2 ++
>  6 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index d1f487b3d809..de6a317df650 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -926,3 +926,33 @@ int cxl_port_get_possible_dports(struct cxl_port *port)
>  
>  	return ctx.count;
>  }
> +
> +/**
> + * cxl_port_pm_init_is_complete - check the downstream port's PM Init Complete
> + * @pdev: downstream port
> + *
> + * Read the Port Power Management Initialization Complete bit in the
> + * Downstream Port's CXL DVSEC Port Extended Status register.
> + *
> + * Return: false only when the bit is observably clear. Return true when PM
> + * init is complete, when @pdev is not a CXL port (no Port DVSEC), or when
> + * the status register cannot be read.
> + */
> +bool cxl_port_pm_init_is_complete(struct pci_dev *pdev)
> +{
> +	u16 status;
> +	u16 dvsec;
> +	int rc;
> +
> +	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> +					  PCI_DVSEC_CXL_PORT);
> +	if (!dvsec)
> +		return true;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
> +				  &status);
> +	if (rc || PCI_POSSIBLE_ERROR(status))
> +		return true;
> +
> +	return !!FIELD_GET(PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP, status);
> +}

The !! seems unnecessary here since this is already a single bit FIELD_GET(),
ie. result is 0 or 1.

I also wouldn't mind this being extra reader friendly like below,
but I'm happy if you just get rid of the !!

bool complete;

complete = FIELD_GET(....);

return complete;

Re:

[Dave Jiang]

On 4/28/26 11:24 AM, Fabio M. De Francesco wrote:
> Subject: [PATCH 0/2] PCI/CXL: Recover CXL Downstream Ports from PM Init failure
> 
> CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which a
> Secondary Bus Reset, a Link Down, or Downstream Port Containment on a

I'm not sure if this series covers a Link Down event (i.e. hotplug). As I recall, cxl_reset_bus_function() only happens via sysfs trigger.

DJ


> CXL Downstream Port prevents Port PM Init from completing when ACS
> Source Validation is enabled on the Downstream Port. The spec states
> that another SBR alone does not recover the port and describes a
> software recovery sequence.  
> 
> Patch 1 extends cxl_reset_bus_function(), the helper backing the cxl_bus
> PCI/CXL reset method exposed to userspace via sysfs. It saves, clears,
> and restores ACS Source Validation and Bus Master Enable on the CXL
> Downstream Port around the SBR it issues. This keeps the userspace
> cxl_bus reset path from leaving the port unable to complete PM Init.
> 
> Patch 2 adds a recovery pass during CXL enumeration. For each CXL
> Downstream Port in a memdev's ancestry, the CXL core checks whether PM
> Init has completed. If it has not, regardless of what caused the
> failure, it invokes cxl_reset_bus_function() on the child below the port
> in the hope of restoring the port to a usable state. CXL enumeration
> re-runs after events that tear down and re-probe the memdev, including
> DPC, AER, and Link Down, so those paths reach this recovery.
> 
> This small series is developed from an old RFC v3:
> https://lore.kernel.org/linux-cxl/20260330193347.25072-1-fabio.m.de.francesco@linux.intel.com/
> 
> Fabio M. De Francesco (2):
>   PCI/CXL: Allow PM Init to complete on cxl_bus reset if ACS SV enabled
>   cxl/core: Recover from PM Init failure via cxl_reset_bus_function()
> 
> drivers/cxl/core/pci.c        | 30 ++++++++++++++++++++
>  drivers/cxl/core/port.c       | 22 +++++++++++++++
>  drivers/cxl/cxlpci.h          |  3 ++
>  drivers/pci/pci.c             | 52 ++++++++++++++++++++++++++++++++++-
>  include/linux/pci.h           |  1 + 
>  include/uapi/linux/pci_regs.h |  2 ++
>  6 files changed, 109 insertions(+), 1 deletion(-)
> 

[NDCTL PATCH v3 2/2] cxl: Add check for regions before disabling memdev

[Dave Jiang]

Add a check for memdev disable to see if there are active regions present
before disabling the device. This is necessary now regions are present to
fulfill the TODO that was left there. The best way to determine if a
region is active is to see if there are decoders enabled for the mem
device. This is also best effort as the state is only a snapshot the
kernel provides and is not atomic WRT the memdev disable operation. The
expectation is the admin issuing the command has full control of the mem
device and there are no other agents also attempt to control the device.

Reviewed-by: Quanquan Cao <caoqq@fujitsu.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Add emission of warning for forcing operation. (Quanquan)
v2:
- Warn if active region regardless of -f. (Alison)
- Expound on -f behavior in man page. (Vishal)
---
 Documentation/cxl/cxl-disable-memdev.txt |    4 +++-
 cxl/memdev.c                             |   20 +++++++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/Documentation/cxl/cxl-disable-memdev.txt b/Documentation/cxl/cxl-disable-memdev.txt
index c4edb93ee94a..34b720288705 100644
--- a/Documentation/cxl/cxl-disable-memdev.txt
+++ b/Documentation/cxl/cxl-disable-memdev.txt
@@ -27,7 +27,9 @@ include::bus-option.txt[]
 	a device if the tool determines the memdev is in active usage. Recall
 	that CXL memory ranges might have been established by platform
 	firmware and disabling an active device is akin to force removing
-	memory from a running system.
+	memory from a running system. Going down this path does not offline
+	active memory if they are currently online. User is recommended to
+	offline and disable the appropriate regions before disabling the memdevs.
 
 -v::
 	Turn on verbose debug messages in the library (if libcxl was built with
diff --git a/cxl/memdev.c b/cxl/memdev.c
index 2dd2e7fcc4dd..ed962d478048 100644
--- a/cxl/memdev.c
+++ b/cxl/memdev.c
@@ -437,14 +437,28 @@ static int action_free_dpa(struct cxl_memdev *memdev,
 
 static int action_disable(struct cxl_memdev *memdev, struct action_context *actx)
 {
+	struct cxl_endpoint *ep;
+	struct cxl_port *port;
+
 	if (!cxl_memdev_is_enabled(memdev))
 		return 0;
 
-	if (!param.force) {
-		/* TODO: actually detect rather than assume active */
+	ep = cxl_memdev_get_endpoint(memdev);
+	if (!ep)
+		return -ENODEV;
+
+	port = cxl_endpoint_get_port(ep);
+	if (!port)
+		return -ENODEV;
+
+	if (cxl_port_decoders_committed(port)) {
 		log_err(&ml, "%s is part of an active region\n",
 			cxl_memdev_get_devname(memdev));
-		return -EBUSY;
+		if (!param.force)
+			return -EBUSY;
+
+		log_err(&ml, "Forcing %s disable with an active region!\n",
+			cxl_memdev_get_devname(memdev));
 	}
 
 	return cxl_memdev_disable_invalidate(memdev);

(no subject)

[Yao Xingtao]

Hi Dave,
  I have applied this patch in my env, and done a lot of testing, this
feature is currently working fine. 
  But it is not merged into master branch yet, are there any updates
on this feature?

Associated patches:
https://lore.kernel.org/linux-cxl/170112921107.2687457.2741231995154639197.stgit@djiang5-mobl3/
https://lore.kernel.org/linux-cxl/170120423159.2725915.14670830315829916850.stgit@djiang5-mobl3/

Thanks
Xingtao

Re:

[Verma, Vishal L]

On Wed, 2024-04-17 at 02:46 -0400, Yao Xingtao wrote:
> 
> Hi Dave,
>   I have applied this patch in my env, and done a lot of testing,
> this
> feature is currently working fine. 
>   But it is not merged into master branch yet, are there any updates
> on this feature?

Hi Xingtao,

Turns out that I had applied this to a branch but forgot to merge and
push it. Thanks for the ping - done now, and pushed to pending.

> 
> Associated patches:
> https://lore.kernel.org/linux-cxl/170112921107.2687457.2741231995154639197.stgit@djiang5-mobl3/
> https://lore.kernel.org/linux-cxl/170120423159.2725915.14670830315829916850.stgit@djiang5-mobl3/
> 
> Thanks
> Xingtao

RE: Re:

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Verma, Vishal L <vishal.l.verma@intel.com>
> Sent: Thursday, April 18, 2024 2:14 AM
> To: Jiang, Dave <dave.jiang@intel.com>; Yao, Xingtao/姚 幸涛
> <yaoxt.fnst@fujitsu.com>
> Cc: Cao, Quanquan/曹 全全 <caoqq@fujitsu.com>; linux-cxl@vger.kernel.org;
> nvdimm@lists.linux.dev
> Subject: Re:
> 
> On Wed, 2024-04-17 at 02:46 -0400, Yao Xingtao wrote:
> >
> > Hi Dave,
> >   I have applied this patch in my env, and done a lot of testing,
> > this
> > feature is currently working fine.
> >   But it is not merged into master branch yet, are there any updates
> > on this feature?
> 
> Hi Xingtao,
> 
> Turns out that I had applied this to a branch but forgot to merge and
> push it. Thanks for the ping - done now, and pushed to pending.
Awesome, many thanks!!!

> 
> >
> > Associated patches:
> >
> https://lore.kernel.org/linux-cxl/170112921107.2687457.2741231995154639197.st
> git@djiang5-mobl3/
> >
> https://lore.kernel.org/linux-cxl/170120423159.2725915.14670830315829916850.s
> tgit@djiang5-mobl3/
> >
> > Thanks
> > Xingtao

[PATCH v5 0/5] CXL Poison List Retrieval & Tracing

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

**RESENDING this cover letter previously mis-threaded.

Changes in v5:
- Rebase on cxl/next 
- Use struct_size() to calc mbox cmd payload .min_out
- s/INTERNAL/INJECTED mocked poison record source
- Added Jonathan Reviewed-by tag on Patch 3

Link to v4:
https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/

Add support for retrieving device poison lists and store the returned
error records as kernel trace events.

The handling of the poison list is guided by the CXL 3.0 Specification
Section 8.2.9.8.4.1. [1] 

Example, triggered by memdev:
$ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0

Example, triggered by region:
$ echo 1 > /sys/bus/cxl/devices/region5/trigger_poison_list
cxl_poison: memdev=mem0 pcidev=cxl_mem.0 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
cxl_poison: memdev=mem1 pcidev=cxl_mem.1 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0

[1]: https://www.computeexpresslink.org/download-the-specification

Alison Schofield (5):
  cxl/mbox: Add GET_POISON_LIST mailbox command
  cxl/trace: Add TRACE support for CXL media-error records
  cxl/memdev: Add trigger_poison_list sysfs attribute
  cxl/region: Add trigger_poison_list sysfs attribute
  tools/testing/cxl: Mock support for Get Poison List

 Documentation/ABI/testing/sysfs-bus-cxl | 28 +++++++++
 drivers/cxl/core/mbox.c                 | 78 +++++++++++++++++++++++
 drivers/cxl/core/memdev.c               | 45 ++++++++++++++
 drivers/cxl/core/region.c               | 33 ++++++++++
 drivers/cxl/core/trace.h                | 83 +++++++++++++++++++++++++
 drivers/cxl/cxlmem.h                    | 69 +++++++++++++++++++-
 drivers/cxl/pci.c                       |  4 ++
 tools/testing/cxl/test/mem.c            | 42 +++++++++++++
 8 files changed, 381 insertions(+), 1 deletion(-)


base-commit: 589c3357370a596ef7c99c00baca8ac799fce531
-- 
2.37.3

(no subject)

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Subject: [PATCH v5 0/5] CXL Poison List Retrieval & Tracing
> 
> Changes in v5:
> - Rebase on cxl/next 
> - Use struct_size() to calc mbox cmd payload .min_out
> - s/INTERNAL/INJECTED mocked poison record source
> - Added Jonathan Reviewed-by tag on Patch 3
> 
> Link to v4:
> https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/
> 
> Add support for retrieving device poison lists and store the returned
> error records as kernel trace events.
> 
> The handling of the poison list is guided by the CXL 3.0 Specification
> Section 8.2.9.8.4.1. [1] 
> 
> Example, triggered by memdev:
> $ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
> cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0

I think the pcidev= field wants to be called something like "host" or
"parent", because there is no strict requirement that a 'struct
cxl_memdev' is related to a 'struct pci_dev'. In fact in that example
"cxl_mem.3" is a 'struct platform_device'. Now that I think about it, I
think all CXL device events should be emitting the PCIe serial number
for the memdev.

I will look in the implementation, but do region= and region_uuid= get
populated when mem3 is a member of the region?

> 
> Example, triggered by region:
> $ echo 1 > /sys/bus/cxl/devices/region5/trigger_poison_list
> cxl_poison: memdev=mem0 pcidev=cxl_mem.0 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> cxl_poison: memdev=mem1 pcidev=cxl_mem.1 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> 
> [1]: https://www.computeexpresslink.org/download-the-specification
> 
> Alison Schofield (5):
>   cxl/mbox: Add GET_POISON_LIST mailbox command
>   cxl/trace: Add TRACE support for CXL media-error records
>   cxl/memdev: Add trigger_poison_list sysfs attribute
>   cxl/region: Add trigger_poison_list sysfs attribute
>   tools/testing/cxl: Mock support for Get Poison List
> 
>  Documentation/ABI/testing/sysfs-bus-cxl | 28 +++++++++
>  drivers/cxl/core/mbox.c                 | 78 +++++++++++++++++++++++
>  drivers/cxl/core/memdev.c               | 45 ++++++++++++++
>  drivers/cxl/core/region.c               | 33 ++++++++++
>  drivers/cxl/core/trace.h                | 83 +++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h                    | 69 +++++++++++++++++++-
>  drivers/cxl/pci.c                       |  4 ++
>  tools/testing/cxl/test/mem.c            | 42 +++++++++++++
>  8 files changed, 381 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 589c3357370a596ef7c99c00baca8ac799fce531
> -- 
> 2.37.3
> 

Re:

[Alison Schofield]

On Thu, Jan 26, 2023 at 05:59:03PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Subject: [PATCH v5 0/5] CXL Poison List Retrieval & Tracing
> > 
> > Changes in v5:
> > - Rebase on cxl/next 
> > - Use struct_size() to calc mbox cmd payload .min_out
> > - s/INTERNAL/INJECTED mocked poison record source
> > - Added Jonathan Reviewed-by tag on Patch 3
> > 
> > Link to v4:
> > https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/
> > 
> > Add support for retrieving device poison lists and store the returned
> > error records as kernel trace events.
> > 
> > The handling of the poison list is guided by the CXL 3.0 Specification
> > Section 8.2.9.8.4.1. [1] 
> > 
> > Example, triggered by memdev:
> > $ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
> > cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> 
> I think the pcidev= field wants to be called something like "host" or
> "parent", because there is no strict requirement that a 'struct
> cxl_memdev' is related to a 'struct pci_dev'. In fact in that example
> "cxl_mem.3" is a 'struct platform_device'. Now that I think about it, I
> think all CXL device events should be emitting the PCIe serial number
> for the memdev.
]

Will do, 'host' and add PCIe serial no.

> 
> I will look in the implementation, but do region= and region_uuid= get
> populated when mem3 is a member of the region?

Not always.
In the case above, where the trigger was by memdev, no.
Region= and region_uuid= (and in the follow-on patch, hpa=) only get
populated if the poison was triggered by region, like the case below.

It could be looked up for the by memdev cases. Is that wanted?

Thanks for the reviews Dan!
> 
> > 
> > Example, triggered by region:
> > $ echo 1 > /sys/bus/cxl/devices/region5/trigger_poison_list
> > cxl_poison: memdev=mem0 pcidev=cxl_mem.0 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > cxl_poison: memdev=mem1 pcidev=cxl_mem.1 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > 
> > [1]: https://www.computeexpresslink.org/download-the-specification
> > 
> > Alison Schofield (5):
> >   cxl/mbox: Add GET_POISON_LIST mailbox command
> >   cxl/trace: Add TRACE support for CXL media-error records
> >   cxl/memdev: Add trigger_poison_list sysfs attribute
> >   cxl/region: Add trigger_poison_list sysfs attribute
> >   tools/testing/cxl: Mock support for Get Poison List
> > 
> >  Documentation/ABI/testing/sysfs-bus-cxl | 28 +++++++++
> >  drivers/cxl/core/mbox.c                 | 78 +++++++++++++++++++++++
> >  drivers/cxl/core/memdev.c               | 45 ++++++++++++++
> >  drivers/cxl/core/region.c               | 33 ++++++++++
> >  drivers/cxl/core/trace.h                | 83 +++++++++++++++++++++++++
> >  drivers/cxl/cxlmem.h                    | 69 +++++++++++++++++++-
> >  drivers/cxl/pci.c                       |  4 ++
> >  tools/testing/cxl/test/mem.c            | 42 +++++++++++++
> >  8 files changed, 381 insertions(+), 1 deletion(-)
> > 
> > 
> > base-commit: 589c3357370a596ef7c99c00baca8ac799fce531
> > -- 
> > 2.37.3
> > 
> 
> 

Re:

[Dan Williams]

Alison Schofield wrote:
> On Thu, Jan 26, 2023 at 05:59:03PM -0800, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Subject: [PATCH v5 0/5] CXL Poison List Retrieval & Tracing
> > > 
> > > Changes in v5:
> > > - Rebase on cxl/next 
> > > - Use struct_size() to calc mbox cmd payload .min_out
> > > - s/INTERNAL/INJECTED mocked poison record source
> > > - Added Jonathan Reviewed-by tag on Patch 3
> > > 
> > > Link to v4:
> > > https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/
> > > 
> > > Add support for retrieving device poison lists and store the returned
> > > error records as kernel trace events.
> > > 
> > > The handling of the poison list is guided by the CXL 3.0 Specification
> > > Section 8.2.9.8.4.1. [1] 
> > > 
> > > Example, triggered by memdev:
> > > $ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
> > > cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > 
> > I think the pcidev= field wants to be called something like "host" or
> > "parent", because there is no strict requirement that a 'struct
> > cxl_memdev' is related to a 'struct pci_dev'. In fact in that example
> > "cxl_mem.3" is a 'struct platform_device'. Now that I think about it, I
> > think all CXL device events should be emitting the PCIe serial number
> > for the memdev.
> ]
> 
> Will do, 'host' and add PCIe serial no.
> 
> > 
> > I will look in the implementation, but do region= and region_uuid= get
> > populated when mem3 is a member of the region?
> 
> Not always.
> In the case above, where the trigger was by memdev, no.
> Region= and region_uuid= (and in the follow-on patch, hpa=) only get
> populated if the poison was triggered by region, like the case below.
> 
> It could be looked up for the by memdev cases. Is that wanted?

Just trying to understand the semantics. However, I do think it makes sense
for a memdev trigger to lookup information on all impacted regions
across all of the device's DPA and the region trigger makes sense to
lookup all memdevs, but bounded by the DPA that contributes to that
region. I just want to avoid someone having to trigger the region to get
extra information that was readily available from a memdev listing.

> 
> Thanks for the reviews Dan!
> > 
> > > 
> > > Example, triggered by region:
> > > $ echo 1 > /sys/bus/cxl/devices/region5/trigger_poison_list
> > > cxl_poison: memdev=mem0 pcidev=cxl_mem.0 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > cxl_poison: memdev=mem1 pcidev=cxl_mem.1 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > 
> > > [1]: https://www.computeexpresslink.org/download-the-specification
> > > 
> > > Alison Schofield (5):
> > >   cxl/mbox: Add GET_POISON_LIST mailbox command
> > >   cxl/trace: Add TRACE support for CXL media-error records
> > >   cxl/memdev: Add trigger_poison_list sysfs attribute
> > >   cxl/region: Add trigger_poison_list sysfs attribute
> > >   tools/testing/cxl: Mock support for Get Poison List
> > > 
> > >  Documentation/ABI/testing/sysfs-bus-cxl | 28 +++++++++
> > >  drivers/cxl/core/mbox.c                 | 78 +++++++++++++++++++++++
> > >  drivers/cxl/core/memdev.c               | 45 ++++++++++++++
> > >  drivers/cxl/core/region.c               | 33 ++++++++++
> > >  drivers/cxl/core/trace.h                | 83 +++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h                    | 69 +++++++++++++++++++-
> > >  drivers/cxl/pci.c                       |  4 ++
> > >  tools/testing/cxl/test/mem.c            | 42 +++++++++++++
> > >  8 files changed, 381 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > base-commit: 589c3357370a596ef7c99c00baca8ac799fce531
> > > -- 
> > > 2.37.3
> > > 
> > 
> > 

Re:

[Alison Schofield]

On Fri, Jan 27, 2023 at 11:16:49AM -0800, Dan Williams wrote:
> Alison Schofield wrote:
> > On Thu, Jan 26, 2023 at 05:59:03PM -0800, Dan Williams wrote:
> > > alison.schofield@ wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > 
> > > > Subject: [PATCH v5 0/5] CXL Poison List Retrieval & Tracing
> > > > 
> > > > Changes in v5:
> > > > - Rebase on cxl/next 
> > > > - Use struct_size() to calc mbox cmd payload .min_out
> > > > - s/INTERNAL/INJECTED mocked poison record source
> > > > - Added Jonathan Reviewed-by tag on Patch 3
> > > > 
> > > > Link to v4:
> > > > https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/
> > > > 
> > > > Add support for retrieving device poison lists and store the returned
> > > > error records as kernel trace events.
> > > > 
> > > > The handling of the poison list is guided by the CXL 3.0 Specification
> > > > Section 8.2.9.8.4.1. [1] 
> > > > 
> > > > Example, triggered by memdev:
> > > > $ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
> > > > cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > 
> > > I think the pcidev= field wants to be called something like "host" or
> > > "parent", because there is no strict requirement that a 'struct
> > > cxl_memdev' is related to a 'struct pci_dev'. In fact in that example
> > > "cxl_mem.3" is a 'struct platform_device'. Now that I think about it, I
> > > think all CXL device events should be emitting the PCIe serial number
> > > for the memdev.
> > ]
> > 
> > Will do, 'host' and add PCIe serial no.
> > 
> > > 
> > > I will look in the implementation, but do region= and region_uuid= get
> > > populated when mem3 is a member of the region?
> > 
> > Not always.
> > In the case above, where the trigger was by memdev, no.
> > Region= and region_uuid= (and in the follow-on patch, hpa=) only get
> > populated if the poison was triggered by region, like the case below.
> > 
> > It could be looked up for the by memdev cases. Is that wanted?
> 
> Just trying to understand the semantics. However, I do think it makes sense
> for a memdev trigger to lookup information on all impacted regions
> across all of the device's DPA and the region trigger makes sense to
> lookup all memdevs, but bounded by the DPA that contributes to that
> region. I just want to avoid someone having to trigger the region to get
> extra information that was readily available from a memdev listing.
> 

Dan - 

Confirming my take-away from this email, and our chat:

Remove the by-region trigger_poison_list option entirely. User space
needs to trigger by-memdev the memdevs participating in the region and
filter those events by region.

Add the region info (region name, uuid) to the TRACE_EVENTs when the
poisoned DPA is part of any region.

Alison

> > 
> > Thanks for the reviews Dan!
> > > 
> > > > 
> > > > Example, triggered by region:
> > > > $ echo 1 > /sys/bus/cxl/devices/region5/trigger_poison_list
> > > > cxl_poison: memdev=mem0 pcidev=cxl_mem.0 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > > cxl_poison: memdev=mem1 pcidev=cxl_mem.1 region=region5 region_uuid=bfcb7a29-890e-4a41-8236-fe22221fc75c dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > > 
> > > > [1]: https://www.computeexpresslink.org/download-the-specification
> > > > 
> > > > Alison Schofield (5):
> > > >   cxl/mbox: Add GET_POISON_LIST mailbox command
> > > >   cxl/trace: Add TRACE support for CXL media-error records
> > > >   cxl/memdev: Add trigger_poison_list sysfs attribute
> > > >   cxl/region: Add trigger_poison_list sysfs attribute
> > > >   tools/testing/cxl: Mock support for Get Poison List
> > > > 
> > > >  Documentation/ABI/testing/sysfs-bus-cxl | 28 +++++++++
> > > >  drivers/cxl/core/mbox.c                 | 78 +++++++++++++++++++++++
> > > >  drivers/cxl/core/memdev.c               | 45 ++++++++++++++
> > > >  drivers/cxl/core/region.c               | 33 ++++++++++
> > > >  drivers/cxl/core/trace.h                | 83 +++++++++++++++++++++++++
> > > >  drivers/cxl/cxlmem.h                    | 69 +++++++++++++++++++-
> > > >  drivers/cxl/pci.c                       |  4 ++
> > > >  tools/testing/cxl/test/mem.c            | 42 +++++++++++++
> > > >  8 files changed, 381 insertions(+), 1 deletion(-)
> > > > 
> > > > 
> > > > base-commit: 589c3357370a596ef7c99c00baca8ac799fce531
> > > > -- 
> > > > 2.37.3
> > > > 
> > > 
> > > 
> 
> 

Re:

[Dan Williams]

Alison Schofield wrote:
> On Fri, Jan 27, 2023 at 11:16:49AM -0800, Dan Williams wrote:
> > Alison Schofield wrote:
> > > On Thu, Jan 26, 2023 at 05:59:03PM -0800, Dan Williams wrote:
> > > > alison.schofield@ wrote:
> > > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > > 
> > > > > Subject: [PATCH v5 0/5] CXL Poison List Retrieval & Tracing
> > > > > 
> > > > > Changes in v5:
> > > > > - Rebase on cxl/next 
> > > > > - Use struct_size() to calc mbox cmd payload .min_out
> > > > > - s/INTERNAL/INJECTED mocked poison record source
> > > > > - Added Jonathan Reviewed-by tag on Patch 3
> > > > > 
> > > > > Link to v4:
> > > > > https://lore.kernel.org/linux-cxl/cover.1671135967.git.alison.schofield@intel.com/
> > > > > 
> > > > > Add support for retrieving device poison lists and store the returned
> > > > > error records as kernel trace events.
> > > > > 
> > > > > The handling of the poison list is guided by the CXL 3.0 Specification
> > > > > Section 8.2.9.8.4.1. [1] 
> > > > > 
> > > > > Example, triggered by memdev:
> > > > > $ echo 1 > /sys/bus/cxl/devices/mem3/trigger_poison_list
> > > > > cxl_poison: memdev=mem3 pcidev=cxl_mem.3 region= region_uuid=00000000-0000-0000-0000-000000000000 dpa=0x0 length=0x40 source=Internal flags= overflow_time=0
> > > > 
> > > > I think the pcidev= field wants to be called something like "host" or
> > > > "parent", because there is no strict requirement that a 'struct
> > > > cxl_memdev' is related to a 'struct pci_dev'. In fact in that example
> > > > "cxl_mem.3" is a 'struct platform_device'. Now that I think about it, I
> > > > think all CXL device events should be emitting the PCIe serial number
> > > > for the memdev.
> > > ]
> > > 
> > > Will do, 'host' and add PCIe serial no.
> > > 
> > > > 
> > > > I will look in the implementation, but do region= and region_uuid= get
> > > > populated when mem3 is a member of the region?
> > > 
> > > Not always.
> > > In the case above, where the trigger was by memdev, no.
> > > Region= and region_uuid= (and in the follow-on patch, hpa=) only get
> > > populated if the poison was triggered by region, like the case below.
> > > 
> > > It could be looked up for the by memdev cases. Is that wanted?
> > 
> > Just trying to understand the semantics. However, I do think it makes sense
> > for a memdev trigger to lookup information on all impacted regions
> > across all of the device's DPA and the region trigger makes sense to
> > lookup all memdevs, but bounded by the DPA that contributes to that
> > region. I just want to avoid someone having to trigger the region to get
> > extra information that was readily available from a memdev listing.
> > 
> 
> Dan - 
> 
> Confirming my take-away from this email, and our chat:
> 
> Remove the by-region trigger_poison_list option entirely. User space
> needs to trigger by-memdev the memdevs participating in the region and
> filter those events by region.
> 
> Add the region info (region name, uuid) to the TRACE_EVENTs when the
> poisoned DPA is part of any region.

That's what I was thinking, yes. So the internals of
cxl_mem_get_poison() will take the cxl_region_rwsem for read and compare
the device's endpoint decoder settings against the media error records
to do the region (and later HPA) lookup.