lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <863beeab7f6ecf36796394c75e95fc7a0396a862.camel@linux.intel.com>
Date: Fri, 23 Aug 2024 09:12:21 -0400
From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Tero
	Kristo <tero.kristo@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
 platform-driver-x86@...r.kernel.org,  LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency
 latency control to sysfs interface

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ