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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67c869403a7d7_77ff42941b@iweiny-mobl.notmuch>
Date: Wed, 5 Mar 2025 09:09:52 -0600
From: Ira Weiny <ira.weiny@...el.com>
To: Robert Richter <rrichter@....com>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ira
 Weiny" <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dave Jiang
	<dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Gregory Price
	<gourry@...rry.net>, Terry Bowman <terry.bowman@....com>, Robert Richter
	<rrichter@....com>
Subject: Re: [PATCH 1/2] cxl/pci: Ignore downstream ports with duplicate port
 IDs

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