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@xxxxxxx>
> ---
>  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
>