From: Stefano Garzarella <sgarzare@redhat.com>
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
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.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/tpm.h | 2 ++
drivers/char/tpm/tpm-interface.c | 7 +++++++
2 files changed, 9 insertions(+)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 6c3125300c00..4e796b8726b5 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -88,6 +88,8 @@ struct tpm_class_ops {
bool (*req_canceled)(struct tpm_chip *chip, u8 status);
int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+ int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
+ size_t cmd_len);
void (*cancel) (struct tpm_chip *chip);
u8 (*status) (struct tpm_chip *chip);
void (*update_timeouts)(struct tpm_chip *chip,
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f62f7871edbd..7f4e01790352 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -82,6 +82,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
return -E2BIG;
}
+ if (chip->ops->send_recv) {
+ rc = 0;
+ len = chip->ops->send_recv(chip, buf, bufsiz, count);
+ goto out_send_recv;
+ }
+
rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
if (rc != -EPIPE)
@@ -124,6 +130,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
out_recv:
len = chip->ops->recv(chip, buf, bufsiz);
+out_send_recv:
if (len < 0) {
rc = len;
dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
--
2.48.1On Thu, Mar 20, 2025 at 04:24:32PM +0100, Stefano Garzarella wrote: > From: Stefano Garzarella <sgarzare@redhat.com> > > 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@kernel.org/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. > 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. BR, Jarkko
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@redhat.com>
>>
>> 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@kernel.org/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.
>
>> 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.
WDYT?
Thanks,
Stefano
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@redhat.com> > > > > > > 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@kernel.org/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
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@redhat.com>
>> > >
>> > > 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@kernel.org/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
© 2016 - 2025 Red Hat, Inc.