linux-kernel - Re: [PATCH 2/3] platform/x86: ISST : Check major minor version

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: <72a2766bc7bb4f9d6b3d5f2ff114f0af1b6646a4.camel@linux.intel.com>
Date:   Sat, 30 Sep 2023 05:42:47 -0700
From:   srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Hans de Goede <hdegoede@...hat.com>, markgross@...nel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] platform/x86: ISST : Check major minor version

On Fri, 2023-09-29 at 17:28 +0300, Ilpo Järvinen wrote:
> On Mon, 25 Sep 2023, Srinivas Pandruvada wrote:
> 
> > Parse major and minor version number from the version field. If
> > there
> > is a mismatch for major version, exit from further processing for
> > that
> > domain.
> > 
> > If there is mismatch in minor version, driver continue to process
> > with
> > an error message.
> 
> This sentence sounds odd.
What is the suggestion here?

> 
> > Minor version change doesn't change offsets and bit
> > structures of TPMI fields.
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@...ux.intel.com>
> > ---
> >  .../x86/intel/speed_select_if/isst_tpmi_core.c   | 16
> > ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> > b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> > index 63faa2ea8327..37f17e229419 100644
> > --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> > +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> > @@ -30,7 +30,8 @@
> >  #include "isst_if_common.h"
> >  
> >  /* Supported SST hardware version by this driver */
> > -#define ISST_HEADER_VERSION            1
> > +#define ISST_MAJOR_VERSION     0
> > +#define ISST_MINOR_VERSION     1
> >  
> >  /*
> >   * Used to indicate if value read from MMIO needs to get
> > multiplied
> > @@ -352,12 +353,19 @@ static int sst_main(struct auxiliary_device
> > *auxdev, struct tpmi_per_power_domai
> >         pd_info->sst_header.cp_offset *= 8;
> >         pd_info->sst_header.pp_offset *= 8;
> >  
> > -       if (pd_info->sst_header.interface_version !=
> > ISST_HEADER_VERSION) {
> > -               dev_err(&auxdev->dev, "SST: Unsupported
> > version:%x\n",
> > -                       pd_info->sst_header.interface_version);
> > +       if (pd_info->sst_header.interface_version ==
> > TPMI_VERSION_INVALID)
> > +               return -ENODEV;
> > +
> > +       if (TPMI_MAJOR_VERSION(pd_info-
> > >sst_header.interface_version) != ISST_MAJOR_VERSION) {
> > +               dev_err(&auxdev->dev, "SST: Unsupported major
> > version:%lx\n",
> > +                       TPMI_MAJOR_VERSION(pd_info-
> > >sst_header.interface_version));
> >                 return -ENODEV;
> >         }
> >  
> > +       if (TPMI_MINOR_VERSION(pd_info-
> > >sst_header.interface_version) != ISST_MINOR_VERSION)
> > +               dev_err(&auxdev->dev, "SST: Ignore: Unsupported
> > minor version:%lx\n",
> > +                       TPMI_MINOR_VERSION(pd_info-
> > >sst_header.interface_version));
> 
> Why is this dev_err(), wouldn't dev_info() be more appropriate since 
> after this patch it's no longer an error?

The distro run with minimum log level. So it is important that they
notice and upgrade as they may be missing some new additions.

Thanks,
Srinivas


> 
> > +
> >         /* Read SST CP Header */
> >         *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base +
> > pd_info->sst_header.cp_offset);
> >  
> > 
>