| Message ID | 20250130085251.155146-1-dhananjay.ugwekar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost | expand |
On 31-01-25, 10:36, Gautham R. Shenoy wrote: > On Thu, Jan 30, 2025 at 08:52:52AM +0000, Dhananjay Ugwekar wrote: > > set_boost is a per-policy function call, hence a driver wide lock is > > unnecessary. Also this mutex_acquire can collide with the mutex_acquire > > from the mode-switch path in status_store(), which can lead to a > > deadlock. So, remove it. > > Looks good to me. The driver lock should only guard the state > changes. Everything else is a per-policy change and is better guarded > by the per-cpudata mutex. > > Once Mario acks this patch, please respond to Viresh's series and let > him know that this patch needs to go in before his series. If he is ok > with it, he can include it in his series. Yeah, I will apply this once rc1 is out.
On 2/3/2025 04:48, Viresh Kumar wrote: > On 30-01-25, 08:52, Dhananjay Ugwekar wrote: >> set_boost is a per-policy function call, hence a driver wide lock is >> unnecessary. Also this mutex_acquire can collide with the mutex_acquire >> from the mode-switch path in status_store(), which can lead to a >> deadlock. So, remove it. >> >> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com> >> --- >> PS: This patch should ideally go before [1], as that patch uncovers this >> bug and actually leads to a deadlock when switching the amd_pstate driver >> mode. >> [1] https://lore.kernel.org/linux-pm/e16c06d4b8ffdb20e802ffe648f14dc515e60426.1737707712.git.viresh.kumar@linaro.org/ >> --- >> drivers/cpufreq/amd-pstate.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index d5be51bf8692..93788bce7e6a 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -740,7 +740,6 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) >> pr_err("Boost mode is not supported by this processor or SBIOS\n"); >> return -EOPNOTSUPP; >> } >> - guard(mutex)(&amd_pstate_driver_lock); >> >> ret = amd_pstate_cpu_boost_update(policy, state); >> refresh_frequency_limits(policy); > > Applied. Thanks. > Sorry for my delay with the recent holiday. I have no concerns with this going to the start of the series. Acked-by: Mario Limonciello <mario.limonciello@amd.com>
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index d5be51bf8692..93788bce7e6a 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -740,7 +740,6 @@ static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state) pr_err("Boost mode is not supported by this processor or SBIOS\n"); return -EOPNOTSUPP; } - guard(mutex)(&amd_pstate_driver_lock); ret = amd_pstate_cpu_boost_update(policy, state); refresh_frequency_limits(policy);
set_boost is a per-policy function call, hence a driver wide lock is unnecessary. Also this mutex_acquire can collide with the mutex_acquire from the mode-switch path in status_store(), which can lead to a deadlock. So, remove it. Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com> --- PS: This patch should ideally go before [1], as that patch uncovers this bug and actually leads to a deadlock when switching the amd_pstate driver mode. [1] https://lore.kernel.org/linux-pm/e16c06d4b8ffdb20e802ffe648f14dc515e60426.1737707712.git.viresh.kumar@linaro.org/ --- drivers/cpufreq/amd-pstate.c | 1 - 1 file changed, 1 deletion(-)