[PATCH 0/6] TPMI update for new defines and permissions

[PATCH 0/6] TPMI update for new defines and permissions

[Srinivas Pandruvada]

Add TPMI information header version 2 fields. Also process read/write
 and enabled state for feature drivers. When a feature is disabled,
don't create a device to load a feature driver. When a read is blocked
then don't load feature drivers. When write is blocked continue to
function in read only mode.

Srinivas Pandruvada (6):
  platform/x86/intel/tpmi: Add additional TPMI header fields
  platform/x86/intel/tpmi: Don't create devices for disabled features
  platform/x86/intel/tpmi: Modify external interface to get read/write
    state
  platform/x86/intel/tpmi: Move TPMI ID definition
  platform/x86: ISST: Process read/write blocked feature status
  platform/x86/intel-uncore-freq: Process read/write blocked feature
    status

 .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++
 drivers/platform/x86/intel/tpmi.c             | 42 ++++++++++---------
 .../uncore-frequency/uncore-frequency-tpmi.c  | 15 +++++++
 include/linux/intel_tpmi.h                    | 24 +++++++++--
 4 files changed, 84 insertions(+), 22 deletions(-)

-- 
2.41.0

[PATCH 1/6] platform/x86/intel/tpmi: Add additional TPMI header fields

[Srinivas Pandruvada]

TPMI information header added additional fields in version 2. Some of the
reserved fields in version 1 are used to define new fields.
Parse new fields and export as part of platform data. These fields include:
- PCI segment ID
- Partition ID of the package, useful when more than one Intel VSEC PCI
device per package
- cdie_mask: Mask of all compute dies in this partition

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/tpmi.c | 11 ++++++++++-
 include/linux/intel_tpmi.h        |  6 ++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index 311abcac894a..c89aa4d14bea 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -128,6 +128,9 @@ struct intel_tpmi_info {
  * @dev:	PCI device number
  * @bus:	PCI bus number
  * @pkg:	CPU Package id
+ * @segment: PCI segment id
+ * @partition: Package Partition id
+ * @cdie_mask: Bitmap of compute dies in the current partition
  * @reserved:	Reserved for future use
  * @lock:	When set to 1 the register is locked and becomes read-only
  *		until next reset. Not for use by the OS driver.
@@ -139,7 +142,10 @@ struct tpmi_info_header {
 	u64 dev:5;
 	u64 bus:8;
 	u64 pkg:8;
-	u64 reserved:39;
+	u64 segment:8;
+	u64 partition:2;
+	u64 cdie_mask:16;
+	u64 reserved:13;
 	u64 lock:1;
 } __packed;
 
@@ -684,6 +690,9 @@ static int tpmi_process_info(struct intel_tpmi_info *tpmi_info,
 	tpmi_info->plat_info.bus_number = header.bus;
 	tpmi_info->plat_info.device_number = header.dev;
 	tpmi_info->plat_info.function_number = header.fn;
+	tpmi_info->plat_info.cdie_mask = header.cdie_mask;
+	tpmi_info->plat_info.partition = header.partition;
+	tpmi_info->plat_info.segment = header.segment;
 
 	iounmap(info_mem);
 
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index ee07393445f9..939663bb095f 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -14,7 +14,10 @@
 
 /**
  * struct intel_tpmi_plat_info - Platform information for a TPMI device instance
+ * @cdie_mask:	Mask of all compute dies in the partition
  * @package_id:	CPU Package id
+ * @partition:  Package partition id when multiple VSEC PCI devices per package
+ * @segment: PCI segment ID
  * @bus_number:	PCI bus number
  * @device_number: PCI device number
  * @function_number: PCI function number
@@ -23,7 +26,10 @@
  * struct is used to return data via tpmi_get_platform_data().
  */
 struct intel_tpmi_plat_info {
+	u16 cdie_mask;
 	u8 package_id;
+	u8 partition;
+	u8 segment;
 	u8 bus_number;
 	u8 device_number;
 	u8 function_number;
-- 
2.41.0

Re: [PATCH 1/6] platform/x86/intel/tpmi: Add additional TPMI header fields

[Ilpo Järvinen]

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> TPMI information header added additional fields in version 2. Some of the
> reserved fields in version 1 are used to define new fields.
> Parse new fields and export as part of platform data. These fields include:
> - PCI segment ID
> - Partition ID of the package, useful when more than one Intel VSEC PCI
> device per package
> - cdie_mask: Mask of all compute dies in this partition
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/tpmi.c | 11 ++++++++++-
>  include/linux/intel_tpmi.h        |  6 ++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index 311abcac894a..c89aa4d14bea 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -128,6 +128,9 @@ struct intel_tpmi_info {
>   * @dev:	PCI device number
>   * @bus:	PCI bus number
>   * @pkg:	CPU Package id
> + * @segment: PCI segment id
> + * @partition: Package Partition id
> + * @cdie_mask: Bitmap of compute dies in the current partition
>   * @reserved:	Reserved for future use
>   * @lock:	When set to 1 the register is locked and becomes read-only
>   *		until next reset. Not for use by the OS driver.
> @@ -139,7 +142,10 @@ struct tpmi_info_header {
>  	u64 dev:5;
>  	u64 bus:8;
>  	u64 pkg:8;
> -	u64 reserved:39;
> +	u64 segment:8;
> +	u64 partition:2;
> +	u64 cdie_mask:16;
> +	u64 reserved:13;
>  	u64 lock:1;
>  } __packed;
> @@ -684,6 +690,9 @@ static int tpmi_process_info(struct intel_tpmi_info *tpmi_info,
>  	tpmi_info->plat_info.bus_number = header.bus;
>  	tpmi_info->plat_info.device_number = header.dev;
>  	tpmi_info->plat_info.function_number = header.fn;
> +	tpmi_info->plat_info.cdie_mask = header.cdie_mask;
> +	tpmi_info->plat_info.partition = header.partition;
> +	tpmi_info->plat_info.segment = header.segment;
>  
>  	iounmap(info_mem);
>  
> diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
> index ee07393445f9..939663bb095f 100644
> --- a/include/linux/intel_tpmi.h
> +++ b/include/linux/intel_tpmi.h
> @@ -14,7 +14,10 @@
>  
>  /**
>   * struct intel_tpmi_plat_info - Platform information for a TPMI device instance
> + * @cdie_mask:	Mask of all compute dies in the partition
>   * @package_id:	CPU Package id
> + * @partition:  Package partition id when multiple VSEC PCI devices per package
> + * @segment: PCI segment ID
>   * @bus_number:	PCI bus number
>   * @device_number: PCI device number
>   * @function_number: PCI function number
> @@ -23,7 +26,10 @@
>   * struct is used to return data via tpmi_get_platform_data().
>   */
>  struct intel_tpmi_plat_info {
> +	u16 cdie_mask;
>  	u8 package_id;
> +	u8 partition;
> +	u8 segment;
>  	u8 bus_number;
>  	u8 device_number;
>  	u8 function_number;

I've a number of questions about this patch...

- There no version check anywhere, yet commit message talks about v2?

- What will those fields have in v1?

- Entirely unrelated to the rest of this serie? So no users for these?
  Why not send this along with the patches containing the actual users
  so it'd have been easier to find the answers from the patches?

-- 
 i.

Re: [PATCH 1/6] platform/x86/intel/tpmi: Add additional TPMI header fields

[srinivas pandruvada]

On Thu, 2023-11-30 at 14:33 +0200, Ilpo Järvinen wrote:
> On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> 
> > TPMI information header added additional fields in version 2. Some
> > of the
> > reserved fields in version 1 are used to define new fields.
> > Parse new fields and export as part of platform data. These fields
> > include:
> > - PCI segment ID
> > - Partition ID of the package, useful when more than one Intel VSEC
> > PCI
> > device per package
> > - cdie_mask: Mask of all compute dies in this partition
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/tpmi.c | 11 ++++++++++-
> >  include/linux/intel_tpmi.h        |  6 ++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel/tpmi.c
> > b/drivers/platform/x86/intel/tpmi.c
> > index 311abcac894a..c89aa4d14bea 100644
> > --- a/drivers/platform/x86/intel/tpmi.c
> > +++ b/drivers/platform/x86/intel/tpmi.c
> > @@ -128,6 +128,9 @@ struct intel_tpmi_info {
> >   * @dev:       PCI device number
> >   * @bus:       PCI bus number
> >   * @pkg:       CPU Package id
> > + * @segment: PCI segment id
> > + * @partition: Package Partition id
> > + * @cdie_mask: Bitmap of compute dies in the current partition
> >   * @reserved:  Reserved for future use
> >   * @lock:      When set to 1 the register is locked and becomes
> > read-only
> >   *             until next reset. Not for use by the OS driver.
> > @@ -139,7 +142,10 @@ struct tpmi_info_header {
> >         u64 dev:5;
> >         u64 bus:8;
> >         u64 pkg:8;
> > -       u64 reserved:39;
> > +       u64 segment:8;
> > +       u64 partition:2;
> > +       u64 cdie_mask:16;
> > +       u64 reserved:13;
> >         u64 lock:1;
> >  } __packed;
> > @@ -684,6 +690,9 @@ static int tpmi_process_info(struct
> > intel_tpmi_info *tpmi_info,
> >         tpmi_info->plat_info.bus_number = header.bus;
> >         tpmi_info->plat_info.device_number = header.dev;
> >         tpmi_info->plat_info.function_number = header.fn;
> > +       tpmi_info->plat_info.cdie_mask = header.cdie_mask;
> > +       tpmi_info->plat_info.partition = header.partition;
> > +       tpmi_info->plat_info.segment = header.segment;
> >  
> >         iounmap(info_mem);
> >  
> > diff --git a/include/linux/intel_tpmi.h
> > b/include/linux/intel_tpmi.h
> > index ee07393445f9..939663bb095f 100644
> > --- a/include/linux/intel_tpmi.h
> > +++ b/include/linux/intel_tpmi.h
> > @@ -14,7 +14,10 @@
> >  
> >  /**
> >   * struct intel_tpmi_plat_info - Platform information for a TPMI
> > device instance
> > + * @cdie_mask: Mask of all compute dies in the partition
> >   * @package_id:        CPU Package id
> > + * @partition:  Package partition id when multiple VSEC PCI
> > devices per package
> > + * @segment: PCI segment ID
> >   * @bus_number:        PCI bus number
> >   * @device_number: PCI device number
> >   * @function_number: PCI function number
> > @@ -23,7 +26,10 @@
> >   * struct is used to return data via tpmi_get_platform_data().
> >   */
> >  struct intel_tpmi_plat_info {
> > +       u16 cdie_mask;
> >         u8 package_id;
> > +       u8 partition;
> > +       u8 segment;
> >         u8 bus_number;
> >         u8 device_number;
> >         u8 function_number;
> 
> I've a number of questions about this patch...
> 
> - There no version check anywhere, yet commit message talks about v2?
> 
No need to check, as the other fields will be 0s in v1.

> - What will those fields have in v1?
They are 0s as they were reserved for future use.

> 
> - Entirely unrelated to the rest of this serie? So no users for
> these?
>   Why not send this along with the patches containing the actual
> users
>   so it'd have been easier to find the answers from the patches?
> 
This is the core changes, so submitted as changes done in specs.
But fine to bundle with next series. I limit number of changes per
kernel release in the order of importance in the current products.

Thanks,
Srinivas

[PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[Srinivas Pandruvada]

If some TPMI features are disabled, don't create auxiliary devices. In
this way feature drivers will not load.

While creating auxiliary devices, call tpmi_read_feature_status() to
check feature state and return if the feature is disabled without
creating a device.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/tpmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index c89aa4d14bea..4edaa182db04 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -604,9 +604,17 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
 	struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
 	char feature_id_name[TPMI_FEATURE_NAME_LEN];
 	struct intel_vsec_device *feature_vsec_dev;
+	struct tpmi_feature_state feature_state;
 	struct resource *res, *tmp;
 	const char *name;
-	int i;
+	int i, ret;
+
+	ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state);
+	if (ret)
+		return ret;
+
+	if (!feature_state.enabled)
+		return -EOPNOTSUPP;
 
 	name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
 	if (!name)
-- 
2.41.0

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[Ilpo Järvinen]

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> If some TPMI features are disabled, don't create auxiliary devices. In
> this way feature drivers will not load.
> 
> While creating auxiliary devices, call tpmi_read_feature_status() to
> check feature state and return if the feature is disabled without
> creating a device.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/tpmi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index c89aa4d14bea..4edaa182db04 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -604,9 +604,17 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  	struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
>  	char feature_id_name[TPMI_FEATURE_NAME_LEN];
>  	struct intel_vsec_device *feature_vsec_dev;
> +	struct tpmi_feature_state feature_state;
>  	struct resource *res, *tmp;
>  	const char *name;
> -	int i;
> +	int i, ret;
> +
> +	ret = tpmi_read_feature_status(tpmi_info, pfs->pfs_header.tpmi_id, &feature_state);
> +	if (ret)
> +		return ret;
> +
> +	if (!feature_state.enabled)
> +		return -EOPNOTSUPP;

-ENODEV sounds more appropriate.  

-- 
 i.

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[srinivas pandruvada]

On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote:
> On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> 
> > If some TPMI features are disabled, don't create auxiliary devices.
> > In
> > this way feature drivers will not load.
> > 
> > While creating auxiliary devices, call tpmi_read_feature_status()
> > to
> > check feature state and return if the feature is disabled without
> > creating a device.
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/tpmi.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel/tpmi.c
> > b/drivers/platform/x86/intel/tpmi.c
> > index c89aa4d14bea..4edaa182db04 100644
> > --- a/drivers/platform/x86/intel/tpmi.c
> > +++ b/drivers/platform/x86/intel/tpmi.c
> > @@ -604,9 +604,17 @@ static int tpmi_create_device(struct
> > intel_tpmi_info *tpmi_info,
> >         struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
> >         char feature_id_name[TPMI_FEATURE_NAME_LEN];
> >         struct intel_vsec_device *feature_vsec_dev;
> > +       struct tpmi_feature_state feature_state;
> >         struct resource *res, *tmp;
> >         const char *name;
> > -       int i;
> > +       int i, ret;
> > +
> > +       ret = tpmi_read_feature_status(tpmi_info, pfs-
> > >pfs_header.tpmi_id, &feature_state);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!feature_state.enabled)
> > +               return -EOPNOTSUPP;
> 
> -ENODEV sounds more appropriate.  
The -EOPNOTSUPP is returned matching the next return statement, which
causes to continue to create devices which are supported and not
disabled. Any other error is real device creation will causes driver
modprobe to fail.

Thanks,
Srinivas

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[Ilpo Järvinen]

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

On Thu, 30 Nov 2023, srinivas pandruvada wrote:

> On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote:
> > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> > 
> > > If some TPMI features are disabled, don't create auxiliary devices.
> > > In
> > > this way feature drivers will not load.
> > > 
> > > While creating auxiliary devices, call tpmi_read_feature_status()
> > > to
> > > check feature state and return if the feature is disabled without
> > > creating a device.
> > > 
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>
> > > ---
> > >  drivers/platform/x86/intel/tpmi.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/tpmi.c
> > > b/drivers/platform/x86/intel/tpmi.c
> > > index c89aa4d14bea..4edaa182db04 100644
> > > --- a/drivers/platform/x86/intel/tpmi.c
> > > +++ b/drivers/platform/x86/intel/tpmi.c
> > > @@ -604,9 +604,17 @@ static int tpmi_create_device(struct
> > > intel_tpmi_info *tpmi_info,
> > >         struct intel_vsec_device *vsec_dev = tpmi_info->vsec_dev;
> > >         char feature_id_name[TPMI_FEATURE_NAME_LEN];
> > >         struct intel_vsec_device *feature_vsec_dev;
> > > +       struct tpmi_feature_state feature_state;
> > >         struct resource *res, *tmp;
> > >         const char *name;
> > > -       int i;
> > > +       int i, ret;
> > > +
> > > +       ret = tpmi_read_feature_status(tpmi_info, pfs-
> > > >pfs_header.tpmi_id, &feature_state);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       if (!feature_state.enabled)
> > > +               return -EOPNOTSUPP;
> > 
> > -ENODEV sounds more appropriate.  
>
> The -EOPNOTSUPP is returned matching the next return statement, which
> causes to continue to create devices which are supported and not
> disabled. Any other error is real device creation will causes driver
> modprobe to fail.

Oh, I see... I didn't look that deep into the code during my review
(perhaps note that down into the commit message?).

-- 
 i.

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[Andy Shevchenko]

On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote:
> On Thu, 30 Nov 2023, srinivas pandruvada wrote:
> > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote:
> > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

...

> > > > +       if (!feature_state.enabled)
> > > > +               return -EOPNOTSUPP;
> > > 
> > > -ENODEV sounds more appropriate.  
> >
> > The -EOPNOTSUPP is returned matching the next return statement, which
> > causes to continue to create devices which are supported and not
> > disabled. Any other error is real device creation will causes driver
> > modprobe to fail.
> 
> Oh, I see... I didn't look that deep into the code during my review
> (perhaps note that down into the commit message?).

Maybe we should even use -ENOTSUPP (Linux internal error code), so
it will be clear that it's _not_ going to user space?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[srinivas pandruvada]

On Thu, 2023-11-30 at 16:38 +0200, Andy Shevchenko wrote:
> On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote:
> > On Thu, 30 Nov 2023, srinivas pandruvada wrote:
> > > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote:
> > > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> 
> ...
> 
> > > > > +       if (!feature_state.enabled)
> > > > > +               return -EOPNOTSUPP;
> > > > 
> > > > -ENODEV sounds more appropriate.  
> > > 
> > > The -EOPNOTSUPP is returned matching the next return statement,
> > > which
> > > causes to continue to create devices which are supported and not
> > > disabled. Any other error is real device creation will causes
> > > driver
> > > modprobe to fail.
> > 
> > Oh, I see... I didn't look that deep into the code during my review
> > (perhaps note that down into the commit message?).
> 
> Maybe we should even use -ENOTSUPP (Linux internal error code), so
> it will be clear that it's _not_ going to user space?

That will be better. I will change and resubmit.

Thanks,
Srinivas

> 

Re: [PATCH 2/6] platform/x86/intel/tpmi: Don't create devices for disabled features

[srinivas pandruvada]

On Thu, 2023-11-30 at 10:00 -0500, srinivas pandruvada wrote:
> On Thu, 2023-11-30 at 16:38 +0200, Andy Shevchenko wrote:
> > On Thu, Nov 30, 2023 at 04:33:00PM +0200, Ilpo Järvinen wrote:
> > > On Thu, 30 Nov 2023, srinivas pandruvada wrote:
> > > > On Thu, 2023-11-30 at 14:26 +0200, Ilpo Järvinen wrote:
> > > > > On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> > 
> > ...
> > 
> > > > > > +       if (!feature_state.enabled)
> > > > > > +               return -EOPNOTSUPP;
> > > > > 
> > > > > -ENODEV sounds more appropriate.  
> > > > 
> > > > The -EOPNOTSUPP is returned matching the next return statement,
> > > > which
> > > > causes to continue to create devices which are supported and
> > > > not
> > > > disabled. Any other error is real device creation will causes
> > > > driver
> > > > modprobe to fail.
> > > 
> > > Oh, I see... I didn't look that deep into the code during my
> > > review
> > > (perhaps note that down into the commit message?).
> > 
> > Maybe we should even use -ENOTSUPP (Linux internal error code), so
> > it will be clear that it's _not_ going to user space?
> 
> That will be better. I will change and resubmit.
The checkpatch gives error with this.


WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
#25: FILE: drivers/platform/x86/intel/tpmi.c:613:
+		return -ENOTSUPP;

Thanks,
Srinivas
> 
> Thanks,
> Srinivas
> 
> > 
> 

[PATCH 3/6] platform/x86/intel/tpmi: Modify external interface to get read/write state

[Srinivas Pandruvada]

Modify the external interface tpmi_get_feature_status() to get read
and write blocked instead of locked and disabled. Since auxiliary device
is not created when disabled, no use of returning disabled state. Also
locked state is not useful as feature driver can't use locked state
in a meaningful way.

Using read and write state, feature driver can decide which operations
to restrict for that feature.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/tpmi.c | 8 ++++----
 include/linux/intel_tpmi.h        | 5 ++---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index 4edaa182db04..44773c210324 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -351,8 +351,8 @@ static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int featu
 	return ret;
 }
 
-int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
-			    int *locked, int *disabled)
+int tpmi_get_feature_status(struct auxiliary_device *auxdev,
+			    int feature_id, int *read_blocked, int *write_blocked)
 {
 	struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(auxdev->dev.parent);
 	struct intel_tpmi_info *tpmi_info = auxiliary_get_drvdata(&intel_vsec_dev->auxdev);
@@ -363,8 +363,8 @@ int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
 	if (ret)
 		return ret;
 
-	*locked = feature_state.locked;
-	*disabled = !feature_state.enabled;
+	*read_blocked = feature_state.read_blocked;
+	*write_blocked = feature_state.write_blocked;
 
 	return 0;
 }
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index 939663bb095f..a240e15ef77f 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -38,7 +38,6 @@ struct intel_tpmi_plat_info {
 struct intel_tpmi_plat_info *tpmi_get_platform_data(struct auxiliary_device *auxdev);
 struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int index);
 int tpmi_get_resource_count(struct auxiliary_device *auxdev);
-
-int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, int *locked,
-			    int *disabled);
+int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, int *read_blocked,
+			    int *write_blocked);
 #endif
-- 
2.41.0

Re: [PATCH 3/6] platform/x86/intel/tpmi: Modify external interface to get read/write state

[Ilpo Järvinen]

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

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> Modify the external interface tpmi_get_feature_status() to get read
> and write blocked instead of locked and disabled. Since auxiliary device
> is not created when disabled, no use of returning disabled state. Also
> locked state is not useful as feature driver can't use locked state
> in a meaningful way.
> 
> Using read and write state, feature driver can decide which operations
> to restrict for that feature.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/tpmi.c | 8 ++++----
>  include/linux/intel_tpmi.h        | 5 ++---
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index 4edaa182db04..44773c210324 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -351,8 +351,8 @@ static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int featu
>  	return ret;
>  }
>  
> -int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
> -			    int *locked, int *disabled)
> +int tpmi_get_feature_status(struct auxiliary_device *auxdev,
> +			    int feature_id, int *read_blocked, int *write_blocked)

Noting down there's logical reversion of the parameters here as to me
locked sound similar to write_blocked and disabled likewise to 
read_blocked but since there are no users for this function so far
I suppose it's fine.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

[PATCH 4/6] platform/x86/intel/tpmi: Move TPMI ID definitions

[Srinivas Pandruvada]

Move TPMI ID definitions to common include file. In this way other
feature drivers don't have to redefine.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/tpmi.c | 13 -------------
 include/linux/intel_tpmi.h        | 13 +++++++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index 44773c210324..14575da91d2c 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -176,19 +176,6 @@ struct tpmi_feature_state {
 	u32 locked:1;
 } __packed;
 
-/*
- * List of supported TMPI IDs.
- * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
- */
-enum intel_tpmi_id {
-	TPMI_ID_RAPL = 0, /* Running Average Power Limit */
-	TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */
-	TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */
-	TPMI_ID_SST = 5, /* Speed Select Technology */
-	TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */
-	TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */
-};
-
 /*
  * The size from hardware is in u32 units. This size is from a trusted hardware,
  * but better to verify for pre silicon platforms. Set size to 0, when invalid.
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index a240e15ef77f..6c31768cdb83 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -12,6 +12,19 @@
 #define TPMI_MINOR_VERSION(val)	FIELD_GET(GENMASK(4, 0), val)
 #define TPMI_MAJOR_VERSION(val)	FIELD_GET(GENMASK(7, 5), val)
 
+/*
+ * List of supported TMPI IDs.
+ * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
+ */
+enum intel_tpmi_id {
+	TPMI_ID_RAPL = 0, /* Running Average Power Limit */
+	TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */
+	TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */
+	TPMI_ID_SST = 5, /* Speed Select Technology */
+	TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */
+	TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */
+};
+
 /**
  * struct intel_tpmi_plat_info - Platform information for a TPMI device instance
  * @cdie_mask:	Mask of all compute dies in the partition
-- 
2.41.0

Re: [PATCH 4/6] platform/x86/intel/tpmi: Move TPMI ID definitions

[Ilpo Järvinen]

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

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> Move TPMI ID definitions to common include file. In this way other
> feature drivers don't have to redefine.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/tpmi.c | 13 -------------
>  include/linux/intel_tpmi.h        | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index 44773c210324..14575da91d2c 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -176,19 +176,6 @@ struct tpmi_feature_state {
>  	u32 locked:1;
>  } __packed;
>  
> -/*
> - * List of supported TMPI IDs.
> - * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
> - */
> -enum intel_tpmi_id {
> -	TPMI_ID_RAPL = 0, /* Running Average Power Limit */
> -	TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */
> -	TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */
> -	TPMI_ID_SST = 5, /* Speed Select Technology */
> -	TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */
> -	TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */
> -};
> -
>  /*
>   * The size from hardware is in u32 units. This size is from a trusted hardware,
>   * but better to verify for pre silicon platforms. Set size to 0, when invalid.
> diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
> index a240e15ef77f..6c31768cdb83 100644
> --- a/include/linux/intel_tpmi.h
> +++ b/include/linux/intel_tpmi.h
> @@ -12,6 +12,19 @@
>  #define TPMI_MINOR_VERSION(val)	FIELD_GET(GENMASK(4, 0), val)
>  #define TPMI_MAJOR_VERSION(val)	FIELD_GET(GENMASK(7, 5), val)
>  
> +/*
> + * List of supported TMPI IDs.
> + * Some TMPI IDs are not used by Linux, so the numbers are not consecutive.
> + */
> +enum intel_tpmi_id {
> +	TPMI_ID_RAPL = 0, /* Running Average Power Limit */
> +	TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */
> +	TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */
> +	TPMI_ID_SST = 5, /* Speed Select Technology */
> +	TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */
> +	TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */
> +};

While at it, could you align the comments to start at the same column so 
it would slightly less messy to read.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH 5/6] platform/x86: ISST: Process read/write blocked feature status

[Srinivas Pandruvada]

When a feature is read blocked, don't continue to read SST information
and register with SST core.

When the feature is write blocked, continue to offer read interface for
SST parameters, but don't allow any operation to change state. A state
change results from SST level change, feature change or class of service
change.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

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 0b6d2c864437..ed3a04d6c99c 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
@@ -234,6 +234,7 @@ struct perf_level {
  * @saved_clos_configs:	Save SST-CP CLOS configuration to store restore for suspend/resume
  * @saved_clos_assocs:	Save SST-CP CLOS association to store restore for suspend/resume
  * @saved_pp_control:	Save SST-PP control information to store restore for suspend/resume
+ * @write_blocked:	Write operation is blocked, so can't change SST state
  *
  * This structure is used store complete SST information for a power_domain. This information
  * is used to read/write request for any SST IOCTL. Each physical CPU package can have multiple
@@ -259,6 +260,7 @@ struct tpmi_per_power_domain_info {
 	u64 saved_clos_configs[4];
 	u64 saved_clos_assocs[4];
 	u64 saved_pp_control;
+	bool write_blocked;
 };
 
 /**
@@ -514,6 +516,9 @@ static long isst_if_clos_param(void __user *argp)
 	if (!power_domain_info)
 		return -EINVAL;
 
+	if (power_domain_info->write_blocked)
+		return -EPERM;
+
 	if (clos_param.get_set) {
 		_write_cp_info("clos.min_freq", clos_param.min_freq_mhz,
 			       (SST_CLOS_CONFIG_0_OFFSET + clos_param.clos * SST_REG_SIZE),
@@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user *argp)
 
 		power_domain_info = &sst_inst->power_domain_info[punit_id];
 
+		if (power_domain_info->write_blocked)
+			return -EPERM;
+
 		offset = SST_CLOS_ASSOC_0_OFFSET +
 				(punit_cpu_no / SST_CLOS_ASSOC_CPUS_PER_REG) * SST_REG_SIZE;
 		shift = punit_cpu_no % SST_CLOS_ASSOC_CPUS_PER_REG;
@@ -752,6 +760,9 @@ static int isst_if_set_perf_level(void __user *argp)
 	if (!power_domain_info)
 		return -EINVAL;
 
+	if (power_domain_info->write_blocked)
+		return -EPERM;
+
 	if (!(power_domain_info->pp_header.allowed_level_mask & BIT(perf_level.level)))
 		return -EINVAL;
 
@@ -809,6 +820,9 @@ static int isst_if_set_perf_feature(void __user *argp)
 	if (!power_domain_info)
 		return -EINVAL;
 
+	if (power_domain_info->write_blocked)
+		return -EPERM;
+
 	_write_pp_info("perf_feature", perf_feature.feature, SST_PP_CONTROL_OFFSET,
 		       SST_PP_FEATURE_STATE_START, SST_PP_FEATURE_STATE_WIDTH,
 		       SST_MUL_FACTOR_NONE)
@@ -1257,11 +1271,21 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd,
 
 int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
 {
+	int read_blocked = 0, write_blocked = 0;
 	struct intel_tpmi_plat_info *plat_info;
 	struct tpmi_sst_struct *tpmi_sst;
 	int i, ret, pkg = 0, inst = 0;
 	int num_resources;
 
+	ret = tpmi_get_feature_status(auxdev, TPMI_ID_SST, &read_blocked, &write_blocked);
+	if (ret)
+		dev_info(&auxdev->dev, "Can't read feature status: ignoring read/write blocked status\n");
+
+	if (read_blocked) {
+		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
+		return -ENODEV;
+	}
+
 	plat_info = tpmi_get_platform_data(auxdev);
 	if (!plat_info) {
 		dev_err(&auxdev->dev, "No platform info\n");
@@ -1306,6 +1330,7 @@ int tpmi_sst_dev_add(struct auxiliary_device *auxdev)
 		tpmi_sst->power_domain_info[i].package_id = pkg;
 		tpmi_sst->power_domain_info[i].power_domain_id = i;
 		tpmi_sst->power_domain_info[i].auxdev = auxdev;
+		tpmi_sst->power_domain_info[i].write_blocked = write_blocked;
 		tpmi_sst->power_domain_info[i].sst_base = devm_ioremap_resource(&auxdev->dev, res);
 		if (IS_ERR(tpmi_sst->power_domain_info[i].sst_base))
 			return PTR_ERR(tpmi_sst->power_domain_info[i].sst_base);
-- 
2.41.0

Re: [PATCH 5/6] platform/x86: ISST: Process read/write blocked feature status

[Ilpo Järvinen]

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> When a feature is read blocked, don't continue to read SST information
> and register with SST core.
> 
> When the feature is write blocked, continue to offer read interface for
> SST parameters, but don't allow any operation to change state. A state
> change results from SST level change, feature change or class of service
> change.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  .../intel/speed_select_if/isst_tpmi_core.c    | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> 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 0b6d2c864437..ed3a04d6c99c 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
> @@ -514,6 +516,9 @@ static long isst_if_clos_param(void __user *argp)
>  	if (!power_domain_info)
>  		return -EINVAL;
>  
> +	if (power_domain_info->write_blocked)
> +		return -EPERM;
> +

I don't understand this, doesn't this now -EPERM both _write_cp_info() AND
_read_cp_info()??? Does _read_cp_info() also change state??

>  	if (clos_param.get_set) {
>  		_write_cp_info("clos.min_freq", clos_param.min_freq_mhz,
>  			       (SST_CLOS_CONFIG_0_OFFSET + clos_param.clos * SST_REG_SIZE),
> @@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user *argp)
>  
>  		power_domain_info = &sst_inst->power_domain_info[punit_id];
>  
> +		if (power_domain_info->write_blocked)
> +			return -EPERM;

Same here, this blocks also the get path?


-- 
 i.

Re: [PATCH 5/6] platform/x86: ISST: Process read/write blocked feature status

[srinivas pandruvada]

On Thu, 2023-11-30 at 14:20 +0200, Ilpo Järvinen wrote:
> On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:
> 
> > When a feature is read blocked, don't continue to read SST
> > information
> > and register with SST core.
> > 
> > When the feature is write blocked, continue to offer read interface
> > for
> > SST parameters, but don't allow any operation to change state. A
> > state
> > change results from SST level change, feature change or class of
> > service
> > change.
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > ---
> >  .../intel/speed_select_if/isst_tpmi_core.c    | 25
> > +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > 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 0b6d2c864437..ed3a04d6c99c 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
> > @@ -514,6 +516,9 @@ static long isst_if_clos_param(void __user
> > *argp)
> >         if (!power_domain_info)
> >                 return -EINVAL;
> >  
> > +       if (power_domain_info->write_blocked)
> > +               return -EPERM;
> > +
> 
> I don't understand this, doesn't this now -EPERM both
> _write_cp_info() AND
> _read_cp_info()??? Does _read_cp_info() also change state??
You have a point here. Unlike other SST features, CP access is useful
for OS as it know workloads and priorities.
But I will change for consistency.

Thanks,
Srinivas

> 
> >         if (clos_param.get_set) {
> >                 _write_cp_info("clos.min_freq",
> > clos_param.min_freq_mhz,
> >                                (SST_CLOS_CONFIG_0_OFFSET +
> > clos_param.clos * SST_REG_SIZE),
> > @@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user
> > *argp)
> >  
> >                 power_domain_info = &sst_inst-
> > >power_domain_info[punit_id];
> >  
> > +               if (power_domain_info->write_blocked)
> > +                       return -EPERM;
> 
> Same here, this blocks also the get path?
> 
> 

[PATCH 6/6] platform/x86/intel-uncore-freq: Process read/write blocked feature status

[Srinivas Pandruvada]

When a feature is read blocked, don't continue to read uncore information
and register with uncore core.

When the feature is write blocked, continue to offer read interface but
block setting uncore limits.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../uncore-frequency/uncore-frequency-tpmi.c      | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

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 4fb790552c47..de5db49a9afe 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -66,6 +66,7 @@ struct tpmi_uncore_struct {
 	int min_ratio;
 	struct tpmi_uncore_power_domain_info *pd_info;
 	struct tpmi_uncore_cluster_info root_cluster;
+	bool write_blocked;
 };
 
 #define UNCORE_GENMASK_MIN_RATIO	GENMASK_ULL(21, 15)
@@ -157,6 +158,9 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
 	uncore_root = cluster_info->uncore_root;
 
+	if (uncore_root->write_blocked)
+		return -EPERM;
+
 	/* Update each cluster in a package */
 	if (cluster_info->root_domain) {
 		struct tpmi_uncore_struct *uncore_root = cluster_info->uncore_root;
@@ -233,11 +237,21 @@ static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
 
 static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
 {
+	int read_blocked = 0, write_blocked = 0;
 	struct intel_tpmi_plat_info *plat_info;
 	struct tpmi_uncore_struct *tpmi_uncore;
 	int ret, i, pkg = 0;
 	int num_resources;
 
+	ret = tpmi_get_feature_status(auxdev, TPMI_ID_UNCORE, &read_blocked, &write_blocked);
+	if (ret)
+		dev_info(&auxdev->dev, "Can't read feature status: ignoring blocked status\n");
+
+	if (read_blocked) {
+		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
+		return -ENODEV;
+	}
+
 	/* Get number of power domains, which is equal to number of resources */
 	num_resources = tpmi_get_resource_count(auxdev);
 	if (!num_resources)
@@ -266,6 +280,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
 	}
 
 	tpmi_uncore->power_domain_count = num_resources;
+	tpmi_uncore->write_blocked = write_blocked;
 
 	/* Get the package ID from the TPMI core */
 	plat_info = tpmi_get_platform_data(auxdev);
-- 
2.41.0

Re: [PATCH 6/6] platform/x86/intel-uncore-freq: Process read/write blocked feature status

[Ilpo Järvinen]

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

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> When a feature is read blocked, don't continue to read uncore information
> and register with uncore core.
> 
> When the feature is write blocked, continue to offer read interface but
> block setting uncore limits.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  .../uncore-frequency/uncore-frequency-tpmi.c      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> 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 4fb790552c47..de5db49a9afe 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> @@ -66,6 +66,7 @@ struct tpmi_uncore_struct {
>  	int min_ratio;
>  	struct tpmi_uncore_power_domain_info *pd_info;
>  	struct tpmi_uncore_cluster_info root_cluster;
> +	bool write_blocked;
>  };
>  
>  #define UNCORE_GENMASK_MIN_RATIO	GENMASK_ULL(21, 15)
> @@ -157,6 +158,9 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
>  	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
>  	uncore_root = cluster_info->uncore_root;
>  
> +	if (uncore_root->write_blocked)
> +		return -EPERM;
> +
>  	/* Update each cluster in a package */
>  	if (cluster_info->root_domain) {
>  		struct tpmi_uncore_struct *uncore_root = cluster_info->uncore_root;
> @@ -233,11 +237,21 @@ static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
>  
>  static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>  {
> +	int read_blocked = 0, write_blocked = 0;
>  	struct intel_tpmi_plat_info *plat_info;
>  	struct tpmi_uncore_struct *tpmi_uncore;
>  	int ret, i, pkg = 0;
>  	int num_resources;
>  
> +	ret = tpmi_get_feature_status(auxdev, TPMI_ID_UNCORE, &read_blocked, &write_blocked);
> +	if (ret)
> +		dev_info(&auxdev->dev, "Can't read feature status: ignoring blocked status\n");
> +
> +	if (read_blocked) {
> +		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
> +		return -ENODEV;
> +	}
> +
>  	/* Get number of power domains, which is equal to number of resources */
>  	num_resources = tpmi_get_resource_count(auxdev);
>  	if (!num_resources)
> @@ -266,6 +280,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
>  	}
>  
>  	tpmi_uncore->power_domain_count = num_resources;
> +	tpmi_uncore->write_blocked = write_blocked;
>  
>  	/* Get the package ID from the TPMI core */
>  	plat_info = tpmi_get_platform_data(auxdev);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

While reviewing this, I begun to wonder why's the 
tpmi_get_feature_status() interface using int * as in practice these will 
always be converted to bool by the users?

-- 
 i.