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

Stefano Garzarella
2025-03-07T15:38:00Z

On Fri, Mar 07, 2025 at 12:15:34AM +0200, Jarkko Sakkinen wrote:
>On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
>> 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
>> :-)
>
>I'd say, unless you have actual identified blocker, please go with
>a driver where the complexity is managed within the driver.

Yep, got it ;-)

Thanks,
Stefano