Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops

[Stefano Garzarella]

On Tue, Mar 04, 2025 at 10:21:55PM +0200, Jarkko Sakkinen wrote:
> On Tue, Mar 04, 2025 at 06:56:02PM +0200, Jarkko Sakkinen wrote:
> > On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
> > > On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
> > > > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
> > > > > +	int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
> > > > > buf_len,
> > > > > +			 size_t to_send);
> > > >
> > > > Please describe the meaning and purpose of to_send.
> > >
> > > Sure, I'll add in the commit description.
> >
> > It's always a command, right? So better be more concerete than
> > "to_send", e.g. "cmd_len".

Right!

> > I'd do instead:
> >
> > if (!chip->send)
> > 	goto out_recv;
> >
> > And change recv into:
> >
> > int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> > 	    cmd_len);
>
> I think I went here over the top, and *if* we need a new callback
> putting send_recv would be fine. Only thing I'd take from this is to
> rename to_len as cmd_len.

Got it.

> However, I don't think there are strong enough reasons to add complexity
> to the callback interface with the basis of this single driver. You
> should deal with this internally inside the driver instead.
>
> So do something along the lines of, e.g.:
>
> 1. Create dummy send() copying the command to internal
>   buffer.
> 2. Create ->status() returning zero, and set req_complete_mask and
>   req_complete_val to zero.
> 3. Performan transaction in recv().
>
> How you split send_recv() between send() and recv() is up to you. This
> was merely showing that we don't need send_recv() desperately.

We did something similar in v1 [1], but instead of your point 2, we just 
set `chip->flags |= TPM_CHIP_FLAG_IRQ;` in the probe() after we 
allocated the chip.

Jason suggested the send_recv() ops [2], which I liked, but if you 
prefer to avoid that, I can restore what we did in v1 and replace the 
TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if 
you think it is fine).

@Jarkko, @Jason, I don't have a strong preference about it, so your 
choice :-)

Thanks,
Stefano

[1] https://lore.kernel.org/linux-integrity/20241210143423.101774-2-sgarzare@xxxxxxxxxx/
[2] https://lore.kernel.org/linux-integrity/CAGxU2F51EoqDqi6By6eBa7qT+VT006DJ9+V-PANQ6GQrwVWt_Q@xxxxxxxxxxxxxx/