Re: [PATCH v5 14/16] power: sequencing: pcie-m2: Split Bluetooth unit based on interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Splitting a single shared physical reset GPIO into two logical power sequencing units bypasses the core's refcounting, causing the USB subsystem's runtime PM to inadvertently power off the Bluetooth controller while it is actively in use by the UART driver.
--

--- Patch [14]: [PATCH v5 14/16] power: sequencing: pcie-m2: Split Bluetooth unit based on interface ---
commit 87ef77e2c86fea3f38f875c67c427d48fa406c76
Author: Chen-Yu Tsai <wenst@chromium.org>

power: sequencing: pcie-m2: Split Bluetooth unit based on interface

This commit splits the Bluetooth power sequencing unit into two distinct
units (UART and USB) to allow independent control. It aims to fix
initialization and recovery for serdev drivers by letting either interface
toggle the power.

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -83,8 +83,22 @@ static int pwrseq_pci_m2_e_bt_disable(struct pwrseq_device *pwrseq)

[ ... ]

> -static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_unit_data = {
> -	.name = "bt-enable",
> +/*
> + * XXX There are two Bluetooth units to allow either one to be able to power
> + * off and thus reset the controller. In practice only one of the interfaces
> + * is used, so there is no conflict. However userspace could power off the
> + * USB unit by disabling the associated USB port, without the UART unit or
> + * its consumer ever knowing.
> + */
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_uart_unit_data = {
> +	.name = "bt-uart-enable",
> +	.deps = pwrseq_pcie_m2_unit_deps,
> +	.enable = pwrseq_pci_m2_e_bt_enable,
> +	.disable = pwrseq_pci_m2_e_bt_disable,
> +};
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_e_bt_usb_unit_data = {
> +	.name = "bt-usb-enable",

[Severity: High]
Does splitting a single physical reset GPIO into two logical units bypass the
power sequencing core's refcounting?

The commit message notes there is no conflict in practice, but won't the USB
subsystem's runtime PM automatically turn off the Bluetooth controller while
the UART driver is actively using it?

Because a power sequencing target exists, the USB core removes the PM QoS
constraint for the port:

drivers/usb/core/port.c:usb_hub_create_port_device() {
    ...
    /* Userspace owns the policy, drop the kernel 'no_poweroff' request. */
    retval = dev_pm_qos_remove_request(port_dev->req);
    ...
}

If the USB port on the M.2 connector is physically empty (since Bluetooth is
using UART), runtime PM automatically suspends the empty port and powers it
off:

drivers/usb/core/port.c:usb_port_runtime_suspend() {
    ...
    retval = usb_hub_set_port_power(hdev, hub, port1, false);
    ...
}

This calls into the power sequence disable callback for the USB target. Since
both logical units control the exact same physical w_disable2_gpio, won't
this disable action blindly assert the reset GPIO and yank the power state
from underneath the active UART driver without any user interaction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715085348.3457359-1-wenst@chromium.org?part=14