Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] Logic flaw prevents tracking multiple PCI devices, defeating the core purpose of the patch.
- [High] Lockless check of `serdev_ctrl->serdev` introduces a data race during concurrent device additions.
--

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

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

This patch introduces a list to track multiple PCI devices per M.2 slot
by creating a 'pwrseq_pci_dev' for each device requiring a serdev. It
replaces the single serdev reference with this list to scale the driver.

> 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

[ ... ]

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

Since the intent is to track multiple PCI devices per M.2 slot, does this
existing early bailout:

	if (serdev_ctrl->serdev) {
		serdev_controller_put(serdev_ctrl);
		return 0;
	}

prevent adding subsequent PCI devices to the ctx->pci_devices list?

If serdev_ctrl->serdev is already set by the first device, won't this return
0 without allocating pwrseq_pci_dev, leaving the list with only one device?

Additionally, is it possible for two PCI devices to trigger the
BUS_NOTIFY_ADD_DEVICE notifier concurrently?

If both threads observe serdev_ctrl->serdev as NULL without synchronization,
could they both proceed to allocate and call serdev_device_add()? This could
overwrite the ctrl->serdev pointer locklessly.

If the slower thread ultimately fails device_add() due to a sysfs name
collision, it could execute ctrl->serdev = NULL in its error path. Would this
corrupt the serdev controller state and silently orphan the serdev that was
successfully registered by the first thread?

>  
> -	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/20260507-pwrseq-m2-bt-v2-0-1740bd478539@oss.qualcomm.com?part=2