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