From: Lifeng Zheng zhenglifeng1@huawei.com
[ Upstream commit 0ae204405095abfbc2d694ee0fbb49bcbbe55c57 ]
Detect the result of starting old governor in cpufreq_set_policy(). If it fails, exit the governor and clear policy->governor.
Signed-off-by: Lifeng Zheng zhenglifeng1@huawei.com Link: https://patch.msgid.link/20250709104145.2348017-5-zhenglifeng1@huawei.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
**Backport Status: YES**
This commit should be backported to stable kernel trees. Here's my extensive analysis:
## Bug Analysis
The commit fixes a **resource leak and inconsistent state bug** in the cpufreq governor switching code. Looking at the code changes in `cpufreq_set_policy()`:
### The Bug: In the original code (lines 2716-2721), when switching governors fails and we need to restore the old governor: ```c if (old_gov) { policy->governor = old_gov; if (cpufreq_init_governor(policy)) policy->governor = NULL; else cpufreq_start_governor(policy); // Bug: no error handling here } ```
If `cpufreq_start_governor()` fails, the code doesn't handle the error. This leaves the system in an **inconsistent state** where: 1. The governor is initialized (`cpufreq_init_governor` succeeded) 2. But the governor failed to start (`cpufreq_start_governor` failed) 3. The policy still points to a non-functional governor 4. Resources allocated during `cpufreq_init_governor` are **leaked** (module reference count, governor's init() allocations)
### The Fix: ```c if (cpufreq_init_governor(policy)) { policy->governor = NULL; } else if (cpufreq_start_governor(policy)) { cpufreq_exit_governor(policy); // NEW: Clean up on failure policy->governor = NULL; // NEW: Clear the governor pointer } ```
## Why This Should Be Backported:
1. **Fixes a Real Bug**: This addresses a resource leak where `cpufreq_init_governor()` acquires resources (notably `try_module_get()` at line 2442 and potential governor->init() allocations) that aren't cleaned up if `cpufreq_start_governor()` fails.
2. **Small and Contained Fix**: The change is minimal - just 4 lines modified to add proper error handling. It doesn't change any APIs or introduce new functionality.
3. **Prevents System Instability**: Leaving the cpufreq subsystem in an inconsistent state (initialized but not started governor) could lead to: - Module reference count leaks - Memory leaks from governor init allocations - Potential crashes if the partially-initialized governor is accessed later
4. **Error Path Fix**: This is clearly an error handling path that was incorrectly implemented. The pattern of calling `cpufreq_exit_governor()` after a failed `cpufreq_start_governor()` is already used elsewhere in the code (line 2711).
5. **No Architectural Changes**: The fix simply adds missing cleanup code in an error path. It doesn't change the normal operation flow or introduce new features.
6. **Critical Subsystem**: The cpufreq subsystem is critical for power management and system stability. Bugs here can affect system reliability.
The commit follows the stable kernel rules perfectly - it's a clear bug fix that prevents resource leaks and system instability, with minimal risk of regression since it only affects an error path that was already broken.
drivers/cpufreq/cpufreq.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2a2fea6743aa..0bb17bb615f5 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2482,10 +2482,12 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, pr_debug("starting governor %s failed\n", policy->governor->name); if (old_gov) { policy->governor = old_gov; - if (cpufreq_init_governor(policy)) + if (cpufreq_init_governor(policy)) { policy->governor = NULL; - else - cpufreq_start_governor(policy); + } else if (cpufreq_start_governor(policy)) { + cpufreq_exit_governor(policy); + policy->governor = NULL; + } }
return ret;