All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Limonciello, Mario" <mario.limonciello@amd.com>
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
Subject: Re: [PATCH v3 7/8] cpufreq: amd-pstate: add frequency dynamic boost sysfs control
Date: Mon, 7 Nov 2022 13:09:41 -0600	[thread overview]
Message-ID: <caeaf2a6-1156-0649-d8bd-b66ab99b589a@amd.com> (raw)
In-Reply-To: <1b7a5cd5-6ecf-8529-f10b-4f7f06fc7462@amd.com>

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


  reply	other threads:[~2022-11-07 19:10 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07 17:56 [PATCH v3 0/8] Implement AMD Pstate EPP Driver Perry Yuan
2022-11-07 17:56 ` [PATCH v3 1/8] ACPI: CPPC: Add AMD pstate energy performance preference cppc control Perry Yuan
2022-11-07 18:44   ` Limonciello, Mario
2022-11-10 14:49     ` Rafael J. Wysocki
2022-11-10 15:51       ` Yuan, Perry
2022-11-10 15:55         ` Rafael J. Wysocki
2022-11-10 16:23           ` Yuan, Perry
2022-11-13 16:28     ` Yuan, Perry
2022-11-07 17:56 ` [PATCH v3 2/8] Documentation: amd-pstate: add EPP profiles introduction Perry Yuan
2022-11-07 18:33   ` Limonciello, Mario
2022-11-13 16:23     ` Yuan, Perry
2022-11-10 14:57   ` Rafael J. Wysocki
2022-11-10 15:07     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in type Perry Yuan
2022-11-07 18:29   ` Limonciello, Mario
2022-11-13 16:21     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 4/8] cpufreq: amd_pstate: add AMD Pstate EPP support for the MSR based processors Perry Yuan
2022-11-07 20:32   ` Limonciello, Mario
2022-11-10 15:59     ` Nathan Fontenot
2022-11-10 16:22       ` Yuan, Perry
2022-11-08  7:21   ` kernel test robot
2022-11-09  5:55   ` kernel test robot
2022-11-07 17:57 ` [PATCH v3 5/8] cpufreq: amd_pstate: implement amd pstate cpu online and offline callback Perry Yuan
2022-11-07 18:22   ` Limonciello, Mario
2022-11-13 16:19     ` Yuan, Perry
2022-11-07 17:57 ` [PATCH v3 6/8] cpufreq: amd-pstate: implement suspend and resume callbacks Perry Yuan
2022-11-07 18:18   ` Limonciello, Mario
2022-11-13 16:19     ` Yuan, Perry
2022-11-10 16:19   ` Nathan Fontenot
2022-11-07 17:57 ` [PATCH v3 7/8] cpufreq: amd-pstate: add frequency dynamic boost sysfs control Perry Yuan
2022-11-07 18:16   ` Limonciello, Mario
2022-11-07 19:09     ` Limonciello, Mario [this message]
2022-11-07 17:57 ` [PATCH v3 8/8] cpufreq: amd_pstate: add driver working mode status sysfs entry Perry Yuan
2022-11-07 18:10   ` Limonciello, Mario
2022-11-13 16:18     ` Yuan, Perry
2022-11-10 16:06   ` Nathan Fontenot
2022-11-10 16:49     ` Yuan, Perry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=caeaf2a6-1156-0649-d8bd-b66ab99b589a@amd.com \
    --to=mario.limonciello@amd.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Deepak.Sharma@amd.com \
    --cc=Li.Meng@amd.com \
    --cc=Nathan.Fontenot@amd.com \
    --cc=Perry.Yuan@amd.com \
    --cc=Shimmer.Huang@amd.com \
    --cc=Xiaojian.Du@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=ray.huang@amd.com \
    --cc=viresh.kumar@linaro.org \
    --cc=wyes.karny@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.