Re: [PATCH v3 2/9] power: sequencing: pcie-m2: Allow creating serdev for multiple PCI devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Broken resource tracking and dead code due to early bailout
- [High] TOCTOU race condition causing serdev controller corruption
- [Medium] Guideline Violation: Mixing goto-based error handling and scope-based cleanup
--

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

power: sequencing: pcie-m2: Allow creating serdev for multiple PCI devices

This commit introduces struct pwrseq_pci_dev and tracks them in a list
within the M.2 power sequencing context. This is intended to scale the
driver to allow creating a shared serdev for multiple PCI devices.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index b2ed336fd5ad9..469e130330faa 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c

[ ... ]

> -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;
>  	struct device *dev = ctx->dev;
> +	struct pwrseq_pci_dev *pci_dev;
>  	int ret;
>  
>  	struct device_node *serdev_parent __free(device_node) =

Does this function mix goto-based error handling with scope-based cleanup
helpers? The cleanup subsystem guidelines state that goto and __free() should
not be mixed in the same function to avoid confusing ownership semantics.

> @@ -248,17 +259,23 @@ static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx)
>  		return 0;
>  	}

Does this early return prevent tracking of subsequent PCI devices?

If the serdev is already attached, this returns 0 before allocating a new
pwrseq_pci_dev and adding it to the pci_devices list. If the first probed
device is later removed, it appears pwrseq_pcie_m2_remove_serdev() would
destroy the shared serdev even though other devices on the same slot might
still need it.

Also, is this check safe from concurrent execution?

During asynchronous PCI enumeration, if two functions trigger the
BUS_NOTIFY_ADD_DEVICE notifier concurrently, could they both read
serdev_ctrl->serdev as NULL and race to call serdev_device_add()? It looks
like they might both overwrite ctrl->serdev, and when the second thread
fails in device_add() due to a sysfs name collision, its error path could
set ctrl->serdev to NULL, potentially dropping the valid pointer for the
first thread's serdev.

> -	ctx->serdev = serdev_device_alloc(serdev_ctrl);
> -	if (!ctx->serdev) {
> +	pci_dev = kzalloc(sizeof(*pci_dev), GFP_KERNEL);
> +	if (!pci_dev) {
>  		ret = -ENOMEM;
>  		goto err_put_ctrl;
>  	}

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