Received-SPF: pass (google.com: domain of linux-kernel+bounces-178314-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <85302e25-0eca-42d2-bee3-733b608bb33f@amd.com>
Date: Tue, 14 May 2024 10:20:55 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 01/10] cpufreq: amd-pstate: optimize the initial
 frequency values verification
To: Perry Yuan <perry.yuan@amd.com>, Mario.Limonciello@amd.com,
 gautham.shenoy@amd.com, Ray.Huang@amd.com, Borislav.Petkov@amd.com
Cc: rafael.j.wysocki@intel.com, Alexander.Deucher@amd.com,
 Xinmei.Huang@amd.com, Xiaojian.Du@amd.com, Li.Meng@amd.com,
 linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <cover.1715356532.git.perry.yuan@amd.com>
 <c7bce101b2d7d107ba3c89779fe6800060e8edc4.1715356532.git.perry.yuan@amd.com>
Content-Language: en-US
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>
In-Reply-To: <c7bce101b2d7d107ba3c89779fe6800060e8edc4.1715356532.git.perry.yuan@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RTJGU0JmYllWQnduQUlDMG5YSG1nN3ZGaUE0VXREMll6aURTcmFKemQ3aDZG?=
 =?utf-8?B?bjN1Ym1pbWhETkVrNE5KOFpGTHgvMWdQWDRSK09wNkZMZWhtSExTY3NUdStv?=
 =?utf-8?B?OWFWeDREeVlET1U5SnhNS1AyOEJYN0dtWnNTVjNZejdnNC9udE1VVzJPUGdG?=
 =?utf-8?B?dFM1R1FwK2RRaXJaakthZlVxVk9kdS83a0YrZGNxUldHY3A0cmxSQjZhNmsr?=
 =?utf-8?B?Q0pBOTFGc0xCdGpNTWR6bFVOSTlQeitwZzRZeVdnNXdEL2tIRGd6K3YrYWJj?=
 =?utf-8?B?bWI1bzFydnU3Q0x6TXRCeHhheS91ZS9GYVYyMXRhU0F6T0F6eTNDQW1BTmF1?=
 =?utf-8?B?SDR6K2pDRjhoamlDMTZhUjdVcmUxMlI4NU01SG1DZ2RZWlRPNHJ2SnRGVWVo?=
 =?utf-8?B?dXZmMTBOZXB4TWhjL0k1YlVYNnkwbk8wdWpJUGN4dFFSeVdCWUg5ME5kcjg5?=
 =?utf-8?B?SktBdk1DSmlDUGJkcTdVUXN4Q2xJRjRJcVRQVUNVUG9VVHRrNXRub0crczFa?=
 =?utf-8?B?TktYKzdXLzR6ektYaVUrU1dlcytwbnlPN1F1Q0lZRHl1b0VyZlVGUmo2S2F0?=
 =?utf-8?B?aFZuMXpnOVlxWnMzWTJnUU9tSVJoa1YzNERLMFVGa3cySUhSWHlPUDNnK3hG?=
 =?utf-8?B?bVBNeUQ4dElWRldUN3NLS3cwWkNKRFcxRWt6N3phUE1pcE1xdHdEZkloV09B?=
 =?utf-8?B?RTVsVkF2OHR1WWdqZzR0bm94UEU0ZGQxWlB5YTQxNTQwRFZpbkFJMGVxU1pL?=
 =?utf-8?B?V1BEcU9vMzNldTBERVhXTXpuNHlITGU1UVNtSDJpVHZkb2pxL004KzMwK0Nv?=
 =?utf-8?B?SE1rcFVVSGVUUWFHLzNYWGt0QTc1YWFjd29HcU5QMlVmOXEzVjJVTkpoVUZk?=
 =?utf-8?B?dUZXZXJMWjl4UkJMQ1Qwa0hiQitXeWFzaHM2VW9HSXBya2V0YkZiTmVzMjVo?=
 =?utf-8?B?dThPMUl1MFFldHcvS2dJckRZNmowTGd3ZEZzVU9oMTBRL3JyUVRSdUFuaGFP?=
 =?utf-8?B?eFpxRnNrd0NtR2I2L0wxWW5hMkNQRm5yWElpa2VrSVNKRVZMRW5Bd0E1NWlj?=
 =?utf-8?B?UFNhRVExZkJOU3Z3TElUTldEbkcxS21mbkMzRDJkb0dmellCMHdJWThKMExZ?=
 =?utf-8?B?c3VGMExnMHF3WG1wKzIzNWx0bU9NWVZQYkd6bjgxMVE3aW9tM3I0aVNWNWxl?=
 =?utf-8?B?ZUVtMjlQY1hlWWxsNGllN0FJY2pMb1l6T0tqMTZhVXBtL1lDMzdxdEcrWjVk?=
 =?utf-8?B?T0FlK2xjbE9XeUUzanY0clIvTjBlaFdxeUpWdTEzZE15K2V5YVhUVTJqdnpZ?=
 =?utf-8?B?YWpKK1hMTERoQkt0OFlhY1pVTFNYd1hLUWwyV0hEb294eHBLSzdmYk03ZFFm?=
 =?utf-8?B?VjQrSE4xR2JaTDBuY1FXRFYwSmZwclNXbUs2Z2RWTmhFSjFTMEpYVitsRVpS?=
 =?utf-8?B?dVdWSGRTVHFFTnJKc3RqdlorK042NnJ6ZmRuMGhFcDBTMkVPRHlMMzM1NjhV?=
 =?utf-8?B?OXY1UWNmdXEzb09yMjNBNVJzOWZyUjY4TFJBT1BQRHQwdnFSK2RKeXpqRmJR?=
 =?utf-8?B?Y3dFRXNxdkVQVVQrbzgzejRmZG03VExvT3g0Z0dFbm8yREQzaVZ5ZnYvbitT?=
 =?utf-8?B?ZkFWTWtpWUtTdERQRnpmN0Z3eFlxRmZ3cHl4SUlVY3cyOTVLeE03akdJVkdF?=
 =?utf-8?B?dXM4T1dNRGpwY09VcUxjQ2hTR0V1cVZVbmtXUEpMclVQbi81ZlF4RGFhZTJv?=
 =?utf-8?B?eFhhb294OG5uV0FUTzYvQ1JUWllZWkpIcGJsbnd6ZVI1akw2d3lFSHBieFBl?=
 =?utf-8?B?VHZ4M3lPQm14Y2pMQ0ZMQmxTcFR2MFpnczNScEFLdldZTlQ5NEpXVW85QmRL?=
 =?utf-8?B?ZFAxRmVXRUVwTFVCc012ZnpiVzZhSVRBYkQzSUJmdnZpUks2Zkd0dE83SURV?=
 =?utf-8?B?dE9heXRRdWdKdjVid0JML21OdVZ4T0hBc2Y5NXJiVWNHODFNemJBOWdkYm1Y?=
 =?utf-8?B?TkpIbXc1YTRkN1Jtbm5HNnVFWTdmQTdBR1VhNXlld0JDZXJhTEYybEhjMjFq?=
 =?utf-8?B?dTNISEJteDhiSTlOK01tamtOL3Zwc0tKcHl2bWFVOGE4WlByT0NQdDQzSjUr?=
 =?utf-8?Q?t9Jxupqjh3YadqmbNo1tqpqPJ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b531636c-e2e8-4c5c-fb9d-08dc73d1759f
X-MS-Exchange-CrossTenant-AuthSource: LV8PR12MB9207.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 04:51:04.2533
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: V9II1dsYnxtnD6HMC4ByvO8u7anur8tWwbguf2oNARLrwLTZa06lsF+6JpCpU2VRStbmEiq7jXjPsDsxjCzzpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR12MB8914

Hello Perry,

On 5/13/2024 7:37 AM, Perry Yuan wrote:
> To enhance the debugging capability of the driver loading failure for
> broken CPPC ACPI tables, it can optimize the expression by moving the
> verification of `min_freq`, `nominal_freq`, and other dependency values
> to the `amd_pstate_init_freq()` function where they are initialized.
> If any of these values are incorrect, the `amd-pstate` driver will not be registered.
> 
> By ensuring that these values are correct before they are used, it will facilitate
> the debugging process when encountering driver loading failures due to faulty CPPC
> ACPI tables from BIOS
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6a342b0c0140..614f6fac0764 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -889,6 +889,24 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
>  	WRITE_ONCE(cpudata->max_freq, max_freq);
>  
> +	/**
> +	 * Below values need to be initialized correctly, otherwise driver will fail to load
> +	 * max_freq is calculated according to (nominal_freq * highest_perf)/nominal_perf
> +	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
> +	 * Check _CPC in ACPI table objects if any values are incorrect
> +	 */
> +	if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {
> +		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
> +			min_freq, max_freq, nominal_freq);
> +		return -EINVAL;
> +	}
> +
> +	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
> +		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
> +			lowest_nonlinear_freq, min_freq, nominal_freq * 1000);

A reminder, we should fix the below code section (due to only nominal freq being in MHz),

685 static int amd_pstate_set_boost(struct cpufreq_policy *policy, int state)                                                        
686 {                                                                                                                                
687         struct amd_cpudata *cpudata = policy->driver_data;                                                                       
688         int ret;                                                                                                                 
689                                                                                                                                  
690         if (!cpudata->boost_supported) {                                                                                         
691                 pr_err("Boost mode is not supported by this processor or SBIOS\n");                                              
692                 return -EINVAL;                                                                                                  
693         }                                                                                                                        
694                                                                                                                                  
695         if (state)                                                                                                               
696                 policy->cpuinfo.max_freq = cpudata->max_freq;                                                                    
697         else                                                                                                                     
698                 policy->cpuinfo.max_freq = cpudata->nominal_freq;      <--- mismatch in left and right hand side units  


To avoid below situation(from a Zen4 AMD EPYC system with boost disabled),

/sys/devices/system/cpu/cpu0/cpufreq# cat scaling_max_freq                                                             
2151                                                             	<--- MHz                                                                    
/sys/devices/system/cpu/cpu0/cpufreq# cat amd_pstate_max_freq                                                          
2287000									<--- KHz

Thanks,
Dhananjay


> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -927,15 +945,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  	max_freq = READ_ONCE(cpudata->max_freq);
>  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>  
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
> -
>  	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
>  	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
>  
> @@ -1388,14 +1397,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	min_freq = READ_ONCE(cpudata->min_freq);
>  	max_freq = READ_ONCE(cpudata->max_freq);
>  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
>  
>  	policy->cpuinfo.min_freq = min_freq;
>  	policy->cpuinfo.max_freq = max_freq;