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

[srinivas pandruvada]

On Fri, 2024-08-23 at 16:03 +0300, Ilpo Järvinen wrote:
> 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@xxxxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxxxx>
> 
> But I have to say I'm not big fan of this function treating any error
> as 
> an implicit indication of ELC not supported.
Also there is a check for version number, which supports ELC. So this
condition will never be true unless some IO read failure.

> 
> Is that even going to be true after this:
> 
>  
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@xxxxxxxxxxxxxxx/
> 
> ...as root_domain is eliminated for other reasons than ELC 
> supported/not-supported (-ENODATA return path)?
Even if ELC is not supported, but all others fields will always be
supported from base version. The above change doesn't do anything with
root domain.

Thanks,
Srinivas
>