Received-SPF: pass (google.com: domain of linux-kernel+bounces-210479-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <109e470b-b408-45c4-9ba9-96a2b3120b02@amd.com>
Date: Tue, 11 Jun 2024 14:00:39 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 04/10] cpufreq: amd-pstate: add debug message while
 CPPC is supported and disabled by SBIOS
To: Perry Yuan <perry.yuan@amd.com>, gautham.shenoy@amd.com
Cc: rafael.j.wysocki@intel.com, viresh.kumar@linaro.org, Ray.Huang@amd.com,
 Borislav.Petkov@amd.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.1718095377.git.perry.yuan@amd.com>
 <adf1b81c44806e73a391aa6bb49a9fddca95550c.1718095377.git.perry.yuan@amd.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <adf1b81c44806e73a391aa6bb49a9fddca95550c.1718095377.git.perry.yuan@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?VGZKTjBNVGUvRmNiQlhuZWJ5MVMyRThqRFN3am1CTVAzSHJETjkyYkdqSUUz?=
 =?utf-8?B?QjBna2h5azBtK2ltcDNhTUd6UEhKaE54TktKUFlLV0loZWRZMFRzWUVpdGxE?=
 =?utf-8?B?cXhOV3dSUjNmTFFuS1o3eTRBL0NnbXBBdEVNMXNoUndua0krNHJoTjVsN3NG?=
 =?utf-8?B?RHg5UE1NcDFpeHA4QUNveW12eGZTQW1yZVZ2cVEzUWRSYUtwblV5T216eVlw?=
 =?utf-8?B?TUpidVExMUhyS0ppd0d4NklZZG5pVkdVZGZvWDhsQU83Y014WUwxT3FtbW5i?=
 =?utf-8?B?ZUU0UU1qWVc3TldZQlVyYlNnQ2xBRDJ4Ui94SmNhUWliYmFrT3c0eUJQSXVv?=
 =?utf-8?B?VmlFQ0I0LzhpOXNjbUNVdkMrVzJTQ1VMVDd4SEM4c2d2Um91eU5sY21XN0Js?=
 =?utf-8?B?UnE4Wk81SnJVYldIMVYwcExIbGNLWit5UldtWnhQL3Y4clQwYU9GaW1sV0M0?=
 =?utf-8?B?TXJmYWdRNldwNjZ2QWZaWTZjUWYrUkFjbEl3SlVFMjRYRG45N2xrL0U3dFEw?=
 =?utf-8?B?YWM1bkptek9QK3gwMHNHMkpjY0UxSWx6elNrMEdmdk9Wdk80OUdqV0owOE90?=
 =?utf-8?B?YUJRdkFncDhLeUFwSnhVS1JnU0NNalFPVWxyZHlxWlE3TDY5WG8xVndxeEM4?=
 =?utf-8?B?a0YvMkVrOGgrWlFCdnAwMmJuaStYVkg1cG5YdGYvN1FaQ1ZPZUdQWnFNOExS?=
 =?utf-8?B?bHBNRGJEc1VjaHVzbG9GOU5ZT3k0VCsyUUsyUDJ5WU00KzNWNE9laTN6WDZX?=
 =?utf-8?B?VHpoWHZWczdGb1JZRVlVcHk4NWY4ZG4yNEdnNmZ0T3gvZ0k4azAxWnNiNHRQ?=
 =?utf-8?B?Nm5ibGQ2d0FsY0RoTFhvb1NYNERBYWNJNVhrQmdIVU9pUTI2Y0pFM053RUdp?=
 =?utf-8?B?bVgzd0NNL0VxUEZta0ZGa0FCeFltc1p3alJCT2lLUWZBYnBLQTBjZnRCczZO?=
 =?utf-8?B?R2hPR3l6VEdNQ0Q3T3grSW45N0hJY0FDOWM4eXlJdlFGRmZFLy8zcVVXa2tx?=
 =?utf-8?B?UUNoNzEybjNvemcxYUZ6WUVvN1hvUHJNeTc0anNvQ0ZoeUU2SStXdk5GNDhm?=
 =?utf-8?B?OVREUDlWd3VVcTZwV3dyTGFKZEQ0dS9vMmxuT2Q0ZjY5RTZ0OFpBeitDd1Ix?=
 =?utf-8?B?VHgrNDdiZTh0OE9Ld29XOXBaQ29WQ3pZalNpdFE3U21EWVJXSEp2Qk5kdnpK?=
 =?utf-8?B?dkF3aSs5Um5EREl0V0RkWUNmTVNSYW1JejkwTkJzWEtyMWpZQnBSS1hnUHo3?=
 =?utf-8?B?TkFhZmtZd3dPOG02SWVaclYyTlp3RTVQMWNhd3NOcThwZ1JMM2pTekpUVVMv?=
 =?utf-8?B?RjhVdGtGbm5kMkNDVThnaUdIZThEa1NNQ1QrK05yZVVYTVlySVc4bS9Ta08r?=
 =?utf-8?B?L2w4bm5TN3dlN3VzQno2WFpNQXBSZWN1amZ6RkZrYnM5c3J4ZlhiZ3ZPYnBa?=
 =?utf-8?B?RUk4dFBxVGt4RXViK25ySER3bkJsVnh6UXJVSHVGaTkzd1YrZ2w4Y0JkZG5R?=
 =?utf-8?B?SVhlWmd1OGNHZ0x6dEttNGZKdjcybms5YWlrZUZ4c0xuMU9tWll4M2tpeWp2?=
 =?utf-8?B?cExqeFBkR3FDTHk1d2k4UFg4ejRJVkNLejhubjRmNnh4M1FXRjZER3RKWGRl?=
 =?utf-8?B?K013UlVMQlp4ZVc5Y3R6VHpOY0lqT0REVklWUC8vZGxZNjN6L3ZOa0VEaEtt?=
 =?utf-8?B?cXAwZkxFRzUvZ1FtVXYxYlpkTURId0drd0VHTml6elVORmlRQkhsb0NaZE9m?=
 =?utf-8?B?dWRwajhobTF5L3pxOHAxbm5vY0c0MDdCMTNySGovYzdBUmVWOTBvdEoxaHY2?=
 =?utf-8?B?Q3c5ZHFsQ3pBcEhCc3N2dEltdzROSmpZUkxldzhGUlQ5T2FhazlLVmlVQTd0?=
 =?utf-8?B?VWdRMW90MExSWTE2b3g3aEg1OXhEd3l5USt4cnJkL2d2MWlLc2szeGJUYjlH?=
 =?utf-8?B?d08wa1VFU3oyOVV0MDVTdXJzUGUxRHhSbHo3MUF1L1NkSTlVaDFLVnZiY0Ro?=
 =?utf-8?B?ZEMrU0JLNnJjaWFNT09tdUVJRHNEUVZNa1pOZjdQUkZ3b3dnSHVaajZCUUVh?=
 =?utf-8?B?dGNFNFcvSW1YUU9HaEVaVi9yRHIxL3BmY3BxUVVGOHp0UDhnZ1RabXcwdytr?=
 =?utf-8?Q?kGRG0A2Uz7YyRE/mByEVazu0Q?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6df29e7a-0349-447d-3d4c-08dc8a48cb13
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 19:00:43.0799
 (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: L8dn9DwDYN1MJKxTQ13ZZCJdQeEsykHfoDfEnWHvpxyi5o7v97UZLiFebgrC/O0oDYXHwrw1195zPDyRIuFH6g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6720

On 6/11/2024 03:52, Perry Yuan wrote:
> If CPPC feature is supported by the CPU however the CPUID flag bit is not
> set by SBIOS, the `amd_pstate` will be failed to load while system
> booting.
> So adding one more debug message to inform user to check the SBIOS setting,
> The change also can help maintainers to debug why amd_pstate driver failed
> to be loaded at system booting if the processor support CPPC.
> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218686
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f166b3b94091..6b9fc24001f2 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1746,11 +1746,36 @@ static int __init amd_pstate_set_driver(int mode_idx)
>    */
>   static bool amd_cppc_supported(void)
>   {
> +	struct cpuinfo_x86 *c = &cpu_data(0);
> +
>   	if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) {
>   		pr_debug_once("CPPC feature is not supported by the processor\n");
>   		return false;
>   	}
>   
> +	/*
> +	 * If the CPPC feature is disabled in the BIOS for processors that support MSR-based CPPC,
> +	 * the AMD Pstate driver may not function correctly.
> +	 * Check the CPPC flag and display a warning message if the platform supports CPPC.
> +	 * Notice: below checking code will not abort the driver registeration process.
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_CPPC)) {
> +		if (cpu_feature_enabled(X86_FEATURE_ZEN1) || cpu_feature_enabled(X86_FEATURE_ZEN2)) {
> +			if (c->x86_model > 0x60 && c->x86_model < 0xaf)
> +				goto warn;
> +		} else if (cpu_feature_enabled(X86_FEATURE_ZEN3) || cpu_feature_enabled(X86_FEATURE_ZEN4)) {
> +			if ((c->x86_model > 0x10 && c->x86_model < 0x1F) || (c->x86_model > 0x40 && c->x86_model < 0xaf))
> +				goto warn;
> +		} else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) {
> +			goto warn;
> +		}
> +	}
> +
> +	return true;
> +
> +warn:
> +	pr_debug_once("The CPPC feature is supported but currently disabled by the BIOS.\n"
> +					"Please enable it if your BIOS has the CPPC option.\n");

This should be pr_warn_once() or pr_notice_once() IMO otherwise people 
won't really be able to know there is a problem.

>   	return true;

I don't really like the goto and warn label with two identical return 
statements.  Instead how about something like this?

    static bool amd_cppc_supported(void)
    {
	struct cpuinfo_x86 *c = &cpu_data(0);
	bool warn = false;

	.
	.
	.

	if (checks) {
		.
		.
		warn = TRUE;
	}

	if (warn)
		pr_warn_once();

	return TRUE;

That would be a better design pattern, especially if we end up having 
different classes of warnings to show to users in this function in the 
future.