Re: [PATCH 1/2] tpm: add send_recv() op in tpm_class_ops

[Stefano Garzarella]

On Thu, Mar 27, 2025 at 03:02:32PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 27, 2025 at 10:48:17AM +0100, Stefano Garzarella wrote:
> > On Wed, Mar 26, 2025 at 06:53:51PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 20, 2025 at 04:24:32PM +0100, Stefano Garzarella wrote:
> > > > From: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > > >
> > > > Some devices do not support interrupts and provide a single operation
> > > > to send the command and receive the response on the same buffer.
> > > >
> > > > To support this scenario, a driver could set TPM_CHIP_FLAG_IRQ in the
> > > > chip's flags to get recv() to be called immediately after send() in
> > > > tpm_try_transmit(), or it needs to implement .status() to return 0,
> > > > and set both .req_complete_mask and .req_complete_val to 0.
> > > >
> > > > In order to simplify these drivers and avoid temporary buffers to be
> > >
> > > Simplification can be addressed with no callback changes:
> > >
> > > https://lore.kernel.org/linux-integrity/20250326161838.123606-1-jarkko@xxxxxxxxxx/T/#u
> > >
> > > I also noticed that tpm_ftpm_tee initalized req_complete_mask and
> > > req_complete_val explictly while they would be already implicitly
> > > zero.
> > >
> > > So it reduces this just a matter of getting rid off the extra
> > > buffer.
> >
> > Yep, as mentioned I think your patch should go either way. So here I can
> > rephrase and put the emphasis on the temporary buffer and the driver
> > simplification.
>
> Yes. Removing extra copy is a goal that can only make sense!
>
> > >
> > > > used between the .send() and .recv() callbacks, introduce a new callback
> > > > send_recv(). If that callback is defined, it is called in
> > > > tpm_try_transmit() to send the command and receive the response on
> > > > the same buffer in a single call.
> > >
> > > I don't find anything in the commit message addressing buf_len an
> > > cmd_len (vs "just len"). Why two lengths are required?
> > >
> > > Not completely rejecting but this explanation is incomplete.
> >
> > Right.
> >
> > The same buffer is used as input and output.
> > For input, the buffer contains the command (cmd_len) but the driver can use
> > the entire buffer for output (buf_len).
> > It's basically the same as in tpm_try_transmit(), but we avoid having to
> > parse the header in each driver since we already do that in
> > tpm_try_transmit().
> >
> > In summary cmd_len = count = be32_to_cpu(header->length).
> >
> > I admit I'm not good with names, would you prefer a different name or is it
> > okay to explain it better in the commit?
> >
> > My idea is to add this:
> >
> >     `buf` is used as input and output. It contains the command
> >     (`cmd_len` bytes) as input. The driver will be able to use the
> >     entire buffer (`buf_len` bytes) for the response as output.
> >     Passing `cmd_len` is an optimization to avoid having to access the
> >     command header again in each driver and check it.
>
> This makes more sense. Maybe we could name them as buf_size and
> cmd_len to further make dead obvious the use and purpose.

Yeah, I see. I'll do!

> > WDYT?
>
> I just want to get this done right if it is done at all, so here's
> one more suggestion:
>
> 1. Add TPM_CHIP_FLAG_SYNC
> 2. Update send() parameters.

So, IIUC something like this:

    int (*send) (struct tpm_chip *chip, u8 *buf, size_t cmd_len, size_t buf_size);

Where `buf_size` is ignored if the driver doesn't set TPM_CHIP_FLAG_SYNC.

Right?

> You don't have to do anything smart with the new parameter other than
> add it to leaf drivers.

Okay, this should answer my question :-) (I leave it just to be sure).

> It makes the first patch bit more involved but
> this way we end up keeping the callback interface as simple as it was.

Yep, I see.
And maybe I need to change something in tpm_try_transmit() because now
send() returns 0 in case of success, but after the change it might
return > 0 in case TPM_CHIP_FLAG_SYNC is set. But I will see how to
handle this.

> I'm also thinking that for async case do we actually need all those
> complicated masks etc. or could we simplify that side but it is
> definitely out-of-scope for this patch set (no need to worry about
> this).

I see, I can take a look later in another series.

Thanks,
Stefano