Re: [PATCH v2] clk: qcom: ipq-cmn-pll: keep the CMN block bus clocks enabled
Mieczyslaw Nalewaj <[email protected]> Wed, 5 Aug 2026 20:53:48 +0200
| Newsgroups | org.kernel.vger.linux-clk,org.kernel.vger.linux-arm-msm,org.kernel.vger.linux-kernel,org.kernel.vger.stable |
|---|---|
| Message-ID | <[email protected]> |
On 8/4/2026 1:53 PM, Stanislaw Pal wrote:
> The probe function takes a runtime PM reference to enable the GCC AHB &
> SYS clocks of the CMN PLL block, registers the clocks, and then drops
> the reference, letting pm_clk gate both clocks a few milliseconds after
> probe has returned. The clock ops access the CMN PLL registers without
> a runtime PM reference of their own, and on IPQ5018 gating the CMN
> block bus clocks makes the SoC hang on a subsequent bus access: boards
> died silently within milliseconds of the CMN PLL probe, up to a 100%
> reproducible boot loop, depending on binary layout (micro-timing).
>
> Take a devres-managed runtime PM reference in probe, so the bus clocks
> stay enabled for as long as the driver is bound and the reference is
> released again on unbind.
>
> Fixes: f81715a4c87c ("clk: qcom: Add CMN PLL clock controller driver for IPQ
[...]
> + /*
> + * The clock ops access the CMN PLL registers without taking a
> + * runtime PM reference of their own, and on IPQ5018 gating the CMN
> + * block AHB & SYS clocks after probe hangs the SoC on a subsequent
> + * bus access. Hold a reference for as long as the driver is bound
> + * so that the bus clocks stay enabled.
> + */
> + ret = devm_pm_runtime_get_noresume(dev);
> + if (ret)
> + return ret;
> +
> /* Register CMN PLL clock and fixed rate output clocks. */
> ret = ipq_cmn_pll_register_clks(pdev);
> pm_runtime_put(dev);
Does this error path leak a runtime PM reference?
devm_pm_runtime_get_noresume() returns before reaching the unconditional pm_runtime_put(dev) further down. If it fails, the earlier pm_runtime_resume_and_get(dev) reference is never released, leaving the usage count elevated permanently — probe returning an error means there's no matching remove() to clean it up.
Suggested fix:
ret = devm_pm_runtime_get_noresume(dev);
if (ret) {
pm_runtime_put(dev);
return ret;
}
This failure mode is rare (devm_pm_runtime_get_noresume() only fails on devres allocation failure, and undoes its own get internally in that case), but the code as written still leaves the earlier reference unbalanced on this path.
Mieczyslaw Nalewaj