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