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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cxl: stale commit message and goto out on success path in read_cdat_data()
--

commit 731df7235bfc37ad38e18947501f81078ce6230b
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Share CXL port upstream PCI device lookup

This commit factors the lookup of a CXL port's upstream PCI device out of
read_cdat_data() into a new cxl_port_get_uport_pci_dev() helper that returns
a referenced struct pci_dev, and updates read_cdat_data() to release that
reference via pci_dev_put().

>     cxl: Share CXL port upstream PCI device lookup
>     
>     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>

[ ... ]

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002ca..905892cfedef6 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);
>  }

[Severity: Medium]
As reported by Jonathan Cameron on v13 [1], having the normal success path
in read_cdat_data() jump over the err: label via goto out makes the control
flow harder to follow, and Jonathan conditioned his Reviewed-by tag on not
having the good path follow a goto.

In addition, when read_cdat_data() switched from __free(pci_dev_put) to
explicit pci_dev_put(pdev) calls, the third paragraph of the commit message
still states:

    Use automatic reference cleanup in read_cdat_data(), as suggested by
    Li Ming, so early returns release the PCI device reference.

Should read_cdat_data() call pci_dev_put(pdev) directly before each return
(and in the err: block) instead of jumping over err: with goto out on the
normal path, and should the stale paragraph in the commit message be dropped?

[1] https://lore.kernel.org/r/20260924022156.5d376e91@jic23-hlaptop

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105185917.2667656-1-smadhavan@nvidia.com?part=2