| Message ID | 20250219201014.174344-3-stuart.yoder@arm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add support for the TPM FF-A start method | expand |
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.yoder@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 <jarkko@kernel.org> BR, Jarkko
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.yoder@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 <jarkko@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
Dear Stuart, Thank you for the patch. Should you respin, you could spell *clean up* in the summary/title with a space. The diff looks good. Kind regards, Paul
On Thu, 2025-02-20 at 13:04 -0600, Stuart Yoder wrote: > > > 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.yoder@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 <jarkko@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? It's fine I can append them :-) You might have to hold up until to the first work week of March because I'm actually on holiday up until that but yeah no need for extra noise. Up until that tested-by's to this patch set version are obviously welcome but it's now definitely good enough as far as I'm concerned. > > Thanks, > Stuart BR, Jarkko
On Thu, 2025-02-20 at 20:09 +0100, Paul Menzel wrote: > Dear Stuart, > > > Thank you for the patch. Should you respin, you could spell *clean > up* > in the summary/title with a space. > > The diff looks good. > > > Kind regards, > > Paul So for the sake of overall good it is better maybe to keep this immutable as this will make it more plausible for potential testers. BR, Jarkko
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))
Refactor TPM idle check to tpm_crb_has_idle(), and reduce paraentheses usage in start method checks Signed-off-by: Stuart Yoder <stuart.yoder@arm.com> --- drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-)