On Fri, 23 Aug 2024, srinivas pandruvada wrote:
> 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.
Okay, I see. I was missing this piece of understanding about the root
domain.
> > 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?
I don't think there will any issue with that other patch now that I
understand the internals a bit better than before.
> > 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.
Okay, lets just keep things as they are for now.
--
i.
