[PATCH 0/4] Fixes for TPM interrupt handling

[PATCH 0/4] Fixes for TPM interrupt handling

[Lino Sanfilippo]

This series fixes issues around the TPM driver interrupt handling. These
issues prevent TPM chips like the SLB 9670 to work over SPI. 

PATCH 1: The SPI implementation of the functions to read/write to/from
registers uses mutexes and thus require a sleepable context. For this
reason request a threaded interrupt handler.

PATCH 2: Simplify locality handling by taking the driver locality (0) at
driver startup and releasing it at driver shutdown. This also fixes a
warning at driver startup that is caused by a register access without prior
locality request.

PATCH 3: Fix and simplify the test for working interrupts.

PATCH 4: Only set the interrupts which are reported as being available.

With these patches applied the SLB 9670 works as expected: interrupts are
detected and the transmit/receive operations are controlled by interrupt
handling.

The patches are based on Linux mainline V5.11


Lino Sanfilippo (4):
  tpm: Use a threaded interrupt handler
  tpm: get locality before writing to TPM chip
  tpm: Fix test for interrupts
  tpm: Only enable supported irqs

 drivers/char/tpm/tpm-chip.c     |  10 ----
 drivers/char/tpm/tpm_tis_core.c | 127 +++++++++++++++++++---------------------
 drivers/char/tpm/tpm_tis_core.h |   2 +-
 include/linux/tpm.h             |   2 +-
 4 files changed, 62 insertions(+), 79 deletions(-)

-- 
2.7.4

[PATCH 1/4] tpm: Use a threaded interrupt handler

[Lino Sanfilippo]

Writing and reading registers over SPI requires a sleepable context, since
a mutex is used in the concerning functions. So instead of installing a
generic interrupt handler use one for threaded interrupts. This provides
the same functionality as before but also allows SPI functions to be
called.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/char/tpm/tpm_tis_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 92c51c6..2c956a1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -771,8 +771,10 @@ 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 (devm_request_threaded_irq(chip->dev.parent, irq, NULL,
+				      tis_int_handler, IRQF_ONESHOT | flags,
+				      dev_name(&chip->dev), chip) != 0) {
 		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
 			 irq);
 		return -1;
-- 
2.7.4

[PATCH 2/4] tpm: get locality before writing to TPM chip

[Lino Sanfilippo]

When running with TPM chips like the SLB 9670VQ2.0 the TPM drivers probe
function prints the following warning:

TPM returned invalid status
WARNING: CPU: 3 PID: 1416 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
Modules linked in: tpm_tis_spi tpm_tis_core tpm spi_bcm2835 sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched raspberrypi_hwmon vc4 cec drm_kms_helper bcm2835_codec(C) drm v4l2_mem2mem drm_panel_orientation_quirks bcm2835_v4l2(C) bcm2835_isp(C) videobuf2_dma_contig snd_soc_core bcm2835_mmal_vchiq(C) snd_bcm2835(C) videobuf2_vmalloc snd_compress videobuf2_memops videobuf2_v4l2 snd_pcm_dmaengine videobuf2_common snd_pcm snd_timer videodev vc_sm_cma(C) mc snd syscopyarea sysfillrect sysimgblt fb_sys_fops backlight rpivid_mem uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
CPU: 3 PID: 1416 Comm: kworker/u8:2 Tainted: G        WC        5.10.5-LS-HOME+ #1
Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
Workqueue: events_unbound async_run_entry_fn
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
sp : ffffffc0120a38f0
x29: ffffffc0120a38f0 x28: ffffffc011238000
x27: 0000000000000016 x26: 000000000000017a
x25: 0000000000000014 x24: ffffff80497f9000
x23: 0000000000000000 x22: 0000000000000016
x21: ffffff8045680080 x20: 0000000000000000
x19: ffffffc011238000 x18: ffffffc011238948
x17: 0000000000000000 x16: 0000000000000000
x15: ffffffc011437c50 x14: ffffffffffffffff
x13: ffffffc0114378a6 x12: ffffffffffffffff
x11: ffffff807fbde5f0 x10: 0000000000000007
x9 : ffffffc0120a38f0 x8 : 6174732064696c61
x7 : 0000000000000000 x6 : ffffffc011239000
x5 : ffffff807fbc78e0 x4 : 0000000000000000
x3 : 0000000000000027 x2 : 0000000000000000
x1 : 670c5477f4483200 x0 : 0000000000000000
Call trace:
tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
tpm_tis_send+0x4c/0x128 [tpm_tis_core]
tpm_transmit+0xd0/0x350 [tpm]
tpm_transmit_cmd+0x3c/0xc0 [tpm]
tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
tpm_tis_probe_irq_single+0x17c/0x388 [tpm_tis_core]
tpm_tis_core_init+0x2e0/0x4f8 [tpm_tis_core]
tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
spi_drv_probe+0x84/0xf0
really_probe+0x118/0x420
driver_probe_device+0x5c/0xc0
__driver_attach_async_helper+0x64/0x68
async_run_entry_fn+0x48/0x150
process_one_work+0x15c/0x4d0
worker_thread+0x50/0x490
kthread+0x118/0x150
ret_from_fork+0x10/0x18
---[ end trace 209c86815cd15eab ]---

The reason is the attempt to read TPM 2.0 properties from the hardware
while the concerning locality has not been maintained before.

To avoid this issue and the need to request and release the locality
multiple times during driver initialization, and since the driver only
supports the legacy locality 0, request it once at driver startup
and only release it at driver shutdown.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/char/tpm/tpm-chip.c     | 10 ----------
 drivers/char/tpm/tpm_tis_core.c | 16 +++++++++-------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb..5eb63f2 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -103,17 +103,8 @@ int tpm_chip_start(struct tpm_chip *chip)
 
 	tpm_clk_enable(chip);
 
-	if (chip->locality == -1) {
-		ret = tpm_request_locality(chip);
-		if (ret) {
-			tpm_clk_disable(chip);
-			return ret;
-		}
-	}
-
 	ret = tpm_cmd_ready(chip);
 	if (ret) {
-		tpm_relinquish_locality(chip);
 		tpm_clk_disable(chip);
 		return ret;
 	}
@@ -133,7 +124,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
 void tpm_chip_stop(struct tpm_chip *chip)
 {
 	tpm_go_idle(chip);
-	tpm_relinquish_locality(chip);
 	tpm_clk_disable(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 2c956a1..e2f8585 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -670,9 +670,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;
@@ -691,7 +688,6 @@ static int probe_itpm(struct tpm_chip *chip)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality);
 
 	return rc;
 }
@@ -870,6 +866,7 @@ 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)
@@ -997,6 +994,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
+	rc = request_locality(chip, 0);
+	if (rc)
+		goto out_err;
+
+	rc = tpm_chip_start(chip);
+	if (rc)
+		goto out_err;
+
 	/* 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)
@@ -1007,9 +1012,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
 
-	rc = tpm_chip_start(chip);
-	if (rc)
-		goto out_err;
 	rc = tpm2_probe(chip);
 	tpm_chip_stop(chip);
 	if (rc)
-- 
2.7.4

[PATCH 3/4] tpm: Fix test for interrupts

[Lino Sanfilippo]

The current test for functional interrupts is broken in multiple ways:
1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
executed.
2. The test includes the check for two variables and the check is done for
each transmission which is unnecessarily complicated.
3. Part of the test is setting a bool variable in an interrupt handler and
then check the value in the main thread. However there is nothing that
guarantees the visibility of the value set in the interrupt handler for
any other thread. Some kind of synchronization primitive is required for
this purpose.

Fix all these issues by a reimplementation:
Instead of doing the test in tpm_tis_send() which is called for each
transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
use proper accessor functions like get_bit()/set_bit() which include the
required synchronization primitives to guarantee visibility between the
interrupt handler and threads.
Finally remove one function which is no longer needed.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/char/tpm/tpm_tis_core.c | 37 ++++++++++---------------------------
 drivers/char/tpm/tpm_tis_core.h |  1 -
 include/linux/tpm.h             |  2 +-
 3 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e2f8585..d98f8e8 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -464,7 +464,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;
@@ -497,29 +497,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) || priv->irq_tested)
-		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 (!priv->irq_tested)
-		tpm_msleep(1);
-	if (!priv->irq_tested)
-		disable_interrupts(chip);
-	priv->irq_tested = true;
-	return rc;
-}
-
 struct tis_vendor_durations_override {
 	u32 did_vid;
 	struct tpm1_version version;
@@ -721,7 +698,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	if (interrupt == 0)
 		return IRQ_NONE;
 
-	priv->irq_tested = true;
+	set_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
+
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&priv->read_queue);
 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -801,7 +779,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_CHIP_FLAG_IRQ, &chip->flags);
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
@@ -810,6 +788,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
+	tpm_msleep(1);
+
+	if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags))
+		disable_interrupts(chip);
+
 	/* tpm_tis_send will either confirm the interrupt is working or it
 	 * will call disable_irq which undoes all of the above.
 	 */
@@ -1080,7 +1063,7 @@ 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 (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a..dc5f92b 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,7 +89,6 @@ struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
-	bool irq_tested;
 	unsigned int flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 8f4ff39..3e77c9d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -126,7 +126,7 @@ struct tpm_chip {
 	struct tpm_chip_seqops bin_log_seqops;
 	struct tpm_chip_seqops ascii_log_seqops;
 
-	unsigned int flags;
+	unsigned long flags;
 
 	int dev_num;		/* /dev/tpm# */
 	unsigned long is_open;	/* only one allowed */
-- 
2.7.4

[PATCH 4/4] tpm: Only enable supported irqs

[Lino Sanfilippo]

Do not set interrupts which are not supported by the hardware. Instead use
the information from the capability query and activate only the reported
interrupts.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/char/tpm/tpm_tis_core.c | 68 ++++++++++++++++++++++-------------------
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d98f8e8..42f8cc3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -985,13 +985,47 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	if (rc)
 		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);
 
@@ -1020,32 +1054,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);
@@ -1113,9 +1121,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 dc5f92b..8ff62213 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,6 +89,7 @@ struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
+	unsigned int supported_irqs;
 	unsigned int flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
-- 
2.7.4