Re: [PATCH 1/2] Bluetooth: hci_qca: Add WCN clock management for pwrseq-based power path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bluetooth: hci_qca: unbalanced clock disable in error paths
--

commit b40a50081c6337711ee58ffef1a77bff3162c4ae
Author: Yepuri Siddu <yepuri.siddu@oss.qualcomm.com>

Bluetooth: hci_qca: Add WCN clock management for pwrseq-based power path

This commit extends the pwrseq-based power path in the hci_qca driver to
acquire and manage an optional WCN clock. It updates qca_serdev_probe() to
acquire the clock, and modifies qca_regulator_enable() and qca_power_off()
to manage the clock state alongside the power sequencer.

> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 7089e9b639b2a..489a059e519a6 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2265,6 +2265,8 @@ static void qca_power_off(struct hci_uart *hu)
>  	}
>  
>  	if (power && power->pwrseq) {
> +		if (qcadev->susclk)
> +			clk_disable_unprepare(qcadev->susclk);
>  		pwrseq_disable(power->pwrseq);

[Severity: High]
Does this code unconditionally disable the clock and power sequencer without
checking if they were actually enabled?

Unlike the regular regulator path which uses power->vregs_on for state
tracking, this pwrseq teardown lacks state tracking. This causes unbalanced
disables in two paths:

1) The module is unloaded without the HCI interface ever being brought up.
qca_close() calls qca_power_off() (since QCA_BT_OFF is initialized to 0),
which unconditionally disables the never-enabled clock and pwrseq.

2) In qca_setup(), if qca_power_on() fails (e.g., clk_prepare_enable()
fails), the error handler calls qca_power_off() which performs an unconditional
teardown despite the rollback already executed in qca_regulator_enable().

This can trigger a WARN in the clock framework due to an unbalanced
clk_disable_unprepare() and cause an unbalanced pwrseq_disable(), which crashes
the kernel if panic_on_warn is set. This is reachable by a local privileged user
loading and unloading the driver, or automatically on hardware initialization
failure.

>  		set_bit(QCA_BT_OFF, &qca->flags);
>  		return;
>  	}

[ ... ]

> @@ -2324,8 +2326,17 @@ static int qca_regulator_enable(struct qca_serdev *qcadev)
>  	struct qca_power *power = qcadev->bt_power;
>  	int ret;
>  
> -	if (power->pwrseq)
> -		return pwrseq_enable(power->pwrseq);
> +	if (power->pwrseq) {
> +		ret = pwrseq_enable(power->pwrseq);
> +		if (ret)
> +			return ret;
> +		if (qcadev->susclk) {
> +			ret = clk_prepare_enable(qcadev->susclk);
> +			if (ret)
> +				pwrseq_disable(power->pwrseq);

[Severity: High]
Does this rollback leave the power sequencer and clock in an untracked state?

If clk_prepare_enable() fails, pwrseq_disable() is correctly called to roll
back the sequence, returning the error to the caller. However, because there
is no tracking to indicate the components are disabled, the error handler
(which calls qca_power_off()) will attempt to disable them a second time,
triggering the unbalanced disables mentioned above.

> +		}
> +		return ret;
> +	}
>  
>  	/* Already enabled */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-bt-wcn-clk-enable-v1-0-30bee88c4de4@oss.qualcomm.com?part=1