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

From: Jarkko Sakkinen <[email protected]>

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

Signed-off-by Jarkko Sakkinen <[email protected]>

---
drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 20 --------------------

2 files changed, 28 insertions(+), 23 deletions(-)

drivers/char/tpm/tpm-interface.c +24 -0

index f62f7871edbd..10ba47a882d8 100644
@@ -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;

drivers/char/tpm/tpm-interface.c +3 -3

 	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;
 

drivers/char/tpm/tpm_ftpm_tee.c +0 -20

index 8d9209dfc384..53ba28ccd5d3 100644
@@ -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,
 };
 
 /*

Signature · 1 line

2.39.5

Quoted 45 lines · Jarkko Sakkinen

From: Jarkko Sakkinen <[email protected]>

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

Signed-off-by Jarkko Sakkinen <[email protected]>

---
drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 20 --------------------

2 files changed, 28 insertions(+), 23 deletions(-)

drivers/char/tpm/tpm-interface.c +24 -0

index f62f7871edbd..10ba47a882d8 100644
@@ -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)
 {

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) · line 92

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

 	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?

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) · line 129

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +0 -0

 	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;
        }

(quoted diff) +1 -1

-       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 <[email protected]>

Quoted 58 lines

drivers/char/tpm/tpm-interface.c +3 -3

 	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;
 

drivers/char/tpm/tpm_ftpm_tee.c +0 -20

index 8d9209dfc384..53ba28ccd5d3 100644
@@ -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,
 };
 

Quoted 4 lines

drivers/char/tpm/tpm_ftpm_tee.c +0 -0

 /*

Signature · 1 line

2.39.5

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:

Quoted 8 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip · line 93

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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.

Quoted 1 line · Jarkko Sakkinen

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:

[...]

Quoted 11 lines

Quoted 8 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip · line 93

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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

Quoted 15 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:

[...]

Quoted 11 lines

Quoted 8 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip · line 93

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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.

Quoted 9 lines

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.

Quoted 26 lines · Jarkko Sakkinen

Quoted 24 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:

[...]

Quoted 11 lines

Quoted 8 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip · line 93

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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.

Quoted 3 lines

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.

Quoted 31 lines · Jarkko Sakkinen

Quoted 29 lines · Jarkko Sakkinen

Quoted 15 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Thu, Mar 27, 2025 at 10:58:00AM +0100, Stefano Garzarella wrote:

[...]

Quoted 11 lines

Quoted 8 lines

Review static ssize_t tpm_try_transmit(struct tpm_chip · line 93

Quoted 6 lines

(quoted diff) +1 -0

@@ -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.

Quoted 9 lines

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.

Quoted 3 lines

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

Quoted 46 lines · Stefano Garzarella

Quoted 45 lines · Jarkko Sakkinen

From: Jarkko Sakkinen <[email protected]>

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

Signed-off-by Jarkko Sakkinen <[email protected]>

---
drivers/char/tpm/tpm-interface.c | 31 ++++++++++++++++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 20 --------------------

2 files changed, 28 insertions(+), 23 deletions(-)

drivers/char/tpm/tpm-interface.c +24 -0

index f62f7871edbd..10ba47a882d8 100644
@@ -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)
 {

Quoted 8 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) · line 92

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +1 -0

 	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.

Quoted 20 lines

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz) · line 129

Quoted 6 lines

drivers/char/tpm/tpm-interface.c +0 -0

 	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;
        }

(quoted diff) +1 -1

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

Quoted 2 lines

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;

Quoted 4 lines

Reviewed-by Stefano Garzarella <[email protected]>

Thank you.