Re: [PATCH v5 2/5] tpm_crb: clean-up and refactor check for idle support

[Stuart Yoder]

On 2/20/25 3:29 AM, Jarkko Sakkinen wrote:

On Wed, Feb 19, 2025 at 02:10:11PM -0600, Stuart Yoder wrote:

Refactor TPM idle check to tpm_crb_has_idle(), and reduce paraentheses
usage in start method checks

Signed-off-by: Stuart Yoder <stuart.yo...@arm.com>
---
  drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++++++---------------
  1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ea085b14ab7c..31db879f1324 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -115,6 +115,16 @@ struct tpm2_crb_pluton {
        u64 reply_addr;
  };

+/*

+ * Returns true if the start method supports idle.
+ */
+static inline bool tpm_crb_has_idle(u32 start_method)
+{
+       return start_method == ACPI_TPM2_START_METHOD ||
+              start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
+              start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
+}
+
  static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
                                unsigned long timeout)
  {
@@ -173,9 +183,7 @@ static int __crb_go_idle(struct device *dev, struct 
crb_priv *priv)
  {
        int rc;

- if ((priv->sm == ACPI_TPM2_START_METHOD) ||

-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
+       if (!tpm_crb_has_idle(priv->sm))
                return 0;

iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);

@@ -222,9 +230,7 @@ static int __crb_cmd_ready(struct device *dev, struct 
crb_priv *priv)
  {
        int rc;

- if ((priv->sm == ACPI_TPM2_START_METHOD) ||

-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
+       if (!tpm_crb_has_idle(priv->sm))
                return 0;

iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);

@@ -423,13 +429,13 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, 
size_t len)
         * report only ACPI start but in practice seems to require both
         * CRB start, hence invoking CRB start method if hid == MSFT0101.
         */
-       if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
-           (priv->sm == ACPI_TPM2_MEMORY_MAPPED) ||
-           (!strcmp(priv->hid, "MSFT0101")))
+       if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
+           priv->sm == ACPI_TPM2_MEMORY_MAPPED ||
+           !strcmp(priv->hid, "MSFT0101"))
                iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);

- if ((priv->sm == ACPI_TPM2_START_METHOD) ||

-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD))
+       if (priv->sm == ACPI_TPM2_START_METHOD ||
+           priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
                rc = crb_do_acpi_start(chip);

if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {

@@ -449,8 +455,8 @@ static void crb_cancel(struct tpm_chip *chip)

iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel); - if (((priv->sm == ACPI_TPM2_START_METHOD) ||

-           (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)) &&
+       if ((priv->sm == ACPI_TPM2_START_METHOD ||
+            priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) &&
             crb_do_acpi_start(chip))
                dev_err(&chip->dev, "ACPI Start failed\n");
  }
@@ -609,8 +615,8 @@ 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->sm == ACPI_TPM2_COMMAND_BUFFER) ||
-           (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
+       if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
+           priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
                if (iores &&
                    buf->control_address == iores->start +

sizeof(*priv->regs_h)) --

2.34.1

Reviewed-by: Jarkko Sakkinen <jar...@kernel.org>

Thanks for the review.  Do you want me to respin and send out
a v6 with your Reviewed-by tags on patches 2/5 and 5/5?

Thanks,
Stuart