[RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Li Ming]

During review all new patches in branch cxl/next. I noticed there may be
a problem in below commit.

commit a52b6a2c1c99 ("cxl/pci: Support Global Persistent Flush (GPF)")

There is a new field gpf_dvsec in struct cxl_port to cache GPF DVSEC for
Port(DVSEC ID 04h) location. When the first EP attaching to a cxl port,
it will trigger locating GPF DVSEC on the cxl dport which the first EP
is under, then the location is cached in port->gpf_dvsec. So if another
EP under another dport is attaching, it will reuse the value of
port->gpf_dvsec as GPF DVSEC location for this another dport. The
problem is the cached location may be a wrong location for other dports
of the port.

Per Table 8-2 in CXL r3.2 section 8.1.1 and CXL r3.2 section 8.1.6, Each
CXL Downstream switch ports and CXL root ports have their own GPF DVSEC
for Port(DVSEC ID 04h). So my understanding is that CXL subsystem should
locate GPF DVSEC for Port for each dport rather than using the cached
location in CXL port.

But I am not sure if all dports under a same port will have same
configuration space layout, if yes, that will not be a problem. If I am
wrong, please let me know, thanks.

base-commit: 3b5d43245f0a56390baaa670e1b6d898772266b3 cxl/next

Li Ming (3):
  cxl/core: Fix caching dport GPF DVSEC issue
  cxl/pci: Update Port GPF timeout only when the first EP attaching
  cxl/pci: Drop the parameter is_port of cxl_gpf_get_dvsec()

 drivers/cxl/core/core.h |  2 +-
 drivers/cxl/core/pci.c  | 27 ++++++++++++++++-----------
 drivers/cxl/core/port.c |  2 +-
 drivers/cxl/cxl.h       |  6 +++---
 drivers/cxl/pmem.c      |  2 +-
 5 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.34.1

[RFC Patch v1 1/3] cxl/core: Fix caching dport GPF DVSEC issue

[Li Ming]

Per Table 8-2 in CXL r3.2 section 8.1.1 and CXL r3.2 section 8.1.6, only
CXL Downstream switch ports and CXL root ports have GPF DVSEC for CXL
Port(DVSEC ID 04h).

CXL subsystem has a gpf_dvsec in struct cxl_port which is used to cache
the offset of a GPF DVSEC in PCIe configuration space. It will be
updated during the first EP attaching to the cxl_port, so the gpf_dvsec
can only cache the GPF DVSEC offset of the dport which the first EP is
under. Will not have chance to update it during other EPs attaching.
That means CXL subsystem will use the same GPF DVSEC offset for all
dports under the port, it will be a problem if the GPF DVSEC offset
cached in cxl_port is not the right offset for a dport.

Moving gpf_dvsec from struct cxl_port to struct cxl_dport, make every
cxl dport has their own GPF DVSEC offset caching, and each cxl dport
uses its own GPF DVSEC offset for GPF DVSEC accessing.

Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/core.h |  2 +-
 drivers/cxl/core/pci.c  | 16 ++++++++--------
 drivers/cxl/core/port.c |  2 +-
 drivers/cxl/cxl.h       |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index e35f6e08ddb5..a808a3529e8c 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -117,7 +117,7 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
 
 int cxl_ras_init(void);
 void cxl_ras_exit(void);
-int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port);
+int cxl_gpf_port_setup(struct cxl_dport *dport);
 int cxl_acpi_get_extended_linear_cache_size(struct resource *backing_res,
 					    int nid, resource_size_t *size);
 
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 96fecb799cbc..aab0a505d527 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1128,26 +1128,26 @@ static int update_gpf_port_dvsec(struct pci_dev *pdev, int dvsec, int phase)
 	return rc;
 }
 
-int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port)
+int cxl_gpf_port_setup(struct cxl_dport *dport)
 {
 	struct pci_dev *pdev;
 
-	if (!port)
+	if (!dport)
 		return -EINVAL;
 
-	if (!port->gpf_dvsec) {
+	if (!dport->gpf_dvsec) {
 		int dvsec;
 
-		dvsec = cxl_gpf_get_dvsec(dport_dev, true);
+		dvsec = cxl_gpf_get_dvsec(dport->dport_dev, true);
 		if (!dvsec)
 			return -EINVAL;
 
-		port->gpf_dvsec = dvsec;
+		dport->gpf_dvsec = dvsec;
 	}
 
-	pdev = to_pci_dev(dport_dev);
-	update_gpf_port_dvsec(pdev, port->gpf_dvsec, 1);
-	update_gpf_port_dvsec(pdev, port->gpf_dvsec, 2);
+	pdev = to_pci_dev(dport->dport_dev);
+	update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 1);
+	update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 2);
 
 	return 0;
 }
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0fd6646c1a2e..726bd4a7de27 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1678,7 +1678,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			if (rc && rc != -EBUSY)
 				return rc;
 
-			cxl_gpf_port_setup(dport_dev, port);
+			cxl_gpf_port_setup(dport);
 
 			/* Any more ports to add between this one and the root? */
 			if (!dev_is_cxl_root_child(&port->dev))
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index be8a7dc77719..2d81ccd83916 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -592,7 +592,6 @@ struct cxl_dax_region {
  * @cdat: Cached CDAT data
  * @cdat_available: Should a CDAT attribute be available in sysfs
  * @pci_latency: Upstream latency in picoseconds
- * @gpf_dvsec: Cached GPF port DVSEC
  */
 struct cxl_port {
 	struct device dev;
@@ -616,7 +615,6 @@ struct cxl_port {
 	} cdat;
 	bool cdat_available;
 	long pci_latency;
-	int gpf_dvsec;
 };
 
 /**
@@ -664,6 +662,7 @@ struct cxl_rcrb_info {
  * @regs: Dport parsed register blocks
  * @coord: access coordinates (bandwidth and latency performance attributes)
  * @link_latency: calculated PCIe downstream latency
+ * @gpf_dvsec: Cached GPF port DVSEC
  */
 struct cxl_dport {
 	struct device *dport_dev;
@@ -675,6 +674,7 @@ struct cxl_dport {
 	struct cxl_regs regs;
 	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	long link_latency;
+	int gpf_dvsec;
 };
 
 /**
-- 
2.34.1

Re: [RFC Patch v1 1/3] cxl/core: Fix caching dport GPF DVSEC issue

[Davidlohr Bueso]

On Wed, 19 Mar 2025, Li Ming wrote:

>Per Table 8-2 in CXL r3.2 section 8.1.1 and CXL r3.2 section 8.1.6, only
>CXL Downstream switch ports and CXL root ports have GPF DVSEC for CXL
>Port(DVSEC ID 04h).
>
>CXL subsystem has a gpf_dvsec in struct cxl_port which is used to cache
>the offset of a GPF DVSEC in PCIe configuration space. It will be
>updated during the first EP attaching to the cxl_port, so the gpf_dvsec
>can only cache the GPF DVSEC offset of the dport which the first EP is
>under. Will not have chance to update it during other EPs attaching.
>That means CXL subsystem will use the same GPF DVSEC offset for all
>dports under the port, it will be a problem if the GPF DVSEC offset
>cached in cxl_port is not the right offset for a dport.
>
>Moving gpf_dvsec from struct cxl_port to struct cxl_dport, make every
>cxl dport has their own GPF DVSEC offset caching, and each cxl dport
>uses its own GPF DVSEC offset for GPF DVSEC accessing.

This conversion looks good if necessary.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [RFC Patch v1 1/3] cxl/core: Fix caching dport GPF DVSEC issue

[Jonathan Cameron]

On Fri, 21 Mar 2025 00:41:54 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 19 Mar 2025, Li Ming wrote:
> 
> >Per Table 8-2 in CXL r3.2 section 8.1.1 and CXL r3.2 section 8.1.6, only
> >CXL Downstream switch ports and CXL root ports have GPF DVSEC for CXL
> >Port(DVSEC ID 04h).
> >
> >CXL subsystem has a gpf_dvsec in struct cxl_port which is used to cache
> >the offset of a GPF DVSEC in PCIe configuration space. It will be
> >updated during the first EP attaching to the cxl_port, so the gpf_dvsec
> >can only cache the GPF DVSEC offset of the dport which the first EP is
> >under. Will not have chance to update it during other EPs attaching.
> >That means CXL subsystem will use the same GPF DVSEC offset for all
> >dports under the port, it will be a problem if the GPF DVSEC offset
> >cached in cxl_port is not the right offset for a dport.
> >
> >Moving gpf_dvsec from struct cxl_port to struct cxl_dport, make every
> >cxl dport has their own GPF DVSEC offset caching, and each cxl dport
> >uses its own GPF DVSEC offset for GPF DVSEC accessing.  
> 
> This conversion looks good if necessary.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Whilst I somewhat doubt we'll see this in the wild, they could indeed
be different.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[RFC Patch v1 2/3] cxl/pci: Update Port GPF timeout only when the first EP attaching

[Li Ming]

If a CXL switch is under a CXL root port, The Port GPF Phase timeout
will be updated on the CXL root port when each cxl memory device under
the CXL switch is attaching. It is possible to be updated more than
once. Actually, it is enough to initialize once, other extra
initializations are redundant.

When the first EP attaching, it always triggers its ancestor dports to
locate their own Port GPF DVSEC. The change is that updating Port GPF
Phase timeout on these ancestor dports after ancestor dport locating a
Port GPF DVSEC. It guaranttess that Port GPF Phase timeout updating on a
dport only happens during the first EP attaching.

Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/pci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index aab0a505d527..edbdaf1681e8 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1130,12 +1130,11 @@ static int update_gpf_port_dvsec(struct pci_dev *pdev, int dvsec, int phase)
 
 int cxl_gpf_port_setup(struct cxl_dport *dport)
 {
-	struct pci_dev *pdev;
-
 	if (!dport)
 		return -EINVAL;
 
 	if (!dport->gpf_dvsec) {
+		struct pci_dev *pdev;
 		int dvsec;
 
 		dvsec = cxl_gpf_get_dvsec(dport->dport_dev, true);
@@ -1143,11 +1142,10 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
 			return -EINVAL;
 
 		dport->gpf_dvsec = dvsec;
+		pdev = to_pci_dev(dport->dport_dev);
+		update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 1);
+		update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 2);
 	}
 
-	pdev = to_pci_dev(dport->dport_dev);
-	update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 1);
-	update_gpf_port_dvsec(pdev, dport->gpf_dvsec, 2);
-
 	return 0;
 }
-- 
2.34.1

Re: [RFC Patch v1 2/3] cxl/pci: Update Port GPF timeout only when the first EP attaching

[Davidlohr Bueso]

On Wed, 19 Mar 2025, Li Ming wrote:

>If a CXL switch is under a CXL root port, The Port GPF Phase timeout
>will be updated on the CXL root port when each cxl memory device under
>the CXL switch is attaching. It is possible to be updated more than
>once. Actually, it is enough to initialize once, other extra
>initializations are redundant.

It's actually not updated more than necessary because update_gpf_port_dvsec()
checks first:

	if (FIELD_GET(base, ctrl) == GPF_TIMEOUT_BASE_MAX &&
	    FIELD_GET(scale, ctrl) == GPF_TIMEOUT_SCALE_MAX)
		return 0;

>When the first EP attaching, it always triggers its ancestor dports to
>locate their own Port GPF DVSEC. The change is that updating Port GPF
>Phase timeout on these ancestor dports after ancestor dport locating a
>Port GPF DVSEC. It guaranttess that Port GPF Phase timeout updating on a

s/guaranttess/guarantees

>dport only happens during the first EP attaching.

... but yeah, I think this is still better, logically.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

(with the caveat that if patch 1 is not necessary then this would need to
be redone).

Re: [RFC Patch v1 2/3] cxl/pci: Update Port GPF timeout only when the first EP attaching

[Jonathan Cameron]

On Thu, 20 Mar 2025 22:40:07 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 19 Mar 2025, Li Ming wrote:
> 
> >If a CXL switch is under a CXL root port, The Port GPF Phase timeout
> >will be updated on the CXL root port when each cxl memory device under
> >the CXL switch is attaching. It is possible to be updated more than
> >once. Actually, it is enough to initialize once, other extra
> >initializations are redundant.  
> 
> It's actually not updated more than necessary because update_gpf_port_dvsec()
> checks first:
> 
> 	if (FIELD_GET(base, ctrl) == GPF_TIMEOUT_BASE_MAX &&
> 	    FIELD_GET(scale, ctrl) == GPF_TIMEOUT_SCALE_MAX)
> 		return 0;
> 
> >When the first EP attaching, it always triggers its ancestor dports to
> >locate their own Port GPF DVSEC. The change is that updating Port GPF
> >Phase timeout on these ancestor dports after ancestor dport locating a
> >Port GPF DVSEC. It guaranttess that Port GPF Phase timeout updating on a  
> 
> s/guaranttess/guarantees
> 
> >dport only happens during the first EP attaching.  
> 
> ... but yeah, I think this is still better, logically.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Agree that this seems sensible.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> (with the caveat that if patch 1 is not necessary then this would need to
> be redone).

[RFC Patch v1 3/3] cxl/pci: Drop the parameter is_port of cxl_gpf_get_dvsec()

[Li Ming]

The first parameter of cxl_gpf_get_dvsec() is a struct device, can be
used to distinguish if the device is a cxl dport or a cxl pci device by
checking the PCIe type of it, so the parameter is_port is unnecessary
to cxl_gpf_get_dvsec(), using parameter struct device is enough.

Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 drivers/cxl/core/pci.c | 12 +++++++++---
 drivers/cxl/cxl.h      |  2 +-
 drivers/cxl/pmem.c     |  2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index edbdaf1681e8..3b80e9a76ba8 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1072,14 +1072,20 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c)
 #define GPF_TIMEOUT_BASE_MAX 2
 #define GPF_TIMEOUT_SCALE_MAX 7 /* 10 seconds */
 
-u16 cxl_gpf_get_dvsec(struct device *dev, bool is_port)
+u16 cxl_gpf_get_dvsec(struct device *dev)
 {
+	struct pci_dev *pdev;
+	bool is_port = true;
 	u16 dvsec;
 
 	if (!dev_is_pci(dev))
 		return 0;
 
-	dvsec = pci_find_dvsec_capability(to_pci_dev(dev), PCI_VENDOR_ID_CXL,
+	pdev = to_pci_dev(dev);
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ENDPOINT)
+		is_port = false;
+
+	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
 			is_port ? CXL_DVSEC_PORT_GPF : CXL_DVSEC_DEVICE_GPF);
 	if (!dvsec)
 		dev_warn(dev, "%s GPF DVSEC not present\n",
@@ -1137,7 +1143,7 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
 		struct pci_dev *pdev;
 		int dvsec;
 
-		dvsec = cxl_gpf_get_dvsec(dport->dport_dev, true);
+		dvsec = cxl_gpf_get_dvsec(dport->dport_dev);
 		if (!dvsec)
 			return -EINVAL;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 2d81ccd83916..a9ab46eb0610 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -910,6 +910,6 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 #define __mock static
 #endif
 
-u16 cxl_gpf_get_dvsec(struct device *dev, bool is_port);
+u16 cxl_gpf_get_dvsec(struct device *dev);
 
 #endif /* __CXL_H__ */
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index d061fe3d2b86..e197883690ef 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -108,7 +108,7 @@ static void cxl_nvdimm_arm_dirty_shutdown_tracking(struct cxl_nvdimm *cxl_nvd)
 		return;
 	}
 
-	if (!cxl_gpf_get_dvsec(cxlds->dev, false))
+	if (!cxl_gpf_get_dvsec(cxlds->dev))
 		return;
 
 	if (cxl_get_dirty_count(mds, &count)) {
-- 
2.34.1

Re: [RFC Patch v1 3/3] cxl/pci: Drop the parameter is_port of cxl_gpf_get_dvsec()

[Davidlohr Bueso]

On Wed, 19 Mar 2025, Li Ming wrote:

>The first parameter of cxl_gpf_get_dvsec() is a struct device, can be
>used to distinguish if the device is a cxl dport or a cxl pci device by
>checking the PCIe type of it, so the parameter is_port is unnecessary
>to cxl_gpf_get_dvsec(), using parameter struct device is enough.
>
>Signed-off-by: Li Ming <ming.li@zohomail.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [RFC Patch v1 3/3] cxl/pci: Drop the parameter is_port of cxl_gpf_get_dvsec()

[Jonathan Cameron]

On Thu, 20 Mar 2025 22:23:09 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 19 Mar 2025, Li Ming wrote:
> 
> >The first parameter of cxl_gpf_get_dvsec() is a struct device, can be
> >used to distinguish if the device is a cxl dport or a cxl pci device by
> >checking the PCIe type of it, so the parameter is_port is unnecessary
> >to cxl_gpf_get_dvsec(), using parameter struct device is enough.
> >
> >Signed-off-by: Li Ming <ming.li@zohomail.com>  
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Davidlohr Bueso]

On Wed, 19 Mar 2025, Li Ming wrote:

>But I am not sure if all dports under a same port will have same
>configuration space layout, if yes, that will not be a problem. If I am
>wrong, please let me know, thanks.

Yes, when caching the dvsec was suggested, it was my assumption that the
config space would be the same.

Thanks,
Davidlohr

Re: [RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Davidlohr Bueso]

On Thu, 20 Mar 2025, Davidlohr Bueso wrote:

>On Wed, 19 Mar 2025, Li Ming wrote:
>
>>But I am not sure if all dports under a same port will have same
>>configuration space layout, if yes, that will not be a problem. If I am
>>wrong, please let me know, thanks.
>
>Yes, when caching the dvsec was suggested, it was my assumption that the
>config space would be the same.

Ultimately I don't know what the expectation is here, but your updates
do allow more flexibility from vendors, I guess(?). It's a bit late
in the cycle, unfortunately, so if these are to go in for v6.15, they
would be considered a fix imo, otherwise perhaps they are wanted for
v6.16 or not at all (patch 3 does look useful regardless)?

Based on some of the topologies listed in qemu, I did some testing (and
this was also why the same dvsec config layout) and see things working as
expected.

https://www.qemu.org/docs/master/system/devices/cxl.html#example-command-lines

(i) 2 direct-attached Type3 devices:
-next
[    3.938238] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    3.939323] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs
[    4.003074] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:01.0: Port GPF phase 1 timeout: 20 secs
[    4.003676] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:01.0: Port GPF phase 2 timeout: 20 secs
-next+fix
[    3.969841] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    3.970957] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs
[    3.971622] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:01.0: Port GPF phase 1 timeout: 20 secs
[    3.972664] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:01.0: Port GPF phase 2 timeout: 20 secs

(ii) 2 CXL host bridges. Each host bridge has 2 CXL Root Ports, with the CXL Type3 device directly attached:
-next
[    6.182333] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:de:00.0: Port GPF phase 1 timeout: 20 secs
[    6.182352] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:de:00.0: Port GPF phase 2 timeout: 20 secs
[    6.183938] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:01.0: Port GPF phase 1 timeout: 20 secs
[    6.183955] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:01.0: Port GPF phase 2 timeout: 20 secs
[    6.204324] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    6.205407] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs
[    6.210006] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:de:01.0: Port GPF phase 1 timeout: 20 secs
[    6.210921] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:de:01.0: Port GPF phase 2 timeout: 20 secs
-next+fix
[    6.153093] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:01.0: Port GPF phase 1 timeout: 20 secs
[    6.153107] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:01.0: Port GPF phase 2 timeout: 20 secs
[    6.154170] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:de:01.0: Port GPF phase 1 timeout: 20 secs
[    6.154187] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:de:01.0: Port GPF phase 2 timeout: 20 secs
[    6.195279] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:de:00.0: Port GPF phase 1 timeout: 20 secs
[    6.195859] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:de:00.0: Port GPF phase 2 timeout: 20 secs
[    6.255782] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    6.257152] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs

(iii) 4 Type3 devices below a CXL Switch:
-next
[    3.940200] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:01.0: Port GPF phase 1 timeout: 20 secs
[    3.940218] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:01.0: Port GPF phase 2 timeout: 20 secs
[    3.940231] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:00.0: Port GPF phase 1 timeout: 20 secs
[    3.940245] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:00.0: Port GPF phase 2 timeout: 20 secs
[    3.940340] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    3.940350] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs
[    3.948114] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:02.0: Port GPF phase 1 timeout: 20 secs
[    3.949203] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:02.0: Port GPF phase 2 timeout: 20 secs
[    3.997620] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:03.0: Port GPF phase 1 timeout: 20 secs
[    3.997641] cxl_core:update_gpf_port_dvsec:1125: pcieport 0000:0e:03.0: Port GPF phase 2 timeout: 20 secs
-next+fix
[    3.983632] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:01.0: Port GPF phase 1 timeout: 20 secs
[    3.983649] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:01.0: Port GPF phase 2 timeout: 20 secs
[    3.984525] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 1 timeout: 20 secs
[    3.984539] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0c:00.0: Port GPF phase 2 timeout: 20 secs
[    3.984667] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:03.0: Port GPF phase 1 timeout: 20 secs
[    3.984692] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:03.0: Port GPF phase 2 timeout: 20 secs
[    3.985074] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:02.0: Port GPF phase 1 timeout: 20 secs
[    3.985090] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:02.0: Port GPF phase 2 timeout: 20 secs
[    3.988954] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:00.0: Port GPF phase 1 timeout: 20 secs
[    3.990464] cxl_core:update_gpf_port_dvsec:1131: pcieport 0000:0e:00.0: Port GPF phase 2 timeout: 20 secs

Thanks,
Davidlohr

Re: [RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Li Ming]

On 3/21/2025 11:59 AM, Davidlohr Bueso wrote:
> On Thu, 20 Mar 2025, Davidlohr Bueso wrote:
>
>> On Wed, 19 Mar 2025, Li Ming wrote:
>>
>>> But I am not sure if all dports under a same port will have same
>>> configuration space layout, if yes, that will not be a problem. If I am
>>> wrong, please let me know, thanks.
>>
>> Yes, when caching the dvsec was suggested, it was my assumption that the
>> config space would be the same.
>
> Ultimately I don't know what the expectation is here, but your updates
> do allow more flexibility from vendors, I guess(?). It's a bit late
> in the cycle, unfortunately, so if these are to go in for v6.15, they
> would be considered a fix imo, otherwise perhaps they are wanted for
> v6.16 or not at all (patch 3 does look useful regardless)?

My understanding is that the expectation of the patchset is to avoid using a wrong GPF DVSEC in case of dports under a same port have different config space layout. And I think the change is more closely to the description of CXL spec.

If the case(dports under a same port have different config space layout) would not happen, maybe add a comment in cxl_gpf_port_setup() is another option.

Yes, if patch 1 & 2 are considered to be merged, they are worth a fix tag. And patch 3 is an obvious cleanup change.

>
> Based on some of the topologies listed in qemu, I did some testing (and
> this was also why the same dvsec config layout) and see things working as
> expected.

Thanks for testing.


Ming

[snip]

Re: [RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Jonathan Cameron]

On Fri, 21 Mar 2025 14:55:42 +0800
Li Ming <ming.li@zohomail.com> wrote:

> On 3/21/2025 11:59 AM, Davidlohr Bueso wrote:
> > On Thu, 20 Mar 2025, Davidlohr Bueso wrote:
> >  
> >> On Wed, 19 Mar 2025, Li Ming wrote:
> >>  
> >>> But I am not sure if all dports under a same port will have same
> >>> configuration space layout, if yes, that will not be a problem. If I am
> >>> wrong, please let me know, thanks.  
> >>
> >> Yes, when caching the dvsec was suggested, it was my assumption that the
> >> config space would be the same.  
> >
> > Ultimately I don't know what the expectation is here, but your updates
> > do allow more flexibility from vendors, I guess(?). It's a bit late
> > in the cycle, unfortunately, so if these are to go in for v6.15, they
> > would be considered a fix imo, otherwise perhaps they are wanted for
> > v6.16 or not at all (patch 3 does look useful regardless)?  
> 
> My understanding is that the expectation of the patchset is to avoid using a wrong GPF DVSEC in case of dports under a same port have different config space layout. And I think the change is more closely to the description of CXL spec.
> 
> If the case(dports under a same port have different config space layout) would not happen, maybe add a comment in cxl_gpf_port_setup() is another option.
> 
> Yes, if patch 1 & 2 are considered to be merged, they are worth a fix tag. And patch 3 is an obvious cleanup change.

I think they can indeed have different layout (in theory).
Seems moderately unlikely to occur in real devices, but you never know.

So I think a fixes tag would be valid.

Jonathan

> 
> >
> > Based on some of the topologies listed in qemu, I did some testing (and
> > this was also why the same dvsec config layout) and see things working as
> > expected.  
> 
> Thanks for testing.
> 
> 
> Ming
> 
> [snip]
> 

Re: [RFC Patch v1 0/3] Fix using wrong GPF DVSEC location issue

[Dan Williams]

Li Ming wrote:
> During review all new patches in branch cxl/next. I noticed there may be
> a problem in below commit.
> 
> commit a52b6a2c1c99 ("cxl/pci: Support Global Persistent Flush (GPF)")
> 
> There is a new field gpf_dvsec in struct cxl_port to cache GPF DVSEC for
> Port(DVSEC ID 04h) location. When the first EP attaching to a cxl port,
> it will trigger locating GPF DVSEC on the cxl dport which the first EP
> is under, then the location is cached in port->gpf_dvsec. So if another
> EP under another dport is attaching, it will reuse the value of
> port->gpf_dvsec as GPF DVSEC location for this another dport. The
> problem is the cached location may be a wrong location for other dports
> of the port.
> 
> Per Table 8-2 in CXL r3.2 section 8.1.1 and CXL r3.2 section 8.1.6, Each
> CXL Downstream switch ports and CXL root ports have their own GPF DVSEC
> for Port(DVSEC ID 04h). So my understanding is that CXL subsystem should
> locate GPF DVSEC for Port for each dport rather than using the cached
> location in CXL port.
> 
> But I am not sure if all dports under a same port will have same
> configuration space layout, if yes, that will not be a problem. If I am
> wrong, please let me know, thanks.
> 
> base-commit: 3b5d43245f0a56390baaa670e1b6d898772266b3 cxl/next
> 
> Li Ming (3):
>   cxl/core: Fix caching dport GPF DVSEC issue
>   cxl/pci: Update Port GPF timeout only when the first EP attaching
>   cxl/pci: Drop the parameter is_port of cxl_gpf_get_dvsec()

These look good. I doubt a device would build different offsets
per-dport, but the real risk is more in confusing future code readers
that dports are uniform.

For the series,

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

A Fixes tag seems reasonable.