Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1b7a5cd5-6ecf-8529-f10b-4f7f06fc7462@amd.com>
Date:   Mon, 7 Nov 2022 12:16:00 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH v3 7/8] cpufreq: amd-pstate: add frequency dynamic boost
 sysfs control
Content-Language: en-US
To:     Perry Yuan <Perry.Yuan@amd.com>, rafael.j.wysocki@intel.com,
        ray.huang@amd.com, viresh.kumar@linaro.org
Cc:     Deepak.Sharma@amd.com, Nathan.Fontenot@amd.com,
        Alexander.Deucher@amd.com, Shimmer.Huang@amd.com,
        Xiaojian.Du@amd.com, Li.Meng@amd.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, wyes.karny@amd.com
References: <20221107175705.2207842-1-Perry.Yuan@amd.com>
 <20221107175705.2207842-8-Perry.Yuan@amd.com>
From:   "Limonciello, Mario" <mario.limonciello@amd.com>
In-Reply-To: <20221107175705.2207842-8-Perry.Yuan@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SllDTWE0MG1pYmlEUmd2Vlk0cm80UVBMa3JlQ0FaaWQ1WTlZZ01rYm9PRHBI?=
 =?utf-8?B?S01lMUgvMjdCNHVPUkU2RWhOL0grK1hQQzZsZnppNWc3SFhUVXNmZ1Fja09B?=
 =?utf-8?B?dDNTVm14c2tKRzBGQndubnh6SnNCcjV6eTVnR1k2TFJmV1R6cEs4b3hpQzUr?=
 =?utf-8?B?NjVuYUwrbkxyT01xR3l1enR4Ly9kS0hUdGh4dHNBdUl3NkFSdnVEOW9pOWpr?=
 =?utf-8?B?c2NMcEpDdDRUUlQ4d0hYdjBpd3o2a3ZoQnJHQlh3Z0Q2c3VqM1U2MFlXV0pm?=
 =?utf-8?B?T2Z6NitmekxqZWVIdW9tK0F0VlNRaGNiWjV1K0ZaN0J1QVFicmdzNk9tOXAw?=
 =?utf-8?B?dDlDamNMUlQxUkFBcFF4SktXWEUwRjZIM3BZNWx3U1ZkRG5UdnhqV0ZsT0Jx?=
 =?utf-8?B?QjZxUHFMdkFlTWQxbDJCOGR0OUNiUm9RcXo2TlVORFdtaUZ0TGUzT0NnM0FV?=
 =?utf-8?B?b2p3eVBIek43aGJKMHRoWXB0cmdBL3N4d0xVN3lZV1d4clkzbFozeWNSZ0U3?=
 =?utf-8?B?OGtSelFuRHZqNWdGaUZmbXZJSFdHdjJtRkYvWThYY25nb1UrdmRpeHliVTFH?=
 =?utf-8?B?aEdXTHFSdSt2VHVNb2F1ZWZWUW9hSkZUV1FXVmNjMVpicncwdHBCTitIZFAr?=
 =?utf-8?B?VGVGQ0l0dEVuT1ljZW54NmdRQkVzQUwraGZPY0xGVCsrV05PMTQySEdTQnNR?=
 =?utf-8?B?K1UxUW42N0NWTUdVckJLMitySklRVXpRejR2d1UvekZWaEhpVU85Y0VMbGtD?=
 =?utf-8?B?SnlKb2pvUFAvcEE1ZDNIczkzOHIwN1RJaFR5Z0Rzbk5sTm5GSHFmZktCYmNz?=
 =?utf-8?B?aWRpSHJjV2I4cnhZNE8zc1ZNL1FVNy8vRTdqKzJKWGJ6dDBTRXNIcFN2ellt?=
 =?utf-8?B?RjY0OEdQL1ZFSi9sUnAyZStVVi9PTUk4S3lrYm9SMm82RXMreGtXUjJJdXBx?=
 =?utf-8?B?MDZJRkhKTDhWRGFMT1JFeDBnaDZKcE1xek40ODZXZ2Z4emhpS1pnL0EyTFZQ?=
 =?utf-8?B?M1M1a0dzZ1RDZUpQNkErR3ZPQWtIeFQzbEptbTN6bnJPdG03RWNpOFA5UUJO?=
 =?utf-8?B?bTR6SEY2a1FYQzVpRmxuc2NyeGpnVU9oVCs3MWl3V1hBUllyUjlKOUM5ZWNP?=
 =?utf-8?B?TjJwb2tLUFpNWW1ZM3J6TGtOSDRNZ2xDMG5pdEd5U0phRXhPRjk2L3BobHlW?=
 =?utf-8?B?NFdpZkg5NmFuNUtYcHNWYnNYcEZLSEFRQlVYOEl5czkvUGhlZXVFdXFmdWJn?=
 =?utf-8?B?MzRsYlVpeTZYUjFBRnNqdWwxK20wK2krUkxDYnltOG93NHZCSUVxOTdQOGdR?=
 =?utf-8?B?MCthRkRoc3RmZWtTNTc5M3MxVk42UE1aU1c5ME5CeWZ3UHFzVUdXeFl6eHJZ?=
 =?utf-8?B?SmxaTzJobWdUU3I3Y3NUSnMzRTNqV3ViVlRZWlUwdHJWZW5kS0NKL2xiOW40?=
 =?utf-8?B?OTZva3NSeDhsRnhsNk1IYjFxM0NKT2xqZHR6Mm9VeW8zOXlFOTNoYml2eFBD?=
 =?utf-8?B?SWJsU2RpNXpKejM3ZDgwUFFWN0dqZll3RDJxRkRYemJWZ2dKQUZZZHhhT25K?=
 =?utf-8?B?aHNEZG5GK1NFbVoyTWZseTZXL0xLNkhSVTJ2QXQ3TmNHWko1MVdyVmZWYzNM?=
 =?utf-8?B?MHdTN3ZQSWtKbTFFeEFxaS9JYzBvMzRrNFV3TnpDc3lHQzJPY2o2VzFiMEQ0?=
 =?utf-8?B?TG5nMjE2cFJMWExRL3dtSFFraHdZTkYyeGlvbGpmdFBvNGR2RlcwV3gza2hi?=
 =?utf-8?B?UjN1amZ4QzluNXpuem5SN1VsV1l6MHMrZTdtanFrL0NnZHcvejJrY0IxYmVn?=
 =?utf-8?B?STR5NUdGVC8yUkNkd1Bpc0ZBQ2swY1hqTkJZNXoyU2VZZUdmTFV0VExLTWZx?=
 =?utf-8?B?Z1dLYkZZSGUvcjlIQ3ZjOVljVjFkejlaTmNYUExxYWVlVzd4eTVlZWZhVDhU?=
 =?utf-8?B?bi90ZWc3My9NWFcvbGxxY3ZxRTJvTHZNRDJ4N0UyREtJdWFVNlBsL3hRQVNX?=
 =?utf-8?B?YVEwOXRQRERSTGJXaGxsaWFWV1BtUng2eU8rcytiREdHSFUvM2J3amc3Q3ZL?=
 =?utf-8?B?cjhBZDZRb1pneFB4Mmdkd1NFSTRtKzZuK05OZTZBczltcDNEcEdMdW95d3BK?=
 =?utf-8?Q?BJMxO60HjRHjlc+QpN0Ah3Fm1?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 136b7024-f38d-4193-574b-08dac0ec21e0
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2022 18:16:04.1243
 (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: /x6cUlB8qp9j49TZaih7s0ArekLdCC69poXNT1IUHBRks05H8VDYMC+kYie879Irr3ejwX1mv1/vmqOfFEUtWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4146
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

+wyes.karny@amd.com

On 11/7/2022 11:57, Perry Yuan wrote:
> Add one sysfs entry to control the CPU cores frequency boost state
> The attribute file can allow user to set max performance boosted or
> keeping at normal perf level.
> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 53 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)

Make sure that you update the documentation for this new sysfs file as well.

> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 841b1e2383b8..6958c5fd9e1c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -786,12 +786,46 @@ static ssize_t show_energy_performance_preference(
>   		return  sprintf(buf, "%s\n", energy_perf_strings[preference]);
>   }
>   
> +static void amd_pstate_update_policies(void)
> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		cpufreq_update_policy(cpu);
> +}
> +
> +static ssize_t show_pstate_dynamic_boost(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%u\n", cppc_boost);
> +}
> +
> +static ssize_t store_pstate_dynamic_boost(struct kobject *a,
> +				       struct kobj_attribute *b,
> +				       const char *buf, size_t count)
> +{
> +	unsigned int input;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &input);
> +	if (ret)
> +		return ret;

To be more flexible maybe kstrtobool would be better here instead?

> +
> +	mutex_lock(&amd_pstate_driver_lock);
> +	cppc_boost = !!input;
> +	amd_pstate_update_policies();
> +	mutex_unlock(&amd_pstate_driver_lock);
> +
> +	return count;
> +}
> +
>   cpufreq_freq_attr_ro(amd_pstate_max_freq);
>   cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>   
>   cpufreq_freq_attr_ro(amd_pstate_highest_perf);
>   cpufreq_freq_attr_rw(energy_performance_preference);
>   cpufreq_freq_attr_ro(energy_performance_available_preferences);
> +define_one_global_rw(pstate_dynamic_boost);
>   
>   static struct freq_attr *amd_pstate_attr[] = {
>   	&amd_pstate_max_freq,
> @@ -809,6 +843,15 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
>   	NULL,
>   };
>   
> +static struct attribute *pstate_global_attributes[] = {
> +	&pstate_dynamic_boost.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group amd_pstate_global_attr_group = {
> +	.attrs = pstate_global_attributes,
> +};
> +
>   static inline void update_boost_state(void)
>   {
>   	u64 misc_en;
> @@ -1416,6 +1459,16 @@ static int __init amd_pstate_init(void)
>   		pr_err("failed to register amd pstate driver with return %d\n",
>   		       ret);
>   
> +	amd_pstate_kobj = kobject_create_and_add("amd-pstate", &cpu_subsys.dev_root->kobj);
> +	if (!amd_pstate_kobj)
> +		pr_err("amd-pstate: Global sysfs registration failed.\n");
> +
> +	ret = sysfs_create_group(amd_pstate_kobj, &amd_pstate_global_attr_group);
> +	if (ret) {
> +		pr_err("amd-pstate: Sysfs attribute export failed with error %d.\n",
> +		       ret);

amd-pstate can currently be a module too.  So don't you need the 
matching cleanup path for this code too?

Also, regarding the error messages you don't need to prefix with 
"amd-pstate: ", amd-pstate.c already sets `pr_fmt`.

> +	}
> +
>   	return ret;
>   }
>   device_initcall(amd_pstate_init);