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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
Move TPMI ID definitions to common include file. In this way other
feature drivers don't have to redefine.
Signed-off-by: Srinivas Pandruvada <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
i.
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 <[email protected]>
> ---
> 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 <[email protected]>
--
i.
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 <[email protected]>
> ---
> .../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.
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 <[email protected]>
> ---
> .../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 <[email protected]>
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.
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 <[email protected]>
> ---
> 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.
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
> > <[email protected]>
> > ---
> > 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
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
> > <[email protected]>
> > ---
> > .../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?
>
>
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
> > > <[email protected]>
> > > ---
> > > 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.
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
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
>
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
>
> >
>