Re: [PATCH 2/9] arm64: dts: airoha: en7581: add CPU OPP and scaling nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cpufreq: airoha: positive return from dev_pm_domain_attach_list() mishandled
- [Medium] arm64: dts: airoha: en7581: zero opp-level in smcc_opp0 conflicts with genpd
--

commit ade97d9a263f63e320dd00b49d71387b254adb06
Author: Christian Marangi <ansuelsmth@gmail.com>

arm64: dts: airoha: en7581: add CPU OPP and scaling nodes

This commit adds the cpufreq node, CPU OPP tables, and CPU clock and power
domain references to arch/arm64/boot/dts/airoha/en7581.dtsi to enable CPU
frequency scaling on Airoha EN7581.

> diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> index 83b30a340950f..c5a3350867410 100644
> --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi
> @@ -74,8 +74,12 @@ cpu0: cpu@0 {
>  			device_type = "cpu";
>  			compatible = "arm,cortex-a53";
>  			reg = <0x0>;
> +			operating-points-v2 = <&cpu_opp_table>;
>  			enable-method = "psci";
> -			clock-frequency = <80000000>;
> +			clocks = <&cpufreq>;
> +			clock-names = "cpu";
> +			power-domains = <&cpufreq>;
> +			power-domain-names = "perf";
>  			next-level-cache = <&l2>;
>  		};

[Severity: High]
Does adding the cpu clock and perf power domain to cpu0 cause
airoha_cpufreq_probe() to fail and leak priv->pd_list on boot?

Prior to this change, airoha_cpufreq_probe() in
drivers/cpufreq/airoha-cpufreq.c returned early at dev_pm_opp_set_config()
with -ENOENT because cpu0 did not have the clock and power domain properties.
With these properties now present, probe reaches dev_pm_domain_attach_list():

drivers/cpufreq/airoha-cpufreq.c:airoha_cpufreq_probe() {
	...
	/* Attach PM for OPP */
	ret = dev_pm_domain_attach_list(cpu_dev, &attach_data,
					&priv->pd_list);
	if (ret)
		goto clear_opp_config;
	...
}

In drivers/base/power/common.c, dev_pm_domain_attach_list() returns the
positive number of attached power domains on success:

drivers/base/power/common.c:dev_pm_domain_attach_list() {
	...
	pds->num_pds = num_pds;
	*list = pds;
	return num_pds;
}

When attaching the single perf domain succeeds, dev_pm_domain_attach_list()
returns 1. Because airoha_cpufreq_probe() checks if (ret) rather than
if (ret < 0), it treats the successful attach as an error, jumps to
clear_opp_config without calling dev_pm_domain_detach_list(priv->pd_list),
leaks priv->pd_list along with the virtual genpd device and device link, and
returns 1 from probe so cpufreq-dt is never registered.

[ ... ]

> @@ -115,6 +131,159 @@ l2: l2-cache {
>  		};
>  	};
>  
> +	cpufreq: cpufreq {
> +		compatible = "airoha,en7581-cpufreq";
> +
> +		operating-points-v2 = <&cpu_smcc_opp_table>;
> +
> +		#power-domain-cells = <0>;
> +		#clock-cells = <0>;
> +	};
> +
> +	cpu_opp_table: opp-table {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +			required-opps = <&smcc_opp0>;
> +		};

[ ... ]

> +	cpu_smcc_opp_table: opp-table-smcc {
> +		compatible = "operating-points-v2";
> +
> +		smcc_opp0: opp-0 {
> +			opp-level = <0>;
> +		};

[Severity: Medium]
Can setting opp-level = <0> in smcc_opp0 cause the first transition to
500 MHz (opp-500000000) to be silently skipped by the generic PM domain core?

In drivers/pmdomain/core.c, performance state 0 represents the initial
unconstrained state, and both gpd_data->performance_state and
genpd->performance_state are zero-initialized when the domain is registered
and attached.

If the CPU boots at a frequency above 500 MHz and the first cpufreq
transition targets opp-500000000, the call chain
dev_pm_opp_set_rate() -> _set_opp() -> _set_required_opps() ->
_set_opp_level() -> dev_pm_domain_set_performance_state() invokes
genpd_set_performance_state() with state = 0:

drivers/pmdomain/core.c:genpd_set_performance_state() {
	...
	prev_state = gpd_data->performance_state;
	if (prev_state == state)
		return 0;
	...
}

Because prev_state is already 0 (and genpd->performance_state in
_genpd_set_performance_state() is also 0), genpd_set_performance_state()
returns 0 immediately without calling
airoha_cpu_pmdomain_set_performance_state() in
drivers/pmdomain/mediatek/airoha-cpu-pmdomain.c, leaving the hardware at the
boot frequency while cpufreq and OPP record 500 MHz as active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026095727.31708-1-ansuelsmth@gmail.com?part=2