Re: [PATCH v3 4/9] power: sequencing: pcie-m2: Create serdev for PCI devices present before probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Leaked serdev_controller reference on early exit in pwrseq_pcie_m2_create_serdev_one()
- [High] Mixing goto-based error handling with scope-based cleanups (__free and scoped_guard) in the same function
--

commit f9e3f8cd7dae54d53f6be50d3fa50b1da0d87fe2
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

power: sequencing: pcie-m2: Create serdev for PCI devices present before probe

This commit adds support for creating serdevs for PCI devices that are
already present before the driver completes its probe phase. It complements
the existing notifier-based hotplug support to handle existing devices.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index 038271207a27e..8164c44289775 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
[ ... ]
> @@ -259,6 +259,14 @@ static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx,
>  		return 0;
>  	}
>  
> +	/* Bail out if the serdev device was already created for the PCI dev */
> +	scoped_guard(mutex, &ctx->list_lock) {
> +		list_for_each_entry(pci_dev, &ctx->pci_devices, list) {
> +			if (pci_dev->pdev == pdev)
> +				return 0;
> +		}
> +	}
> +

Earlier in this function, of_find_serdev_controller_by_node() acquires a
reference to the serdev controller. If the device is found in the list,
the code returns 0 directly from within the scoped_guard block.

Does this early return leak the serdev controller reference by bypassing
the required serdev_controller_put() call at the end of the function?

Additionally, include/linux/cleanup.h notes that mixing goto and cleanup
helpers like guard() in the same function is not expected. This function
still relies on multiple goto labels (e.g., err_put_ctrl, err_free_pci_dev)
for error handling. Could this function be converted fully to scope-based
cleanups to avoid potential LIFO ordering issues?

[ ... ]
> @@ -408,18 +416,56 @@ static int pwrseq_pcie_m2_register_notifier(struct pwrseq_pcie_m2_ctx *ctx, stru
[ ... ]
> +static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx)
> +{
> +	struct pci_dev *pdev = NULL;
> +	int ret;
> +
> +	if (!pwrseq_pcie_m2_check_remote_node(ctx->dev, 3, 0, "serial") ||
> +	    !pwrseq_pcie_m2_check_remote_node(ctx->dev, 0, 0, "pcie"))
> +		return 0;
> +
> +	struct device_node *pci_parent __free(device_node) =
> +				of_graph_get_remote_node(dev_of_node(ctx->dev), 0, 0);
> +	if (!pci_parent)
> +		return 0;
> +
> +	/* Create serdev for existing PCI devices if required */
> +	for_each_pci_dev(pdev) {
> +		if (!pdev->dev.parent || pci_parent != pdev->dev.parent->of_node)
> +			continue;
> +
> +		if (!pci_match_id(pwrseq_m2_pci_ids, pdev))
> +			continue;
> +
> +		ret = pwrseq_pcie_m2_create_serdev_one(ctx, pdev);
> +		if (ret) {
> +			dev_err_probe(ctx->dev, ret,
> +				      "Failed to create serdev for PCI device (%s)\n",
> +				      pci_name(pdev));
> +			pci_dev_put(pdev);
> +			goto err_remove_serdev;
> +		}
>  	}
>  
>  	return 0;
> +
> +err_remove_serdev:
> +	pwrseq_pcie_m2_remove_serdev(ctx, NULL);
> +
> +	return ret;
>  }

Similar to the concern above, this newly introduced function declares
pci_parent using __free(device_node) but also uses goto-based error
handling with the err_remove_serdev label.

Does this mix of goto and __free() violate the subsystem guidelines
recommending against combining both cleanup styles in the same function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-pwrseq-m2-bt-v3-0-b39dc2ae3966@oss.qualcomm.com?part=4