[PATCH v4 0/6] TPM irq fixes

[PATCH v4 0/6] TPM irq fixes

[Lino Sanfilippo]

This series fixes issues around the interrupt handling in the TPM TIS core.

Patch 1:
Request threaded interrupt handler for SPI. This is needed since SPI uses a
mutex for data transmission and since we exchange data via SPI int the irq
handler we need a sleepable context.

Patch 2:
Make locality handling simpler by only claiming it at driver startup and
releasing it at driver shutdown.

Patch 3:
Enable the irq test which is always skipped in the current code.

Patch 4:
Fix the irq test by ensuring CPU cache coherency when setting the irq test
condition on one and checking it on another CPU.

Patch 5:
Move the irq test and the check for irq test from tpm_tis_send() to
tpm_tis_probe_irq_single() so the check has not to be done for each data
transmission.

Patch 6:
Instead of blindly trying to enable all possible interrupts, use the result
from the capability query and request only the interrupts that are actually
supported.


Changes in v4:
- only request threaded irq in case of SPI as requested by Jarko.
- reimplement patch 2 to limit locality handling changes to the TIS core.
- separate fixes from cleanups as requested by Jarko.
- rephrase commit messages 

Changes in v3:
- fixed compiler error reported by kernel test robot
- rephrased commit message as suggested by Jarko Sakkinen
- added Reviewed-by tag

Changes in v2:
- rebase against 5.12
- free irq on error path


Lino Sanfilippo (6):
  tpm, tpm_tis_spi: Request threaded irq
  tpm, tpm_tis: Claim and release locality only once
  tpm, tpm_tis: enable irq test
  tpm, tpm_tis: avoid CPU cache incoherency in irq test
  tpm, tpm_tis: Move irq test from tpm_tis_send() to
    tpm_tis_probe_irq_single()
  tpm, tpm_tis: Only enable supported IRQs

 drivers/char/tpm/tpm_tis_core.c     | 202 ++++++++++++----------------
 drivers/char/tpm/tpm_tis_core.h     |   8 +-
 drivers/char/tpm/tpm_tis_spi_main.c |   5 +-
 3 files changed, 96 insertions(+), 119 deletions(-)


base-commit: fe27d189e3f42e31d3c8223d5daed7285e334c5e
-- 
2.36.0

[PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Interrupt handling at least includes reading and writing the interrupt
status register within the interrupt routine. Since accesses over the SPI
bus are synchronized by a mutex, request a threaded interrupt handler to
ensure a sleepable context during interrupt processing.

Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c     | 15 +++++++++++++--
 drivers/char/tpm/tpm_tis_core.h     |  1 +
 drivers/char/tpm/tpm_tis_spi_main.c |  5 +++--
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index dc56b976d816..52369ef39b03 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -747,8 +747,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	int rc;
 	u32 int_status;
 
-	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
-			     dev_name(&chip->dev), chip) != 0) {
+
+	if (priv->flags & TPM_TIS_USE_THREADED_IRQ) {
+		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
+					       tis_int_handler,
+					       IRQF_ONESHOT | flags,
+					       dev_name(&chip->dev),
+					       chip);
+	} else {
+		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
+				      flags, dev_name(&chip->dev), chip);
+	}
+
+	if (rc) {
 		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
 			 irq);
 		return -1;
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 3be24f221e32..43b724e55192 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -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_USE_THREADED_IRQ	= BIT(2),
 };
 
 struct tpm_tis_data {
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index 184396b3af50..f56613f2946f 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -223,9 +223,10 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
 	phy->flow_control = tpm_tis_spi_flow_control;
 
 	/* If the SPI device has an IRQ then use that */
-	if (dev->irq > 0)
+	if (dev->irq > 0) {
 		irq = dev->irq;
-	else
+		phy->priv.flags |= TPM_TIS_USE_THREADED_IRQ;
+	} else
 		irq = -1;
 
 	init_completion(&phy->ready);
-- 
2.36.0

[PATCH v4 2/6] tpm, tpm_tis: Claim and release locality only once

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

It is not necessary to claim and release the default locality for each TPM
command. Instead claim the locality once at driver startup and release it
at driver shutdown.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 50 +++++++++------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 52369ef39b03..46f504fb5084 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -638,9 +638,6 @@ static int probe_itpm(struct tpm_chip *chip)
 	if (vendor != TPM_VID_INTEL)
 		return 0;
 
-	if (request_locality(chip, 0) != 0)
-		return -EBUSY;
-
 	rc = tpm_tis_send_data(chip, cmd_getticks, len);
 	if (rc == 0)
 		goto out;
@@ -659,7 +656,6 @@ static int probe_itpm(struct tpm_chip *chip)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality);
 
 	return rc;
 }
@@ -721,17 +717,11 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
 	cap_t cap;
 	int ret;
 
-	ret = request_locality(chip, 0);
-	if (ret < 0)
-		return ret;
-
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
 	else
 		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
 
-	release_locality(chip, 0);
-
 	return ret;
 }
 
@@ -855,6 +845,8 @@ void tpm_tis_remove(struct tpm_chip *chip)
 
 	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
 
+	release_locality(chip, 0);
+
 	tpm_tis_clkrun_enable(chip, false);
 
 	if (priv->ilb_base_addr)
@@ -925,8 +917,6 @@ static const struct tpm_class_ops tpm_tis = {
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = tpm_tis_req_canceled,
-	.request_locality = request_locality,
-	.relinquish_locality = release_locality,
 	.clk_enable = tpm_tis_clkrun_enable,
 };
 
@@ -963,9 +953,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 	dev_set_drvdata(&chip->dev, priv);
 
+	rc = request_locality(chip, 0);
+	if (rc)
+		return rc;
+
 	rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, 0);
 		return rc;
+	}
 
 	priv->manufacturer_id = vendor;
 
@@ -978,8 +974,10 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (is_bsw()) {
 		priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
 					ILB_REMAP_SIZE);
-		if (!priv->ilb_base_addr)
+		if (!priv->ilb_base_addr) {
+			release_locality(chip, 0);
 			return -ENOMEM;
+		}
 
 		clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
 		/* Check if CLKRUN# is already not enabled in the LPC bus */
@@ -1006,14 +1004,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 
-	rc = request_locality(chip, 0);
-	if (rc < 0) {
-		rc = -ENODEV;
-		goto out_err;
-	}
-
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
-	release_locality(chip, 0);
 
 	rc = tpm_chip_start(chip);
 	if (rc)
@@ -1072,15 +1063,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		 * to make sure it works. May as well use that command to set the
 		 * proper timeouts for the driver.
 		 */
-
-		rc = request_locality(chip, 0);
-		if (rc < 0)
-			goto out_err;
-
 		rc = tpm_get_timeouts(chip);
-
-		release_locality(chip, 0);
-
 		if (rc) {
 			dev_err(dev, "Could not get TPM timeouts and durations\n");
 			rc = -ENODEV;
@@ -1169,16 +1152,9 @@ int tpm_tis_resume(struct device *dev)
 	 * TPM 1.2 requires self-test on resume. This function actually returns
 	 * an error code but for unknown reason it isn't handled.
 	 */
-	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
-		ret = request_locality(chip, 0);
-		if (ret < 0)
-			return ret;
-
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
 		tpm1_do_selftest(chip);
 
-		release_locality(chip, 0);
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_tis_resume);
-- 
2.36.0

[PATCH v4 3/6] tpm, tpm_tis: enable irq test

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

The test for working irqs which is done in tpm_tis_send() is only executed
if both priv->irq_tested is false and TPM_CHIP_FLAG_IRQ is set in
chip->flags.

While the first condition is initially met, the TPM_CHIP_FLAG_IRQ flag is
never set, which prevents the irq test from being ever executed. Fix this
by setting TPM_CHIP_FLAG_IRQ just before the test is made.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 46f504fb5084..4f3b82c3f205 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -781,6 +781,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 		return rc;
 
 	priv->irq_tested = false;
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
-- 
2.36.0

[PATCH v4 4/6] tpm, tpm_tis: avoid CPU cache incoherency in irq test

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

The interrupt handler that sets irq_tested to indicate that interrupts are
working may run on another CPU than the thread that checks this variable in
tmp_tis_send().

Since no synchronization is used to access irq_tested, there is no
guarantee for cache coherency between the CPUs, so that the value set by
the interrupt handler might not be visible to the testing thread.

Avoid this issue by using a bitfield instead of a boolean variable and by
accessing this field with bit manipulating functions that guarantee cache
coherency.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
 drivers/char/tpm/tpm_tis_core.h |  6 +++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4f3b82c3f205..bdfde1cd71fe 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	int rc, irq;
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
+	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
 		return tpm_tis_send_main(chip, buf, len);
 
 	/* Verify receipt of the expected IRQ */
@@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	rc = tpm_tis_send_main(chip, buf, len);
 	priv->irq = irq;
 	chip->flags |= TPM_CHIP_FLAG_IRQ;
-	if (!priv->irq_tested)
+	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
 		tpm_msleep(1);
-	if (!priv->irq_tested)
+	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
 		disable_interrupts(chip);
-	priv->irq_tested = true;
+	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
 	return rc;
 }
 
@@ -689,7 +690,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	if (interrupt == 0)
 		return IRQ_NONE;
 
-	priv->irq_tested = true;
+	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&priv->read_queue);
 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -780,7 +781,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
-	priv->irq_tested = false;
+	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
 	chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 	/* Generate an interrupt by having the core call through to
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 43b724e55192..c8972ea8e13e 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,11 +89,15 @@ enum tpm_tis_flags {
 	TPM_TIS_USE_THREADED_IRQ	= BIT(2),
 };
 
+enum tpm_tis_irqtest_flags {
+	TPM_TIS_IRQTEST_OK		= BIT(0),
+};
+
 struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
-	bool irq_tested;
+	unsigned long irqtest_flags;
 	unsigned long flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
-- 
2.36.0

[PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

There is no need to check for the irq test completion at each data
transmission during the driver livetime. Instead do the check only once at
driver startup.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------------
 1 file changed, 22 insertions(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index bdfde1cd71fe..4c65718feb7d 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip)
  * tpm.c can skip polling for the data to be available as the interrupt is
  * waited for here
  */
-static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
+static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc;
@@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
 	return rc;
 }
 
-static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
-{
-	int rc, irq;
-	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
-	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
-		return tpm_tis_send_main(chip, buf, len);
-
-	/* Verify receipt of the expected IRQ */
-	irq = priv->irq;
-	priv->irq = 0;
-	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
-	rc = tpm_tis_send_main(chip, buf, len);
-	priv->irq = irq;
-	chip->flags |= TPM_CHIP_FLAG_IRQ;
-	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
-		tpm_msleep(1);
-	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
-		disable_interrupts(chip);
-	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
-	return rc;
-}
-
 struct tis_vendor_durations_override {
 	u32 did_vid;
 	struct tpm1_version version;
@@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
 			   &original_int_vec);
-	if (rc < 0)
+	if (rc < 0) {
+		disable_interrupts(chip);
 		return rc;
+	}
 
 	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
 	if (rc < 0)
-		return rc;
+		goto out_err;
 
 	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
 	if (rc < 0)
-		return rc;
+		goto out_err;
 
 	/* Clear all existing */
 	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
 	if (rc < 0)
-		return rc;
+		goto out_err;
 
 	/* Turn on */
 	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
 			     intmask | TPM_GLOBAL_INT_ENABLE);
 	if (rc < 0)
-		return rc;
+		goto out_err;
 
 	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
-	chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
 	 */
 	rc = tpm_tis_gen_interrupt(chip);
 	if (rc < 0)
-		return rc;
+		goto out_err;
 
-	/* tpm_tis_send will either confirm the interrupt is working or it
-	 * will call disable_irq which undoes all of the above.
-	 */
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
-		rc = tpm_tis_write8(priv, original_int_vec,
-				TPM_INT_VECTOR(priv->locality));
-		if (rc < 0)
-			return rc;
+	tpm_msleep(1);
 
-		return 1;
-	}
+	/* Verify receipt of the expected IRQ */
+	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
+		goto out_err;
+
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 	return 0;
+
+out_err:
+	disable_interrupts(chip);
+	tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality));
+
+	return rc;
 }
 
 /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
@@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		if (irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
 						 irq);
-			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+			if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
-
-				disable_interrupts(chip);
-			}
 		} else {
 			tpm_tis_probe_irq(chip, intmask);
 		}
-- 
2.36.0

[PATCH v4 6/6] tpm, tpm_tis: Only enable supported IRQs

[Lino Sanfilippo]

From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Instead of blindly trying to enable all possible interrupts, use the result
from the capability query and request only the interrupts that are actually
supported.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 67 ++++++++++++++++++---------------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4c65718feb7d..784e153e2895 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -976,13 +976,46 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
+	/* Figure out the capabilities */
+	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
+	if (rc < 0)
+		goto out_err;
+
+	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
+		intfcaps);
+	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
+		dev_dbg(dev, "\tBurst Count Static\n");
+	if (intfcaps & TPM_INTF_CMD_READY_INT) {
+		priv->supported_irqs |= TPM_INTF_CMD_READY_INT;
+		dev_dbg(dev, "\tCommand Ready Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
+		dev_dbg(dev, "\tInterrupt Edge Falling\n");
+	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
+		dev_dbg(dev, "\tInterrupt Edge Rising\n");
+	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
+		dev_dbg(dev, "\tInterrupt Level Low\n");
+	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
+		dev_dbg(dev, "\tInterrupt Level High\n");
+	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT) {
+		priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
+		dev_dbg(dev, "\tLocality Change Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_STS_VALID_INT) {
+		priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
+		dev_dbg(dev, "\tSts Valid Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+		priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
+		dev_dbg(dev, "\tData Avail Int Support\n");
+	}
+
 	/* Take control of the TPM's interrupt hardware and shut it off */
 	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
 	if (rc < 0)
 		goto out_err;
 
-	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
-		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+	intmask |= priv->supported_irqs;
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
@@ -1009,32 +1042,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
-	/* Figure out the capabilities */
-	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
-	if (rc < 0)
-		goto out_err;
-
-	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
-		intfcaps);
-	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
-		dev_dbg(dev, "\tBurst Count Static\n");
-	if (intfcaps & TPM_INTF_CMD_READY_INT)
-		dev_dbg(dev, "\tCommand Ready Int Support\n");
-	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
-		dev_dbg(dev, "\tInterrupt Edge Falling\n");
-	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
-		dev_dbg(dev, "\tInterrupt Edge Rising\n");
-	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
-		dev_dbg(dev, "\tInterrupt Level Low\n");
-	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
-		dev_dbg(dev, "\tInterrupt Level High\n");
-	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
-		dev_dbg(dev, "\tLocality Change Int Support\n");
-	if (intfcaps & TPM_INTF_STS_VALID_INT)
-		dev_dbg(dev, "\tSts Valid Int Support\n");
-	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
-		dev_dbg(dev, "\tData Avail Int Support\n");
-
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&priv->read_queue);
 	init_waitqueue_head(&priv->int_queue);
@@ -1101,9 +1108,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 	if (rc < 0)
 		goto out;
 
-	intmask |= TPM_INTF_CMD_READY_INT
-	    | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
-	    | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
+	intmask |= priv->supported_irqs | TPM_GLOBAL_INT_ENABLE;
 
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index c8972ea8e13e..3d6b05c6fdba 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -97,6 +97,7 @@ struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
+	unsigned int supported_irqs;
 	unsigned long irqtest_flags;
 	unsigned long flags;
 	void __iomem *ilb_base_addr;
-- 
2.36.0

Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Interrupt handling at least includes reading and writing the interrupt
> status register within the interrupt routine. Since accesses over the SPI
> bus are synchronized by a mutex, request a threaded interrupt handler to
> ensure a sleepable context during interrupt processing.
> 
> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

When you state that it needs a sleepable context, you should bring a
context why it needs it. This not to disregard the code change overally but
you cannot make even the most obvious claim without backing data.

> ---
>  drivers/char/tpm/tpm_tis_core.c     | 15 +++++++++++++--
>  drivers/char/tpm/tpm_tis_core.h     |  1 +
>  drivers/char/tpm/tpm_tis_spi_main.c |  5 +++--
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index dc56b976d816..52369ef39b03 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -747,8 +747,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  	int rc;
>  	u32 int_status;
>  
> -	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
> -			     dev_name(&chip->dev), chip) != 0) {
> +
> +	if (priv->flags & TPM_TIS_USE_THREADED_IRQ) {
> +		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
> +					       tis_int_handler,
> +					       IRQF_ONESHOT | flags,
> +					       dev_name(&chip->dev),
> +					       chip);
> +	} else {
> +		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
> +				      flags, dev_name(&chip->dev), chip);
> +	}
> +
> +	if (rc) {
>  		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
>  			 irq);
>  		return -1;
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 3be24f221e32..43b724e55192 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -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_USE_THREADED_IRQ	= BIT(2),
>  };
>  
>  struct tpm_tis_data {
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index 184396b3af50..f56613f2946f 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -223,9 +223,10 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
>  	phy->flow_control = tpm_tis_spi_flow_control;
>  
>  	/* If the SPI device has an IRQ then use that */
> -	if (dev->irq > 0)
> +	if (dev->irq > 0) {
>  		irq = dev->irq;
> -	else
> +		phy->priv.flags |= TPM_TIS_USE_THREADED_IRQ;
> +	} else
>  		irq = -1;
>  
>  	init_completion(&phy->ready);
> -- 
> 2.36.0
> 


BR, Jarkko

Re: [PATCH v4 2/6] tpm, tpm_tis: Claim and release locality only once

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> It is not necessary to claim and release the default locality for each TPM
> command. Instead claim the locality once at driver startup and release it
> at driver shutdown.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

We are doing what we're being because of Intel TXT:

https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/

Unfortunately cannot accept this change.

BR, Jarkko

Re: [PATCH v4 4/6] tpm, tpm_tis: avoid CPU cache incoherency in irq test

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:57AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> The interrupt handler that sets irq_tested to indicate that interrupts are
> working may run on another CPU than the thread that checks this variable in
> tmp_tis_send().
> 
> Since no synchronization is used to access irq_tested, there is no
> guarantee for cache coherency between the CPUs, so that the value set by
> the interrupt handler might not be visible to the testing thread.
> 
> Avoid this issue by using a bitfield instead of a boolean variable and by
> accessing this field with bit manipulating functions that guarantee cache
> coherency.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
>  drivers/char/tpm/tpm_tis_core.h |  6 +++++-
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4f3b82c3f205..bdfde1cd71fe 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	int rc, irq;
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  
> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
> +	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> +	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>  		return tpm_tis_send_main(chip, buf, len);
>  
>  	/* Verify receipt of the expected IRQ */
> @@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	rc = tpm_tis_send_main(chip, buf, len);
>  	priv->irq = irq;
>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
> -	if (!priv->irq_tested)
> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>  		tpm_msleep(1);
> -	if (!priv->irq_tested)
> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>  		disable_interrupts(chip);
> -	priv->irq_tested = true;
> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>  	return rc;
>  }
>  
> @@ -689,7 +690,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>  	if (interrupt == 0)
>  		return IRQ_NONE;
>  
> -	priv->irq_tested = true;
> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>  		wake_up_interruptible(&priv->read_queue);
>  	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> @@ -780,7 +781,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  	if (rc < 0)
>  		return rc;
>  
> -	priv->irq_tested = false;
> +	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
>  
>  	/* Generate an interrupt by having the core call through to
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 43b724e55192..c8972ea8e13e 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -89,11 +89,15 @@ enum tpm_tis_flags {
>  	TPM_TIS_USE_THREADED_IRQ	= BIT(2),
>  };
>  
> +enum tpm_tis_irqtest_flags {
> +	TPM_TIS_IRQTEST_OK		= BIT(0),
> +};
> +
>  struct tpm_tis_data {
>  	u16 manufacturer_id;
>  	int locality;
>  	int irq;
> -	bool irq_tested;
> +	unsigned long irqtest_flags;
>  	unsigned long flags;
>  	void __iomem *ilb_base_addr;
>  	u16 clkrun_enabled;
> -- 
> 2.36.0
> 

So, this would caused by changing to threaded IRQs?

BR, Jarkko

Re: [PATCH v4 6/6] tpm, tpm_tis: Only enable supported IRQs

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:59AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Instead of blindly trying to enable all possible interrupts, use the result
> from the capability query and request only the interrupts that are actually
> supported.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 67 ++++++++++++++++++---------------
>  drivers/char/tpm/tpm_tis_core.h |  1 +
>  2 files changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4c65718feb7d..784e153e2895 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -976,13 +976,46 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> +	/* Figure out the capabilities */
> +	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> +	if (rc < 0)
> +		goto out_err;
> +
> +	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> +		intfcaps);
> +	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> +		dev_dbg(dev, "\tBurst Count Static\n");
> +	if (intfcaps & TPM_INTF_CMD_READY_INT) {
> +		priv->supported_irqs |= TPM_INTF_CMD_READY_INT;
> +		dev_dbg(dev, "\tCommand Ready Int Support\n");
> +	}
> +	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> +		dev_dbg(dev, "\tInterrupt Edge Falling\n");
> +	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> +		dev_dbg(dev, "\tInterrupt Edge Rising\n");
> +	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> +		dev_dbg(dev, "\tInterrupt Level Low\n");
> +	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> +		dev_dbg(dev, "\tInterrupt Level High\n");
> +	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT) {
> +		priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
> +		dev_dbg(dev, "\tLocality Change Int Support\n");
> +	}
> +	if (intfcaps & TPM_INTF_STS_VALID_INT) {
> +		priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
> +		dev_dbg(dev, "\tSts Valid Int Support\n");
> +	}
> +	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
> +		priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
> +		dev_dbg(dev, "\tData Avail Int Support\n");
> +	}
> +
>  	/* Take control of the TPM's interrupt hardware and shut it off */
>  	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>  	if (rc < 0)
>  		goto out_err;
>  
> -	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> -		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
> +	intmask |= priv->supported_irqs;
>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>  
>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> @@ -1009,32 +1042,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> -	/* Figure out the capabilities */
> -	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> -	if (rc < 0)
> -		goto out_err;
> -
> -	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> -		intfcaps);
> -	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> -		dev_dbg(dev, "\tBurst Count Static\n");
> -	if (intfcaps & TPM_INTF_CMD_READY_INT)
> -		dev_dbg(dev, "\tCommand Ready Int Support\n");
> -	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> -		dev_dbg(dev, "\tInterrupt Edge Falling\n");
> -	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> -		dev_dbg(dev, "\tInterrupt Edge Rising\n");
> -	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> -		dev_dbg(dev, "\tInterrupt Level Low\n");
> -	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> -		dev_dbg(dev, "\tInterrupt Level High\n");
> -	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> -		dev_dbg(dev, "\tLocality Change Int Support\n");
> -	if (intfcaps & TPM_INTF_STS_VALID_INT)
> -		dev_dbg(dev, "\tSts Valid Int Support\n");
> -	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> -		dev_dbg(dev, "\tData Avail Int Support\n");
> -
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&priv->read_queue);
>  	init_waitqueue_head(&priv->int_queue);
> @@ -1101,9 +1108,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  	if (rc < 0)
>  		goto out;
>  
> -	intmask |= TPM_INTF_CMD_READY_INT
> -	    | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
> -	    | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
> +	intmask |= priv->supported_irqs | TPM_GLOBAL_INT_ENABLE;
>  
>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index c8972ea8e13e..3d6b05c6fdba 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -97,6 +97,7 @@ struct tpm_tis_data {
>  	u16 manufacturer_id;
>  	int locality;
>  	int irq;
> +	unsigned int supported_irqs;
>  	unsigned long irqtest_flags;
>  	unsigned long flags;
>  	void __iomem *ilb_base_addr;
> -- 
> 2.36.0
> 

Does the existing code cause issues in a some specific environment?

BR, Jarkko

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> There is no need to check for the irq test completion at each data
> transmission during the driver livetime. Instead do the check only once at
> driver startup.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------------
>  1 file changed, 22 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index bdfde1cd71fe..4c65718feb7d 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip)
>   * tpm.c can skip polling for the data to be available as the interrupt is
>   * waited for here
>   */
> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	int rc;
> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>  	return rc;
>  }
>  
> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> -{
> -	int rc, irq;
> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -
> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> -	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> -		return tpm_tis_send_main(chip, buf, len);
> -
> -	/* Verify receipt of the expected IRQ */
> -	irq = priv->irq;
> -	priv->irq = 0;
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> -	rc = tpm_tis_send_main(chip, buf, len);
> -	priv->irq = irq;
> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> -		tpm_msleep(1);
> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> -		disable_interrupts(chip);
> -	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> -	return rc;
> -}
> -
>  struct tis_vendor_durations_override {
>  	u32 did_vid;
>  	struct tpm1_version version;
> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  
>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>  			   &original_int_vec);
> -	if (rc < 0)
> +	if (rc < 0) {
> +		disable_interrupts(chip);
>  		return rc;
> +	}
>  
>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>  	if (rc < 0)
> -		return rc;
> +		goto out_err;
>  
>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>  	if (rc < 0)
> -		return rc;
> +		goto out_err;
>  
>  	/* Clear all existing */
>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>  	if (rc < 0)
> -		return rc;
> +		goto out_err;
>  
>  	/* Turn on */
>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>  			     intmask | TPM_GLOBAL_INT_ENABLE);
>  	if (rc < 0)
> -		return rc;
> +		goto out_err;
>  
>  	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
>  
>  	/* Generate an interrupt by having the core call through to
>  	 * tpm_tis_send
>  	 */
>  	rc = tpm_tis_gen_interrupt(chip);
>  	if (rc < 0)
> -		return rc;
> +		goto out_err;
>  
> -	/* tpm_tis_send will either confirm the interrupt is working or it
> -	 * will call disable_irq which undoes all of the above.
> -	 */
> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> -		rc = tpm_tis_write8(priv, original_int_vec,
> -				TPM_INT_VECTOR(priv->locality));
> -		if (rc < 0)
> -			return rc;
> +	tpm_msleep(1);
>  
> -		return 1;
> -	}
> +	/* Verify receipt of the expected IRQ */
> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> +		goto out_err;
> +
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
>  
>  	return 0;
> +
> +out_err:
> +	disable_interrupts(chip);
> +	tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality));
> +
> +	return rc;
>  }
>  
>  /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		if (irq) {
>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
>  						 irq);
> -			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> +			if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
>  				dev_err(&chip->dev, FW_BUG
>  					"TPM interrupt not working, polling instead\n");
> -
> -				disable_interrupts(chip);
> -			}
>  		} else {
>  			tpm_tis_probe_irq(chip, intmask);
>  		}
> -- 
> 2.36.0
> 

For me this looks just code shuffling.

I don't disagree but changing working code without actual semantical
reasons neither makes sense.

BR, Jarkko

Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Lino Sanfilippo]

Hi,

On 11.05.22 at 13:22, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Interrupt handling at least includes reading and writing the interrupt
>> status register within the interrupt routine. Since accesses over the SPI
>> bus are synchronized by a mutex, request a threaded interrupt handler to
>> ensure a sleepable context during interrupt processing.
>>
>> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> When you state that it needs a sleepable context, you should bring a
> context why it needs it. This not to disregard the code change overally but
> you cannot make even the most obvious claim without backing data.
>

so what kind of backing data do you have in mind? Would it help to emphasize more
that the irq handler is running in hard irq context in the current code and thus
must not access registers over SPI since SPI uses a mutex (I consider it as basic
knowledge that a mutex must not be taken in hard irq context)?


Regards,
Lino


>> ---
>>  drivers/char/tpm/tpm_tis_core.c     | 15 +++++++++++++--
>>  drivers/char/tpm/tpm_tis_core.h     |  1 +
>>  drivers/char/tpm/tpm_tis_spi_main.c |  5 +++--
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index dc56b976d816..52369ef39b03 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -747,8 +747,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	int rc;
>>  	u32 int_status;
>>
>> -	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
>> -			     dev_name(&chip->dev), chip) != 0) {
>> +
>> +	if (priv->flags & TPM_TIS_USE_THREADED_IRQ) {
>> +		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
>> +					       tis_int_handler,
>> +					       IRQF_ONESHOT | flags,
>> +					       dev_name(&chip->dev),
>> +					       chip);
>> +	} else {
>> +		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
>> +				      flags, dev_name(&chip->dev), chip);
>> +	}
>> +
>> +	if (rc) {
>>  		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
>>  			 irq);
>>  		return -1;
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 3be24f221e32..43b724e55192 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -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_USE_THREADED_IRQ	= BIT(2),
>>  };
>>
>>  struct tpm_tis_data {
>> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
>> index 184396b3af50..f56613f2946f 100644
>> --- a/drivers/char/tpm/tpm_tis_spi_main.c
>> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
>> @@ -223,9 +223,10 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
>>  	phy->flow_control = tpm_tis_spi_flow_control;
>>
>>  	/* If the SPI device has an IRQ then use that */
>> -	if (dev->irq > 0)
>> +	if (dev->irq > 0) {
>>  		irq = dev->irq;
>> -	else
>> +		phy->priv.flags |= TPM_TIS_USE_THREADED_IRQ;
>> +	} else
>>  		irq = -1;
>>
>>  	init_completion(&phy->ready);
>> --
>> 2.36.0
>>
>
>
> BR, Jarkko
>

Re: [PATCH v4 2/6] tpm, tpm_tis: Claim and release locality only once

[Lino Sanfilippo]

On 11.05.22 at 13:27, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> It is not necessary to claim and release the default locality for each TPM
>> command. Instead claim the locality once at driver startup and release it
>> at driver shutdown.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> We are doing what we're being because of Intel TXT:
>
> https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/
>
> Unfortunately cannot accept this change.
>

I do not see how the patch affects the crb code since the only changes concern the
tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops
which are still used to claim and release the locality.

Or do I miss something?

Regards,
Lino

Re: [PATCH v4 4/6] tpm, tpm_tis: avoid CPU cache incoherency in irq test

[Lino Sanfilippo]

On 11.05.22 at 17:06, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:57AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> The interrupt handler that sets irq_tested to indicate that interrupts are
>> working may run on another CPU than the thread that checks this variable in
>> tmp_tis_send().
>>
>> Since no synchronization is used to access irq_tested, there is no
>> guarantee for cache coherency between the CPUs, so that the value set by
>> the interrupt handler might not be visible to the testing thread.
>>
>> Avoid this issue by using a bitfield instead of a boolean variable and by
>> accessing this field with bit manipulating functions that guarantee cache
>> coherency.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
>>  drivers/char/tpm/tpm_tis_core.h |  6 +++++-
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 4f3b82c3f205..bdfde1cd71fe 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  	int rc, irq;
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>
>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>> +	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
>> +	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		return tpm_tis_send_main(chip, buf, len);
>>
>>  	/* Verify receipt of the expected IRQ */
>> @@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  	rc = tpm_tis_send_main(chip, buf, len);
>>  	priv->irq = irq;
>>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		tpm_msleep(1);
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		disable_interrupts(chip);
>> -	priv->irq_tested = true;
>> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	return rc;
>>  }
>>
>> @@ -689,7 +690,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>>  	if (interrupt == 0)
>>  		return IRQ_NONE;
>>
>> -	priv->irq_tested = true;
>> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>>  		wake_up_interruptible(&priv->read_queue);
>>  	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
>> @@ -780,7 +781,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	if (rc < 0)
>>  		return rc;
>>
>> -	priv->irq_tested = false;
>> +	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>
>>  	/* Generate an interrupt by having the core call through to
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 43b724e55192..c8972ea8e13e 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -89,11 +89,15 @@ enum tpm_tis_flags {
>>  	TPM_TIS_USE_THREADED_IRQ	= BIT(2),
>>  };
>>
>> +enum tpm_tis_irqtest_flags {
>> +	TPM_TIS_IRQTEST_OK		= BIT(0),
>> +};
>> +
>>  struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> -	bool irq_tested;
>> +	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>>  	u16 clkrun_enabled;
>> --
>> 2.36.0
>>
>
> So, this would caused by changing to threaded IRQs?
>
> BR, Jarkko
>

No, this is caused by the fact that we access irq_tested from different paths of execution.
The writing and reading to/from irq_tested from different execution paths without any
synchronization is a plain bug in the existing code:
We simply cannot guarantee that the value we set for irq_tested in the interrupt handler
which may run on CPU 1 is visible in tpm_tis_send() which may run on CPU2. This is because
there is no enforced data transfer between the CPUs and thus the written value may end up in
the CPU cache of CPU 1 and never (or at least not within the timout of 1 ms as used in tpm_tis_send())
be seen by CPU 2.
This kind of issues are in length described in memory-barriers.txt.

The patch is supposed to fix this bug.

Regards,
Lino

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Lino Sanfilippo]

On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> There is no need to check for the irq test completion at each data
>> transmission during the driver livetime. Instead do the check only once at
>> driver startup.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------------
>>  1 file changed, 22 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index bdfde1cd71fe..4c65718feb7d 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip)
>>   * tpm.c can skip polling for the data to be available as the interrupt is
>>   * waited for here
>>   */
>> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  {
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>  	int rc;
>> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>>  	return rc;
>>  }
>>
>> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>> -{
>> -	int rc, irq;
>> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -
>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
>> -	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>> -		return tpm_tis_send_main(chip, buf, len);
>> -
>> -	/* Verify receipt of the expected IRQ */
>> -	irq = priv->irq;
>> -	priv->irq = 0;
>> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>> -	rc = tpm_tis_send_main(chip, buf, len);
>> -	priv->irq = irq;
>> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
>> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>> -		tpm_msleep(1);
>> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>> -		disable_interrupts(chip);
>> -	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>> -	return rc;
>> -}
>> -
>>  struct tis_vendor_durations_override {
>>  	u32 did_vid;
>>  	struct tpm1_version version;
>> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>
>>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>>  			   &original_int_vec);
>> -	if (rc < 0)
>> +	if (rc < 0) {
>> +		disable_interrupts(chip);
>>  		return rc;
>> +	}
>>
>>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	/* Clear all existing */
>>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	/* Turn on */
>>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>>  			     intmask | TPM_GLOBAL_INT_ENABLE);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>
>>  	/* Generate an interrupt by having the core call through to
>>  	 * tpm_tis_send
>>  	 */
>>  	rc = tpm_tis_gen_interrupt(chip);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>> -	/* tpm_tis_send will either confirm the interrupt is working or it
>> -	 * will call disable_irq which undoes all of the above.
>> -	 */
>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>> -		rc = tpm_tis_write8(priv, original_int_vec,
>> -				TPM_INT_VECTOR(priv->locality));
>> -		if (rc < 0)
>> -			return rc;
>> +	tpm_msleep(1);
>>
>> -		return 1;
>> -	}
>> +	/* Verify receipt of the expected IRQ */
>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>> +		goto out_err;
>> +
>> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>
>>  	return 0;
>> +
>> +out_err:
>> +	disable_interrupts(chip);
>> +	tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality));
>> +
>> +	return rc;
>>  }
>>
>>  /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
>> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		if (irq) {
>>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
>>  						 irq);
>> -			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>> +			if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
>>  				dev_err(&chip->dev, FW_BUG
>>  					"TPM interrupt not working, polling instead\n");
>> -
>> -				disable_interrupts(chip);
>> -			}
>>  		} else {
>>  			tpm_tis_probe_irq(chip, intmask);
>>  		}
>> --
>> 2.36.0
>>
>
> For me this looks just code shuffling.
>
> I don't disagree but changing working code without actual semantical
> reasons neither makes sense.
>
> BR, Jarkko
>

Well the semantical reason for this change is that the check for irq test completion
only has to be done once for the driver livetime. There is no point in doing it
over and over again for each transmission.
So the code is not simply shuffled around, it is shifted to a place where it is only
executed once.

This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood
from your comments on the earlier versions of this patch set cleanups are also ok as
long as they are not intermixed with bugfixes.

Regards,
Lino

Re: [PATCH v4 6/6] tpm, tpm_tis: Only enable supported IRQs

[Lino Sanfilippo]

On 11.05.22 at 17:08, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:59AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Instead of blindly trying to enable all possible interrupts, use the result
>> from the capability query and request only the interrupts that are actually
>> supported.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 67 ++++++++++++++++++---------------
>>  drivers/char/tpm/tpm_tis_core.h |  1 +
>>  2 files changed, 37 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 4c65718feb7d..784e153e2895 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -976,13 +976,46 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		goto out_err;
>>  	}
>>
>> +	/* Figure out the capabilities */
>> +	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> +	if (rc < 0)
>> +		goto out_err;
>> +
>> +	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> +		intfcaps);
>> +	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> +		dev_dbg(dev, "\tBurst Count Static\n");
>> +	if (intfcaps & TPM_INTF_CMD_READY_INT) {
>> +		priv->supported_irqs |= TPM_INTF_CMD_READY_INT;
>> +		dev_dbg(dev, "\tCommand Ready Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> +		dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> +	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> +		dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> +	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> +		dev_dbg(dev, "\tInterrupt Level Low\n");
>> +	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> +		dev_dbg(dev, "\tInterrupt Level High\n");
>> +	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT) {
>> +		priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
>> +		dev_dbg(dev, "\tLocality Change Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_STS_VALID_INT) {
>> +		priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
>> +		dev_dbg(dev, "\tSts Valid Int Support\n");
>> +	}
>> +	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
>> +		priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
>> +		dev_dbg(dev, "\tData Avail Int Support\n");
>> +	}
>> +
>>  	/* Take control of the TPM's interrupt hardware and shut it off */
>>  	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>>  	if (rc < 0)
>>  		goto out_err;
>>
>> -	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
>> -		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
>> +	intmask |= priv->supported_irqs;
>>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>>
>>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> @@ -1009,32 +1042,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		goto out_err;
>>  	}
>>
>> -	/* Figure out the capabilities */
>> -	rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> -	if (rc < 0)
>> -		goto out_err;
>> -
>> -	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> -		intfcaps);
>> -	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> -		dev_dbg(dev, "\tBurst Count Static\n");
>> -	if (intfcaps & TPM_INTF_CMD_READY_INT)
>> -		dev_dbg(dev, "\tCommand Ready Int Support\n");
>> -	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> -		dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> -	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> -		dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> -	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> -		dev_dbg(dev, "\tInterrupt Level Low\n");
>> -	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> -		dev_dbg(dev, "\tInterrupt Level High\n");
>> -	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>> -		dev_dbg(dev, "\tLocality Change Int Support\n");
>> -	if (intfcaps & TPM_INTF_STS_VALID_INT)
>> -		dev_dbg(dev, "\tSts Valid Int Support\n");
>> -	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
>> -		dev_dbg(dev, "\tData Avail Int Support\n");
>> -
>>  	/* INTERRUPT Setup */
>>  	init_waitqueue_head(&priv->read_queue);
>>  	init_waitqueue_head(&priv->int_queue);
>> @@ -1101,9 +1108,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>>  	if (rc < 0)
>>  		goto out;
>>
>> -	intmask |= TPM_INTF_CMD_READY_INT
>> -	    | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
>> -	    | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
>> +	intmask |= priv->supported_irqs | TPM_GLOBAL_INT_ENABLE;
>>
>>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index c8972ea8e13e..3d6b05c6fdba 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -97,6 +97,7 @@ struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> +	unsigned int supported_irqs;
>>  	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>> --
>> 2.36.0
>>
>
> Does the existing code cause issues in a some specific environment?
>
> BR, Jarkko
>

Not that I know of. This patch is not supposed to be a bugfix but an improvement of
the existing code by using the information about supported interrupts which is collected during
the capability query. After the query we know exactly which irqs are supported, so why not use
this knowledge when setting the irq mask?

Regards,
Lino

Re: [PATCH v4 2/6] tpm, tpm_tis: Claim and release locality only once

[Jarkko Sakkinen]

On Wed, May 11, 2022 at 09:29:57PM +0200, Lino Sanfilippo wrote:
> 
> 
> On 11.05.22 at 13:27, Jarkko Sakkinen wrote:
> > On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote:
> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >>
> >> It is not necessary to claim and release the default locality for each TPM
> >> command. Instead claim the locality once at driver startup and release it
> >> at driver shutdown.
> >>
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >
> > We are doing what we're being because of Intel TXT:
> >
> > https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/
> >
> > Unfortunately cannot accept this change.
> >
> 
> I do not see how the patch affects the crb code since the only changes concern the
> tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops
> which are still used to claim and release the locality.
> 
> Or do I miss something?

Ugh, yes breaking everything when TXT is used with tpm_tis.

> Regards,
> Lino

BR, Jarkko

Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Jarkko Sakkinen]

On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote:
> Hi,
> 
> On 11.05.22 at 13:22, Jarkko Sakkinen wrote:
> > On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >>
> >> Interrupt handling at least includes reading and writing the interrupt
> >> status register within the interrupt routine. Since accesses over the SPI
> >> bus are synchronized by a mutex, request a threaded interrupt handler to
> >> ensure a sleepable context during interrupt processing.
> >>
> >> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >
> > When you state that it needs a sleepable context, you should bring a
> > context why it needs it. This not to disregard the code change overally but
> > you cannot make even the most obvious claim without backing data.
> >
> 
> so what kind of backing data do you have in mind? Would it help to emphasize more
> that the irq handler is running in hard irq context in the current code and thus
> must not access registers over SPI since SPI uses a mutex (I consider it as basic
> knowledge that a mutex must not be taken in hard irq context)?

There's zero mention about specific lock you are talking about. Providing
the basic knowledge what you are trying to do is the whole point of the
commit message in the first place. I'd presume this patch is related to the
use bus_lock_mutex but it is fully ingored here.

BR, Jarkko

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Jarkko Sakkinen]

On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >>
> >> There is no need to check for the irq test completion at each data
> >> transmission during the driver livetime. Instead do the check only once at
> >> driver startup.
> >>
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >> ---
> >>  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------------
> >>  1 file changed, 22 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index bdfde1cd71fe..4c65718feb7d 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip)
> >>   * tpm.c can skip polling for the data to be available as the interrupt is
> >>   * waited for here
> >>   */
> >> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
> >> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> >>  {
> >>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>  	int rc;
> >> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
> >>  	return rc;
> >>  }
> >>
> >> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> >> -{
> >> -	int rc, irq;
> >> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> -
> >> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> >> -	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> >> -		return tpm_tis_send_main(chip, buf, len);
> >> -
> >> -	/* Verify receipt of the expected IRQ */
> >> -	irq = priv->irq;
> >> -	priv->irq = 0;
> >> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> >> -	rc = tpm_tis_send_main(chip, buf, len);
> >> -	priv->irq = irq;
> >> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
> >> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> >> -		tpm_msleep(1);
> >> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> >> -		disable_interrupts(chip);
> >> -	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> >> -	return rc;
> >> -}
> >> -
> >>  struct tis_vendor_durations_override {
> >>  	u32 did_vid;
> >>  	struct tpm1_version version;
> >> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> >>
> >>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> >>  			   &original_int_vec);
> >> -	if (rc < 0)
> >> +	if (rc < 0) {
> >> +		disable_interrupts(chip);
> >>  		return rc;
> >> +	}
> >>
> >>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
> >>  	if (rc < 0)
> >> -		return rc;
> >> +		goto out_err;
> >>
> >>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
> >>  	if (rc < 0)
> >> -		return rc;
> >> +		goto out_err;
> >>
> >>  	/* Clear all existing */
> >>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
> >>  	if (rc < 0)
> >> -		return rc;
> >> +		goto out_err;
> >>
> >>  	/* Turn on */
> >>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> >>  			     intmask | TPM_GLOBAL_INT_ENABLE);
> >>  	if (rc < 0)
> >> -		return rc;
> >> +		goto out_err;
> >>
> >>  	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> >> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
> >>
> >>  	/* Generate an interrupt by having the core call through to
> >>  	 * tpm_tis_send
> >>  	 */
> >>  	rc = tpm_tis_gen_interrupt(chip);
> >>  	if (rc < 0)
> >> -		return rc;
> >> +		goto out_err;
> >>
> >> -	/* tpm_tis_send will either confirm the interrupt is working or it
> >> -	 * will call disable_irq which undoes all of the above.
> >> -	 */
> >> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> >> -		rc = tpm_tis_write8(priv, original_int_vec,
> >> -				TPM_INT_VECTOR(priv->locality));
> >> -		if (rc < 0)
> >> -			return rc;
> >> +	tpm_msleep(1);
> >>
> >> -		return 1;
> >> -	}
> >> +	/* Verify receipt of the expected IRQ */
> >> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> >> +		goto out_err;
> >> +
> >> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
> >>
> >>  	return 0;
> >> +
> >> +out_err:

Rename this as just 'err'.

> >> +	disable_interrupts(chip);
> >> +	tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality));
> >> +
> >> +	return rc;
> >>  }
> >>
> >>  /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
> >> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >>  		if (irq) {
> >>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
> >>  						 irq);
> >> -			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> >> +			if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> >>  				dev_err(&chip->dev, FW_BUG
> >>  					"TPM interrupt not working, polling instead\n");
> >> -
> >> -				disable_interrupts(chip);
> >> -			}
> >>  		} else {
> >>  			tpm_tis_probe_irq(chip, intmask);
> >>  		}
> >> --
> >> 2.36.0
> >>
> >
> > For me this looks just code shuffling.
> >
> > I don't disagree but changing working code without actual semantical
> > reasons neither makes sense.
> >
> > BR, Jarkko
> >
> 
> Well the semantical reason for this change is that the check for irq test completion
> only has to be done once for the driver livetime. There is no point in doing it
> over and over again for each transmission.
> So the code is not simply shuffled around, it is shifted to a place where it is only
> executed once.
> 
> This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood
> from your comments on the earlier versions of this patch set cleanups are also ok as
> long as they are not intermixed with bugfixes.

The patch does not do anything particulary useful IMHO. There's no
stimulus to do this change.

> Regards,
> Lino

BR, Jarkko

Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Lino Sanfilippo]

Hi,

On 13.05.22 at 20:08, Jarkko Sakkinen wrote:
> On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote:
>> Hi,
>>
>> On 11.05.22 at 13:22, Jarkko Sakkinen wrote:
>>> On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
>>>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>>
>>>> Interrupt handling at least includes reading and writing the interrupt
>>>> status register within the interrupt routine. Since accesses over the SPI
>>>> bus are synchronized by a mutex, request a threaded interrupt handler to
>>>> ensure a sleepable context during interrupt processing.
>>>>
>>>> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
>>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>
>>> When you state that it needs a sleepable context, you should bring a
>>> context why it needs it. This not to disregard the code change overally but
>>> you cannot make even the most obvious claim without backing data.
>>>
>>
>> so what kind of backing data do you have in mind? Would it help to emphasize more
>> that the irq handler is running in hard irq context in the current code and thus
>> must not access registers over SPI since SPI uses a mutex (I consider it as basic
>> knowledge that a mutex must not be taken in hard irq context)?
>
> There's zero mention about specific lock you are talking about. Providing
> the basic knowledge what you are trying to do is the whole point of the
> commit message in the first place. I'd presume this patch is related to the
> use bus_lock_mutex but it is fully ingored here.
>

Ok, understood. I will amend the commit message to make more clear that
reading and writing registers from the interrupt handler results in
a call to tpm_tis_spi_transfer() which uses the bus_lock_mutex of the
spi device and thus requires a sleepable context.


Regards,
Lino



> BR, Jarkko
>

Re: [PATCH v4 2/6] tpm, tpm_tis: Claim and release locality only once

[Lino Sanfilippo]

On 13.05.22 at 19:59, Jarkko Sakkinen wrote:
> On Wed, May 11, 2022 at 09:29:57PM +0200, Lino Sanfilippo wrote:
>>
>>
>> On 11.05.22 at 13:27, Jarkko Sakkinen wrote:
>>> On Mon, May 09, 2022 at 10:05:55AM +0200, Lino Sanfilippo wrote:
>>>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>>
>>>> It is not necessary to claim and release the default locality for each TPM
>>>> command. Instead claim the locality once at driver startup and release it
>>>> at driver shutdown.
>>>>
>>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>
>>> We are doing what we're being because of Intel TXT:
>>>
>>> https://lore.kernel.org/tpmdd-devel/20170315055738.11088-1-jarkko.sakkinen@iki.fi/
>>>
>>> Unfortunately cannot accept this change.
>>>
>>
>> I do not see how the patch affects the crb code since the only changes concern the
>> tpm_class_ops of the tis core. AFAICS crb uses its own set of tpm_class_ops
>> which are still used to claim and release the locality.
>>
>> Or do I miss something?
>
> Ugh, yes breaking everything when TXT is used with tpm_tis.
>
>> Regards,
>> Lino
>
> BR, Jarkko
>

Ok, I got it now. The idea of this patch was to maintain the locality 0 for the whole
driver lifetime, so that we can always be sure that the locality is already claimed
when the interrupt status register is read or written in the interrupt handler.

But of course this does not work if some other instance like TXT also wants
to claim localities.

I will try another approach to make sure that the locality is already taken when the
irq handler is called and only released after the handler has finished.

Regards,
Lino

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Lino Sanfilippo]

On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
>> On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
>>> On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
>>>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>>
>>>> There is no need to check for the irq test completion at each data
>>>> transmission during the driver livetime. Instead do the check only once at
>>>> driver startup.
>>>>
>>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>> ---
>>>>  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------------
>>>>  1 file changed, 22 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>>>> index bdfde1cd71fe..4c65718feb7d 100644
>>>> --- a/drivers/char/tpm/tpm_tis_core.c
>>>> +++ b/drivers/char/tpm/tpm_tis_core.c
>>>> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip)
>>>>   * tpm.c can skip polling for the data to be available as the interrupt is
>>>>   * waited for here
>>>>   */
>>>> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>>>> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>>>  {
>>>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>>  	int rc;
>>>> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
>>>>  	return rc;
>>>>  }
>>>>
>>>> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>>> -{
>>>> -	int rc, irq;
>>>> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>> -
>>>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
>>>> -	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>>> -		return tpm_tis_send_main(chip, buf, len);
>>>> -
>>>> -	/* Verify receipt of the expected IRQ */
>>>> -	irq = priv->irq;
>>>> -	priv->irq = 0;
>>>> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>>>> -	rc = tpm_tis_send_main(chip, buf, len);
>>>> -	priv->irq = irq;
>>>> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>>> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>>> -		tpm_msleep(1);
>>>> -	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>>> -		disable_interrupts(chip);
>>>> -	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>>> -	return rc;
>>>> -}
>>>> -
>>>>  struct tis_vendor_durations_override {
>>>>  	u32 did_vid;
>>>>  	struct tpm1_version version;
>>>> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>>>
>>>>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>>>>  			   &original_int_vec);
>>>> -	if (rc < 0)
>>>> +	if (rc < 0) {
>>>> +		disable_interrupts(chip);
>>>>  		return rc;
>>>> +	}
>>>>
>>>>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>>>>  	if (rc < 0)
>>>> -		return rc;
>>>> +		goto out_err;
>>>>
>>>>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>>>>  	if (rc < 0)
>>>> -		return rc;
>>>> +		goto out_err;
>>>>
>>>>  	/* Clear all existing */
>>>>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>>>>  	if (rc < 0)
>>>> -		return rc;
>>>> +		goto out_err;
>>>>
>>>>  	/* Turn on */
>>>>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>>>>  			     intmask | TPM_GLOBAL_INT_ENABLE);
>>>>  	if (rc < 0)
>>>> -		return rc;
>>>> +		goto out_err;
>>>>
>>>>  	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>>> -	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>>>
>>>>  	/* Generate an interrupt by having the core call through to
>>>>  	 * tpm_tis_send
>>>>  	 */
>>>>  	rc = tpm_tis_gen_interrupt(chip);
>>>>  	if (rc < 0)
>>>> -		return rc;
>>>> +		goto out_err;
>>>>
>>>> -	/* tpm_tis_send will either confirm the interrupt is working or it
>>>> -	 * will call disable_irq which undoes all of the above.
>>>> -	 */
>>>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>>>> -		rc = tpm_tis_write8(priv, original_int_vec,
>>>> -				TPM_INT_VECTOR(priv->locality));
>>>> -		if (rc < 0)
>>>> -			return rc;
>>>> +	tpm_msleep(1);
>>>>
>>>> -		return 1;
>>>> -	}
>>>> +	/* Verify receipt of the expected IRQ */
>>>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>>> +		goto out_err;
>>>> +
>>>> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
>>>>
>>>>  	return 0;
>>>> +
>>>> +out_err:
>
> Rename this as just 'err'.
>
>>>> +	disable_interrupts(chip);
>>>> +	tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality));
>>>> +
>>>> +	return rc;
>>>>  }
>>>>
>>>>  /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
>>>> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>>>  		if (irq) {
>>>>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
>>>>  						 irq);
>>>> -			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>>>> +			if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
>>>>  				dev_err(&chip->dev, FW_BUG
>>>>  					"TPM interrupt not working, polling instead\n");
>>>> -
>>>> -				disable_interrupts(chip);
>>>> -			}
>>>>  		} else {
>>>>  			tpm_tis_probe_irq(chip, intmask);
>>>>  		}
>>>> --
>>>> 2.36.0
>>>>
>>>
>>> For me this looks just code shuffling.
>>>
>>> I don't disagree but changing working code without actual semantical
>>> reasons neither makes sense.
>>>
>>> BR, Jarkko
>>>
>>
>> Well the semantical reason for this change is that the check for irq test completion
>> only has to be done once for the driver livetime. There is no point in doing it
>> over and over again for each transmission.
>> So the code is not simply shuffled around, it is shifted to a place where it is only
>> executed once.
>>
>> This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood
>> from your comments on the earlier versions of this patch set cleanups are also ok as
>> long as they are not intermixed with bugfixes.
>
> The patch does not do anything particulary useful IMHO. There's no
> stimulus to do this change.
>

Ok, I will drop this patch in the next version of this series then.

Regards,
Lino

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Michael Niewöhner]

Hi guys,


On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote:
> On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > 
> > > > > There is no need to check for the irq test completion at each data
> > > > > transmission during the driver livetime. Instead do the check only
> > > > > once at
> > > > > driver startup.
> > > > > 
> > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > ---
> > > > >  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------
> > > > > -
> > > > >  1 file changed, 22 insertions(+), 46 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > index bdfde1cd71fe..4c65718feb7d 100644
> > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip
> > > > > *chip)
> > > > >   * tpm.c can skip polling for the data to be available as the
> > > > > interrupt is
> > > > >   * waited for here
> > > > >   */
> > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf,
> > > > > size_t len)
> > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > >  {
> > > > >         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > >         int rc;
> > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip
> > > > > *chip, const u8 *buf, size_t len)
> > > > >         return rc;
> > > > >  }
> > > > > 
> > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > > -{
> > > > > -       int rc, irq;
> > > > > -       struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > -
> > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> > > > > -            test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > -               return tpm_tis_send_main(chip, buf, len);
> > > > > -
> > > > > -       /* Verify receipt of the expected IRQ */
> > > > > -       irq = priv->irq;
> > > > > -       priv->irq = 0;
> > > > > -       chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > > > > -       rc = tpm_tis_send_main(chip, buf, len);
> > > > > -       priv->irq = irq;
> > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > -               tpm_msleep(1);
> > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > -               disable_interrupts(chip);
> > > > > -       set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > -       return rc;
> > > > > -}
> > > > > -
> > > > >  struct tis_vendor_durations_override {
> > > > >         u32 did_vid;
> > > > >         struct tpm1_version version;
> > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct
> > > > > tpm_chip *chip, u32 intmask,
> > > > > 
> > > > >         rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> > > > >                            &original_int_vec);
> > > > > -       if (rc < 0)
> > > > > +       if (rc < 0) {
> > > > > +               disable_interrupts(chip);
> > > > >                 return rc;
> > > > > +       }
> > > > > 
> > > > >         rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > irq);
> > > > >         if (rc < 0)
> > > > > -               return rc;
> > > > > +               goto out_err;
> > > > > 
> > > > >         rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
> > > > > &int_status);
> > > > >         if (rc < 0)
> > > > > -               return rc;
> > > > > +               goto out_err;
> > > > > 
> > > > >         /* Clear all existing */
> > > > >         rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality),
> > > > > int_status);
> > > > >         if (rc < 0)
> > > > > -               return rc;
> > > > > +               goto out_err;
> > > > > 
> > > > >         /* Turn on */
> > > > >         rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> > > > >                              intmask | TPM_GLOBAL_INT_ENABLE);
> > > > >         if (rc < 0)
> > > > > -               return rc;
> > > > > +               goto out_err;
> > > > > 
> > > > >         clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > 
> > > > >         /* Generate an interrupt by having the core call through to
> > > > >          * tpm_tis_send
> > > > >          */
> > > > >         rc = tpm_tis_gen_interrupt(chip);
> > > > >         if (rc < 0)
> > > > > -               return rc;
> > > > > +               goto out_err;
> > > > > 
> > > > > -       /* tpm_tis_send will either confirm the interrupt is working
> > > > > or it
> > > > > -        * will call disable_irq which undoes all of the above.
> > > > > -        */
> > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > -               rc = tpm_tis_write8(priv, original_int_vec,
> > > > > -                               TPM_INT_VECTOR(priv->locality));
> > > > > -               if (rc < 0)
> > > > > -                       return rc;
> > > > > +       tpm_msleep(1);
> > > > > 
> > > > > -               return 1;
> > > > > -       }
> > > > > +       /* Verify receipt of the expected IRQ */
> > > > > +       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > +               goto out_err;
> > > > > +
> > > > > +       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > 
> > > > >         return 0;
> > > > > +
> > > > > +out_err:
> > 
> > Rename this as just 'err'.
> > 
> > > > > +       disable_interrupts(chip);
> > > > > +       tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv-
> > > > > >locality));
> > > > > +
> > > > > +       return rc;
> > > > >  }
> > > > > 
> > > > >  /* Try to find the IRQ the TPM is using. This is for legacy x86
> > > > > systems that
> > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev,
> > > > > struct tpm_tis_data *priv, int irq,
> > > > >                 if (irq) {
> > > > >                         tpm_tis_probe_irq_single(chip, intmask,
> > > > > IRQF_SHARED,
> > > > >                                                  irq);
> > > > > -                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > +                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > > > >                                 dev_err(&chip->dev, FW_BUG
> > > > >                                         "TPM interrupt not working,
> > > > > polling instead\n");
> > > > > -
> > > > > -                               disable_interrupts(chip);
> > > > > -                       }
> > > > >                 } else {
> > > > >                         tpm_tis_probe_irq(chip, intmask);
> > > > >                 }
> > > > > --
> > > > > 2.36.0
> > > > > 
> > > > 
> > > > For me this looks just code shuffling.
> > > > 
> > > > I don't disagree but changing working code without actual semantical
> > > > reasons neither makes sense.
> > > > 
> > > > BR, Jarkko
> > > > 
> > > 
> > > Well the semantical reason for this change is that the check for irq test
> > > completion
> > > only has to be done once for the driver livetime. There is no point in
> > > doing it
> > > over and over again for each transmission.
> > > So the code is not simply shuffled around, it is shifted to a place where
> > > it is only
> > > executed once.
> > > 
> > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I
> > > understood
> > > from your comments on the earlier versions of this patch set cleanups are
> > > also ok as
> > > long as they are not intermixed with bugfixes.
> > 
> > The patch does not do anything particulary useful IMHO. There's no
> > stimulus to do this change.
> > 

I don't agree. IMHO preventing useless actions (like checking the interrupt
again and again) *is* useful and I think it's reason enough.

> 
> Ok, I will drop this patch in the next version of this series then.
> 
> Regards,
> Lino
> 

Michael

Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

[Jarkko Sakkinen]

On Mon, 2022-05-16 at 21:52 +0200, Lino Sanfilippo wrote:
> Hi,
> 
> On 13.05.22 at 20:08, Jarkko Sakkinen wrote:
> > On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote:
> > > Hi,
> > > 
> > > On 11.05.22 at 13:22, Jarkko Sakkinen wrote:
> > > > On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
> > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > 
> > > > > Interrupt handling at least includes reading and writing the interrupt
> > > > > status register within the interrupt routine. Since accesses over the SPI
> > > > > bus are synchronized by a mutex, request a threaded interrupt handler to
> > > > > ensure a sleepable context during interrupt processing.
> > > > > 
> > > > > Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > 
> > > > When you state that it needs a sleepable context, you should bring a
> > > > context why it needs it. This not to disregard the code change overally but
> > > > you cannot make even the most obvious claim without backing data.
> > > > 
> > > 
> > > so what kind of backing data do you have in mind? Would it help to emphasize more
> > > that the irq handler is running in hard irq context in the current code and thus
> > > must not access registers over SPI since SPI uses a mutex (I consider it as basic
> > > knowledge that a mutex must not be taken in hard irq context)?
> > 
> > There's zero mention about specific lock you are talking about. Providing
> > the basic knowledge what you are trying to do is the whole point of the
> > commit message in the first place. I'd presume this patch is related to the
> > use bus_lock_mutex but it is fully ingored here.
> > 
> 
> Ok, understood. I will amend the commit message to make more clear that
> reading and writing registers from the interrupt handler results in
> a call to tpm_tis_spi_transfer() which uses the bus_lock_mutex of the
> spi device and thus requires a sleepable context.

Yeah, please be always as explicit as possible, so that it is impossible
to get it wrong. Then a reader of your patch saves time from seeking e.g.
the current name of the data structure. Just dumb things down as far as
you can.

Commit messages have a dual function:

1. They *lower* the barrier to look into a code change, which helps
the patches get any attention.
2. Proper commit messages save tons of time from the maintainer, when
you have revisit years old commits, e.g. when bisecting a bug.

Comparing to e.g. Github the key difference is this: in Github you have
commits and issues. In kernel commit is both issue and the commit bundled
together. Therefore, every commit also needs to have a "bug report"
included.

BR, Jarkko

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Jarkko Sakkinen]

On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote:
> Hi guys,
> 
> 
> On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote:
> > On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> > > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > > 
> > > > > > There is no need to check for the irq test completion at each data
> > > > > > transmission during the driver livetime. Instead do the check only
> > > > > > once at
> > > > > > driver startup.
> > > > > > 
> > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > > ---
> > > > > >  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------
> > > > > > -
> > > > > >  1 file changed, 22 insertions(+), 46 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > > index bdfde1cd71fe..4c65718feb7d 100644
> > > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip
> > > > > > *chip)
> > > > > >   * tpm.c can skip polling for the data to be available as the
> > > > > > interrupt is
> > > > > >   * waited for here
> > > > > >   */
> > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf,
> > > > > > size_t len)
> > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > > >  {
> > > > > >         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > >         int rc;
> > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip
> > > > > > *chip, const u8 *buf, size_t len)
> > > > > >         return rc;
> > > > > >  }
> > > > > > 
> > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > > > -{
> > > > > > -       int rc, irq;
> > > > > > -       struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > > -
> > > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> > > > > > -            test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > -               return tpm_tis_send_main(chip, buf, len);
> > > > > > -
> > > > > > -       /* Verify receipt of the expected IRQ */
> > > > > > -       irq = priv->irq;
> > > > > > -       priv->irq = 0;
> > > > > > -       chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > > > > > -       rc = tpm_tis_send_main(chip, buf, len);
> > > > > > -       priv->irq = irq;
> > > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > -               tpm_msleep(1);
> > > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > -               disable_interrupts(chip);
> > > > > > -       set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > -       return rc;
> > > > > > -}
> > > > > > -
> > > > > >  struct tis_vendor_durations_override {
> > > > > >         u32 did_vid;
> > > > > >         struct tpm1_version version;
> > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct
> > > > > > tpm_chip *chip, u32 intmask,
> > > > > > 
> > > > > >         rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > >                            &original_int_vec);
> > > > > > -       if (rc < 0)
> > > > > > +       if (rc < 0) {
> > > > > > +               disable_interrupts(chip);
> > > > > >                 return rc;
> > > > > > +       }
> > > > > > 
> > > > > >         rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > > irq);
> > > > > >         if (rc < 0)
> > > > > > -               return rc;
> > > > > > +               goto out_err;
> > > > > > 
> > > > > >         rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > &int_status);
> > > > > >         if (rc < 0)
> > > > > > -               return rc;
> > > > > > +               goto out_err;
> > > > > > 
> > > > > >         /* Clear all existing */
> > > > > >         rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > int_status);
> > > > > >         if (rc < 0)
> > > > > > -               return rc;
> > > > > > +               goto out_err;
> > > > > > 
> > > > > >         /* Turn on */
> > > > > >         rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> > > > > >                              intmask | TPM_GLOBAL_INT_ENABLE);
> > > > > >         if (rc < 0)
> > > > > > -               return rc;
> > > > > > +               goto out_err;
> > > > > > 
> > > > > >         clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > 
> > > > > >         /* Generate an interrupt by having the core call through to
> > > > > >          * tpm_tis_send
> > > > > >          */
> > > > > >         rc = tpm_tis_gen_interrupt(chip);
> > > > > >         if (rc < 0)
> > > > > > -               return rc;
> > > > > > +               goto out_err;
> > > > > > 
> > > > > > -       /* tpm_tis_send will either confirm the interrupt is working
> > > > > > or it
> > > > > > -        * will call disable_irq which undoes all of the above.
> > > > > > -        */
> > > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > -               rc = tpm_tis_write8(priv, original_int_vec,
> > > > > > -                               TPM_INT_VECTOR(priv->locality));
> > > > > > -               if (rc < 0)
> > > > > > -                       return rc;
> > > > > > +       tpm_msleep(1);
> > > > > > 
> > > > > > -               return 1;
> > > > > > -       }
> > > > > > +       /* Verify receipt of the expected IRQ */
> > > > > > +       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > +               goto out_err;
> > > > > > +
> > > > > > +       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > 
> > > > > >         return 0;
> > > > > > +
> > > > > > +out_err:
> > > 
> > > Rename this as just 'err'.
> > > 
> > > > > > +       disable_interrupts(chip);
> > > > > > +       tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv-
> > > > > > > locality));
> > > > > > +
> > > > > > +       return rc;
> > > > > >  }
> > > > > > 
> > > > > >  /* Try to find the IRQ the TPM is using. This is for legacy x86
> > > > > > systems that
> > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev,
> > > > > > struct tpm_tis_data *priv, int irq,
> > > > > >                 if (irq) {
> > > > > >                         tpm_tis_probe_irq_single(chip, intmask,
> > > > > > IRQF_SHARED,
> > > > > >                                                  irq);
> > > > > > -                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > +                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > > > > >                                 dev_err(&chip->dev, FW_BUG
> > > > > >                                         "TPM interrupt not working,
> > > > > > polling instead\n");
> > > > > > -
> > > > > > -                               disable_interrupts(chip);
> > > > > > -                       }
> > > > > >                 } else {
> > > > > >                         tpm_tis_probe_irq(chip, intmask);
> > > > > >                 }
> > > > > > --
> > > > > > 2.36.0
> > > > > > 
> > > > > 
> > > > > For me this looks just code shuffling.
> > > > > 
> > > > > I don't disagree but changing working code without actual semantical
> > > > > reasons neither makes sense.
> > > > > 
> > > > > BR, Jarkko
> > > > > 
> > > > 
> > > > Well the semantical reason for this change is that the check for irq test
> > > > completion
> > > > only has to be done once for the driver livetime. There is no point in
> > > > doing it
> > > > over and over again for each transmission.
> > > > So the code is not simply shuffled around, it is shifted to a place where
> > > > it is only
> > > > executed once.
> > > > 
> > > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I
> > > > understood
> > > > from your comments on the earlier versions of this patch set cleanups are
> > > > also ok as
> > > > long as they are not intermixed with bugfixes.
> > > 
> > > The patch does not do anything particulary useful IMHO. There's no
> > > stimulus to do this change.
> > > 
> 
> I don't agree. IMHO preventing useless actions (like checking the interrupt
> again and again) *is* useful and I think it's reason enough.

Show me the test data to back this up.

BR, Jarkko

Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

[Michael Niewöhner]

On Wed, 2022-05-18 at 04:26 +0300, Jarkko Sakkinen wrote:
> On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote:
> > Hi guys,
> > 
> > 
> > On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote:
> > > On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> > > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> > > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> > > > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > > > 
> > > > > > > There is no need to check for the irq test completion at each data
> > > > > > > transmission during the driver livetime. Instead do the check only
> > > > > > > once at
> > > > > > > driver startup.
> > > > > > > 
> > > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > > > > > > ---
> > > > > > >  drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++-----------------
> > > > > > > ----
> > > > > > > -
> > > > > > >  1 file changed, 22 insertions(+), 46 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > > > index bdfde1cd71fe..4c65718feb7d 100644
> > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip
> > > > > > > *chip)
> > > > > > >   * tpm.c can skip polling for the data to be available as the
> > > > > > > interrupt is
> > > > > > >   * waited for here
> > > > > > >   */
> > > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8
> > > > > > > *buf,
> > > > > > > size_t len)
> > > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t
> > > > > > > len)
> > > > > > >  {
> > > > > > >         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > > >         int rc;
> > > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip
> > > > > > > *chip, const u8 *buf, size_t len)
> > > > > > >         return rc;
> > > > > > >  }
> > > > > > > 
> > > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t
> > > > > > > len)
> > > > > > > -{
> > > > > > > -       int rc, irq;
> > > > > > > -       struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > > > -
> > > > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> > > > > > > -            test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > -               return tpm_tis_send_main(chip, buf, len);
> > > > > > > -
> > > > > > > -       /* Verify receipt of the expected IRQ */
> > > > > > > -       irq = priv->irq;
> > > > > > > -       priv->irq = 0;
> > > > > > > -       chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > > > > > > -       rc = tpm_tis_send_main(chip, buf, len);
> > > > > > > -       priv->irq = irq;
> > > > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > -               tpm_msleep(1);
> > > > > > > -       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > -               disable_interrupts(chip);
> > > > > > > -       set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > > -       return rc;
> > > > > > > -}
> > > > > > > -
> > > > > > >  struct tis_vendor_durations_override {
> > > > > > >         u32 did_vid;
> > > > > > >         struct tpm1_version version;
> > > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct
> > > > > > > tpm_chip *chip, u32 intmask,
> > > > > > > 
> > > > > > >         rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > > >                            &original_int_vec);
> > > > > > > -       if (rc < 0)
> > > > > > > +       if (rc < 0) {
> > > > > > > +               disable_interrupts(chip);
> > > > > > >                 return rc;
> > > > > > > +       }
> > > > > > > 
> > > > > > >         rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > > > irq);
> > > > > > >         if (rc < 0)
> > > > > > > -               return rc;
> > > > > > > +               goto out_err;
> > > > > > > 
> > > > > > >         rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > > &int_status);
> > > > > > >         if (rc < 0)
> > > > > > > -               return rc;
> > > > > > > +               goto out_err;
> > > > > > > 
> > > > > > >         /* Clear all existing */
> > > > > > >         rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > > int_status);
> > > > > > >         if (rc < 0)
> > > > > > > -               return rc;
> > > > > > > +               goto out_err;
> > > > > > > 
> > > > > > >         /* Turn on */
> > > > > > >         rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> > > > > > >                              intmask | TPM_GLOBAL_INT_ENABLE);
> > > > > > >         if (rc < 0)
> > > > > > > -               return rc;
> > > > > > > +               goto out_err;
> > > > > > > 
> > > > > > >         clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > > -       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > > 
> > > > > > >         /* Generate an interrupt by having the core call through
> > > > > > > to
> > > > > > >          * tpm_tis_send
> > > > > > >          */
> > > > > > >         rc = tpm_tis_gen_interrupt(chip);
> > > > > > >         if (rc < 0)
> > > > > > > -               return rc;
> > > > > > > +               goto out_err;
> > > > > > > 
> > > > > > > -       /* tpm_tis_send will either confirm the interrupt is
> > > > > > > working
> > > > > > > or it
> > > > > > > -        * will call disable_irq which undoes all of the above.
> > > > > > > -        */
> > > > > > > -       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > > -               rc = tpm_tis_write8(priv, original_int_vec,
> > > > > > > -                               TPM_INT_VECTOR(priv->locality));
> > > > > > > -               if (rc < 0)
> > > > > > > -                       return rc;
> > > > > > > +       tpm_msleep(1);
> > > > > > > 
> > > > > > > -               return 1;
> > > > > > > -       }
> > > > > > > +       /* Verify receipt of the expected IRQ */
> > > > > > > +       if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > +               goto out_err;
> > > > > > > +
> > > > > > > +       chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > > 
> > > > > > >         return 0;
> > > > > > > +
> > > > > > > +out_err:
> > > > 
> > > > Rename this as just 'err'.
> > > > 
> > > > > > > +       disable_interrupts(chip);
> > > > > > > +       tpm_tis_write8(priv, original_int_vec,
> > > > > > > TPM_INT_VECTOR(priv-
> > > > > > > > locality));
> > > > > > > +
> > > > > > > +       return rc;
> > > > > > >  }
> > > > > > > 
> > > > > > >  /* Try to find the IRQ the TPM is using. This is for legacy x86
> > > > > > > systems that
> > > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev,
> > > > > > > struct tpm_tis_data *priv, int irq,
> > > > > > >                 if (irq) {
> > > > > > >                         tpm_tis_probe_irq_single(chip, intmask,
> > > > > > > IRQF_SHARED,
> > > > > > >                                                  irq);
> > > > > > > -                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > > +                       if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > > > > > >                                 dev_err(&chip->dev, FW_BUG
> > > > > > >                                         "TPM interrupt not
> > > > > > > working,
> > > > > > > polling instead\n");
> > > > > > > -
> > > > > > > -                               disable_interrupts(chip);
> > > > > > > -                       }
> > > > > > >                 } else {
> > > > > > >                         tpm_tis_probe_irq(chip, intmask);
> > > > > > >                 }
> > > > > > > --
> > > > > > > 2.36.0
> > > > > > > 
> > > > > > 
> > > > > > For me this looks just code shuffling.
> > > > > > 
> > > > > > I don't disagree but changing working code without actual semantical
> > > > > > reasons neither makes sense.
> > > > > > 
> > > > > > BR, Jarkko
> > > > > > 
> > > > > 
> > > > > Well the semantical reason for this change is that the check for irq
> > > > > test
> > > > > completion
> > > > > only has to be done once for the driver livetime. There is no point in
> > > > > doing it
> > > > > over and over again for each transmission.
> > > > > So the code is not simply shuffled around, it is shifted to a place
> > > > > where
> > > > > it is only
> > > > > executed once.
> > > > > 
> > > > > This is not a bugfix but it is clearly an improvement/cleanup. As far
> > > > > as I
> > > > > understood
> > > > > from your comments on the earlier versions of this patch set cleanups
> > > > > are
> > > > > also ok as
> > > > > long as they are not intermixed with bugfixes.
> > > > 
> > > > The patch does not do anything particulary useful IMHO. There's no
> > > > stimulus to do this change.
> > > > 
> > 
> > I don't agree. IMHO preventing useless actions (like checking the interrupt
> > again and again) *is* useful and I think it's reason enough.
> 
> Show me the test data to back this up.

Why do you need test data as an argument for not doing useless actions? o.O

> 
> BR, Jarkko