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

[Jarkko Sakkinen]

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.

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

You don't have to do anything smart with the new parameter other than
add it to leaf drivers. It makes the first patch bit more involved but
this way we end up keeping the callback interface as simple as it was.

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

> 
> Thanks,
> Stefano
> 
> 

BR, Jarkko