Re: [PATCH v17 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception

[Jarkko Sakkinen]

On Sun, 2021-10-24 at 19:48 +0300, amirmizi6@xxxxxxxxx wrote:
> From: Amir Mizinski <amirmizi6@xxxxxxxxx>
> 
> 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.

> Suggested-by: Benoit Houyere <benoit.houyere@xxxxxx>
> Signed-off-by: Amir Mizinski <amirmizi6@xxxxxxxxx>
> ---
>  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