Re: [PATCH v15 02/16] cxl: Share CXL port upstream PCI device lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] cleanup: Mixing goto-based error handling with scope-based cleanup in read_cdat_data()
- [Low] cxl: mixing goto and __free() in read_cdat_data()
--

commit 3f404cf13bcc3f018e17d22aaca79f82e169bf5d
Author: Srirangan Madhavan <smadhavan@nvidia.com>
cxl: Share CXL port upstream PCI device lookup

This commit factors out the lookup of a CXL port's upstream PCI device
into a new helper cxl_port_get_uport_pci_dev(). It also updates
read_cdat_data() to use this helper with a scope-based cleanup variable
to manage the PCI device reference.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 9d807c1a002ca..c3c41efae5fde 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -564,26 +564,14 @@ 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 __free(pci_dev_put) =
> +		cxl_port_get_uport_pci_dev(port);
>  	struct device *dev = &port->dev;

[Severity: Low]
Does this introduction of __free() conflict with the existing goto-based
error handling in this function?

Looking further down in read_cdat_data(), the function still relies on
legacy goto-based error handling for the buffer allocation:

	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;
	...
err:
	/* Don't leave table data allocated on error */
	devm_kfree(dev, buf);
	dev_err(dev, "Failed to read/validate CDAT.\n");

The cleanup subsystem guidelines recommend that for a given routine, we
should convert all resources that need a goto cleanup to scope-based
cleanup, or convert none of them. Mixing goto-based cleanup labels with
__free() declarations can create confusing ownership semantics and
complicate future refactoring.

Should buf also be converted to use scope-based cleanup, or should pdev be
managed explicitly to keep the error handling style consistent?

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