tpm: Add flag to use default cancellation policy

PATCH Superseded Eddie James 3 participants 5 messages Sep 7, 2022 – Sep 30, 2022

Subsystems (2)

Keys-trusted TPM device driver

lore

Share & tools

lore

Eddie James

author

2022-09-07 16:44 UTC #0 lore raw

PATCH 1/1 tpm: Add flag to use default cancellation policy

The check for cancelled request depends on the VID of the chip, but some chips share VID which shouldn't share their cancellation behavior. This is the case for the Nuvoton NPCT75X, which should use the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate that the chip should use the default cancellation check and set it for the I2C TPM2 TIS driver.

Signed-off-by Eddie James <[email protected]>

---
 drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 drivers/char/tpm/tpm_tis_i2c.c  |  1 +
 3 files changed, 12 insertions(+), 8 deletions(-)

drivers/char/tpm/tpm_tis_core.c +10 -8

index 757623bacfd5..175e75337395 100644
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	switch (priv->manufacturer_id) {
-	case TPM_VID_WINBOND:
-		return ((status == TPM_STS_VALID) ||
-			(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
-	case TPM_VID_STM:
-		return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
-	default:
-		return (status == TPM_STS_COMMAND_READY);
+	if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
+		switch (priv->manufacturer_id) {
+		case TPM_VID_WINBOND:
+			return ((status == TPM_STS_VALID) ||
+				(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+		case TPM_VID_STM:
+			return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+		}
 	}
+
+	return status == TPM_STS_COMMAND_READY;
 }
 
 static irqreturn_t tis_int_handler(int dummy, void *dev_id)

drivers/char/tpm/tpm_tis_core.h +1 -0

index 66a5a13cd1df..b68479e0de10 100644
@@ -86,6 +86,7 @@ enum tis_defaults {
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
 	TPM_TIS_INVALID_STATUS		= BIT(1),
+	TPM_TIS_DEFAULT_CANCELLATION	= BIT(2),
 };
 
 struct tpm_tis_data {

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

index 0692510dfcab..6722588e0217 100644
@@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
 	if (!phy->io_buf)
 		return -ENOMEM;
 
+	set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
 	phy->i2c_client = dev;
 
 	/* must precede all communication with the tpm */

Signature · 1 line

2.31.1

Jarkko Sakkinen

maintainer

2022-09-08 05:22 UTC #1 lore raw

Quoted 29 lines · Eddie James

The check for cancelled request depends on the VID of the chip, but some chips share VID which shouldn't share their cancellation behavior. This is the case for the Nuvoton NPCT75X, which should use the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate that the chip should use the default cancellation check and set it for the I2C TPM2 TIS driver.
Signed-off-by Eddie James <[email protected]>
---
 drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 drivers/char/tpm/tpm_tis_i2c.c  |  1 +
 3 files changed, 12 insertions(+), 8 deletions(-)
drivers/char/tpm/tpm_tis_core.c +0 -6
index 757623bacfd5..175e75337395 100644
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	switch (priv->manufacturer_id) {
-	case TPM_VID_WINBOND:
-		return ((status == TPM_STS_VALID) ||
-			(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
-	case TPM_VID_STM:
-		return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));

drivers/char/tpm/tpm_tis_core.c static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) · line 692

Quoted 10 lines

drivers/char/tpm/tpm_tis_core.c +8 -2

-	default:
-		return (status == TPM_STS_COMMAND_READY);
+	if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
+		switch (priv->manufacturer_id) {
+		case TPM_VID_WINBOND:
+			return ((status == TPM_STS_VALID) ||
+				(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+		case TPM_VID_STM:
+			return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+		}

Why there is no default: ?

Quoted 23 lines

drivers/char/tpm/tpm_tis_core.c +2 -0

 	}
+
+	return status == TPM_STS_COMMAND_READY;
 }
 
 static irqreturn_t tis_int_handler(int dummy, void *dev_id)

drivers/char/tpm/tpm_tis_core.h +1 -0

index 66a5a13cd1df..b68479e0de10 100644
@@ -86,6 +86,7 @@ enum tis_defaults {
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
 	TPM_TIS_INVALID_STATUS		= BIT(1),
+	TPM_TIS_DEFAULT_CANCELLATION	= BIT(2),
 };
 
 struct tpm_tis_data {

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

index 0692510dfcab..6722588e0217 100644

drivers/char/tpm/tpm_tis_i2c.c static int tpm_tis_i2c_probe(struct i2c_client *dev, · line 332

Quoted 4 lines

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

@@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
 	if (!phy->io_buf)
 		return -ENOMEM;
 
+	set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);

What if you just zeroed manufacturer ID?

Quoted 6 lines

drivers/char/tpm/tpm_tis_i2c.c +0 -0
 	phy->i2c_client = dev;
 
 	/* must precede all communication with the tpm */
Signature · 1 line
2.31.1

BR, Jarkko

Eddie James

author

2022-09-08 13:53 UTC #2 lore raw

Quoted 33 lines · Jarkko Sakkinen

Quoted 32 lines · Eddie James
The check for cancelled request depends on the VID of the chip, but some chips share VID which shouldn't share their cancellation behavior. This is the case for the Nuvoton NPCT75X, which should use the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate that the chip should use the default cancellation check and set it for the I2C TPM2 TIS driver.
Signed-off-by Eddie James <[email protected]>
---
  drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
  drivers/char/tpm/tpm_tis_core.h |  1 +
  drivers/char/tpm/tpm_tis_i2c.c  |  1 +
  3 files changed, 12 insertions(+), 8 deletions(-)
drivers/char/tpm/tpm_tis_core.c +1 -8
index 757623bacfd5..175e75337395 100644
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
  {
  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	switch (priv->manufacturer_id) {
-	case TPM_VID_WINBOND:
-		return ((status == TPM_STS_VALID) ||
-			(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
-	case TPM_VID_STM:
-		return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
-	default:
-		return (status == TPM_STS_COMMAND_READY);
+	if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {

Quoted 8 lines

drivers/char/tpm/tpm_tis_core.c static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) · line 692
Quoted 7 lines
drivers/char/tpm/tpm_tis_core.c +7 -0
+		switch (priv->manufacturer_id) {
+		case TPM_VID_WINBOND:
+			return ((status == TPM_STS_VALID) ||
+				(status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+		case TPM_VID_STM:
+			return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+		}
Why there is no default: ?

Well I didn't want to duplicate the line "status == TPM_STS_COMMAND_READY" in the default case and for the flagged case. So now the switch just falls through for default. I can add default: break instead

Quoted 27 lines

drivers/char/tpm/tpm_tis_core.c +2 -0

  	}
+
+	return status == TPM_STS_COMMAND_READY;
  }
 
  static irqreturn_t tis_int_handler(int dummy, void *dev_id)

drivers/char/tpm/tpm_tis_core.h +1 -0

index 66a5a13cd1df..b68479e0de10 100644
@@ -86,6 +86,7 @@ enum tis_defaults {
  enum tpm_tis_flags {
  	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
  	TPM_TIS_INVALID_STATUS		= BIT(1),
+	TPM_TIS_DEFAULT_CANCELLATION	= BIT(2),
  };
 
  struct tpm_tis_data {

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

index 0692510dfcab..6722588e0217 100644
@@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
  	if (!phy->io_buf)
  		return -ENOMEM;

Quoted 2 lines

drivers/char/tpm/tpm_tis_i2c.c static int tpm_tis_i2c_probe(struct i2c_client *dev, · line 332
Quoted 1 line
drivers/char/tpm/tpm_tis_i2c.c +1 -0
+	set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
What if you just zeroed manufacturer ID?

It's already zero there, and gets set to the VID as part of the core init function.

Thanks,

Eddie

Quoted 8 lines

Quoted 7 lines
drivers/char/tpm/tpm_tis_i2c.c +0 -0
  	phy->i2c_client = dev;
 
  	/* must precede all communication with the tpm */
Signature · 1 line
2.31.1
BR, Jarkko

Joel Stanley

2022-09-28 05:10 UTC #3 lore raw

Quoted 46 lines · Eddie James

Quoted 33 lines · Jarkko Sakkinen
Quoted 32 lines · Eddie James
The check for cancelled request depends on the VID of the chip, but some chips share VID which shouldn't share their cancellation behavior. This is the case for the Nuvoton NPCT75X, which should use the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate that the chip should use the default cancellation check and set it for the I2C TPM2 TIS driver.
Signed-off-by Eddie James <[email protected]>
---
  drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
  drivers/char/tpm/tpm_tis_core.h |  1 +
  drivers/char/tpm/tpm_tis_i2c.c  |  1 +
  3 files changed, 12 insertions(+), 8 deletions(-)
drivers/char/tpm/tpm_tis_core.c +1 -8
index 757623bacfd5..175e75337395 100644
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
  {
     struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-    switch (priv->manufacturer_id) {
-    case TPM_VID_WINBOND:
-            return ((status == TPM_STS_VALID) ||
-                    (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
-    case TPM_VID_STM:
-            return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
-    default:
-            return (status == TPM_STS_COMMAND_READY);
+    if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
Quoted 8 lines
drivers/char/tpm/tpm_tis_core.c static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) · line 692
Quoted 7 lines
drivers/char/tpm/tpm_tis_core.c +7 -0
+            switch (priv->manufacturer_id) {
+            case TPM_VID_WINBOND:
+                    return ((status == TPM_STS_VALID) ||
+                            (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+            case TPM_VID_STM:
+                    return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+            }
Why there is no default: ?

Quoted 4 lines

Well I didn't want to duplicate the line "status == TPM_STS_COMMAND_READY" in the default case and for the flagged case. So now the switch just falls through for default. I can add default: break instead

This code was in the original patch series submitted by Nuvoton:

https://lore.kernel.org/r/[email protected]

Perhaps something like that would be better?

Quoted 50 lines

Quoted 29 lines
Quoted 29 lines
drivers/char/tpm/tpm_tis_core.c +2 -0
     }
+
+    return status == TPM_STS_COMMAND_READY;
  }
 
  static irqreturn_t tis_int_handler(int dummy, void *dev_id)
drivers/char/tpm/tpm_tis_core.h +1 -0
index 66a5a13cd1df..b68479e0de10 100644
@@ -86,6 +86,7 @@ enum tis_defaults {
  enum tpm_tis_flags {
     TPM_TIS_ITPM_WORKAROUND         = BIT(0),
     TPM_TIS_INVALID_STATUS          = BIT(1),
+    TPM_TIS_DEFAULT_CANCELLATION    = BIT(2),
  };
 
  struct tpm_tis_data {
drivers/char/tpm/tpm_tis_i2c.c +0 -0
index 0692510dfcab..6722588e0217 100644
@@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
     if (!phy->io_buf)
             return -ENOMEM;
 
Quoted 2 lines
drivers/char/tpm/tpm_tis_i2c.c static int tpm_tis_i2c_probe(struct i2c_client *dev, · line 332
Quoted 1 line
drivers/char/tpm/tpm_tis_i2c.c +1 -0
+    set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
What if you just zeroed manufacturer ID?
It's already zero there, and gets set to the VID as part of the core init function.
Thanks,
Eddie
Quoted 8 lines
Quoted 7 lines
drivers/char/tpm/tpm_tis_i2c.c +0 -1
     phy->i2c_client = dev;
 
     /* must precede all communication with the tpm */
--
2.31.1
BR, Jarkko

Jarkko Sakkinen

maintainer

2022-09-30 21:49 UTC #4 lore raw

Quoted 52 lines · Joel Stanley

Quoted 46 lines · Eddie James
Quoted 33 lines · Jarkko Sakkinen
Quoted 32 lines · Eddie James
The check for cancelled request depends on the VID of the chip, but some chips share VID which shouldn't share their cancellation behavior. This is the case for the Nuvoton NPCT75X, which should use the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate that the chip should use the default cancellation check and set it for the I2C TPM2 TIS driver.
Signed-off-by Eddie James <[email protected]>
---
  drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
  drivers/char/tpm/tpm_tis_core.h |  1 +
  drivers/char/tpm/tpm_tis_i2c.c  |  1 +
  3 files changed, 12 insertions(+), 8 deletions(-)
drivers/char/tpm/tpm_tis_core.c +1 -8
index 757623bacfd5..175e75337395 100644
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
  {
     struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-    switch (priv->manufacturer_id) {
-    case TPM_VID_WINBOND:
-            return ((status == TPM_STS_VALID) ||
-                    (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
-    case TPM_VID_STM:
-            return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
-    default:
-            return (status == TPM_STS_COMMAND_READY);
+    if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
Quoted 8 lines
drivers/char/tpm/tpm_tis_core.c static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) · line 692
Quoted 7 lines
drivers/char/tpm/tpm_tis_core.c +7 -0
+            switch (priv->manufacturer_id) {
+            case TPM_VID_WINBOND:
+                    return ((status == TPM_STS_VALID) ||
+                            (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+            case TPM_VID_STM:
+                    return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+            }
Why there is no default: ?
Quoted 4 lines
Well I didn't want to duplicate the line "status == TPM_STS_COMMAND_READY" in the default case and for the flagged case. So now the switch just falls through for default. I can add default: break instead

Quoted 5 lines

This code was in the original patch series submitted by Nuvoton:
https://lore.kernel.org/r/[email protected]
Perhaps something like that would be better?

The current patch could have

        default:
                /* fall-through */
                break;

BR, Jarkko