Re: [PATCH v17 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: 2021-10-25 23:41:19
Also in:
linux-devicetree, lkml
On Sun, 2021-10-24 at 19:48 +0300, amirmizi6@gmail.com wrote:
From: Amir Mizinski <redacted>
Currently, the driver polls the TPM_STS.stsValid field until TRUE; then it
reads TPM_STS register again to verify only that TPM_STS.expect field is
FALSE (i.e., it ignores TPM_STS.stsValid).
Since TPM_STS.stsValid represents the TPM_STS.expect validity, a check of
only one of these fields is wrong. Fix this condition so that both fields
are checked in the same TPM_STS register read.
Modify the signature of wait_for_tpm_stat(), adding an additional
"result" parameter to its call.
wait_for_tpm_stat() is now polling the TPM_STS with a mask and waits
for the value in result. This modification adds the ability to check if
certain TPM_STS bits have been cleared.
For example, use the new parameter to check in status that TPM_STS_VALID
is set and also that TPM_STS_EXPECT is zeroed. This prevents a racy
check.
Fixes: 27084efee0c3 ("tpm: driver for next generation TPM chips")Where does this failure occur in practice? If nowhere, this can be dropped, and the patch can be considered as a feature. Defining it as a fix makes only sense, if it needs to be backported to stable kernels. This requires something to be actually broken. The commit message does not contain a real bug report. It is just referring to the specification, which is not a workload.
quoted hunk ↗ jump to hunk
Suggested-by: Benoit Houyere <redacted> Signed-off-by: Amir Mizinski <redacted> --- drivers/char/tpm/tpm_tis_core.c | 59 ++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 33 deletions(-)diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 69579ef..98de2fd 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c@@ -44,9 +44,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,return false; } -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, - unsigned long timeout, wait_queue_head_t *queue, - bool check_cancel) +static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 result, + unsigned long timeout, + wait_queue_head_t *queue, bool check_cancel)
I would consider renaming this as, given that you are changing the signature anyway: tpm_tis_wait_for_stat() This would be more consistent with the other naming, and make e.g. grepping kernel tree easier. How did you end up to the name "result"? I have hard time deriving from that name the actual semantics. E.g. "expected" would already a way more sane name. /Jarkko