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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
> On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > 
> > tpm_ftpm_tee does not require chip->status, chip->cancel and
> > chip->req_canceled. Make them optional.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > 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?

I'm not a huge fan of stack allocations inside blocks, unless there is
a particular reason to do so.


> 
> > 
> > 	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!

If you refined send() and had that the sync flag, this would become:

	if (chip->flags & (TPM_CHIP_FLAG_IRQ | TPM_CHIP_FLAG_SYNC))
		goto out_recv;

> 
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thank you.


> 
> 
> > 	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
> > 
> 

BR, Jarkko