[PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Jarkko Sakkinen]

From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>

tpm_ftpm_tee does not require chip->status, chip->cancel and
chip->req_canceled. Make them optional.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
 drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
 drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
 2 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f62f7871edbd..10ba47a882d8 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
+static void tpm_chip_cancel(struct tpm_chip *chip)
+{
+	if (!chip->ops->cancel)
+		return;
+
+	chip->ops->cancel(chip);
+}
+
+static u8 tpm_chip_status(struct tpm_chip *chip)
+{
+	if (!chip->ops->status)
+		return 0;
+
+	return chip->ops->status(chip);
+}
+
+static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	if (!chip->ops->req_canceled)
+		return false;
+
+	return chip->ops->req_canceled(chip, status);
+}
+
 static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
 {
 	struct tpm_header *header = buf;
@@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
 	ssize_t len = 0;
 	u32 count, ordinal;
 	unsigned long stop;
+	u8 status;
 
 	if (bufsiz < TPM_HEADER_SIZE)
 		return -EINVAL;
@@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
 
 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
 	do {
-		u8 status = chip->ops->status(chip);
+		status = tpm_chip_status(chip);
 		if ((status & chip->ops->req_complete_mask) ==
 		    chip->ops->req_complete_val)
 			goto out_recv;
 
-		if (chip->ops->req_canceled(chip, status)) {
+		if (tpm_chip_req_canceled(chip, status)) {
 			dev_err(&chip->dev, "Operation Canceled\n");
 			return -ECANCELED;
 		}
@@ -118,7 +143,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
 		rmb();
 	} while (time_before(jiffies, stop));
 
-	chip->ops->cancel(chip);
+	tpm_chip_cancel(chip);
 	dev_err(&chip->dev, "Operation Timed out\n");
 	return -ETIME;
 
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 8d9209dfc384..53ba28ccd5d3 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -164,30 +164,10 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	return 0;
 }
 
-static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
-{
-	/* not supported */
-}
-
-static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
-{
-	return 0;
-}
-
-static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
-{
-	return false;
-}
-
 static const struct tpm_class_ops ftpm_tee_tpm_ops = {
 	.flags = TPM_OPS_AUTO_STARTUP,
 	.recv = ftpm_tee_tpm_op_recv,
 	.send = ftpm_tee_tpm_op_send,
-	.cancel = ftpm_tee_tpm_op_cancel,
-	.status = ftpm_tee_tpm_op_status,
-	.req_complete_mask = 0,
-	.req_complete_val = 0,
-	.req_canceled = ftpm_tee_tpm_req_canceled,
 };
 
 /*
-- 
2.39.5

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Stefano Garzarella]

On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>tpm_ftpm_tee does not require chip->status, chip->cancel and
>chip->req_canceled. Make them optional.
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
> drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
> drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
> 2 files changed, 28 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>index f62f7871edbd..10ba47a882d8 100644
>--- a/drivers/char/tpm/tpm-interface.c
>+++ b/drivers/char/tpm/tpm-interface.c
>@@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> }
> EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>
>+static void tpm_chip_cancel(struct tpm_chip *chip)
>+{
>+	if (!chip->ops->cancel)
>+		return;
>+
>+	chip->ops->cancel(chip);
>+}
>+
>+static u8 tpm_chip_status(struct tpm_chip *chip)
>+{
>+	if (!chip->ops->status)
>+		return 0;
>+
>+	return chip->ops->status(chip);
>+}
>+
>+static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
>+{
>+	if (!chip->ops->req_canceled)
>+		return false;
>+
>+	return chip->ops->req_canceled(chip, status);
>+}
>+
> static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> {
> 	struct tpm_header *header = buf;
>@@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 	ssize_t len = 0;
> 	u32 count, ordinal;
> 	unsigned long stop;
>+	u8 status;

Why move `status` out of the do/while block?

>
> 	if (bufsiz < TPM_HEADER_SIZE)
> 		return -EINVAL;
>@@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>

What about doing an early return avoiding also calling
tpm_calc_ordinal_duration()?

I mean something like this:

                 rc = 0;
         }

-       if (chip->flags & TPM_CHIP_FLAG_IRQ)
+       if (!chip->ops->status || chip->flags & TPM_CHIP_FLAG_IRQ)
                 goto out_recv;


Anyway, those are small things, the patch LGTM and it's a great cleanup
for ftpm and the svsm driver I'm developing!


Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>


> 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> 	do {
>-		u8 status = chip->ops->status(chip);
>+		status = tpm_chip_status(chip);
> 		if ((status & chip->ops->req_complete_mask) ==
> 		    chip->ops->req_complete_val)
> 			goto out_recv;
>
>-		if (chip->ops->req_canceled(chip, status)) {
>+		if (tpm_chip_req_canceled(chip, status)) {
> 			dev_err(&chip->dev, "Operation Canceled\n");
> 			return -ECANCELED;
> 		}
>@@ -118,7 +143,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> 		rmb();
> 	} while (time_before(jiffies, stop));
>
>-	chip->ops->cancel(chip);
>+	tpm_chip_cancel(chip);
> 	dev_err(&chip->dev, "Operation Timed out\n");
> 	return -ETIME;
>
>diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
>index 8d9209dfc384..53ba28ccd5d3 100644
>--- a/drivers/char/tpm/tpm_ftpm_tee.c
>+++ b/drivers/char/tpm/tpm_ftpm_tee.c
>@@ -164,30 +164,10 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
> 	return 0;
> }
>
>-static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
>-{
>-	/* not supported */
>-}
>-
>-static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
>-{
>-	return 0;
>-}
>-
>-static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
>-{
>-	return false;
>-}
>-
> static const struct tpm_class_ops ftpm_tee_tpm_ops = {
> 	.flags = TPM_OPS_AUTO_STARTUP,
> 	.recv = ftpm_tee_tpm_op_recv,
> 	.send = ftpm_tee_tpm_op_send,
>-	.cancel = ftpm_tee_tpm_op_cancel,
>-	.status = ftpm_tee_tpm_op_status,
>-	.req_complete_mask = 0,
>-	.req_complete_val = 0,
>-	.req_canceled = ftpm_tee_tpm_req_canceled,
> };
>
> /*
>-- 
>2.39.5
>

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
> On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > 
> > tpm_ftpm_tee does not require chip->status, chip->cancel and
> > chip->req_canceled. Make them optional.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
> > drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
> > 2 files changed, 28 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index f62f7871edbd..10ba47a882d8 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> > }
> > EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
> > 
> > +static void tpm_chip_cancel(struct tpm_chip *chip)
> > +{
> > +	if (!chip->ops->cancel)
> > +		return;
> > +
> > +	chip->ops->cancel(chip);
> > +}
> > +
> > +static u8 tpm_chip_status(struct tpm_chip *chip)
> > +{
> > +	if (!chip->ops->status)
> > +		return 0;
> > +
> > +	return chip->ops->status(chip);
> > +}
> > +
> > +static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
> > +{
> > +	if (!chip->ops->req_canceled)
> > +		return false;
> > +
> > +	return chip->ops->req_canceled(chip, status);
> > +}
> > +
> > static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > {
> > 	struct tpm_header *header = buf;
> > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > 	ssize_t len = 0;
> > 	u32 count, ordinal;
> > 	unsigned long stop;
> > +	u8 status;
> 
> Why move `status` out of the do/while block?

I'm not a huge fan of stack allocations inside blocks, unless there is
a particular reason to do so.


> 
> > 
> > 	if (bufsiz < TPM_HEADER_SIZE)
> > 		return -EINVAL;
> > @@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > 
> 
> What about doing an early return avoiding also calling
> tpm_calc_ordinal_duration()?
> 
> I mean something like this:
> 
>                 rc = 0;
>         }
> 
> -       if (chip->flags & TPM_CHIP_FLAG_IRQ)
> +       if (!chip->ops->status || chip->flags & TPM_CHIP_FLAG_IRQ)
>                 goto out_recv;
> 
> 
> Anyway, those are small things, the patch LGTM and it's a great cleanup
> for ftpm and the svsm driver I'm developing!

If you refined send() and had that the sync flag, this would become:

	if (chip->flags & (TPM_CHIP_FLAG_IRQ | TPM_CHIP_FLAG_SYNC))
		goto out_recv;

> 
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thank you.


> 
> 
> > 	stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
> > 	do {
> > -		u8 status = chip->ops->status(chip);
> > +		status = tpm_chip_status(chip);
> > 		if ((status & chip->ops->req_complete_mask) ==
> > 		    chip->ops->req_complete_val)
> > 			goto out_recv;
> > 
> > -		if (chip->ops->req_canceled(chip, status)) {
> > +		if (tpm_chip_req_canceled(chip, status)) {
> > 			dev_err(&chip->dev, "Operation Canceled\n");
> > 			return -ECANCELED;
> > 		}
> > @@ -118,7 +143,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> > 		rmb();
> > 	} while (time_before(jiffies, stop));
> > 
> > -	chip->ops->cancel(chip);
> > +	tpm_chip_cancel(chip);
> > 	dev_err(&chip->dev, "Operation Timed out\n");
> > 	return -ETIME;
> > 
> > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> > index 8d9209dfc384..53ba28ccd5d3 100644
> > --- a/drivers/char/tpm/tpm_ftpm_tee.c
> > +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> > @@ -164,30 +164,10 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > 	return 0;
> > }
> > 
> > -static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
> > -{
> > -	/* not supported */
> > -}
> > -
> > -static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
> > -{
> > -	return 0;
> > -}
> > -
> > -static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
> > -{
> > -	return false;
> > -}
> > -
> > static const struct tpm_class_ops ftpm_tee_tpm_ops = {
> > 	.flags = TPM_OPS_AUTO_STARTUP,
> > 	.recv = ftpm_tee_tpm_op_recv,
> > 	.send = ftpm_tee_tpm_op_send,
> > -	.cancel = ftpm_tee_tpm_op_cancel,
> > -	.status = ftpm_tee_tpm_op_status,
> > -	.req_complete_mask = 0,
> > -	.req_complete_val = 0,
> > -	.req_canceled = ftpm_tee_tpm_req_canceled,
> > };
> > 
> > /*
> > -- 
> > 2.39.5
> > 
> 

BR, Jarkko

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[James Bottomley]

On Thu, 2025-03-27 at 15:23 +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
[...]
> > > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > *chip, void *buf, size_t bufsiz)
> > > 	ssize_t len = 0;
> > > 	u32 count, ordinal;
> > > 	unsigned long stop;
> > > +	u8 status;
> > 
> > Why move `status` out of the do/while block?
> 
> I'm not a huge fan of stack allocations inside blocks, unless there
> is a particular reason to do so.

The move to scope based locking and freeing in cleanup.h necessitates
using scope based variables as well, so they're something we all have
to embrace.  They're also useful to tell the compiler when it can
reclaim the variable and they often create an extra stack frame that
allows the reclaim to be effective (even if the compiler can work out
where a variable is no longer reference, the space can't be reclaimed
if it's in the middle of an in-use stack frame).  I'd say the rule of
thumb should be only do something like this if it improves readability
or allows you to remove an additional block from the code.

Regards,

James

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 10:12:36AM -0400, James Bottomley wrote:
> On Thu, 2025-03-27 at 15:23 +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
> [...]
> > > > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > > *chip, void *buf, size_t bufsiz)
> > > > 	ssize_t len = 0;
> > > > 	u32 count, ordinal;
> > > > 	unsigned long stop;
> > > > +	u8 status;
> > > 
> > > Why move `status` out of the do/while block?
> > 
> > I'm not a huge fan of stack allocations inside blocks, unless there
> > is a particular reason to do so.
> 
> The move to scope based locking and freeing in cleanup.h necessitates
> using scope based variables as well, so they're something we all have
> to embrace.  They're also useful to tell the compiler when it can
> reclaim the variable and they often create an extra stack frame that
> allows the reclaim to be effective (even if the compiler can work out
> where a variable is no longer reference, the space can't be reclaimed
> if it's in the middle of an in-use stack frame).  I'd say the rule of
> thumb should be only do something like this if it improves readability
> or allows you to remove an additional block from the code.

Reclaiming here is only shift in the frame pointer, nothing to do with
reclaiming resources or freeing locks. Consolidating value state into
single location does improve readability as far as I'm concerned.
 
> Regards,
> 
> James
> 

BR, Jarkko

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 05:00:11PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 27, 2025 at 10:12:36AM -0400, James Bottomley wrote:
> > On Thu, 2025-03-27 at 15:23 +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
> > [...]
> > > > > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > > > *chip, void *buf, size_t bufsiz)
> > > > > 	ssize_t len = 0;
> > > > > 	u32 count, ordinal;
> > > > > 	unsigned long stop;
> > > > > +	u8 status;
> > > > 
> > > > Why move `status` out of the do/while block?
> > > 
> > > I'm not a huge fan of stack allocations inside blocks, unless there
> > > is a particular reason to do so.
> > 
> > The move to scope based locking and freeing in cleanup.h necessitates
> > using scope based variables as well, so they're something we all have
> > to embrace.  They're also useful to tell the compiler when it can
> > reclaim the variable and they often create an extra stack frame that
> > allows the reclaim to be effective (even if the compiler can work out
> > where a variable is no longer reference, the space can't be reclaimed
> > if it's in the middle of an in-use stack frame).  I'd say the rule of
> > thumb should be only do something like this if it improves readability
> > or allows you to remove an additional block from the code.
> 
> Reclaiming here is only shift in the frame pointer, nothing to do with
> reclaiming resources or freeing locks. Consolidating value state into
> single location does improve readability as far as I'm concerned.

Anyhow, I reverted that change given the feedback :-)

Since I'm late sending PR, I'll put this patch to my 6.15 PR.

BR, Jarkko

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Stefano Garzarella]

On Thu, Mar 27, 2025 at 05:06:53PM +0200, Jarkko Sakkinen wrote:
>On Thu, Mar 27, 2025 at 05:00:11PM +0200, Jarkko Sakkinen wrote:
>> On Thu, Mar 27, 2025 at 10:12:36AM -0400, James Bottomley wrote:
>> > On Thu, 2025-03-27 at 15:23 +0200, Jarkko Sakkinen wrote:
>> > > On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
>> > [...]
>> > > > > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
>> > > > > *chip, void *buf, size_t bufsiz)
>> > > > > 	ssize_t len = 0;
>> > > > > 	u32 count, ordinal;
>> > > > > 	unsigned long stop;
>> > > > > +	u8 status;
>> > > >
>> > > > Why move `status` out of the do/while block?
>> > >
>> > > I'm not a huge fan of stack allocations inside blocks, unless there
>> > > is a particular reason to do so.
>> >
>> > The move to scope based locking and freeing in cleanup.h necessitates
>> > using scope based variables as well, so they're something we all have
>> > to embrace.  They're also useful to tell the compiler when it can
>> > reclaim the variable and they often create an extra stack frame that
>> > allows the reclaim to be effective (even if the compiler can work out
>> > where a variable is no longer reference, the space can't be reclaimed
>> > if it's in the middle of an in-use stack frame).  I'd say the rule of
>> > thumb should be only do something like this if it improves readability
>> > or allows you to remove an additional block from the code.
>>
>> Reclaiming here is only shift in the frame pointer, nothing to do with
>> reclaiming resources or freeing locks. Consolidating value state into
>> single location does improve readability as far as I'm concerned.
>
>Anyhow, I reverted that change given the feedback :-)
>
>Since I'm late sending PR, I'll put this patch to my 6.15 PR.

Okay, so I'll not include it in my series and I'll rebase my series on 
your tree.

Thanks,
Stefano

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Jarkko Sakkinen]

On Thu, Mar 27, 2025 at 04:37:13PM +0100, Stefano Garzarella wrote:
> On Thu, Mar 27, 2025 at 05:06:53PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 27, 2025 at 05:00:11PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 27, 2025 at 10:12:36AM -0400, James Bottomley wrote:
> > > > On Thu, 2025-03-27 at 15:23 +0200, Jarkko Sakkinen wrote:
> > > > > On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
> > > > [...]
> > > > > > > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > > > > > *chip, void *buf, size_t bufsiz)
> > > > > > > 	ssize_t len = 0;
> > > > > > > 	u32 count, ordinal;
> > > > > > > 	unsigned long stop;
> > > > > > > +	u8 status;
> > > > > >
> > > > > > Why move `status` out of the do/while block?
> > > > >
> > > > > I'm not a huge fan of stack allocations inside blocks, unless there
> > > > > is a particular reason to do so.
> > > >
> > > > The move to scope based locking and freeing in cleanup.h necessitates
> > > > using scope based variables as well, so they're something we all have
> > > > to embrace.  They're also useful to tell the compiler when it can
> > > > reclaim the variable and they often create an extra stack frame that
> > > > allows the reclaim to be effective (even if the compiler can work out
> > > > where a variable is no longer reference, the space can't be reclaimed
> > > > if it's in the middle of an in-use stack frame).  I'd say the rule of
> > > > thumb should be only do something like this if it improves readability
> > > > or allows you to remove an additional block from the code.
> > > 
> > > Reclaiming here is only shift in the frame pointer, nothing to do with
> > > reclaiming resources or freeing locks. Consolidating value state into
> > > single location does improve readability as far as I'm concerned.
> > 
> > Anyhow, I reverted that change given the feedback :-)
> > 
> > Since I'm late sending PR, I'll put this patch to my 6.15 PR.
> 
> Okay, so I'll not include it in my series and I'll rebase my series on your
> tree.

Let's hold on for what Linus think (i.e. pr-tracker-bot).

I.e., conditional yes.

> 
> Thanks,
> Stefano
> 

BR, Jarkko

Re: [PATCH] tpm: Make chip->{status,cancel,req_canceled} opt

[Stefano Garzarella]

On Thu, Mar 27, 2025 at 03:23:39PM +0200, Jarkko Sakkinen wrote:
>On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:
>> On Wed, Mar 26, 2025 at 06:18:38PM +0200, Jarkko Sakkinen wrote:
>> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> >
>> > tpm_ftpm_tee does not require chip->status, chip->cancel and
>> > chip->req_canceled. Make them optional.
>> >
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> > ---
>> > drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++---
>> > drivers/char/tpm/tpm_ftpm_tee.c  | 20 --------------------
>> > 2 files changed, 28 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> > index f62f7871edbd..10ba47a882d8 100644
>> > --- a/drivers/char/tpm/tpm-interface.c
>> > +++ b/drivers/char/tpm/tpm-interface.c
>> > @@ -58,6 +58,30 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>> >
>> > +static void tpm_chip_cancel(struct tpm_chip *chip)
>> > +{
>> > +	if (!chip->ops->cancel)
>> > +		return;
>> > +
>> > +	chip->ops->cancel(chip);
>> > +}
>> > +
>> > +static u8 tpm_chip_status(struct tpm_chip *chip)
>> > +{
>> > +	if (!chip->ops->status)
>> > +		return 0;
>> > +
>> > +	return chip->ops->status(chip);
>> > +}
>> > +
>> > +static bool tpm_chip_req_canceled(struct tpm_chip *chip, u8 status)
>> > +{
>> > +	if (!chip->ops->req_canceled)
>> > +		return false;
>> > +
>> > +	return chip->ops->req_canceled(chip, status);
>> > +}
>> > +
>> > static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> > {
>> > 	struct tpm_header *header = buf;
>> > @@ -65,6 +89,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> > 	ssize_t len = 0;
>> > 	u32 count, ordinal;
>> > 	unsigned long stop;
>> > +	u8 status;
>>
>> Why move `status` out of the do/while block?
>
>I'm not a huge fan of stack allocations inside blocks, unless there is
>a particular reason to do so.
>
>
>>
>> >
>> > 	if (bufsiz < TPM_HEADER_SIZE)
>> > 		return -EINVAL;
>> > @@ -104,12 +129,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> >
>>
>> What about doing an early return avoiding also calling
>> tpm_calc_ordinal_duration()?
>>
>> I mean something like this:
>>
>>                 rc = 0;
>>         }
>>
>> -       if (chip->flags & TPM_CHIP_FLAG_IRQ)
>> +       if (!chip->ops->status || chip->flags & TPM_CHIP_FLAG_IRQ)
>>                 goto out_recv;
>>
>>
>> Anyway, those are small things, the patch LGTM and it's a great cleanup
>> for ftpm and the svsm driver I'm developing!
>
>If you refined send() and had that the sync flag, this would become:
>
>	if (chip->flags & (TPM_CHIP_FLAG_IRQ | TPM_CHIP_FLAG_SYNC))
>		goto out_recv;

Yep, good point!

>
>>
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>Thank you.

You're welcome!
Stefano