Re: [PATCH v2 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 3 potential issue(s) to consider:
- [High] Reference leak of 'serdev_ctrl' on early return in '__pwrseq_pcie_m2_create_serdev()'.
- [High] Potential NULL pointer dereference in 'pwrseq_pcie_m2_notify()' when accessing 'pdev->dev.parent->of_node'.
- [Medium] Subsystem guidelines violation: mixing 'goto' error handling with '__free()' cleanup helpers.
--

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

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

This commit adds support to create serdev devices for PCI devices that
are already present before the driver's probe function registers the
PCI notifier.

> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -236,7 +236,7 @@ static int pwrseq_pcie_m2_create_bt_node(struct pwrseq_pcie_m2_ctx *ctx,
>  	return ret;
>  }
>  
> -static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx,
> +static int __pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx,
>  					struct pci_dev *pdev)
>  {
>  	struct serdev_controller *serdev_ctrl;
[ ... ]
>  	/* Bail out if the device was already attached to this controller */
>  	if (serdev_ctrl->serdev) {
>  		serdev_controller_put(serdev_ctrl);
>  		return 0;
>  	}
>  
> +	/* Bail out if the serdev device was already created for the PCI dev */
> +	mutex_lock(&ctx->list_lock);
> +	list_for_each_entry(pci_dev, &ctx->pci_devices, list) {
> +		if (pci_dev->pdev == pdev) {
> +			mutex_unlock(&ctx->list_lock);
> +			return 0;

Does this early return path leak the reference to serdev_ctrl?

The of_find_serdev_controller_by_node() call earlier in the function acquires
a reference, and the previous bailout path correctly calls
serdev_controller_put(serdev_ctrl), but this new path appears to return
without releasing the reference.

Also, looking at the rest of __pwrseq_pcie_m2_create_serdev() in the source,
it declares serdev_parent using the __free(device_node) macro while also
using goto labels for error paths like err_put_ctrl. Does this mix of goto
and __free() cleanup helpers violate the subsystem guidelines?

[ ... ]
> +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;

It is good to see the defensive !pdev->dev.parent check here to prevent a
null pointer dereference.

However, the existing pwrseq_pcie_m2_notify() function performs a similar
check without validating the parent:

	if (!pci_parent || (pci_parent != pdev->dev.parent->of_node))

Could a PCI device without a parent triggering a BUS_NOTIFY_ADD_DEVICE
event cause a crash in pwrseq_pcie_m2_notify() because it is missing the
same defensive check?

[ ... ]
> +		ret = __pwrseq_pcie_m2_create_serdev(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 function above, this newly added function mixes the
__free(device_node) cleanup helper for pci_parent with the err_remove_serdev
goto label. Should all resources here be converted to scope-based cleanup to
avoid ownership confusion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-pwrseq-m2-bt-v2-0-1740bd478539@oss.qualcomm.com?part=4