tpm/tpm_crb: Access locality for only CRB_START method
PATCH v2 1/1 tpm/tpm_crb: Access locality for only CRB_START method
For ARM64, the locality is handled by Trust Zone in FW. The layout does not have crb_regs_head. It is hitting the following line.dev_warn(dev, FW_BUG "Bad ACPI memory layout");
Current code excludes CRB_FL_ACPI_START and when CRB_FL_CRB_SMC_START is added around the same time locality support is added, it should also be excluded. Change the if condition to be only for CRB_FL_CRB_START.
For goIdle and cmdReady where code was excluding CRB_FL_ACPI_START only (do nothing for ACPI start method), CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start method does not have TPM_CRB_CTRL_REQ.
Change the if condition to be only for CRB_FL_CRB_START in crb_go_idle() and crb_cmd_ready().
---
v2
Changed if condition to CRB_FL_CRB_START for go idle and crb_cmd_ready per maintainer's comment.
drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
drivers/char/tpm/tpm_crb.c +17 -19
@@ -128,18 +128,16 @@ struct tpm2_crb_smc {* Anyhow, we do not wait here as a consequent CMD_READY request * will be handled correctly even if idle was not completed. * -* The function does nothing for devices with ACPI-start method. +* The function does nothing for devices with ACPI-start method +* and SMC-start method. * * Return: 0 always */ static int __maybe_unused crb_go_idle (struct device *dev,struct crb_priv *priv) { -if ((priv->flags &CRB_FL_ACPI_START ) || - (priv->flags &CRB_FL_CRB_SMC_START )) -return 0 ; - -iowrite32 (CRB_CTRL_REQ_GO_IDLE , &priv->regs_t->ctrl_req); -/* we don't really care when this settles */ +if (priv->flags &CRB_FL_CRB_START ) +iowrite32 (CRB_CTRL_REQ_GO_IDLE , &priv->regs_t->ctrl_req); +/* we don't really care when this settles */ return 0 ; }
@@ -174,23 +172,23 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,* The device should respond within TIMEOUT_C. * * The function does nothing for devices with ACPI-start method +* and SMC-start method. * * Return: 0 on success -ETIME on timeout; */ static int __maybe_unused crb_cmd_ready (struct device *dev,struct crb_priv *priv) { -if ((priv->flags &CRB_FL_ACPI_START ) || - (priv->flags &CRB_FL_CRB_SMC_START )) -return 0 ; - -iowrite32 (CRB_CTRL_REQ_CMD_READY , &priv->regs_t->ctrl_req); -if (!crb_wait_for_reg_32 (&priv->regs_t->ctrl_req, -CRB_CTRL_REQ_CMD_READY /* mask */ , -0 ,/* value */ -TPM2_TIMEOUT_C )) { -dev_warn (dev,"cmdReady timed out\n" ); -return -ETIME ; +if (priv->flags &CRB_FL_CRB_START ) + { +iowrite32 (CRB_CTRL_REQ_CMD_READY , &priv->regs_t->ctrl_req); +if (!crb_wait_for_reg_32 (&priv->regs_t->ctrl_req, +CRB_CTRL_REQ_CMD_READY /* mask */ , +0 ,/* value */ +TPM2_TIMEOUT_C )) { +dev_warn (dev,"cmdReady timed out\n" ); +return -ETIME ; + } }return 0 ;
@@ -458,7 +456,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,* the control area, as one nice sane region except for some older * stuff that puts the control area outside the ACPI IO region. */ -if (!(priv->flags &CRB_FL_ACPI_START )) { +if (priv->flags &CRB_FL_CRB_START ) {if (buf->control_address == io_res.start +sizeof (*priv->regs_h)) priv->regs_h = priv->iobase;
Signature · 3 lines
Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 17 lines · Jiandi An
For ARM64, the locality is handled by Trust Zone in FW. The layout does not have
crb_regs_head. It is hitting the following line.dev_warn(dev, FW_BUG "Bad ACPI memory layout");Current code excludes
CRB_FL_ACPI_STARTand whenCRB_FL_CRB_SMC_STARTis added around the same time locality support is added, it should also be excluded. Change the if condition to be only forCRB_FL_CRB_START.For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_STARTonly (do nothing for ACPI start method),CRB_FL_CRB_SMC_STARTwas also excluded as ARM64 SMC start method does not haveTPM_CRB_CTRL_REQ.
Change the if condition to be only forCRB_FL_CRB_STARTincrb_go_idle()andcrb_cmd_ready().
Quoted 5 lines
Signed-off-by Jiandi An <[email protected]>---
v2
Changed if condition toCRB_FL_CRB_STARTfor go idle andcrb_cmd_readyper maintainer's comment.
What about platforms with firmware bug that they report only requiring ACPI start but actually also require CRB start.
if (sm == ACPI_TPM2_START_METHOD ||
sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) priv->flags |= CRB_FL_ACPI_START;
I've encountered a platform that reports to require start method 2 (ACPI start) while it actually requires start method 8.
That's why we have this nasty workaround:
/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
!strcmp(acpi_device_hid(device), "MSFT0101")) priv->flags |= CRB_FL_CRB_START;
Also, since start method 8 exist in the spec, it is a legit config.
Quoted 46 lines
drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)drivers/char/tpm/tpm_crb.c +6
-10@@ -128,18 +128,16 @@ struct tpm2_crb_smc {* Anyhow, we do not wait here as a consequent CMD_READY request * will be handled correctly even if idle was not completed. * -* The function does nothing for devices with ACPI-start method. +* The function does nothing for devices with ACPI-start method +* and SMC-start method. * * Return: 0 always */ static int __maybe_unused crb_go_idle (struct device *dev,struct crb_priv *priv) { -if ((priv->flags &CRB_FL_ACPI_START ) || - (priv->flags &CRB_FL_CRB_SMC_START )) -return 0 ; - -iowrite32 (CRB_CTRL_REQ_GO_IDLE , &priv->regs_t->ctrl_req); -/* we don't really care when this settles */ +if (priv->flags &CRB_FL_CRB_START ) +iowrite32 (CRB_CTRL_REQ_GO_IDLE , &priv->regs_t->ctrl_req); +/* we don't really care when this settles */ return 0 ; }@@ -174,23 +172,23 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,* The device should respond within TIMEOUT_C. * * The function does nothing for devices with ACPI-start method +* and SMC-start method. * * Return: 0 on success -ETIME on timeout; */ static int __maybe_unused crb_cmd_ready (struct device *dev,struct crb_priv *priv) { -if ((priv->flags &CRB_FL_ACPI_START ) || - (priv->flags &CRB_FL_CRB_SMC_START )) -return 0 ;
drivers/char/tpm/tpm_crb.c static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, · line 183
Quoted 10 lines
drivers/char/tpm/tpm_crb.c +2
-8- -iowrite32 (CRB_CTRL_REQ_CMD_READY , &priv->regs_t->ctrl_req); -if (!crb_wait_for_reg_32 (&priv->regs_t->ctrl_req, -CRB_CTRL_REQ_CMD_READY /* mask */ , -0 ,/* value */ -TPM2_TIMEOUT_C )) { -dev_warn (dev,"cmdReady timed out\n" ); -return -ETIME ; +if (priv->flags &CRB_FL_CRB_START ) + {
The opening curly brace should be in the same line as the condition statement.
Quoted 25 lines
drivers/char/tpm/tpm_crb.c +9
-1+iowrite32 (CRB_CTRL_REQ_CMD_READY , &priv->regs_t->ctrl_req); +if (!crb_wait_for_reg_32 (&priv->regs_t->ctrl_req, +CRB_CTRL_REQ_CMD_READY /* mask */ , +0 ,/* value */ +TPM2_TIMEOUT_C )) { +dev_warn (dev,"cmdReady timed out\n" ); +return -ETIME ; + } }return 0 ;@@ -458,7 +456,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,* the control area, as one nice sane region except for some older * stuff that puts the control area outside the ACPI IO region. */ -if (!(priv->flags &CRB_FL_ACPI_START )) { +if (priv->flags &CRB_FL_CRB_START ) {if (buf->control_address == io_res.start +sizeof (*priv->regs_h)) priv->regs_h = priv->iobase;Signature · 3 lines
Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
/Jarkko