Re: [PATCH v17 02/19] cxl: Share CXL port upstream PCI device lookup

[Li Ming <ming.li@zohomail.com>]

在 2026/11/6 02:59, Srirangan Madhavan 写道:
> read_cdat_data() resolves a CXL port's upstream device to its backing PCI
> device. HDM cache updates need the same mapping.
>
> Factor the lookup into cxl_port_get_uport_pci_dev() and return a referenced
> PCI device to make caller ownership explicit.
>
> Use automatic reference cleanup in read_cdat_data(), as suggested by
> Li Ming, so early returns release the PCI device reference.
>
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
>
> ---
> Changes in v17:
> - Use an explicit pci_dev_put() on a single exit path in
>    read_cdat_data() instead of mixing __free() with goto cleanup
>    (Sashiko)
>    https://sashiko.dev/#/patchset/20261029145703.2185231-1-smadhavan@nvidia.com?part=2

Reviewed-by: Li Ming <ming.li@zohomail.com>


I still think the new goto makes the code harder to follow. But I saw 
your discussion in v13[1], both Jonathan and Dave agree with your 
current changes. I post my proposal here which can eliminate all goto 
labels. If there is no response from them, or if they disagree with my 
changes, we can continue with your implementation.

[1]: 
https://lore.kernel.org/all/fb513893-7c6c-408c-811f-1eae20f0436c@intel.com/

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9d807c1a002c..fbf4b56a39c4 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -573,17 +573,8 @@ void read_cdat_data(struct cxl_port *port)
         size_t table_length, length;
         int rc;

-       if (is_cxl_memdev(uport)) {
-               struct device *host;
-
-               cxlmd = to_cxl_memdev(uport);
-               host = cxlmd->dev.parent;
-               if (dev_is_pci(host))
-                       pdev = to_pci_dev(host);
-       } else if (dev_is_pci(uport)) {
-               pdev = to_pci_dev(uport);
-       }
-
+       struct pci_dev *pdev __free(pci_dev_put) =
+                               cxl_port_get_uport_pci_dev(port);
         if (!pdev)
                 return;

@@ -606,30 +597,32 @@ void read_cdat_data(struct cxl_port *port)
          * bytes for the DOE header. Table data starts afterwards.
          */
         buf = devm_kzalloc(dev, sizeof(*buf) + length, GFP_KERNEL);
-       if (!buf)
-               goto err;
+       if (!buf) {
+               dev_err(dev, "Failed to allocate CDAT buffer.\n");
+               return;
+       }

         table_length = length;

         rc = cxl_cdat_read_table(dev, doe_mb, buf, &length);
-       if (rc)
-               goto err;
+       if (rc) {
+               devm_kfree(dev, buf);
+               dev_err(dev, "Failed to read CDAT.\n");
+               return;
+       }

         if (table_length != length)
                 dev_warn(dev, "Malformed CDAT table length (%zu:%zu), 
discarding trailing data\n",
                         table_length, length);

-       if (cdat_checksum(buf->data, length))
-               goto err;
+       if (cdat_checksum(buf->data, length)) {
+               devm_kfree(dev, buf);
+               dev_err(dev, "Failed to validate CDAT.\n");
+               return;
+       }

         port->cdat.table = buf->data;
         port->cdat.length = length;
-
-       return;
-err:
-       /* Don't leave table data allocated on error */
-       devm_kfree(dev, buf);
-       dev_err(dev, "Failed to read/validate CDAT.\n");
  }


> ---
>   drivers/cxl/core/core.h |  1 +
>   drivers/cxl/core/pci.c  | 24 ++++++------------------
>   drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
>   3 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 35eaf636adc9..983d7690c3a5 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -157,6 +157,7 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
>   int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c);
>   int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
>   					struct access_coordinate *c);
> +struct pci_dev *cxl_port_get_uport_pci_dev(struct cxl_port *port);
>   
>   static inline struct device *port_to_host(struct cxl_port *port)
>   {
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002c..905892cfedef 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -564,26 +564,13 @@ static unsigned char cdat_checksum(void *buf, size_t size)
>    */
>   void read_cdat_data(struct cxl_port *port)
>   {
> -	struct device *uport = port->uport_dev;
> +	struct pci_dev *pdev = cxl_port_get_uport_pci_dev(port);
>   	struct device *dev = &port->dev;
>   	struct pci_doe_mb *doe_mb;
> -	struct pci_dev *pdev = NULL;
> -	struct cxl_memdev *cxlmd;
>   	struct cdat_doe_rsp *buf;
>   	size_t table_length, length;
>   	int rc;
>   
> -	if (is_cxl_memdev(uport)) {
> -		struct device *host;
> -
> -		cxlmd = to_cxl_memdev(uport);
> -		host = cxlmd->dev.parent;
> -		if (dev_is_pci(host))
> -			pdev = to_pci_dev(host);
> -	} else if (dev_is_pci(uport)) {
> -		pdev = to_pci_dev(uport);
> -	}
> -
>   	if (!pdev)
>   		return;
>   
> @@ -591,14 +578,14 @@ void read_cdat_data(struct cxl_port *port)
>   				      CXL_DOE_PROTOCOL_TABLE_ACCESS);
>   	if (!doe_mb) {
>   		dev_dbg(dev, "No CDAT mailbox\n");
> -		return;
> +		goto out;
>   	}
>   
>   	port->cdat_available = true;
>   
>   	if (cxl_cdat_get_length(dev, doe_mb, &length)) {
>   		dev_dbg(dev, "No CDAT length\n");
> -		return;
> +		goto out;
>   	}
>   
>   	/*
> @@ -624,12 +611,13 @@ void read_cdat_data(struct cxl_port *port)
>   
>   	port->cdat.table = buf->data;
>   	port->cdat.length = length;
> -
> -	return;
> +	goto out;
>   err:
>   	/* Don't leave table data allocated on error */
>   	devm_kfree(dev, buf);
>   	dev_err(dev, "Failed to read/validate CDAT.\n");
> +out:
> +	pci_dev_put(pdev);
>   }
>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, "CXL");
>   
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 625e4aa427db..5dc2815d82d8 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -34,6 +34,32 @@
>   static DEFINE_IDA(cxl_port_ida);
>   static DEFINE_XARRAY(cxl_root_buses);
>   
> +/**
> + * cxl_port_get_uport_pci_dev - get the PCI device for a port's upstream device
> + * @port: CXL port to map to a PCI device
> + *
> + * Return: A referenced PCI device, or NULL if the upstream device is not PCI.
> + * The caller must release the returned reference with pci_dev_put().
> + */
> +struct pci_dev *cxl_port_get_uport_pci_dev(struct cxl_port *port)
> +{
> +	struct device *uport = port->uport_dev;
> +	struct device *host;
> +
> +	if (is_cxl_memdev(uport)) {
> +		struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
> +
> +		host = cxlmd->dev.parent;
> +	} else {
> +		host = uport;
> +	}
> +
> +	if (!host || !dev_is_pci(host))
> +		return NULL;
> +
> +	return pci_dev_get(to_pci_dev(host));
> +}
> +
>   /*
>    * The terminal device in PCI is NULL and @platform_bus
>    * for platform devices (for cxl_test)