[PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[Dhananjay Ugwekar]

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);
-- 
2.34.1

Re: [PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[Gautham R. Shenoy]

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
> 

-- 
Thanks and Regards
gautham.

Re: [PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH] cpufreq/amd-pstate: Remove unnecessary driver_lock in set_boost

[Mario Limonciello]

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>