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.sakki...@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.sakki...@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 <sgarz...@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
