Skip to contentSkip to conversation

cpufreq: amd_freq_sensitivity: Fix sensitivity clamping in amd_powersave_bias_target

PATCH In review Thorsten Blum 2 participants 4 messages Dec 2, 2025 – Dec 23, 2025

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cpufreq: amd_freq_sensitivity: Fix sensitivity clamping in amd_powersave_bias_target

The local variable 'sensitivity' was never clamped to 0 or POWERSAVE_BIAS_MAX because the return value of clamp() was not used. Fix this by assigning the clamped value back to 'sensitivity'.

Fixes 9c5320c8ea8b ("cpufreq: AMD "frequency sensitivity feedback" powersave bias for ondemand governor")
---
 drivers/cpufreq/amd_freq_sensitivity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/amd_freq_sensitivity.c +1 -1
index 13fed4b9e02b..713ccf24c97d 100644
@@ -76,7 +76,7 @@ static unsigned int amd_powersave_bias_target(struct cpufreq_policy *policy,
 	sensitivity = POWERSAVE_BIAS_MAX -
 		(POWERSAVE_BIAS_MAX * (d_reference - d_actual) / d_reference);
 
-	clamp(sensitivity, 0, POWERSAVE_BIAS_MAX);
+	sensitivity = clamp(sensitivity, 0, POWERSAVE_BIAS_MAX);
 
 	/* this workload is not CPU bound, so choose a lower freq */
 	if (sensitivity < od_tuners->powersave_bias) {
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
#1 lore raw
Quoted 3 lines · On Tue, 2 Dec 2025 13:44:28 +0100 Thorsten Blum <[email protected]>…

The local variable 'sensitivity' was never clamped to 0 or POWERSAVE_BIAS_MAX because the return value of clamp() was not used. Fix this by assigning the clamped value back to 'sensitivity'.

This actually makes no difference
(assuming od_tuners->powersave_bias <= POWERSAVE_BIAS_MAX). The only use of 'sensitivity' is the test at the end of the diff.

So I think you could just delete the line.

David

Quoted 21 lines
Fixes 9c5320c8ea8b ("cpufreq: AMD "frequency sensitivity feedback" powersave bias for ondemand governor")
---
 drivers/cpufreq/amd_freq_sensitivity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpufreq/amd_freq_sensitivity.c +1 -1
index 13fed4b9e02b..713ccf24c97d 100644
@@ -76,7 +76,7 @@ static unsigned int amd_powersave_bias_target(struct cpufreq_policy *policy,
 	sensitivity = POWERSAVE_BIAS_MAX -
 		(POWERSAVE_BIAS_MAX * (d_reference - d_actual) / d_reference);
 
-	clamp(sensitivity, 0, POWERSAVE_BIAS_MAX);
+	sensitivity = clamp(sensitivity, 0, POWERSAVE_BIAS_MAX);
 
 	/* this workload is not CPU bound, so choose a lower freq */
 	if (sensitivity < od_tuners->powersave_bias) {
#2 lore raw
Quoted 7 lines · David Laight
Quoted 3 lines · On Tue, 2 Dec 2025 13:44:28 +0100 Thorsten Blum <[email protected]>…

The local variable 'sensitivity' was never clamped to 0 or POWERSAVE_BIAS_MAX because the return value of clamp() was not used. Fix this by assigning the clamped value back to 'sensitivity'.

Quoted 5 lines

This actually makes no difference
(assuming od_tuners->powersave_bias <= POWERSAVE_BIAS_MAX). The only use of 'sensitivity' is the test at the end of the diff.

So I think you could just delete the line.

The local variable 'sensitivity' is an 'int', while '->powersave_bias' is an 'unsigned int'. If 'sensitivity' were ever negative, it would be converted to an 'unsigned int', producing an incorrect result. That's probably what the clamping was meant to prevent.

However, calculating 'sensitivity' can be simplified from:

	sensitivity = POWERSAVE_BIAS_MAX -
		(POWERSAVE_BIAS_MAX * (d_reference - d_actual) / d_reference);

to:

	sensitivity = POWERSAVE_BIAS_MAX * d_actual / d_reference;

which makes it clearer that, in practice, 'sensitivity' is never negative. How about simplifying the formula as above, changing 'sensitivity' to 'unsigned int', and removing the clamping?

Thanks,
Thorsten

#3 lore raw
Quoted 24 lines · Thorsten Blum
Quoted 7 lines · David Laight
Quoted 3 lines · On Tue, 2 Dec 2025 13:44:28 +0100 Thorsten Blum <[email protected]>…

The local variable 'sensitivity' was never clamped to 0 or POWERSAVE_BIAS_MAX because the return value of clamp() was not used. Fix this by assigning the clamped value back to 'sensitivity'.

Quoted 5 lines

This actually makes no difference
(assuming od_tuners->powersave_bias <= POWERSAVE_BIAS_MAX). The only use of 'sensitivity' is the test at the end of the diff.

So I think you could just delete the line.

The local variable 'sensitivity' is an 'int', while '->powersave_bias' is an 'unsigned int'. If 'sensitivity' were ever negative, it would be converted to an 'unsigned int', producing an incorrect result. That's probably what the clamping was meant to prevent.

However, calculating 'sensitivity' can be simplified from:

	sensitivity = POWERSAVE_BIAS_MAX -
		(POWERSAVE_BIAS_MAX * (d_reference - d_actual) / d_reference);
Quoted 7 lines

to:

	sensitivity = POWERSAVE_BIAS_MAX * d_actual / d_reference;

which makes it clearer that, in practice, 'sensitivity' is never negative. How about simplifying the formula as above, changing 'sensitivity' to 'unsigned int', and removing the clamping?

Hm, changing the formula could alter the integer arithmetic, potentially producing slightly different results, and might even overflow.

I guess we should keep the formula as is and either defensively clamp 'sensitivity', as originally intended, or just remove the line (since this seems to have been working since 2013 and as suggested by David).

I'm slightly in favor of clamping the value because the assumptions aren't obvious to me.

Any preferences or other suggestions?

Thanks,
Thorsten