diff mbox series

[v3,7/8] cpufreq: amd-pstate: add frequency dynamic boost sysfs control

Message ID 20221107175705.2207842-8-Perry.Yuan@amd.com
State New
Headers
Series Implement AMD Pstate EPP Driver |

Commit Message

Yuan, Perry Nov. 7, 2022, 5:57 p.m. UTC 
  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(+)

Comments

Mario Limonciello Nov. 7, 2022, 6:16 p.m. UTC | #1 
+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);
Mario Limonciello Nov. 7, 2022, 7:09 p.m. UTC | #2 
On 11/7/2022 12:16, Limonciello, Mario wrote:
> +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?

I noticed your patch 3/8 makes this change, so this specific comment 
might not be as important anymore.

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

Also, shouldn't failing to make these attributes fail the rest of 
amd-pstate init rather than just showing error messages?

> 
>> +    }
>> +
>>       return ret;
>>   }
>>   device_initcall(amd_pstate_init);
>
diff mbox series

Patch

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;
+
+	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);
+	}
+
 	return ret;
 }
 device_initcall(amd_pstate_init);