[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <72f8e103-b5d1-0def-685a-280b061fa759@amd.com>
Date: Mon, 7 Nov 2022 12:10:28 -0600
From: "Limonciello, Mario" <mario.limonciello@....com>
To: Perry Yuan <Perry.Yuan@....com>, rafael.j.wysocki@...el.com,
ray.huang@....com, viresh.kumar@...aro.org
Cc: Deepak.Sharma@....com, Nathan.Fontenot@....com,
Alexander.Deucher@....com, Shimmer.Huang@....com,
Xiaojian.Du@....com, Li.Meng@....com, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 8/8] cpufreq: amd_pstate: add driver working mode
status sysfs entry
On 11/7/2022 11:57, Perry Yuan wrote:
> While amd-pstate driver was loaded with specific driver mode, it will
> need to check which mode is enabled for the pstate driver,add this sysfs
> entry to show the current status
>
> $ cat /sys/devices/system/cpu/amd-pstate/status
> active >
> Signed-off-by: Perry Yuan <Perry.Yuan@....com>
> ---
> drivers/cpufreq/amd-pstate.c | 44 ++++++++++++++++++++++++++++++++++++
You should update the matching amd-pstate documentation for this new
sysfs file.
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6958c5fd9e1c..eadcc9d61d39 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -65,6 +65,8 @@ static int disable_pstate_load __initdata;
> static int epp_off __initdata;
>
> static struct cpufreq_driver *default_pstate_driver;
> +static struct cpufreq_driver amd_pstate_epp_driver;
> +static struct cpufreq_driver amd_pstate_driver;
> static struct amd_cpudata **all_cpu_data;
>
> static struct amd_pstate_params global_params;
> @@ -819,6 +821,46 @@ static ssize_t store_pstate_dynamic_boost(struct kobject *a,
> return count;
> }
>
> +static ssize_t amd_pstate_show_status(char *buf)
> +{
> + if (!default_pstate_driver)
> + return sprintf(buf, "off\n");
> +
> + return sprintf(buf, "%s\n", default_pstate_driver == &amd_pstate_epp_driver ?
> + "active" : "passive");
> +}
> +
> +static int amd_pstate_update_status(const char *buf, size_t size)
> +{
> + /* FIXME: add driver dynamic switching code */
> + return 0;
> +}
So "Effectively" this is read only right now. I think it's better to
mark the sysfs file as read only and not bother to populate "store"
callback until you're ready to allow dynamically swapping back and forth.
If you want to keep the code there, then I think you should return an
-EOPNOTSUPP error code for amd_pstate_update_status.
> +
> +static ssize_t show_status(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + ssize_t ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_show_status(buf);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret;
> +}
> +
> +static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
> + const char *buf, size_t count)
> +{
> + char *p = memchr(buf, '\n', count);
> + int ret;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> + ret = amd_pstate_update_status(buf, p ? p - buf : count);
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> + return ret < 0 ? ret : count;
> +}
> +
> cpufreq_freq_attr_ro(amd_pstate_max_freq);
> cpufreq_freq_attr_ro(amd_pstate_lowest_nonlinear_freq);
>
> @@ -826,6 +868,7 @@ 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);
> +define_one_global_rw(status);
>
> static struct freq_attr *amd_pstate_attr[] = {
> &amd_pstate_max_freq,
> @@ -845,6 +888,7 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
>
> static struct attribute *pstate_global_attributes[] = {
> &pstate_dynamic_boost.attr,
> + &status.attr,
> NULL
> };
>
Powered by blists - more mailing lists