| Message ID | 20250130085251.155146-1-dhananjay.ugwekar@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost | expand |
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. > > 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); > -- > 2.34.1 >
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 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.
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(-)