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.
v3:
No code change. Rebased on top of
https://kernel.googlesource.com/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86 review-hans
v2:
Dropped patch platform/x86/intel/tpmi: Add additional TPMI header fields
Addressed other review comments, details are in each patch
Srinivas Pandruvada (5):
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 | 35 +++++++++----------
.../uncore-frequency/uncore-frequency-tpmi.c | 15 ++++++++
include/linux/intel_tpmi.h | 18 ++++++++--
4 files changed, 72 insertions(+), 21 deletions(-)
--
2.40.1
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]>
Reviewed-by: Hans de Goede <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
v3
- Rebased on top of /pdx86/platform-drivers-x86 review-hans
v2
- Add comment for returning -EOPNOTSUPP
drivers/platform/x86/intel/tpmi.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index 311abcac894a..09972e2b8a06 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -598,9 +598,21 @@ 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 not enabled, continue to look at other features in the PFS, so return -EOPNOTSUPP.
+ * This will not cause failure of loading of this driver.
+ */
+ if (!feature_state.enabled)
+ return -EOPNOTSUPP;
name = intel_tpmi_name(pfs->pfs_header.tpmi_id);
if (!name)
--
2.40.1
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]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
v3
- Rebased on top of /pdx86/platform-drivers-x86 review-hans
v2
- Aligned comments for TPMI IDs
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 92c5c3a90d53..e73cdea67fff 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -170,19 +170,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 4f89c5bd8663..a3529b962be6 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
* @package_id: CPU Package id
--
2.40.1
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]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
v3
- Rebased on top of /pdx86/platform-drivers-x86 review-hans
v2:
- Change read_blocked/write_blocked to bool *
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 09972e2b8a06..92c5c3a90d53 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -345,8 +345,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, bool *read_blocked, bool *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);
@@ -357,8 +357,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 ee07393445f9..4f89c5bd8663 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -32,7 +32,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, bool *read_blocked,
+ bool *write_blocked);
#endif
--
2.40.1
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]>
Reviewed-by: Hans de Goede <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
v3
- Rebased on top of /pdx86/platform-drivers-x86 review-hans
v2
- Change read_blocked, write_blocked to bool
- Move the check for power_domain_info->write_blocked for SST-CP
to only write operations
.../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..2662fbbddf0c 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;
};
/**
@@ -515,6 +517,9 @@ static long isst_if_clos_param(void __user *argp)
return -EINVAL;
if (clos_param.get_set) {
+ if (power_domain_info->write_blocked)
+ return -EPERM;
+
_write_cp_info("clos.min_freq", clos_param.min_freq_mhz,
(SST_CLOS_CONFIG_0_OFFSET + clos_param.clos * SST_REG_SIZE),
SST_CLOS_CONFIG_MIN_START, SST_CLOS_CONFIG_MIN_WIDTH,
@@ -602,6 +607,9 @@ static long isst_if_clos_assoc(void __user *argp)
power_domain_info = &sst_inst->power_domain_info[punit_id];
+ if (assoc_cmds.get_set && 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)
{
+ bool 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.40.1
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]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
v3
- Rebased on top of /pdx86/platform-drivers-x86 review-hans
v2
- Change read_blocked, write_blocked to bool
.../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..bd75d61ff8a6 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)
{
+ bool 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.40.1
Hi,
On 12/4/23 23:17, Srinivas Pandruvada wrote:
> 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.
Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> v3:
> No code change. Rebased on top of
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86 review-hans
>
> v2:
> Dropped patch platform/x86/intel/tpmi: Add additional TPMI header fields
> Addressed other review comments, details are in each patch
>
> Srinivas Pandruvada (5):
> 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 | 35 +++++++++----------
> .../uncore-frequency/uncore-frequency-tpmi.c | 15 ++++++++
> include/linux/intel_tpmi.h | 18 ++++++++--
> 4 files changed, 72 insertions(+), 21 deletions(-)
>