[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z-VRWy8jLkA0cpow@kernel.org>
Date: Thu, 27 Mar 2025 15:23:39 +0200
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: linux-integrity@...r.kernel.org, Sumit Garg <sumit.garg@...nel.org>,
Jens Wiklander <jens.wiklander@...aro.org>,
James Bottomley <James.Bottomley@...senpartnership.com>,
Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>,
Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt
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@...nsys.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@...nsys.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@...hat.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
Powered by blists - more mailing lists