Re: [PATCH v7 5/8] Bluetooth: hci_qca: Support QCA2066 on M.2 connector via pwrseq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hci_qca: QCA6390 probe fails on M.2 connectors due to overwritten pwrseq
- [Medium] hci_qca: missing error check for power sequencer enable in qca_power_on()
- [Medium] hci_qca: asymmetric power-off leaks bt_en GPIO assertion
--

commit d33d72433b2dc1cfeaa1bb5a87c7d716233a54a5
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

Bluetooth: hci_qca: Support QCA2066 on M.2 connector via pwrseq

This patch modifies the QCA HCI driver to support QCA2066 (and similar chips)
on M.2 connectors. It updates the probe sequence to acquire a power sequencer
for the M.2 UART connection instead of relying on a dedicated BT enable GPIO.

> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 1d27ff98034ba..b6ec1a57248e2 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1875,6 +1875,9 @@ static int qca_power_on(struct hci_dev *hdev)
>  			/* Controller needs time to bootup. */
>  			msleep(150);
>  		}
> +
> +		if (qcadev->bt_power.pwrseq)
> +			pwrseq_power_on(qcadev->bt_power.pwrseq);
>  	}

[Severity: Medium]
Does this code need to check the return value of pwrseq_power_on()?

If the power sequencer API returns an error (for example, due to a regulator
error or timeout), the result is discarded and the driver will incorrectly
assume power-on succeeded.

This causes the driver to attempt communication with an unpowered Bluetooth
controller, leading to HCI command timeouts and driver malfunction.

[ ... ]

> @@ -2426,6 +2457,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	else
>  		qcadev->btsoc_type = QCA_ROME;
>  
> +	err = qca_serdev_get_m2_pwrseq(qcadev);
> +	if (err)
> +		return err;
> +

[Severity: High]
Does this code overwrite the M.2 power sequencer and break probe for
QCA6390 chips?

Because qca_serdev_get_m2_pwrseq() is now called unconditionally for all
chips before the switch statement, it correctly acquires the M.2 power
sequencer when present. However, the QCA_QCA6390 branch further down in
this function lacks a guard to prevent overwriting it:

	case QCA_QCA6390:
		if (dev_of_node(&serdev->dev)) {
			qcadev->bt_power.pwrseq = devm_pwrseq_get(&serdev->dev,
								  "bluetooth");
			if (IS_ERR(qcadev->bt_power.pwrseq))
				return PTR_ERR(qcadev->bt_power.pwrseq);
			break;
		}

If the serdev has an OF node, this overwrites the M.2 power sequencer
with a request for a 'bluetooth' power sequencer. This fails with -ENOENT
and aborts the probe.

[Severity: Medium]
Can this early acquisition cause an asymmetric power-off that leaks the bt_en
GPIO assertion?

For legacy/default SoC types (like QCA_ROME), the default branch of the
switch statement doesn't check if the power sequencer was already acquired:

	default:
		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
					       GPIOD_OUT_LOW);

If both exist, qca_power_on() asserts the bt_en GPIO and enables the power
sequencer. However, qca_power_off() disables the power sequencer and
immediately returns:

	if (power->pwrseq) {
		pwrseq_disable(power->pwrseq);
		set_bit(QCA_BT_OFF, &qca->flags);
		return;
	}

This early return bypasses the fallback logic that de-asserts bt_en, leaving
the hardware partially enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-monza-wireless-v7-0-5a6de7662dcb@oss.qualcomm.com?part=5