| Message ID | 20240821131321.824326-4-tero.kristo@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: Add support for Intel uncore ELC feature | expand |
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. 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)?
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. So this condition will never be true unless some 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. Thanks, Srinivas >
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).
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
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@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. 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.
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; + } + data->uncore_attrs[index] = NULL; data->uncore_attr_group.name = name; diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h index b5c7311bfa05..26c854cd5d97 100644 --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h @@ -34,6 +34,13 @@ * @domain_id_kobj_attr: Storage for kobject attribute domain_id * @fabric_cluster_id_kobj_attr: Storage for kobject attribute fabric_cluster_id * @package_id_kobj_attr: Storage for kobject attribute package_id + * @elc_low_threshold_percent_kobj_attr: + Storage for kobject attribute elc_low_threshold_percent + * @elc_high_threshold_percent_kobj_attr: + Storage for kobject attribute elc_high_threshold_percent + * @elc_high_threshold_enable_kobj_attr: + Storage for kobject attribute elc_high_threshold_enable + * @elc_floor_freq_khz_kobj_attr: Storage for kobject attribute elc_floor_freq_khz * @uncore_attrs: Attribute storage for group creation * * This structure is used to encapsulate all data related to uncore sysfs @@ -61,7 +68,11 @@ struct uncore_data { struct kobj_attribute domain_id_kobj_attr; struct kobj_attribute fabric_cluster_id_kobj_attr; struct kobj_attribute package_id_kobj_attr; - struct attribute *uncore_attrs[9]; + struct kobj_attribute elc_low_threshold_percent_kobj_attr; + struct kobj_attribute elc_high_threshold_percent_kobj_attr; + struct kobj_attribute elc_high_threshold_enable_kobj_attr; + struct kobj_attribute elc_floor_freq_khz_kobj_attr; + struct attribute *uncore_attrs[13]; }; #define UNCORE_DOMAIN_ID_INVALID -1
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(-)