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. > > 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/ BR, Jarkko
