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: 7619 bytes --]

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 <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.
>
> 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.