Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Stefano Garzarella]

On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxx>
>
> tpm_ftpm_tee does not require chip->status, chip->cancel and
> chip->req_canceled. Make them optional.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxx>
> ---
> drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
> drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
> 2 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f62f7871edbd..10ba47a882d8 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> }
> EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>
> +static void tpm_chip_cancel(struct tpm_chip *chip)
> +{
> +	if (!chip->ops->cancel)
> +		return;
> +
> +	chip->ops->cancel(chip);
> +}
> +
> +static u8 tpm_chip_status(struct tpm_chip *chip)
> +{
> +	if (!chip->ops->status)
> +		return 0;
> +
> +	return chip->ops->status(chip);
> +}
> +
> +static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> +	if (!chip->ops->req_canceled)
> +		return false;
> +
> +	return chip->ops->req_canceled(chip, status);
> +}
> +
> static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> {
> 	struct tpm_header *header = buf;
> @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 	ssize_t len = 0;
> 	u32 count, ordinal;
> 	unsigned long stop;
> +	u8 status;

Why move `status` out of the do/while block?

> 	if (bufsiz < TPM_HEADER_SIZE)
> 		return -EINVAL;
> @@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)

What about doing an early return avoiding also calling
tpm_calc_ordinal_duration()?

I mean something like this:

                rc = 0;
        }

-       if (chip->flags & TPM_CHIP_FLAG_IRQ)
+       if (!chip->ops->status || chip->flags & TPM_CHIP_FLAG_IRQ)
                goto out_recv;


Anyway, those are small things, the patch LGTM and it's a great cleanup
for ftpm and the svsm driver I'm developing!


Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>


> 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> 	do {
> -		u8 status = chip->ops->status(chip);
> +		status = tpm_chip_status(chip);
> 		if ((status & chip->ops->req_complete_mask) ==
> 		    chip->ops->req_complete_val)
> 			goto out_recv;
>
> -		if (chip->ops->req_canceled(chip, status)) {
> +		if (tpm_chip_req_canceled(chip, status)) {
> 			dev_err(&chip->dev, "Operation Canceled\n");
> 			return -ECANCELED;
> 		}
> @@ -118,7 +143,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 		rmb();
> 	} while (time_before(jiffies, stop));
>
> -	chip->ops->cancel(chip);
> +	tpm_chip_cancel(chip);
> 	dev_err(&chip->dev, "Operation Timed out\n");
> 	return -ETIME;
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 8d9209dfc384..53ba28ccd5d3 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -164,30 +164,10 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
> 	return 0;
> }
>
> -static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
> -{
> -	/* not supported */
> -}
> -
> -static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
> -{
> -	return 0;
> -}
> -
> -static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
> -{
> -	return false;
> -}
> -
> static const struct tpm_class_ops ftpm_tee_tpm_ops = {
> 	.flags = TPM_OPS_AUTO_STARTUP,
> 	.recv = ftpm_tee_tpm_op_recv,
> 	.send = ftpm_tee_tpm_op_send,
> -	.cancel = ftpm_tee_tpm_op_cancel,
> -	.status = ftpm_tee_tpm_op_status,
> -	.req_complete_mask = 0,
> -	.req_complete_val = 0,
> -	.req_canceled = ftpm_tee_tpm_req_canceled,
> };
>
> /*
> --
> 2.39.5