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

[Dave Jiang <dave.jiang@intel.com>]

On 11/10/26 6:36 AM, Li Ming wrote:
> 
> 在 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");
>  }
> 
> 

How about something like this? I think it's cleaner:

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 905892cfedef..c924904faec4 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -556,6 +556,27 @@ static unsigned char cdat_checksum(void *buf, size_t size)
 	return sum;
 }
 
+static int cxl_cdat_read_validate(struct device *dev,
+				  struct pci_doe_mb *doe_mb,
+				  struct cdat_doe_rsp *buf, size_t *length)
+{
+	size_t table_length = *length;
+	int rc;
+
+	rc = cxl_cdat_read_table(dev, doe_mb, buf, length);
+	if (rc)
+		return rc;
+
+	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))
+		return -EIO;
+
+	return 0;
+}
+
 /**
  * read_cdat_data - Read the CDAT data on this port
  * @port: Port to read data from
@@ -564,13 +585,13 @@ static unsigned char cdat_checksum(void *buf, size_t size)
  */
 void read_cdat_data(struct cxl_port *port)
 {
-	struct pci_dev *pdev = cxl_port_get_uport_pci_dev(port);
 	struct device *dev = &port->dev;
 	struct pci_doe_mb *doe_mb;
 	struct cdat_doe_rsp *buf;
-	size_t table_length, length;
-	int rc;
+	size_t length;
 
+	struct pci_dev *pdev __free(pci_dev_put) =
+		cxl_port_get_uport_pci_dev(port);
 	if (!pdev)
 		return;
 
@@ -578,14 +599,14 @@ void read_cdat_data(struct cxl_port *port)
 				      CXL_DOE_PROTOCOL_TABLE_ACCESS);
 	if (!doe_mb) {
 		dev_dbg(dev, "No CDAT mailbox\n");
-		goto out;
+		return;
 	}
 
 	port->cdat_available = true;
 
 	if (cxl_cdat_get_length(dev, doe_mb, &length)) {
 		dev_dbg(dev, "No CDAT length\n");
-		goto out;
+		return;
 	}
 
 	/*
@@ -593,31 +614,15 @@ 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;
-
-	table_length = length;
-
-	rc = cxl_cdat_read_table(dev, doe_mb, buf, &length);
-	if (rc)
-		goto err;
-
-	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 (!buf || cxl_cdat_read_validate(dev, doe_mb, buf, &length)) {
+		/* Don't leave table data allocated on error */
+		devm_kfree(dev, buf);
+		dev_err(dev, "Failed to read/validate CDAT.\n");
+		return;
+	}
 
 	port->cdat.table = buf->data;
 	port->cdat.length = length;
-	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");