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

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Fri, 2024-08-23 at 16:29 +0300, Ilpo Järvinen wrote:
> 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.

I mean uncore_read() should fail if the current platform doesn't
support ELC.
Here that check is via a flag cluster_info->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"??
> 
I take back IO read fail. readq() will never fail here. uncore_read()
can only fail on non TPMI platforms only for IO issues.

 We should check elc_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)?
> > 
> > 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;
> 
Yes and will return without calling any callbacks for any attribute for
root domain only on these systems. So read/write_eff_lat_ctrl() will
not be called for root domain. For other domains the callbacks are
called before this check. 

> Will that cause an issue (for read/write_eff_lat_ctrl())?
We don't present ELC fields on root_domain on any system.

Can you tell what kind of issues you are worried about, may be I am not
getting?

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

Attribute creation is in common part which includes non TPMI systems,
which we still support for all clients for several gens.

We can add a feature mask as part of struct uncore_data and avoid
calling uncore_read() and treat uncore_read() error as hard errors as a
separate change. elc_supported can be moved to this structure, but I
want to avoid as we will be adding some more features, which are again
TPMI specific, so more flags will be needed.

Thanks,
Srinivas