Re: [PATCH 2/2] arm64: dts: qcom: shikra: Add WCN clock to Bluetooth node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dt-bindings: incorrect WCN clock placement breaks WiFi power sequencing
- [High] bluetooth: hci_qca: unbalanced clock and pwrseq disable on error path
--

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

arm64: dts: qcom: shikra: Add WCN clock to Bluetooth node

This commit explicitly declares the WCN clock dependency in the Bluetooth node
for the shikra CQM and IQS SOM boards. It is intended to allow the hci_qca
driver to vote for the clock during Bluetooth power sequencing.

> diff --git a/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi b/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> index 8ac42ff625a0e..4e03e459c7c16 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> @@ -220,6 +220,8 @@ bluetooth {
>  		vddxo-supply = <&vreg_pmu_xo>;
>  		vddrf-supply = <&vreg_pmu_rf>;
>  		vddch0-supply = <&vreg_pmu_ch0>;
> +
> +		clocks = <&rpmcc RPM_SMD_RF_CLK2>;

[Severity: High]
Does this code place the WCN clock in the correct node?

The WCN PMU driver (drivers/power/sequencing/pwrseq-qcom-wcn.c) manages the
shared WCN RF clock for both Bluetooth and WLAN. It retrieves the clock from
its own PMU device node via devm_clk_get_optional(dev, NULL).

If the main RF clock is placed inside the bluetooth node instead of the
wcn3988-pmu node, the PMU driver's ctx->clk evaluates to NULL.

If WiFi is powered on while Bluetooth is off, will the PMU driver attempt to
enable the clock but do nothing, leaving the hardware clock disabled and
causing the WiFi subsystem to silently fail or crash?

>  	};
>  };
>  
> diff --git a/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi b/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> index 4ff97945274dd..227283ab74a81 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> @@ -242,6 +242,8 @@ bluetooth {
>  		vddxo-supply = <&vreg_pmu_xo>;
>  		vddrf-supply = <&vreg_pmu_rf>;
>  		vddch0-supply = <&vreg_pmu_ch0>;
> +
> +		clocks = <&rpmcc RPM_SMD_RF_CLK1>;

[Severity: High]
Is the WCN clock placement correct here?

Similar to the CQM SOM, if the clock is placed in the bluetooth node instead
of the wcn3988-pmu node, will this deprive the PMU power sequencer of its
clock reference and break WiFi power sequencing?

>  	};
>  };

I also have a question regarding the pwrseq path mentioned in the commit
message.

drivers/bluetooth/hci_qca.c:qca_regulator_enable() {
    ...
		if (qcadev->susclk) {
			ret = clk_prepare_enable(qcadev->susclk);
			if (ret)
				pwrseq_disable(power->pwrseq);
		}
		return ret;
}

[Severity: High]
If clk_prepare_enable(qcadev->susclk) fails here, pwrseq_disable() is called
and the error is returned. This error propagates up through qca_power_on() to
qca_setup(). Upon failure, qca_setup() jumps to the out label and invokes
qca_power_off():

drivers/bluetooth/hci_qca.c:qca_power_off() {
    ...
	if (power && power->pwrseq) {
		if (qcadev->susclk)
			clk_disable_unprepare(qcadev->susclk);
		pwrseq_disable(power->pwrseq);
		set_bit(QCA_BT_OFF, &qca->flags);
		return;
	}
    ...
}

Because qca_power_off() lacks state tracking for the pwrseq path, will this
unconditionally execute clk_disable_unprepare(qcadev->susclk) and
pwrseq_disable(power->pwrseq) again?

Could this unbalanced clock unprepare trigger a kernel WARN, and could the
double pwrseq_disable() cause a reference count underflow, potentially leading
to crashes or resource corruption?

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