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

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

On 11/13/2026 11:50 PM, Dave Jiang wrote:
>
> 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:

Hi Dave,


Your implementation also works for me.


Ming

>
> 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");