Re: [PATCH v6 08/11] cpufreq: amd_pstate: add driver working mode status sysfs entry

[Limonciello, Mario]

On 12/2/2022 01:47, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@xxxxxxx>
>
> While amd-pstate driver was loaded with specific driver mode, it will

The *driver* doesn't need to check which mode is enabled from the sysfs 
file, but userspace may want to check this.  I think you should reword 
this accordingly in the commit message.

> 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@xxxxxxx>
> ---
>   drivers/cpufreq/amd-pstate.c | 44 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6936df6e8642..7f748a579023 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -66,6 +66,8 @@ static bool cppc_active;
>   static int cppc_load __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;
>   
> @@ -807,6 +809,46 @@ static ssize_t store_boost(struct kobject *a,
>   	return count;
>   }
>   
> +static ssize_t amd_pstate_show_status(char *buf)
> +{
> +	if (!default_pstate_driver)
> +		return sysfs_emit(buf, "off\n");
> +
> +	return sysfs_emit(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! */
> +	return -EOPNOTSUPP;
> +}

Why not just fix this as part of the series?  It should be no different 
than unloading the driver and reloading it in the appropriate mode, right?

Is this going to be a short term FIXME/TODO or a long term?  If it's 
long term, it might be better to just make the attribute ro and and have 
just the show callback.  Then when you're ready to make it rw you can 
add the store callback, change it from ro to rw and update documentation 
at that time.

> +
> +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);
>   
> @@ -814,6 +856,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(boost);
> +define_one_global_rw(status);
>   
>   static struct freq_attr *amd_pstate_attr[] = {
>   	&amd_pstate_max_freq,
> @@ -833,6 +876,7 @@ static struct freq_attr *amd_pstate_epp_attr[] = {
>   
>   static struct attribute *pstate_global_attributes[] = {
>   	&boost.attr,
> +	&status.attr,

In the series I didn't see a matching Documentation update for the new 
sysfs file, can you please add one?

>   	NULL
>   };
>