Re: [PATCH 3/3] power: sequencing: pcie-m2: Deassert W_DISABLE2# when no UART serdev is created

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional toggling of shared W_DISABLE2# GPIO causes premature assertion for multi-function PCI devices.
- [High] Missing cleanup of manually managed W_DISABLE2# GPIO on module removal and probe error paths.

Pre-existing issues:
- [High] Missing notification of PCI devices hotplugged between existing device iteration and notifier registration.
--

--- Patch [3]: [PATCH 3/3] power: sequencing: pcie-m2: Deassert W_DISABLE2# when no UART serdev is created ---
commit b301a8670a82271ce018fc9f630c8d02f37d6f72
Author: Wei Deng <wei.deng@oss.qualcomm.com>
Subject: power: sequencing: pcie-m2: Deassert W_DISABLE2# when no UART serdev is created

This commit modifies the PCIe M.2 power sequencing driver to handle
non-UART Bluetooth devices, such as USB variants. It deasserts the
W_DISABLE2# GPIO for devices not in the UART list to allow them to
enumerate, and reasserts it symmetrically upon device removal.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index cf51122d54fdf..06eb5eb0676bd 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c

[ ... ]

> @@ -401,11 +401,23 @@ static int pwrseq_pcie_m2_notify(struct notifier_block *nb, unsigned long action
>  			ret = pwrseq_pcie_m2_create_serdev_one(ctx, pdev);
>  			if (ret)
>  				return notifier_from_errno(ret);
> +		} else if (ctx->w_disable2_gpio) {
> +			/*
> +			 * PCIe device not in the UART BT table. This covers
> +			 * USB BT variants of the same combo chip (same PCIe
> +			 * device ID, different sub-system ID, BT exposed over
> +			 * USB instead of UART). No UART serdev is needed, but
> +			 * W_DISABLE2# must be deasserted to enable the BT
> +			 * subsystem so the USB BT interface can enumerate.
> +			 */
> +			gpiod_set_value_cansleep(ctx->w_disable2_gpio, 0);
>  		}
>  		break;
>  	case BUS_NOTIFY_REMOVED_DEVICE:
>  		if (pci_match_id(pwrseq_m2_pci_ids, pdev))
>  			pwrseq_pcie_m2_remove_serdev(ctx, pdev);
> +		else if (ctx->w_disable2_gpio)
> +			gpiod_set_value_cansleep(ctx->w_disable2_gpio, 1);

[Severity: High]
If a multi-function M.2 combo card exposes multiple PCI functions (e.g., a
dual-function endpoint where both functions share the same root port parent),
does this code prematurely power off the Bluetooth subsystem when one
function is removed?

Adding a multi-function non-UART PCI device triggers BUS_NOTIFY_ADD_DEVICE
multiple times (once per function), which will redundantly set the GPIO
to 0.

When one of these functions is later unbound or removed,
BUS_NOTIFY_REMOVED_DEVICE fires and unconditionally sets the GPIO to 1.

Will this break the remaining active functions and the USB BT subsystem
since there is no state tracking or refcounting for the GPIO toggling?

[ ... ]

> @@ -469,16 +481,17 @@ static int pwrseq_pcie_m2_create_serdev(struct pwrseq_pcie_m2_ctx *ctx)
>  		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;
> +		if (pci_match_id(pwrseq_m2_pci_ids, pdev)) {
> +			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;
> +			}
> +		} else if (ctx->w_disable2_gpio) {
> +			gpiod_set_value_cansleep(ctx->w_disable2_gpio, 0);
>  		}
>  	}
>  

[Severity: High]
Does this error path leak the hardware state if an error occurs after the
GPIO has been deasserted?

If pwrseq_pcie_m2_create_serdev() fails on a subsequent PCI device in the
loop, it jumps to err_remove_serdev. That cleanup path removes the UART
serdevs but does not appear to reset w_disable2_gpio to 1 for non-UART
devices.

Additionally, during driver removal in pwrseq_pcie_m2_remove(), the GPIO
is not symmetrically re-asserted. Can this leave the hardware permanently
powered on after driver unload?

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
between iterating existing PCI devices and registering the notifier in
pwrseq_pcie_m2_probe()?

In pwrseq_pcie_m2_probe(), the code first invokes
pwrseq_pcie_m2_create_serdev() to handle existing devices, and then calls
pwrseq_pcie_m2_register_notifier().

If a PCI device is enumerated or hot-added in the narrow window between
these two statements, it seems it would be entirely missed, resulting in
missing hardware initialization.

Should the notifier be registered before iterating existing devices to
close this window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-fix-hamoa-m2-w-disable2-v1-0-5e725091266a@oss.qualcomm.com?part=3