[PATCH 0/2] cxl/pci: Inactive downstream port handling

[PATCH 0/2] cxl/pci: Inactive downstream port handling

[Robert Richter]

A small series with individual patches to handle inactive downstream
ports during enumeration. First patch changes downstream port
enumeration to ignore those with duplicate port IDs. Second patch only
enables active downstream ports with the link status up.

Patches are independent each and can be applied individually.

Robert Richter (2):
  cxl/pci: Ignore downstream ports with duplicate port IDs
  cxl/pci: Check link status and only enable active dports

 drivers/cxl/core/pci.c  | 40 ++++++++++++++++++++++++++++++++++------
 drivers/cxl/core/port.c |  2 +-
 2 files changed, 35 insertions(+), 7 deletions(-)


-- 
2.39.5

[PATCH 1/2] cxl/pci: Ignore downstream ports with duplicate port IDs

[Robert Richter]

If a link is inactive, the port ID in the PCIe Link Capability
Register of a downstream port may not be assigned yet. Another
downstream port with an inactive link on the same Downstream Switch
Port may have the same port ID. In this case the port enumeration of
the root or downstream port fails due to duplicate port IDs
(devm_cxl_port_enumerate_dports()/add_dport()).

Relax the check and just ignore downstream ports with duplicate port
IDs. Do not fail and continue to enumerate all downstream ports of a
CXL Root Port or CXL Switch. Turn the related dev_err() messages into
a dev_dbg().

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c  | 10 ++++++++--
 drivers/cxl/core/port.c |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index fbc50b1156b8..524b8749cc0b 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -59,8 +59,14 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
 	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
 	if (IS_ERR(dport)) {
-		ctx->error = PTR_ERR(dport);
-		return PTR_ERR(dport);
+		rc = PTR_ERR(dport);
+		if (rc == -EBUSY) {
+			dev_dbg(&port->dev, "failed to add dport %s, continuing\n",
+				dev_name(&pdev->dev));
+			return 0;
+		}
+		ctx->error = rc;
+		return rc;
 	}
 	ctx->count++;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 33607301c5d3..8038cbeffbf7 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1071,7 +1071,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
 	device_lock_assert(&port->dev);
 	dup = find_dport(port, dport->port_id);
 	if (dup) {
-		dev_err(&port->dev,
+		dev_dbg(&port->dev,
 			"unable to add dport%d-%s non-unique port id (%s)\n",
 			dport->port_id, dev_name(dport->dport_dev),
 			dev_name(dup->dport_dev));
-- 
2.39.5

Re: [PATCH 1/2] cxl/pci: Ignore downstream ports with duplicate port IDs

[Ira Weiny]

Robert Richter wrote:
> If a link is inactive, the port ID in the PCIe Link Capability
> Register of a downstream port may not be assigned yet. Another
> downstream port with an inactive link on the same Downstream Switch
> Port may have the same port ID.

Is it possible that an active link would have the same ID?

I'm not clear why failing with a duplicate port ID is a bad thing.

>
> In this case the port enumeration of
> the root or downstream port fails due to duplicate port IDs
> (devm_cxl_port_enumerate_dports()/add_dport()).
> 
> Relax the check and just ignore downstream ports with duplicate port
> IDs.

Ah.  So do not add the dport...

It may not matter but I __think__ this adds a subtle memory leak where the
dport object is allocated, not added to the xarray, and upon the port
being probed later a new dport object is allocated in it's place.  That
might be ok as the memory will be recovered when the switch device is
destroyed (via devm).  But could a series of unplug/hotplugs cause issues?

Ira

>
> Do not fail and continue to enumerate all downstream ports of a
> CXL Root Port or CXL Switch. Turn the related dev_err() messages into
> a dev_dbg().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c  | 10 ++++++++--
>  drivers/cxl/core/port.c |  2 +-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index fbc50b1156b8..524b8749cc0b 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -59,8 +59,14 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>  	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
>  	if (IS_ERR(dport)) {
> -		ctx->error = PTR_ERR(dport);
> -		return PTR_ERR(dport);
> +		rc = PTR_ERR(dport);
> +		if (rc == -EBUSY) {
> +			dev_dbg(&port->dev, "failed to add dport %s, continuing\n",
> +				dev_name(&pdev->dev));
> +			return 0;
> +		}
> +		ctx->error = rc;
> +		return rc;
>  	}
>  	ctx->count++;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 33607301c5d3..8038cbeffbf7 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1071,7 +1071,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>  	device_lock_assert(&port->dev);
>  	dup = find_dport(port, dport->port_id);
>  	if (dup) {
> -		dev_err(&port->dev,
> +		dev_dbg(&port->dev,
>  			"unable to add dport%d-%s non-unique port id (%s)\n",
>  			dport->port_id, dev_name(dport->dport_dev),
>  			dev_name(dup->dport_dev));
> -- 
> 2.39.5
> 

Re: [PATCH 1/2] cxl/pci: Ignore downstream ports with duplicate port IDs

[Robert Richter]

Hi Ira,

thanks for your review.

On 05.03.25 09:09:52, Ira Weiny wrote:
> Robert Richter wrote:
> > If a link is inactive, the port ID in the PCIe Link Capability
> > Register of a downstream port may not be assigned yet. Another
> > downstream port with an inactive link on the same Downstream Switch
> > Port may have the same port ID.
> 
> Is it possible that an active link would have the same ID?
> 
> I'm not clear why failing with a duplicate port ID is a bad thing.
> 
> >
> > In this case the port enumeration of
> > the root or downstream port fails due to duplicate port IDs
> > (devm_cxl_port_enumerate_dports()/add_dport()).
> > 
> > Relax the check and just ignore downstream ports with duplicate port
> > IDs.
> 
> Ah.  So do not add the dport...
> 
> It may not matter but I __think__ this adds a subtle memory leak where the
> dport object is allocated, not added to the xarray, and upon the port
> being probed later a new dport object is allocated in it's place.  That
> might be ok as the memory will be recovered when the switch device is
> destroyed (via devm).  But could a series of unplug/hotplugs cause issues?

No, this patches do not change anything regarding devm allocation.

I have looked into __devm_cxl_add_dport(). If the function returns an
error, there might be allocated memory left which is not released
before the host device is released. This is current implementation and
these patches do not change anything here. If at all, it is a general
issue with the devm implementation in that function (and probably not
only there).

-Robert

Re: [PATCH 1/2] cxl/pci: Ignore downstream ports with duplicate port IDs

[Jonathan Cameron]

On Wed, 5 Mar 2025 11:01:22 +0100
Robert Richter <rrichter@amd.com> wrote:

> If a link is inactive, the port ID in the PCIe Link Capability
> Register of a downstream port may not be assigned yet. Another
> downstream port with an inactive link on the same Downstream Switch
> Port may have the same port ID. In this case the port enumeration of
> the root or downstream port fails due to duplicate port IDs
> (devm_cxl_port_enumerate_dports()/add_dport()).

Obviously it's a bit irrelevant if there is hardware doing that, but
can you add a PCI spec reference that says the port ID may not be configured.

In my (often wrong) mental model, these are fixed values. It's marked
HwInit and text for that talks about conventional reset (which I'm not
sure applies here).  I can't find any other references to when you
are allowed to read this.

Anyhow I'd like to know if this is a quirk or a bug fix for compliant
hardware.

Jonathan


> 
> Relax the check and just ignore downstream ports with duplicate port
> IDs. Do not fail and continue to enumerate all downstream ports of a
> CXL Root Port or CXL Switch. Turn the related dev_err() messages into
> a dev_dbg().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c  | 10 ++++++++--
>  drivers/cxl/core/port.c |  2 +-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index fbc50b1156b8..524b8749cc0b 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -59,8 +59,14 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>  	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
>  	if (IS_ERR(dport)) {
> -		ctx->error = PTR_ERR(dport);
> -		return PTR_ERR(dport);
> +		rc = PTR_ERR(dport);
> +		if (rc == -EBUSY) {
> +			dev_dbg(&port->dev, "failed to add dport %s, continuing\n",
> +				dev_name(&pdev->dev));
> +			return 0;
> +		}
> +		ctx->error = rc;
> +		return rc;
>  	}
>  	ctx->count++;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 33607301c5d3..8038cbeffbf7 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1071,7 +1071,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>  	device_lock_assert(&port->dev);
>  	dup = find_dport(port, dport->port_id);
>  	if (dup) {
> -		dev_err(&port->dev,
> +		dev_dbg(&port->dev,
>  			"unable to add dport%d-%s non-unique port id (%s)\n",
>  			dport->port_id, dev_name(dport->dport_dev),
>  			dev_name(dup->dport_dev));

[PATCH 2/2] cxl/pci: Check link status and only enable active dports

[Robert Richter]

When downstream ports are enumerated, some of them may not be
connected to a corresponding endpoint or upstream switch port. The
dport is inactive and its link status is down then. For permanently
disabled ports a HwInit configuration mechanism (set by hardware or
firmware) may assign a (further unused) default port number. The port
number may be set to the same value accross other inactive links.
Those duplicate port numbers cause the downstream port enumeration to
fail including the root or switch port initialization
(cxl_switch_port_probe()) and all its active downstream ports.

Prevent a port initialization failure by checking the link status and
only enabling active dports. If a dport is inactive, there is no
matching component (endpoint or switch) connected to and thus, it must
not be enumerated and added to the kernel's CXL device hierarchy.
There is no device that will connect to an inactive dport.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 524b8749cc0b..72683e1b41e3 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -32,6 +32,26 @@ struct cxl_walk_context {
 	int count;
 };
 
+static int get_port_num(struct pci_dev *pdev)
+{
+	u32 lnkcap, port_num;
+	u16 lnksta;
+
+	if (pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &lnkcap))
+		return -ENXIO;
+
+	/* Skip inactive links */
+	if (lnkcap & PCI_EXP_LNKCAP_DLLLARC) {
+		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
+		if (!(lnksta & PCI_EXP_LNKSTA_DLLLA))
+			return -ENOENT;
+	}
+
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+
+	return port_num;
+}
+
 static int match_add_dports(struct pci_dev *pdev, void *data)
 {
 	struct cxl_walk_context *ctx = data;
@@ -39,7 +59,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 	int type = pci_pcie_type(pdev);
 	struct cxl_register_map map;
 	struct cxl_dport *dport;
-	u32 lnkcap, port_num;
+	int port_num;
 	int rc;
 
 	if (pdev->bus != ctx->bus)
@@ -48,15 +68,17 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		return 0;
 	if (type != ctx->type)
 		return 0;
-	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
-				  &lnkcap))
+
+	port_num = get_port_num(pdev);
+	if (port_num == -ENOENT)
+		pci_dbg(pdev, "Skipping dport, link is down\n");
+	if (port_num < 0)
 		return 0;
 
 	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
 	if (rc)
 		dev_dbg(&port->dev, "failed to find component registers\n");
 
-	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
 	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
 	if (IS_ERR(dport)) {
 		rc = PTR_ERR(dport);
-- 
2.39.5

Re: [PATCH 2/2] cxl/pci: Check link status and only enable active dports

[Ira Weiny]

Robert Richter wrote:
> When downstream ports are enumerated, some of them may not be
> connected to a corresponding endpoint or upstream switch port. The
> dport is inactive and its link status is down then. For permanently
> disabled ports a HwInit configuration mechanism (set by hardware or
> firmware) may assign a (further unused) default port number. The port
> number may be set to the same value accross other inactive links.
> Those duplicate port numbers cause the downstream port enumeration to
> fail including the root or switch port initialization
> (cxl_switch_port_probe()) and all its active downstream ports.
> 
> Prevent a port initialization failure by checking the link status and
> only enabling active dports. If a dport is inactive, there is no
> matching component (endpoint or switch) connected to and thus, it must
> not be enumerated and added to the kernel's CXL device hierarchy.
> There is no device that will connect to an inactive dport.

This makes much more sense.

Wouldn't it be better to use this patch and leave the old error
messages/failures on duplicate port ids?  IOW drop patch 1?

What happens if a link is having issues (perhaps going up and down) and
RAS events fire for this dport?  Does the lack of a dport object cause
issues?

Ira

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 524b8749cc0b..72683e1b41e3 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -32,6 +32,26 @@ struct cxl_walk_context {
>  	int count;
>  };
>  
> +static int get_port_num(struct pci_dev *pdev)
> +{
> +	u32 lnkcap, port_num;
> +	u16 lnksta;
> +
> +	if (pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &lnkcap))
> +		return -ENXIO;
> +
> +	/* Skip inactive links */
> +	if (lnkcap & PCI_EXP_LNKCAP_DLLLARC) {
> +		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
> +		if (!(lnksta & PCI_EXP_LNKSTA_DLLLA))
> +			return -ENOENT;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +
> +	return port_num;
> +}
> +
>  static int match_add_dports(struct pci_dev *pdev, void *data)
>  {
>  	struct cxl_walk_context *ctx = data;
> @@ -39,7 +59,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  	int type = pci_pcie_type(pdev);
>  	struct cxl_register_map map;
>  	struct cxl_dport *dport;
> -	u32 lnkcap, port_num;
> +	int port_num;
>  	int rc;
>  
>  	if (pdev->bus != ctx->bus)
> @@ -48,15 +68,17 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  		return 0;
>  	if (type != ctx->type)
>  		return 0;
> -	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> -				  &lnkcap))
> +
> +	port_num = get_port_num(pdev);
> +	if (port_num == -ENOENT)
> +		pci_dbg(pdev, "Skipping dport, link is down\n");
> +	if (port_num < 0)
>  		return 0;
>  
>  	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>  	if (rc)
>  		dev_dbg(&port->dev, "failed to find component registers\n");
>  
> -	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>  	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
>  	if (IS_ERR(dport)) {
>  		rc = PTR_ERR(dport);
> -- 
> 2.39.5
> 

Re: [PATCH 2/2] cxl/pci: Check link status and only enable active dports

[Robert Richter]

On 05.03.25 09:19:59, Ira Weiny wrote:
> Robert Richter wrote:
> > When downstream ports are enumerated, some of them may not be
> > connected to a corresponding endpoint or upstream switch port. The
> > dport is inactive and its link status is down then. For permanently
> > disabled ports a HwInit configuration mechanism (set by hardware or
> > firmware) may assign a (further unused) default port number. The port
> > number may be set to the same value accross other inactive links.
> > Those duplicate port numbers cause the downstream port enumeration to
> > fail including the root or switch port initialization
> > (cxl_switch_port_probe()) and all its active downstream ports.
> > 
> > Prevent a port initialization failure by checking the link status and
> > only enabling active dports. If a dport is inactive, there is no
> > matching component (endpoint or switch) connected to and thus, it must
> > not be enumerated and added to the kernel's CXL device hierarchy.
> > There is no device that will connect to an inactive dport.
> 
> This makes much more sense.
> 
> Wouldn't it be better to use this patch and leave the old error
> messages/failures on duplicate port ids?  IOW drop patch 1?

The link check only works if Data Link Layer Link Active Reporting is
supported. That is why there is the patch 1. That patch also handles
cases where dups are seen that are unrelated to the link status.

> What happens if a link is having issues (perhaps going up and down) and
> RAS events fire for this dport?  Does the lack of a dport object cause
> issues?

Once a dport was enumerated it exists as long as the parent exists.

-Robert

Re: [PATCH 2/2] cxl/pci: Check link status and only enable active dports

[Ira Weiny]

Robert Richter wrote:
> On 05.03.25 09:19:59, Ira Weiny wrote:
> > Robert Richter wrote:
> > > When downstream ports are enumerated, some of them may not be
> > > connected to a corresponding endpoint or upstream switch port. The
> > > dport is inactive and its link status is down then. For permanently
> > > disabled ports a HwInit configuration mechanism (set by hardware or
> > > firmware) may assign a (further unused) default port number. The port
> > > number may be set to the same value accross other inactive links.
> > > Those duplicate port numbers cause the downstream port enumeration to
> > > fail including the root or switch port initialization
> > > (cxl_switch_port_probe()) and all its active downstream ports.
> > > 
> > > Prevent a port initialization failure by checking the link status and
> > > only enabling active dports. If a dport is inactive, there is no
> > > matching component (endpoint or switch) connected to and thus, it must
> > > not be enumerated and added to the kernel's CXL device hierarchy.
> > > There is no device that will connect to an inactive dport.
> > 
> > This makes much more sense.
> > 
> > Wouldn't it be better to use this patch and leave the old error
> > messages/failures on duplicate port ids?  IOW drop patch 1?
> 
> The link check only works if Data Link Layer Link Active Reporting is
> supported. That is why there is the patch 1. That patch also handles
> cases where dups are seen that are unrelated to the link status.

Isn't it a bug in the hardware to have 2 active dports with the same ID?

> 
> > What happens if a link is having issues (perhaps going up and down) and
> > RAS events fire for this dport?  Does the lack of a dport object cause
> > issues?
> 
> Once a dport was enumerated it exists as long as the parent exists.

Yes but do we care enough about this unused memory being held until the
destruction of the device object?

Currently I think it all get's torn down which means we are not keeping
memory around.

Ira

Re: [PATCH 2/2] cxl/pci: Check link status and only enable active dports

[Jonathan Cameron]

On Wed, 5 Mar 2025 11:01:23 +0100
Robert Richter <rrichter@amd.com> wrote:

> When downstream ports are enumerated, some of them may not be
> connected to a corresponding endpoint or upstream switch port. The
> dport is inactive and its link status is down then. For permanently
> disabled ports a HwInit configuration mechanism (set by hardware or
> firmware) may assign a (further unused) default port number. The port
> number may be set to the same value accross other inactive links.

Spec reference needed.  This sounds like it came from the spec but
searching for it isn't finding anything.

> Those duplicate port numbers cause the downstream port enumeration to
> fail including the root or switch port initialization
> (cxl_switch_port_probe()) and all its active downstream ports.
> 
> Prevent a port initialization failure by checking the link status and
> only enabling active dports. If a dport is inactive, there is no
> matching component (endpoint or switch) connected to and thus, it must
> not be enumerated and added to the kernel's CXL device hierarchy.
> There is no device that will connect to an inactive dport.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 524b8749cc0b..72683e1b41e3 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -32,6 +32,26 @@ struct cxl_walk_context {
>  	int count;
>  };
>  
> +static int get_port_num(struct pci_dev *pdev)
> +{
> +	u32 lnkcap, port_num;
> +	u16 lnksta;
> +
> +	if (pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &lnkcap))
> +		return -ENXIO;
> +
> +	/* Skip inactive links */
> +	if (lnkcap & PCI_EXP_LNKCAP_DLLLARC) {
> +		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
> +		if (!(lnksta & PCI_EXP_LNKSTA_DLLLA))
> +			return -ENOENT;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +
> +	return port_num;
> +}
> +
>  static int match_add_dports(struct pci_dev *pdev, void *data)
>  {
>  	struct cxl_walk_context *ctx = data;
> @@ -39,7 +59,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  	int type = pci_pcie_type(pdev);
>  	struct cxl_register_map map;
>  	struct cxl_dport *dport;
> -	u32 lnkcap, port_num;
> +	int port_num;
>  	int rc;
>  
>  	if (pdev->bus != ctx->bus)
> @@ -48,15 +68,17 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  		return 0;
>  	if (type != ctx->type)
>  		return 0;
> -	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> -				  &lnkcap))
> +
> +	port_num = get_port_num(pdev);
> +	if (port_num == -ENOENT)
> +		pci_dbg(pdev, "Skipping dport, link is down\n");
> +	if (port_num < 0)
>  		return 0;
>  
>  	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>  	if (rc)
>  		dev_dbg(&port->dev, "failed to find component registers\n");
>  
> -	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>  	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
>  	if (IS_ERR(dport)) {
>  		rc = PTR_ERR(dport);

Re: [PATCH 0/2] cxl/pci: Inactive downstream port handling

[Dan Williams]

Robert Richter wrote:
> A small series with individual patches to handle inactive downstream
> ports during enumeration. First patch changes downstream port
> enumeration to ignore those with duplicate port IDs. Second patch only
> enables active downstream ports with the link status up.
> 
> Patches are independent each and can be applied individually.
> 
> Robert Richter (2):
>   cxl/pci: Ignore downstream ports with duplicate port IDs
>   cxl/pci: Check link status and only enable active dports

Both of these problems are to addressed by work in progress patches to
delay dport enumeration until a cxl_memdev is registered beneath that
leg of CXL topology.

I would prefer to focus on that solution and skip these band-aids in
the near term unless there is an urgent need that makes it clear that
waiting for v6.16 is not tenable.

Re: [PATCH 0/2] cxl/pci: Inactive downstream port handling

[Robert Richter]

Hi Dan,

thanks for review.

On 05.03.25 11:06:52, Dan Williams wrote:
> Robert Richter wrote:
> > A small series with individual patches to handle inactive downstream
> > ports during enumeration. First patch changes downstream port
> > enumeration to ignore those with duplicate port IDs. Second patch only
> > enables active downstream ports with the link status up.
> > 
> > Patches are independent each and can be applied individually.
> > 
> > Robert Richter (2):
> >   cxl/pci: Ignore downstream ports with duplicate port IDs
> >   cxl/pci: Check link status and only enable active dports
> 
> Both of these problems are to addressed by work in progress patches to
> delay dport enumeration until a cxl_memdev is registered beneath that
> leg of CXL topology.
> 
> I would prefer to focus on that solution and skip these band-aids in
> the near term unless there is an urgent need that makes it clear that
> waiting for v6.16 is not tenable.

Port ids could be set only by hardware and there will be no other way
then, than the driver to handle the duplicates. Relaxing the check
looks reasonable to prevent the whole port being shut down. There are
other cases where dport enumeration also just continues, e.g. if the
link capablity cannot be read or the component registers do not exist.

The delayed port enumeration series will hide duplicates too (not yet
tested), but since this is marked RFC and 'long term fix', how about
having those patches first and then update them with the delayed port
enumeration patches? The duplicate port handling could be changed
again or even made obsolete. Otherwise, until then, the kernel fails
to enumerate CXL devices.

Thanks,

-Robert

Re: [PATCH 0/2] cxl/pci: Inactive downstream port handling

[Dan Williams]

Robert Richter wrote:
> Hi Dan,
> 
> thanks for review.
> 
> On 05.03.25 11:06:52, Dan Williams wrote:
> > Robert Richter wrote:
> > > A small series with individual patches to handle inactive downstream
> > > ports during enumeration. First patch changes downstream port
> > > enumeration to ignore those with duplicate port IDs. Second patch only
> > > enables active downstream ports with the link status up.
> > > 
> > > Patches are independent each and can be applied individually.
> > > 
> > > Robert Richter (2):
> > >   cxl/pci: Ignore downstream ports with duplicate port IDs
> > >   cxl/pci: Check link status and only enable active dports
> > 
> > Both of these problems are to addressed by work in progress patches to
> > delay dport enumeration until a cxl_memdev is registered beneath that
> > leg of CXL topology.
> > 
> > I would prefer to focus on that solution and skip these band-aids in
> > the near term unless there is an urgent need that makes it clear that
> > waiting for v6.16 is not tenable.
> 
> Port ids could be set only by hardware and there will be no other way
> then, than the driver to handle the duplicates. Relaxing the check
> looks reasonable to prevent the whole port being shut down. There are
> other cases where dport enumeration also just continues, e.g. if the
> link capablity cannot be read or the component registers do not exist.
> 
> The delayed port enumeration series will hide duplicates too (not yet
> tested), but since this is marked RFC and 'long term fix', how about
> having those patches first and then update them with the delayed port
> enumeration patches? The duplicate port handling could be changed
> again or even made obsolete. Otherwise, until then, the kernel fails
> to enumerate CXL devices.

Right, but this is something that has apparently be tolerable for
several kernel cycles. So the question is one more kernel cycle to
develop the comprehensive solution, or put in a band-aid that still
leaves dports in a broken state relative to hotplug.

I would prefer to not need to onboard short-term debt, unless this there
is another mitigating factor that increases the priority. Something
like, "on platform X the kernel hits this much more frequently than
platform Y". Essentially, clarify the end user impact of not addressing
this immediately with the half-step solution.