The first patch provides interface to read feature status. This is
generic patch to be used by other feature drivers.
The second patch patch add support for debugfs. Debugfs also display
feature status using the first patch.
Srinivas Pandruvada (2):
platform/x86/intel/tpmi: Read feature control status
platform/x86/intel/tpmi: Add debugfs interface
drivers/platform/x86/intel/tpmi.c | 392 +++++++++++++++++++++++++++++-
include/linux/intel_tpmi.h | 2 +
2 files changed, 387 insertions(+), 7 deletions(-)
--
2.38.1
Some of the PM features can be locked or disabled. In that case, write
interface can be locked.
This status is read via a mailbox. There is one TPMI ID which provides
base address for interface and data register for mail box operation.
The mailbox operations is defined in the TPMI specification. Refer to
https://github.com/intel/tpmi_power_management/ for TPMI specifications.
An API is exposed to feature drivers to read feature control status.
Signed-off-by: Srinivas Pandruvada <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
As suggested by Dan Williams changed ioremap to devm_ioremap() after
review by Andy.
drivers/platform/x86/intel/tpmi.c | 147 ++++++++++++++++++++++++++++++
include/linux/intel_tpmi.h | 2 +
2 files changed, 149 insertions(+)
diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index a5227951decc..9545e9cdb924 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -47,8 +47,11 @@
*/
#include <linux/auxiliary_bus.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
#include <linux/intel_tpmi.h>
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/pci.h>
@@ -98,6 +101,7 @@ struct intel_tpmi_pm_feature {
* @feature_count: Number of TPMI of TPMI instances pointed by tpmi_features
* @pfs_start: Start of PFS offset for the TPMI instances in this device
* @plat_info: Stores platform info which can be used by the client drivers
+ * @tpmi_control_mem: Memory mapped IO for getting control information
*
* Stores the information for all TPMI devices enumerated from a single PCI device.
*/
@@ -107,6 +111,7 @@ struct intel_tpmi_info {
int feature_count;
u64 pfs_start;
struct intel_tpmi_plat_info plat_info;
+ void __iomem *tpmi_control_mem;
};
/**
@@ -139,6 +144,7 @@ enum intel_tpmi_id {
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 */
};
@@ -175,6 +181,144 @@ struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int
}
EXPORT_SYMBOL_NS_GPL(tpmi_get_resource_at_index, INTEL_TPMI);
+/* TPMI Control Interface */
+
+#define TPMI_CONTROL_STATUS_OFFSET 0x00
+#define TPMI_COMMAND_OFFSET 0x08
+#define TPMI_DATA_OFFSET 0x0C
+/*
+ * Spec is calling for max 1 seconds to get ownership at the worst
+ * case. Read at 10 ms timeouts and repeat up to 1 second.
+ */
+#define TPMI_CONTROL_TIMEOUT_US (10 * USEC_PER_MSEC)
+#define TPMI_CONTROL_TIMEOUT_MAX_US USEC_PER_SEC
+
+#define TPMI_RB_TIMEOUT_US (10 * USEC_PER_MSEC)
+#define TPMI_RB_TIMEOUT_MAX_US USEC_PER_SEC
+
+#define TPMI_OWNER_NONE 0
+#define TPMI_OWNER_IN_BAND 1
+
+#define TPMI_GENMASK_OWNER GENMASK_ULL(5, 4)
+#define TPMI_GENMASK_STATUS GENMASK_ULL(15, 8)
+
+#define TPMI_GET_STATE_CMD 0x10
+#define TPMI_GET_STATE_CMD_DATA_OFFSET 8
+#define TPMI_CMD_DATA_OFFSET 32
+#define TPMI_CMD_PKT_LEN_OFFSET 16
+#define TPMI_CMD_PKT_LEN 2
+#define TPMI_CONTROL_RB_BIT 0
+#define TPMI_CONTROL_CPL_BIT 6
+#define TPMI_CMD_STATUS_SUCCESS 0x40
+#define TPMI_GET_STATUS_BIT_ENABLE 0
+#define TPMI_GET_STATUS_BIT_LOCKED 31
+
+/* Mutex to complete get feature status without interruption */
+static DEFINE_MUTEX(tpmi_dev_lock);
+
+static int tpmi_wait_for_owner(struct intel_tpmi_info *tpmi_info, u8 owner)
+{
+ u64 control;
+
+ return read_poll_timeout(readq, control, owner == FIELD_GET(TPMI_GENMASK_OWNER, control),
+ TPMI_CONTROL_TIMEOUT_US, TPMI_CONTROL_TIMEOUT_MAX_US, false,
+ tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
+}
+
+static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int feature_id,
+ int *locked, int *disabled)
+{
+ u64 control, data;
+ int ret;
+
+ if (!tpmi_info->tpmi_control_mem)
+ return -EFAULT;
+
+ mutex_lock(&tpmi_dev_lock);
+
+ ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_NONE);
+ if (ret)
+ goto err_unlock;
+
+ /* set command id to 0x10 for TPMI_GET_STATE */
+ data = TPMI_GET_STATE_CMD;
+ /* 32 bits for DATA offset and +8 for feature_id field */
+ data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET + TPMI_GET_STATE_CMD_DATA_OFFSET));
+
+ /* Write at command offset for qword access */
+ writeq(data, tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET);
+
+ ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND);
+ if (ret)
+ goto err_unlock;
+
+ /* Set Run Busy and packet length of 2 dwords */
+ writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN << TPMI_CMD_PKT_LEN_OFFSET),
+ tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
+
+ ret = read_poll_timeout(readq, control, !(control & BIT_ULL(TPMI_CONTROL_RB_BIT)),
+ TPMI_RB_TIMEOUT_US, TPMI_RB_TIMEOUT_MAX_US, false,
+ tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
+ if (ret)
+ goto done_proc;
+
+ control = FIELD_GET(TPMI_GENMASK_STATUS, control);
+ if (control != TPMI_CMD_STATUS_SUCCESS) {
+ ret = -EBUSY;
+ goto done_proc;
+ }
+
+ data = readq(tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET);
+ data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for TPMI_DATA */
+
+ *disabled = 0;
+ *locked = 0;
+
+ if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE)))
+ *disabled = 1;
+
+ if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED))
+ *locked = 1;
+
+ ret = 0;
+
+done_proc:
+ /* SET CPL "completion"bit */
+ writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT),
+ tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
+
+err_unlock:
+ mutex_unlock(&tpmi_dev_lock);
+
+ return ret;
+}
+
+int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
+ int *locked, int *disabled)
+{
+ 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);
+
+ return tpmi_read_feature_status(tpmi_info, feature_id, locked, disabled);
+}
+EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI);
+
+static void tpmi_set_control_base(struct auxiliary_device *auxdev,
+ struct intel_tpmi_info *tpmi_info,
+ struct intel_tpmi_pm_feature *pfs)
+{
+ void __iomem *mem;
+ u16 size;
+
+ size = pfs->pfs_header.num_entries * pfs->pfs_header.entry_size * 4;
+ mem = devm_ioremap(&auxdev->dev, pfs->vsec_offset, size);
+ if (!mem)
+ return;
+
+ /* mem is pointing to TPMI CONTROL base */
+ tpmi_info->tpmi_control_mem = mem;
+}
+
static const char *intel_tpmi_name(enum intel_tpmi_id id)
{
switch (id) {
@@ -367,6 +511,9 @@ static int intel_vsec_tpmi_init(struct auxiliary_device *auxdev)
*/
if (pfs->pfs_header.tpmi_id == TPMI_INFO_ID)
tpmi_process_info(tpmi_info, pfs);
+
+ if (pfs->pfs_header.tpmi_id == TPMI_CONTROL_ID)
+ tpmi_set_control_base(auxdev, tpmi_info, pfs);
}
tpmi_info->pfs_start = pfs_start;
diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
index f505788c05da..04d937ad4dc4 100644
--- a/include/linux/intel_tpmi.h
+++ b/include/linux/intel_tpmi.h
@@ -27,4 +27,6 @@ struct intel_tpmi_plat_info *tpmi_get_platform_data(struct auxiliary_device *aux
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);
#endif
--
2.38.1
On Thu, 15 Jun 2023, Srinivas Pandruvada wrote:
> Some of the PM features can be locked or disabled. In that case, write
> interface can be locked.
>
> This status is read via a mailbox. There is one TPMI ID which provides
> base address for interface and data register for mail box operation.
> The mailbox operations is defined in the TPMI specification. Refer to
> https://github.com/intel/tpmi_power_management/ for TPMI specifications.
>
> An API is exposed to feature drivers to read feature control status.
>
> Signed-off-by: Srinivas Pandruvada <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> ---
> As suggested by Dan Williams changed ioremap to devm_ioremap() after
> review by Andy.
>
> drivers/platform/x86/intel/tpmi.c | 147 ++++++++++++++++++++++++++++++
> include/linux/intel_tpmi.h | 2 +
> 2 files changed, 149 insertions(+)
>
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index a5227951decc..9545e9cdb924 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -47,8 +47,11 @@
> */
>
> #include <linux/auxiliary_bus.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> #include <linux/intel_tpmi.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/pci.h>
>
> @@ -98,6 +101,7 @@ struct intel_tpmi_pm_feature {
> * @feature_count: Number of TPMI of TPMI instances pointed by tpmi_features
> * @pfs_start: Start of PFS offset for the TPMI instances in this device
> * @plat_info: Stores platform info which can be used by the client drivers
> + * @tpmi_control_mem: Memory mapped IO for getting control information
> *
> * Stores the information for all TPMI devices enumerated from a single PCI device.
> */
> @@ -107,6 +111,7 @@ struct intel_tpmi_info {
> int feature_count;
> u64 pfs_start;
> struct intel_tpmi_plat_info plat_info;
> + void __iomem *tpmi_control_mem;
> };
>
> /**
> @@ -139,6 +144,7 @@ enum intel_tpmi_id {
> 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 */
> };
>
> @@ -175,6 +181,144 @@ struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int
> }
> EXPORT_SYMBOL_NS_GPL(tpmi_get_resource_at_index, INTEL_TPMI);
>
> +/* TPMI Control Interface */
> +
> +#define TPMI_CONTROL_STATUS_OFFSET 0x00
> +#define TPMI_COMMAND_OFFSET 0x08
> +#define TPMI_DATA_OFFSET 0x0C
> +/*
> + * Spec is calling for max 1 seconds to get ownership at the worst
> + * case. Read at 10 ms timeouts and repeat up to 1 second.
> + */
> +#define TPMI_CONTROL_TIMEOUT_US (10 * USEC_PER_MSEC)
> +#define TPMI_CONTROL_TIMEOUT_MAX_US USEC_PER_SEC
> +
> +#define TPMI_RB_TIMEOUT_US (10 * USEC_PER_MSEC)
> +#define TPMI_RB_TIMEOUT_MAX_US USEC_PER_SEC
> +
> +#define TPMI_OWNER_NONE 0
> +#define TPMI_OWNER_IN_BAND 1
> +
> +#define TPMI_GENMASK_OWNER GENMASK_ULL(5, 4)
> +#define TPMI_GENMASK_STATUS GENMASK_ULL(15, 8)
> +
> +#define TPMI_GET_STATE_CMD 0x10
> +#define TPMI_GET_STATE_CMD_DATA_OFFSET 8
> +#define TPMI_CMD_DATA_OFFSET 32
> +#define TPMI_CMD_PKT_LEN_OFFSET 16
> +#define TPMI_CMD_PKT_LEN 2
> +#define TPMI_CONTROL_RB_BIT 0
> +#define TPMI_CONTROL_CPL_BIT 6
> +#define TPMI_CMD_STATUS_SUCCESS 0x40
> +#define TPMI_GET_STATUS_BIT_ENABLE 0
> +#define TPMI_GET_STATUS_BIT_LOCKED 31
> +
> +/* Mutex to complete get feature status without interruption */
> +static DEFINE_MUTEX(tpmi_dev_lock);
> +
> +static int tpmi_wait_for_owner(struct intel_tpmi_info *tpmi_info, u8 owner)
> +{
> + u64 control;
> +
> + return read_poll_timeout(readq, control, owner == FIELD_GET(TPMI_GENMASK_OWNER, control),
> + TPMI_CONTROL_TIMEOUT_US, TPMI_CONTROL_TIMEOUT_MAX_US, false,
> + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
> +}
> +
> +static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int feature_id,
> + int *locked, int *disabled)
> +{
> + u64 control, data;
> + int ret;
> +
> + if (!tpmi_info->tpmi_control_mem)
> + return -EFAULT;
> +
> + mutex_lock(&tpmi_dev_lock);
> +
> + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_NONE);
> + if (ret)
> + goto err_unlock;
> +
> + /* set command id to 0x10 for TPMI_GET_STATE */
> + data = TPMI_GET_STATE_CMD;
> + /* 32 bits for DATA offset and +8 for feature_id field */
> + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET + TPMI_GET_STATE_CMD_DATA_OFFSET));
This looks like you should add the GENMASK_ULL() for the fields and use
FIELD_PREP() instead of adding all those OFFSET defines + custom shifting.
> +
> + /* Write at command offset for qword access */
> + writeq(data, tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET);
> +
> + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND);
> + if (ret)
> + goto err_unlock;
> +
> + /* Set Run Busy and packet length of 2 dwords */
> + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN << TPMI_CMD_PKT_LEN_OFFSET),
Define using BIT_ULL(0) instead. Use FIELD_PREP().
I'd drop _BIT from the define name but I leave it up to you, it just
makes your lines longer w/o much added value.
> + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
> +
> + ret = read_poll_timeout(readq, control, !(control & BIT_ULL(TPMI_CONTROL_RB_BIT)),
> + TPMI_RB_TIMEOUT_US, TPMI_RB_TIMEOUT_MAX_US, false,
> + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
> + if (ret)
> + goto done_proc;
> +
> + control = FIELD_GET(TPMI_GENMASK_STATUS, control);
> + if (control != TPMI_CMD_STATUS_SUCCESS) {
> + ret = -EBUSY;
> + goto done_proc;
> + }
> +
> + data = readq(tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET);
> + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for TPMI_DATA */
Define the field with GENMASK() and use FIELD_GET().
> +
> + *disabled = 0;
> + *locked = 0;
> +
> + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE)))
Put BIT_ULL() into the define.
Perhaps drop _BIT_ from the name.
> + *disabled = 1;
> +
> + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED))
Ditto.
> + *locked = 1;
> +
> + ret = 0;
> +
> +done_proc:
> + /* SET CPL "completion"bit */
Missing space.
> + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT),
BIT_ULL() to define.
> + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET);
> +
> +err_unlock:
> + mutex_unlock(&tpmi_dev_lock);
> +
> + return ret;
> +}
> +
> +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id,
> + int *locked, int *disabled)
> +{
> + 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);
> +
> + return tpmi_read_feature_status(tpmi_info, feature_id, locked, disabled);
> +}
> +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI);
> +
> +static void tpmi_set_control_base(struct auxiliary_device *auxdev,
> + struct intel_tpmi_info *tpmi_info,
> + struct intel_tpmi_pm_feature *pfs)
> +{
> + void __iomem *mem;
> + u16 size;
> +
> + size = pfs->pfs_header.num_entries * pfs->pfs_header.entry_size * 4;
Can this overflow u16? Where does pfs_header content originate from? If
from HW, how is it the input validated?
--
i.
> + mem = devm_ioremap(&auxdev->dev, pfs->vsec_offset, size);
> + if (!mem)
> + return;
> +
> + /* mem is pointing to TPMI CONTROL base */
> + tpmi_info->tpmi_control_mem = mem;
> +}
> +
> static const char *intel_tpmi_name(enum intel_tpmi_id id)
> {
> switch (id) {
> @@ -367,6 +511,9 @@ static int intel_vsec_tpmi_init(struct auxiliary_device *auxdev)
> */
> if (pfs->pfs_header.tpmi_id == TPMI_INFO_ID)
> tpmi_process_info(tpmi_info, pfs);
> +
> + if (pfs->pfs_header.tpmi_id == TPMI_CONTROL_ID)
> + tpmi_set_control_base(auxdev, tpmi_info, pfs);
> }
>
> tpmi_info->pfs_start = pfs_start;
> diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h
> index f505788c05da..04d937ad4dc4 100644
> --- a/include/linux/intel_tpmi.h
> +++ b/include/linux/intel_tpmi.h
> @@ -27,4 +27,6 @@ struct intel_tpmi_plat_info *tpmi_get_platform_data(struct auxiliary_device *aux
> 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);
> #endif
>
On Fri, 2023-06-16 at 10:13 +0300, Ilpo Järvinen wrote:
> On Thu, 15 Jun 2023, Srinivas Pandruvada wrote:
>
> >
[...]
> > + /* set command id to 0x10 for TPMI_GET_STATE */
> > + data = TPMI_GET_STATE_CMD;
> > + /* 32 bits for DATA offset and +8 for feature_id field */
> > + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET +
> > TPMI_GET_STATE_CMD_DATA_OFFSET));
>
> This looks like you should add the GENMASK_ULL() for the fields and
> use
> FIELD_PREP() instead of adding all those OFFSET defines + custom
> shifting.
You mean, I should change one shift instruction, to FIELD_PREP()
which will use three instructions to shift, sub and AND?
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);
>
> > +
> > + /* Write at command offset for qword access */
> > + writeq(data, tpmi_info->tpmi_control_mem +
> > TPMI_COMMAND_OFFSET);
> > +
> > + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND);
> > + if (ret)
> > + goto err_unlock;
> > +
> > + /* Set Run Busy and packet length of 2 dwords */
> > + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN <<
> > TPMI_CMD_PKT_LEN_OFFSET),
>
> Define using BIT_ULL(0) instead. Use FIELD_PREP().
This code will run only on X86 64 bit, not a common device driver which
will run in any architecture.
Please let me know why FIELD_PREP() is better.
>
> I'd drop _BIT from the define name but I leave it up to you, it just
> makes your lines longer w/o much added value.
>
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > +
> > + ret = read_poll_timeout(readq, control, !(control &
> > BIT_ULL(TPMI_CONTROL_RB_BIT)),
> > + TPMI_RB_TIMEOUT_US,
> > TPMI_RB_TIMEOUT_MAX_US, false,
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > + if (ret)
> > + goto done_proc;
> > +
> > + control = FIELD_GET(TPMI_GENMASK_STATUS, control);
> > + if (control != TPMI_CMD_STATUS_SUCCESS) {
> > + ret = -EBUSY;
> > + goto done_proc;
> > + }
> > +
> > + data = readq(tpmi_info->tpmi_control_mem +
> > TPMI_COMMAND_OFFSET);
> > + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for
> > TPMI_DATA */
>
> Define the field with GENMASK() and use FIELD_GET().
>
Again 3 instructions instead of 1.
> > +
> > + *disabled = 0;
> > + *locked = 0;
> > +
> > + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE)))
>
> Put BIT_ULL() into the define.
Good idea.
>
> Perhaps drop _BIT_ from the name.
I can do that.
>
> > + *disabled = 1;
> > +
> > + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED))
>
> Ditto.
>
> > + *locked = 1;
> > +
> > + ret = 0;
> > +
> > +done_proc:
> > + /* SET CPL "completion"bit */
>
> Missing space.
>
OK
> > + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT),
>
> BIT_ULL() to define.
>
> > + tpmi_info->tpmi_control_mem +
> > TPMI_CONTROL_STATUS_OFFSET);
> > +
> > +err_unlock:
> > + mutex_unlock(&tpmi_dev_lock);
> > +
> > + return ret;
> > +}
> > +
> > +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int
> > feature_id,
> > + int *locked, int *disabled)
> > +{
> > + 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);
> > +
> > + return tpmi_read_feature_status(tpmi_info, feature_id,
> > locked, disabled);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI);
> > +
> > +static void tpmi_set_control_base(struct auxiliary_device *auxdev,
> > + struct intel_tpmi_info
> > *tpmi_info,
> > + struct intel_tpmi_pm_feature
> > *pfs)
> > +{
> > + void __iomem *mem;
> > + u16 size;
> > +
> > + size = pfs->pfs_header.num_entries * pfs-
> > >pfs_header.entry_size * 4;
>
> Can this overflow u16? Where does pfs_header content originate from?
We can add a check, but this is coming from a trusted and validated x86
core (Not an add on IP), which not only driver uses but all PM IP in
the hardware.
Thanks,
Srinivas
> If
> from HW, how is it the input validated?
>