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

[Stefano Garzarella]

On Thu, Mar 27, 2025 at 03:23:39PM +0200, Jarkko Sakkinen wrote:
> 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@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?
>
> 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;

Yep, good point!

> > Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
>
> Thank you.

You're welcome!
Stefano