On Fri, 23 Aug 2024, srinivas pandruvada wrote: > 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 > > > --- > > >  .../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 > > > > 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. AFAICT, the version number check is not on the path that is called from create_attr_group(). The version number check is in uncore_probe() which then propagates this knowledge into read/write_eff_lat_ctrl() using ->elc_supported. > So this > condition will never be true unless some IO read failure. So are you saying ->elc_supported check is not required (added by patch 2)? It return -EOPNOTSUPP not because of an "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@linux.intel.com/ > > > > ...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. ?? read/write_eff_lat_ctrl() check for ->root_domain and return -ENODATA if it is true. If that patch from you I linked above is applied, this line won't execute on some systems: tpmi_uncore->root_cluster.root_domain = true; Will that cause an issue (for read/write_eff_lat_ctrl())? My concern here is that misusing error values like this to do supported/not-supported check leads to fragility that would not occur if errors would be treated as hard errors and supported is checked by other means (which would be easy here using ->elc_supported, AFAICT). -- i.