Changes for core and feature drivers to check major minor version.
Srinivas Pandruvada (3):
platform/x86/intel/tpmi: Add defines to get version information
platform/x86: ISST : Check major minor version
platform/x86/intel-uncore-freq: Check major minor version
.../x86/intel/speed_select_if/isst_tpmi_core.c | 16 ++++++++++++----
.../uncore-frequency/uncore-frequency-tpmi.c | 17 +++++++++++++----
include/linux/intel_tpmi.h | 6 ++++++
3 files changed, 31 insertions(+), 8 deletions(-)
--
2.41.0
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. Minor version change doesn't change offsets and bit
structures of TPMI fields.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
.../uncore-frequency/uncore-frequency-tpmi.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index 7d0a67f8b517..4d63ee6bc6d2 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -28,7 +28,8 @@
#include "uncore-frequency-common.h"
-#define UNCORE_HEADER_VERSION 1
+#define UNCORE_MAJOR_VERSION 0
+#define UNCORE_MINOR_VERSION 1
#define UNCORE_HEADER_INDEX 0
#define UNCORE_FABRIC_CLUSTER_OFFSET 8
@@ -302,12 +303,20 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
/* Check for version and skip this resource if there is mismatch */
header = readq(pd_info->uncore_base);
pd_info->ufs_header_ver = header & UNCORE_VERSION_MASK;
- if (pd_info->ufs_header_ver != UNCORE_HEADER_VERSION) {
- dev_info(&auxdev->dev, "Uncore: Unsupported version:%d\n",
- pd_info->ufs_header_ver);
+
+ if (pd_info->ufs_header_ver == TPMI_VERSION_INVALID)
+ continue;
+
+ if (TPMI_MAJOR_VERSION(pd_info->ufs_header_ver) != UNCORE_MAJOR_VERSION) {
+ dev_info(&auxdev->dev, "Uncore: Unsupported major version:%lx\n",
+ TPMI_MAJOR_VERSION(pd_info->ufs_header_ver));
continue;
}
+ if (TPMI_MINOR_VERSION(pd_info->ufs_header_ver) != UNCORE_MINOR_VERSION)
+ dev_info(&auxdev->dev, "Uncore: Ignore: Unsupported minor version:%lx\n",
+ TPMI_MINOR_VERSION(pd_info->ufs_header_ver));
+
/* Get Cluster ID Mask */
cluster_mask = FIELD_GET(UNCORE_LOCAL_FABRIC_CLUSTER_ID_MASK, header);
if (!cluster_mask) {
--
2.41.0
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. Minor version change doesn't change offsets and bit
structures of TPMI fields.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
.../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));
+
/* Read SST CP Header */
*((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
--
2.41.0
Add defines to get major and minor version from a TPMI version field
value. This will avoid code duplication to convert in every feature
driver. Also add define for invalid version field.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
include/linux/intel_tpmi.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index 04d937ad4dc4..ee07393445f9 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -6,6 +6,12 @@
#ifndef _INTEL_TPMI_H_
#define _INTEL_TPMI_H_
+#include <linux/bitfield.h>
+
+#define TPMI_VERSION_INVALID 0xff
+#define TPMI_MINOR_VERSION(val) FIELD_GET(GENMASK(4, 0), val)
+#define TPMI_MAJOR_VERSION(val) FIELD_GET(GENMASK(7, 5), val)
+
/**
* struct intel_tpmi_plat_info - Platform information for a TPMI device instance
* @package_id: CPU Package id
--
2.41.0
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.
> Minor version change doesn't change offsets and bit
> structures of TPMI fields.
>
> Signed-off-by: Srinivas Pandruvada <[email protected]>
> ---
> .../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?
> +
> /* Read SST CP Header */
> *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
>
>
--
i.
Also the shortlog could be improved, IMO. E.g., Ignore minor version
mismatch.
--
i.
On Fri, 29 Sep 2023, 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.
>
> > Minor version change doesn't change offsets and bit
> > structures of TPMI fields.
> >
> > Signed-off-by: Srinivas Pandruvada <[email protected]>
> > ---
> > .../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?
>
> > +
> > /* Read SST CP Header */
> > *((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
> >
> >
>
>
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
> > <[email protected]>
> > ---
> > .../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);
> >
> >
>
On Fri, 2023-09-29 at 17:33 +0300, Ilpo Järvinen wrote:
> Also the shortlog could be improved, IMO. E.g., Ignore minor version
> mismatch.
>
This is not just ignoring minor version, important is error on major
version change. Instead I can change to
"Process major and minor version"
or
"Check major and minor version"
On Mon, 2 Oct 2023, srinivas pandruvada wrote:
> On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, srinivas pandruvada wrote:
> >
> > > 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?
> >
> > These things sound wrong: It should be "continues"
> OK
>
> > but I also find "to
> > process" odd when lacking what it processes.
>
> Further processing means, whatever this driver is supposed to do. You
> don't want to fully explain again what this driver is doing.
Well, whatever. I'm not a native anyway so I cannot really be the defining
authority how the language gets used so feel free to leave "to process"
as is.
--
i.
On Mon, 2023-10-02 at 17:24 +0300, Ilpo Järvinen wrote:
> On Mon, 2 Oct 2023, srinivas pandruvada wrote:
>
> > On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote:
> > > On Sat, 30 Sep 2023, srinivas pandruvada wrote:
> > >
> > > > 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?
> > >
> > > These things sound wrong: It should be "continues"
> > OK
> >
> > > but I also find "to
> > > process" odd when lacking what it processes.
> >
> > Further processing means, whatever this driver is supposed to do.
> > You
> > don't want to fully explain again what this driver is doing.
>
> Well, whatever. I'm not a native anyway so I cannot really be the
> defining
> authority how the language gets used so feel free to leave "to
> process"
> as is.'
That is not my intention! I want to make it as good as possible. Let me
come up with new description.
Thanks,
Srinivas
>
On Mon, 2023-10-02 at 16:49 +0300, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, srinivas pandruvada wrote:
>
> > 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?
>
> These things sound wrong: It should be "continues"
OK
> but I also find "to
> process" odd when lacking what it processes.
Further processing means, whatever this driver is supposed to do. You
don't want to fully explain again what this driver is doing.
>
On Sat, 30 Sep 2023, srinivas pandruvada wrote:
> 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?
These things sound wrong: It should be "continues" but I also find "to
process" odd when lacking what it processes.
--
i.