Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4563 bytes --]

On Wed, 21 Aug 2024, Tero Kristo wrote:

> Add the TPMI efficiency latency control fields to the sysfs interface.
> The sysfs files are mapped to the TPMI uncore driver via the registered
> uncore_read and uncore_write driver callbacks. These fields are not
> populated on older non TPMI hardware.
> 
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  .../uncore-frequency-common.c                 | 42 ++++++++++++++++---
>  .../uncore-frequency-common.h                 | 13 +++++-
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> index 4e880585cbe4..e22b683a7a43 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data *data, char *buf, enum uncore_index
>  static ssize_t store_attr(struct uncore_data *data, const char *buf, ssize_t count,
>  			  enum uncore_index index)
>  {
> -	unsigned int input;
> +	unsigned int input = 0;
>  	int ret;
>  
> -	if (kstrtouint(buf, 10, &input))
> -		return -EINVAL;
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> +		if (kstrtobool(buf, (bool *)&input))
> +			return -EINVAL;
> +	} else {
> +		if (kstrtouint(buf, 10, &input))
> +			return -EINVAL;
> +	}
>  
>  	mutex_lock(&uncore_lock);
>  	ret = uncore_write(data, input, index);
> @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
>  
>  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
>  
> +store_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +store_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> +store_uncore_attr(elc_high_threshold_enable,
> +		  UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> +store_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> +
> +show_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +show_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> +show_uncore_attr(elc_high_threshold_enable,
> +		 UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> +show_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> +
>  #define show_uncore_data(member_name)					\
>  	static ssize_t show_##member_name(struct kobject *kobj,	\
>  					   struct kobj_attribute *attr, char *buf)\
> @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
>  
>  static int create_attr_group(struct uncore_data *data, char *name)
>  {
> -	int ret, freq, index = 0;
> +	int ret, index = 0;
> +	unsigned int val;
>  
>  	init_attribute_rw(max_freq_khz);
>  	init_attribute_rw(min_freq_khz);
> @@ -168,10 +186,24 @@ static int create_attr_group(struct uncore_data *data, char *name)
>  	data->uncore_attrs[index++] = &data->initial_min_freq_khz_kobj_attr.attr;
>  	data->uncore_attrs[index++] = &data->initial_max_freq_khz_kobj_attr.attr;
>  
> -	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
> +	ret = uncore_read(data, &val, UNCORE_INDEX_CURRENT_FREQ);
>  	if (!ret)
>  		data->uncore_attrs[index++] = &data->current_freq_khz_kobj_attr.attr;
>  
> +	ret = uncore_read(data, &val, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +	if (!ret) {
> +		init_attribute_rw(elc_low_threshold_percent);
> +		init_attribute_rw(elc_high_threshold_percent);
> +		init_attribute_rw(elc_high_threshold_enable);
> +		init_attribute_rw(elc_floor_freq_khz);
> +
> +		data->uncore_attrs[index++] = &data->elc_low_threshold_percent_kobj_attr.attr;
> +		data->uncore_attrs[index++] = &data->elc_high_threshold_percent_kobj_attr.attr;
> +		data->uncore_attrs[index++] =
> +			&data->elc_high_threshold_enable_kobj_attr.attr;
> +		data->uncore_attrs[index++] = &data->elc_floor_freq_khz_kobj_attr.attr;
> +	}

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

But I have to say I'm not big fan of this function treating any error as 
an implicit indication of ELC not supported.

Is that even going to be true after this:

  https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/

...as root_domain is eliminated for other reasons than ELC 
supported/not-supported (-ENODATA return path)?

-- 
 i.