[PATCH 0/3] TPMI feature major/minor version check

[PATCH 0/3] TPMI feature major/minor version check

[Srinivas Pandruvada]

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

[PATCH 1/3] platform/x86/intel/tpmi: Add defines to get version information

[Srinivas Pandruvada]

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 <srinivas.pandruvada@linux.intel.com>
---
 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

[PATCH 2/3] platform/x86: ISST : Check major minor version

[Srinivas Pandruvada]

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 <srinivas.pandruvada@linux.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));
+
 	/* Read SST CP Header */
 	*((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
 
-- 
2.41.0

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

[Ilpo Järvinen]

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 <srinivas.pandruvada@linux.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?

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

-- 
 i.

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

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 <srinivas.pandruvada@linux.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?
> 
> > +
> >  	/* Read SST CP Header */
> >  	*((u64 *)&pd_info->cp_header) = readq(pd_info->sst_base + pd_info->sst_header.cp_offset);
> >  
> > 
> 
> 

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

[srinivas pandruvada]

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"

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

[srinivas pandruvada]

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

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.

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

[srinivas pandruvada]

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.


> 

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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.

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

[srinivas pandruvada]

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


> 

[PATCH 3/3] platform/x86/intel-uncore-freq: Check major minor version

[Srinivas Pandruvada]

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 <srinivas.pandruvada@linux.intel.com>
---
 .../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