[PATCH 0/2] cpufreq: mediatek: Suspend at the opp-suspend OPP, and mark one on MT8173

[PATCH 0/2] cpufreq: mediatek: Suspend at the opp-suspend OPP, and mark one on MT8173

[Ryan Brue]

mediatek-cpufreq has no .suspend callback, so each cluster enters system
sleep at whatever OPP the governor last chose. On the Amazon Fire HD 10
(2017), an MT8173 tablet that is not upstream yet, entering
suspend-to-RAM with both clusters at their highest OPP (2106 and
1703 MHz) measured 63.5 and 63.6 mA, against 52.0 and 52.0 mA with this
series. Those are 15-minute windows of the PMIC's coulomb counter,
alternating with and without the change; one count is about 2.9 mA.
Earlier, on the board's previous firmware path, s2idle drew 210-232 mA
against 158 mA.

Patch 1 takes the suspend frequency from the OPP table's opp-suspend
entry, as cpufreq-dt does, rather than always using the lowest OPP. That
keeps it opt-in per SoC: this driver also runs mt2701, mt7622, mt8183,
mt8186 and others I cannot test, and no MediaTek table has opp-suspend
today, so they are unchanged. Patch 2 marks the lowest OPP of both MT8173
clusters.

Patch 2 also reaches elm, hana and the EVB, which I cannot test either.
What they get is a switch to 507 MHz in cpufreq_suspend(), which runs
before any device is suspended, so the regulators it needs should still
be up. cpufreq_suspend() also runs from device_shutdown(), so reboot and
power-off switch first as well; on the tablet, reboot -f still resets in
1.4 s.

Patch 2 does nothing without patch 1 and is harmless on its own, so the
two can go through the cpufreq and MediaTek trees independently.

---
Ryan Brue (2):
      cpufreq: mediatek: Enter system sleep at the suspend OPP
      arm64: dts: mediatek: mt8173: Mark the lowest CPU OPPs for suspend

 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 ++
 drivers/cpufreq/mediatek-cpufreq.c       | 2 ++
 2 files changed, 4 insertions(+)
---
base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
change-id: 20260929-rbrue-suez-upstreaming-mtk-cpufreq-suspend-opp-81d12d60671b

Best regards,
--  
Ryan Brue <ryanbrue.dev@gmail.com>

[PATCH 1/2] cpufreq: mediatek: Enter system sleep at the suspend OPP

[Ryan Brue]

cpufreq_suspend() stops the governors at the start of system suspend, and
this driver has no .suspend callback, so each cluster sleeps at whatever
OPP it was last left at. On an MT8173 tablet (Amazon Fire HD 10, 2017),
entering suspend-to-RAM with both clusters at their highest OPP measured
63.5 mA. With this change and the lowest OPP marked, the same entry
measured 52.0 mA.

Use the generic suspend-frequency support, as cpufreq-dt does. Tables
without an opp-suspend entry leave suspend_freq at 0, which makes
cpufreq_generic_suspend() a no-op, so they are unaffected.

Assisted-by: LLM
---
 drivers/cpufreq/mediatek-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 052ca7cd2f4f..f9b1eca176eb 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -603,6 +603,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	policy->freq_table = freq_table;
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
+	policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(info->cpu_dev) / 1000;
 
 	return 0;
 }
@@ -624,6 +625,7 @@ static struct cpufreq_driver mtk_cpufreq_driver = {
 	.init = mtk_cpufreq_init,
 	.exit = mtk_cpufreq_exit,
 	.register_em = cpufreq_register_em_with_opp,
+	.suspend = cpufreq_generic_suspend,
 	.name = "mtk-cpufreq",
 };
 

-- 
2.55.0

Re: [PATCH 1/2] cpufreq: mediatek: Enter system sleep at the suspend OPP

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] opp: UAF and data race on opp_table->suspend_opp
--

commit e45cc103dadbac91ac1b6c64b8b61a195f1c5f95
Author: Ryan Brue <ryanbrue.dev@gmail.com>

cpufreq: mediatek: Enter system sleep at the suspend OPP

This commit updates the mediatek-cpufreq driver to use the generic
suspend-frequency support. It allows the driver to enter system sleep at the
suspend OPP, reducing power consumption during suspend-to-RAM.

> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 052ca7cd2f4fd..f9b1eca176eb8 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -603,6 +603,7 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->freq_table = freq_table;
>  	policy->driver_data = info;
>  	policy->clk = info->cpu_clk;
> +	policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(info->cpu_dev) / 1000;

[Severity: High]
Is it safe to call dev_pm_opp_get_suspend_opp_freq() here?

In drivers/opp/core.c, dev_pm_opp_get_suspend_opp_freq() locklessly
dereferences opp_table->suspend_opp:

	if (opp_table->suspend_opp && opp_table->suspend_opp->available)
		freq = dev_pm_opp_get_freq(opp_table->suspend_opp);

However, in drivers/opp/of.c, _opp_add_static_v2() modifies
opp_table->suspend_opp dynamically without holding opp_table->lock, creating a
data race:

				opp_table->suspend_opp->suspend = false;
				new_opp->suspend = true;
				opp_table->suspend_opp = new_opp;

Furthermore, when an OPP is removed, _opp_kref_release() in drivers/opp/core.c
frees the dev_pm_opp object without checking or clearing
opp_table->suspend_opp:

	_of_clear_opp(opp_table, opp);
	opp_debug_remove_one(opp);
	kfree(opp);

This leaves a permanent dangling pointer if opp_table->suspend_opp pointed to
the dying OPP. Any subsequent call to dev_pm_opp_get_suspend_opp_freq()
will locklessly dereference the now-dangling pointer, potentially causing a
use-after-free.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-rbrue-suez-upstreaming-mtk-cpufreq-suspend-opp-v1-0-9100f79b4080@gmail.com?part=1

Re: [PATCH 1/2] cpufreq: mediatek: Enter system sleep at the suspend OPP

[Ryan Brue]

On 10/9/26 1:43 PM, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Is it safe to call dev_pm_opp_get_suspend_opp_freq() here?
>
> In drivers/opp/core.c, dev_pm_opp_get_suspend_opp_freq() locklessly
> dereferences opp_table->suspend_opp:
>
> 	if (opp_table->suspend_opp && opp_table->suspend_opp->available)
> 		freq = dev_pm_opp_get_freq(opp_table->suspend_opp);
>
> However, in drivers/opp/of.c, _opp_add_static_v2() modifies
> opp_table->suspend_opp dynamically without holding opp_table->lock, creating a
> data race:
>
> 				opp_table->suspend_opp->suspend = false;
> 				new_opp->suspend = true;
> 				opp_table->suspend_opp = new_opp;
>
> Furthermore, when an OPP is removed, _opp_kref_release() in drivers/opp/core.c
> frees the dev_pm_opp object without checking or clearing
> opp_table->suspend_opp:
>
> 	_of_clear_opp(opp_table, opp);
> 	opp_debug_remove_one(opp);
> 	kfree(opp);
>
> This leaves a permanent dangling pointer if opp_table->suspend_opp pointed to
> the dying OPP. Any subsequent call to dev_pm_opp_get_suspend_opp_freq()
> will locklessly dereference the now-dangling pointer, potentially causing a
> use-after-free.
The suspend OPP is read once, from ->init, into policy->suspend_freq. 
mediatek-cpufreq adds the whole OPP table in probe before 
cpufreq_register_driver(), and never adds, removes, or disables 
individual OPPs, and has no .remove, so the table can't change or go 
away while ->init can run. cpufreq-dt makes the same call.

I also realized that I forgot my sign-off on the commits. I will send 
out a v2 that has the sign-off. My apologies.

Best regards,
Ryan Brue

[PATCH 2/2] arm64: dts: mediatek: mt8173: Mark the lowest CPU OPPs for suspend

[Ryan Brue]

Both clusters otherwise enter system sleep at whatever OPP the governor
last chose. On the Amazon Fire HD 10 (2017), entering suspend-to-RAM with
both clusters at their highest OPP measured 63.5 mA, against 52.0 mA
when they suspend at 507 MHz. Mark the 507 MHz OPP of each cluster, the
lowest in both tables, as the one to suspend at. mediatek-cpufreq uses
it for system sleep, and a kernel without that support ignores it.

Assisted-by: LLM
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 78c2ccd5be13..7f1af15cb934 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -56,6 +56,7 @@ cluster0_opp: opp-table-0 {
 		opp-507000000 {
 			opp-hz = /bits/ 64 <507000000>;
 			opp-microvolt = <859000>;
+			opp-suspend;
 		};
 		opp-702000000 {
 			opp-hz = /bits/ 64 <702000000>;
@@ -93,6 +94,7 @@ cluster1_opp: opp-table-1 {
 		opp-507000000 {
 			opp-hz = /bits/ 64 <507000000>;
 			opp-microvolt = <828000>;
+			opp-suspend;
 		};
 		opp-702000000 {
 			opp-hz = /bits/ 64 <702000000>;

-- 
2.55.0