[PATCH v2 0/4] Fixes for TPM interrupt handling

[PATCH v2 0/4] Fixes for TPM interrupt handling

[Lino Sanfilippo]

This series enables interrupts for TPM. For this some obstacles had to be
removed first, like the interrupt handler running in interrupt context and
thus not allowing to access registers over SPI. Also the locality handling
has been simplified to make a complicated synchronization between threads
and irq handler unnecessary. As a side effect of this simplification a bug
is fixed in which a TMP command is issued without a claimed locality in
case of TPM 2.
Another fix concerns the interrupt test which currently is broken.
Finally the results of the capability query at startup is used to only set
the interrupts which are actually supported by the hardware.

These patches are based on Linux mainline V5.12 and tested on a SLB 9670
which uses TPM 2 via SPI.

Of course any further testing is highly appreciated.

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 bug
in case of TMP 2.

PATCH 3: Fix and simplify the test for interrupts.

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

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

Lino Sanfilippo (4):
  tpm: Use a threaded interrupt handler
  tpm: Simplify locality handling
  tpm: Fix test for interrupts
  tpm: Only enable supported irqs

 drivers/char/tpm/tpm-chip.c     |  40 ----------
 drivers/char/tpm/tpm_tis_core.c | 170 +++++++++++++++++-----------------------
 drivers/char/tpm/tpm_tis_core.h |   2 +-
 include/linux/tpm.h             |   5 +-
 4 files changed, 72 insertions(+), 145 deletions(-)

-- 
2.7.4

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

[Lino Sanfilippo]

Interrupt handling at least includes reading and writing the interrupt
status register from the interrupt routine. However over SPI those accesses
require a sleepable context, since a mutex is used in the concerning
functions.
For this reason request a threaded interrupt handler which is running in
(sleepable) process context.

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 e7d1eab..0959559 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -781,8 +781,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 v2 2/4] tpm: Simplify locality handling

[Lino Sanfilippo]

Currently the TPM (default) locality is claimed and released for each
access to the TPM registers which require a claimed locality. This results
in locality claim/release combos at various code places. For interrupt
handling we also need such a combo in the interrupt handler (for clearing
the interrupts) which makes the locality handling even more complicated
since now we also have to synchronize concurrent accesses in process and in
interrupt context.

Since currently the driver uses only one locality anyway, avoid the
increasing complexity by claiming it once at driver startup and only
releasing it at driver shutdown.

Due to the simplifications the functions tpm_request_locality() and
tpm_relinquish_locality() are not needed any more an can be removed.

As a side-effect these modifications fix a bug which results in the
following warning when using TPM 2:

TPM returned invalid status
WARNING: CPU: 0 PID: 874 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 sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G        WC        5.11.0-rc2-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 : ffffffc0127e38f0
x29: ffffffc0127e38f0 x28: ffffffc011238000
x27: 0000000000000016 x26: 000000000000017a
x25: 0000000000000014 x24: ffffff8047f60000
x23: 0000000000000000 x22: 0000000000000016
x21: ffffff8044e8a480 x20: 0000000000000000
x19: ffffffc011238000 x18: ffffffc011238948
x17: 0000000000000000 x16: 0000000000000000
x15: ffffffc01141be81 x14: ffffffffffffffff
x13: ffffffc01141be7e x12: ffffffffffffffff
x11: ffffff807fb797f0 x10: 00000000000019b0
x9 : ffffffc0127e38f0 x8 : 766e692064656e72
x7 : 0000000000000000 x6 : ffffffc011239000
x5 : ffffff807fb628b8 x4 : 0000000000000000
x3 : 0000000000000027 x2 : 0000000000000000
x1 : 6818b6f22fdef800 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/0xe0 [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+0x198/0x364 [tpm_tis_core]
tpm_tis_core_init+0x304/0x520 [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_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/0x1c

The reason for this issue is that in case of TPM 2 function
tpm2_get_timeouts() which executes a TPM command is called without a
claimed locality. Since with this patch the locality is taken once at
driver startup and never released before shutdown the issue does not occur
any more.

Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/char/tpm/tpm-chip.c     | 40 ----------------------------------------
 drivers/char/tpm/tpm_tis_core.c | 35 ++++++++++-------------------------
 include/linux/tpm.h             |  3 ---
 3 files changed, 10 insertions(+), 68 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb..a09b652 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -32,35 +32,6 @@ struct class *tpm_class;
 struct class *tpmrm_class;
 dev_t tpm_devt;
 
-static int tpm_request_locality(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (!chip->ops->request_locality)
-		return 0;
-
-	rc = chip->ops->request_locality(chip, 0);
-	if (rc < 0)
-		return rc;
-
-	chip->locality = rc;
-	return 0;
-}
-
-static void tpm_relinquish_locality(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (!chip->ops->relinquish_locality)
-		return;
-
-	rc = chip->ops->relinquish_locality(chip, chip->locality);
-	if (rc)
-		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
-
-	chip->locality = -1;
-}
-
 static int tpm_cmd_ready(struct tpm_chip *chip)
 {
 	if (!chip->ops->cmd_ready)
@@ -103,17 +74,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 +95,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);
@@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 		goto out;
 	}
 
-	chip->locality = -1;
 	return chip;
 
 out:
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 0959559..a95daf8 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;
 }
@@ -751,22 +747,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
 	const char *desc = "attempting to generate an interrupt";
 	u32 cap2;
 	cap_t cap;
-	int ret;
 
 	/* TPM 2.0 */
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
 
 	/* TPM 1.2 */
-	ret = request_locality(chip, 0);
-	if (ret < 0)
-		return ret;
-
-	ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
-
-	release_locality(chip, 0);
-
-	return ret;
+	return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
 }
 
 /* Register the IRQ and issue a command that will cause an interrupt. If an
@@ -880,6 +867,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)
@@ -1007,6 +995,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)
@@ -1017,9 +1013,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)
@@ -1080,15 +1073,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;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 8f4ff39..55debe6 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -160,9 +160,6 @@ struct tpm_chip {
 	u32 last_cc;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
-
-	/* active locality */
-	int locality;
 };
 
 #define TPM_HEADER_SIZE		10
-- 
2.7.4

[PATCH v2 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 | 61 +++++++++++++----------------------------
 drivers/char/tpm/tpm_tis_core.h |  1 -
 include/linux/tpm.h             |  2 +-
 3 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index a95daf8..e8ab218 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)
@@ -778,45 +756,44 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	}
 	priv->irq = irq;
 
+	clear_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
+
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
 			   &original_int_vec);
 	if (rc < 0)
-		return rc;
+		goto out;
 
 	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
 	if (rc < 0)
-		return rc;
+		goto out;
 
 	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
 	if (rc < 0)
-		return rc;
+		goto out;
 
 	/* Clear all existing */
 	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
 	if (rc < 0)
-		return rc;
+		goto out;
 
 	/* Turn on */
 	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
 			     intmask | TPM_GLOBAL_INT_ENABLE);
 	if (rc < 0)
-		return rc;
-
-	priv->irq_tested = false;
+		goto out;
 
-	/* Generate an interrupt by having the core call through to
-	 * tpm_tis_send
-	 */
+	/* Generate an interrupt by transmitting a command to the chip */
 	rc = tpm_tis_gen_interrupt(chip);
 	if (rc < 0)
-		return rc;
+		goto out;
+
+	tpm_msleep(1);
+out:
+	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.
-	 */
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
 		rc = tpm_tis_write8(priv, original_int_vec,
-				TPM_INT_VECTOR(priv->locality));
+				    TPM_INT_VECTOR(priv->locality));
 		if (rc < 0)
 			return rc;
 
@@ -1083,7 +1060,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 55debe6..e9882acf 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 v2 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 e8ab218..7b00161 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -980,13 +980,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);
 
@@ -1015,32 +1049,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);
@@ -1110,9 +1118,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 |= chip->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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3053 bytes --]

Hi Lino,

I love your patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on kees/for-next/pstore linus/master v5.12 next-20210423]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lino-Sanfilippo/Fixes-for-TPM-interrupt-handling/20210426-075042
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git e2cb6b891ad2b8caa9131e3be70f45243df82a80
config: x86_64-randconfig-a001-20210426 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d941863de2becb3d8d2e00676fc7125974934c7f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/1b37b97e7f8601b56f1e8aa069aec29c7d80d175
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Lino-Sanfilippo/Fixes-for-TPM-interrupt-handling/20210426-075042
        git checkout 1b37b97e7f8601b56f1e8aa069aec29c7d80d175
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/char/tpm/tpm_tis_core.c:1077:19: error: no member named 'supported_irqs' in 'struct tpm_chip'
           intmask |= chip->supported_irqs | TPM_GLOBAL_INT_ENABLE;
                      ~~~~  ^
   1 error generated.


vim +1077 drivers/char/tpm/tpm_tis_core.c

  1055	
  1056	#ifdef CONFIG_PM_SLEEP
  1057	static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
  1058	{
  1059		struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
  1060		u32 intmask;
  1061		int rc;
  1062	
  1063		if (chip->ops->clk_enable != NULL)
  1064			chip->ops->clk_enable(chip, true);
  1065	
  1066		/* reenable interrupts that device may have lost or
  1067		 * BIOS/firmware may have disabled
  1068		 */
  1069		rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), priv->irq);
  1070		if (rc < 0)
  1071			goto out;
  1072	
  1073		rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
  1074		if (rc < 0)
  1075			goto out;
  1076	
> 1077		intmask |= chip->supported_irqs | TPM_GLOBAL_INT_ENABLE;
  1078	
  1079		tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
  1080	
  1081	out:
  1082		if (chip->ops->clk_enable != NULL)
  1083			chip->ops->clk_enable(chip, false);
  1084	
  1085		return;
  1086	}
  1087	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37909 bytes --]

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

[Stefan Berger]

On 4/25/21 7:47 PM, Lino Sanfilippo wrote:
> 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>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

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

[Stefan Berger]

On 4/25/21 7:47 PM, Lino Sanfilippo wrote:
> Interrupt handling at least includes reading and writing the interrupt
> status register from the interrupt routine. However over SPI those accesses
> require a sleepable context, since a mutex is used in the concerning
> functions.
> For this reason request a threaded interrupt handler which is running in
> (sleepable) process context.
>
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

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

[Stefan Berger]

On 4/25/21 7:47 PM, Lino Sanfilippo wrote:
> 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 | 61 +++++++++++++----------------------------
>   drivers/char/tpm/tpm_tis_core.h |  1 -
>   include/linux/tpm.h             |  2 +-
>   3 files changed, 20 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index a95daf8..e8ab218 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)
> @@ -778,45 +756,44 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>   	}
>   	priv->irq = irq;
>   
> +	clear_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
> +
>   	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>   			   &original_int_vec);
>   	if (rc < 0)
> -		return rc;
> +		goto out;
>   
>   	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>   	if (rc < 0)
> -		return rc;
> +		goto out;
>   
>   	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>   	if (rc < 0)
> -		return rc;
> +		goto out;
>   
>   	/* Clear all existing */
>   	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>   	if (rc < 0)
> -		return rc;
> +		goto out;
>   
>   	/* Turn on */
>   	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>   			     intmask | TPM_GLOBAL_INT_ENABLE);
>   	if (rc < 0)
> -		return rc;
> -
> -	priv->irq_tested = false;
> +		goto out;
>   
> -	/* Generate an interrupt by having the core call through to
> -	 * tpm_tis_send
> -	 */
> +	/* Generate an interrupt by transmitting a command to the chip */
>   	rc = tpm_tis_gen_interrupt(chip);
>   	if (rc < 0)
> -		return rc;
> +		goto out;
> +
> +	tpm_msleep(1);
> +out:
> +	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.
> -	 */
> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>   		rc = tpm_tis_write8(priv, original_int_vec,
> -				TPM_INT_VECTOR(priv->locality));
> +				    TPM_INT_VECTOR(priv->locality));
>   		if (rc < 0)
>   			return rc;
>   
> @@ -1083,7 +1060,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 55debe6..e9882acf 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;


This doesn't seem to be necessary.

The rest looks good to me. I remember that last time I had tried to 
activate it some laptop didn't cooperate and we ended up reverting some 
code, but maybe your changes fixed all of that now. Though you may want 
to 'prepare for unforseen consequences' :-).


>   
>   	int dev_num;		/* /dev/tpm# */
>   	unsigned long is_open;	/* only one allowed */

Re: [PATCH v2 2/4] tpm: Simplify locality handling

[Jarkko Sakkinen]

On Mon, Apr 26, 2021 at 01:47:18AM +0200, Lino Sanfilippo wrote:
> Currently the TPM (default) locality is claimed and released for each
> access to the TPM registers which require a claimed locality. This results
> in locality claim/release combos at various code places. For interrupt
> handling we also need such a combo in the interrupt handler (for clearing
> the interrupts) which makes the locality handling even more complicated
> since now we also have to synchronize concurrent accesses in process and in
> interrupt context.
> 
> Since currently the driver uses only one locality anyway, avoid the
> increasing complexity by claiming it once at driver startup and only
> releasing it at driver shutdown.
> 
> Due to the simplifications the functions tpm_request_locality() and
> tpm_relinquish_locality() are not needed any more an can be removed.
> 
> As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:
> 
> TPM returned invalid status
> WARNING: CPU: 0 PID: 874 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 sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G        WC        5.11.0-rc2-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 : ffffffc0127e38f0
> x29: ffffffc0127e38f0 x28: ffffffc011238000
> x27: 0000000000000016 x26: 000000000000017a
> x25: 0000000000000014 x24: ffffff8047f60000
> x23: 0000000000000000 x22: 0000000000000016
> x21: ffffff8044e8a480 x20: 0000000000000000
> x19: ffffffc011238000 x18: ffffffc011238948
> x17: 0000000000000000 x16: 0000000000000000
> x15: ffffffc01141be81 x14: ffffffffffffffff
> x13: ffffffc01141be7e x12: ffffffffffffffff
> x11: ffffff807fb797f0 x10: 00000000000019b0
> x9 : ffffffc0127e38f0 x8 : 766e692064656e72
> x7 : 0000000000000000 x6 : ffffffc011239000
> x5 : ffffff807fb628b8 x4 : 0000000000000000
> x3 : 0000000000000027 x2 : 0000000000000000
> x1 : 6818b6f22fdef800 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/0xe0 [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+0x198/0x364 [tpm_tis_core]
> tpm_tis_core_init+0x304/0x520 [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_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/0x1c
> 
> The reason for this issue is that in case of TPM 2 function
> tpm2_get_timeouts() which executes a TPM command is called without a
> claimed locality. Since with this patch the locality is taken once at
> driver startup and never released before shutdown the issue does not occur
> any more.
> 
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Hi, what hardware? Just if I could find something comparable lying around.

> ---
>  drivers/char/tpm/tpm-chip.c     | 40 ----------------------------------------
>  drivers/char/tpm/tpm_tis_core.c | 35 ++++++++++-------------------------
>  include/linux/tpm.h             |  3 ---
>  3 files changed, 10 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb..a09b652 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -32,35 +32,6 @@ struct class *tpm_class;
>  struct class *tpmrm_class;
>  dev_t tpm_devt;
>  
> -static int tpm_request_locality(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (!chip->ops->request_locality)
> -		return 0;
> -
> -	rc = chip->ops->request_locality(chip, 0);
> -	if (rc < 0)
> -		return rc;
> -
> -	chip->locality = rc;
> -	return 0;
> -}
> -
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (!chip->ops->relinquish_locality)
> -		return;
> -
> -	rc = chip->ops->relinquish_locality(chip, chip->locality);
> -	if (rc)
> -		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
> -
> -	chip->locality = -1;
> -}
> -
>  static int tpm_cmd_ready(struct tpm_chip *chip)
>  {
>  	if (!chip->ops->cmd_ready)
> @@ -103,17 +74,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 +95,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);
> @@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  		goto out;
>  	}
>  
> -	chip->locality = -1;
>  	return chip;
>  
>  out:
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 0959559..a95daf8 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;
>  }
> @@ -751,22 +747,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	const char *desc = "attempting to generate an interrupt";
>  	u32 cap2;
>  	cap_t cap;
> -	int ret;
>  
>  	/* TPM 2.0 */
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
>  
>  	/* TPM 1.2 */
> -	ret = request_locality(chip, 0);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
> -
> -	release_locality(chip, 0);
> -
> -	return ret;
> +	return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>  }
>  
>  /* Register the IRQ and issue a command that will cause an interrupt. If an
> @@ -880,6 +867,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)
> @@ -1007,6 +995,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)
> @@ -1017,9 +1013,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)
> @@ -1080,15 +1073,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;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 8f4ff39..55debe6 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -160,9 +160,6 @@ struct tpm_chip {
>  	u32 last_cc;
>  	u32 nr_commands;
>  	u32 *cc_attrs_tbl;
> -
> -	/* active locality */
> -	int locality;
>  };
>  
>  #define TPM_HEADER_SIZE		10
> -- 
> 2.7.4
> 

/Jarkko

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

[Jarkko Sakkinen]

On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote:
> Interrupt handling at least includes reading and writing the interrupt
> status register from the interrupt routine. However over SPI those accesses
> require a sleepable context, since a mutex is used in the concerning
> functions.
> For this reason request a threaded interrupt handler which is running in
> (sleepable) process context.
> 
> 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 e7d1eab..0959559 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -781,8 +781,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
> 

Why?

https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670

I don't see anything that sleeps there.

/Jarkko1

RE: [PATCH v2 2/4] tpm: Simplify locality handling

[Peter.Huewe]

On Mon, Apr 26, 2021 at 01:47:18AM +0200, Lino Sanfilippo wrote:
> Currently the TPM (default) locality is claimed and released for each 
> access to the TPM registers which require a claimed locality. This 
> results in locality claim/release combos at various code places. For 
> interrupt handling we also need such a combo in the interrupt handler 
> (for clearing the interrupts) which makes the locality handling even 
> more complicated since now we also have to synchronize concurrent 
> accesses in process and in interrupt context.
>
> Since currently the driver uses only one locality anyway, avoid the 
> increasing complexity by claiming it once at driver startup and only 
> releasing it at driver shutdown.
>
> Due to the simplifications the functions tpm_request_locality() and
> tpm_relinquish_locality() are not needed any more an can be removed.
>

+1
I like the idea, as it also improves performance a bit.
Peter

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

[Lino Sanfilippo]

Hi,

On 26.04.21 at 16:49, Stefan Berger wrote:

>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 55debe6..e9882acf 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;
>
>
> This doesn't seem to be necessary.
>

The bitop makros require an unsigned long, leaving it as unsigned int results in
a compiler error:

error: passing argument 2 of ‘test_bit’ from incompatible pointer type [-Werror=incompatible-pointer-types]

> The rest looks good to me. I remember that last time I had tried to activate it some laptop didn't cooperate and we ended up reverting some code, but maybe your changes fixed all of that now. Though you may want to 'prepare for unforseen consequences' :-).
>

You may be right with your concerns and I will try to find some more hardware to test the patches.

Thanks for the review(s).

Regards,
Lino

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

[Lino Sanfilippo]

Hi,

On 28.04.21 at 01:53, Jarkko Sakkinen wrote:
> On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote:
>> Interrupt handling at least includes reading and writing the interrupt
>> status register from the interrupt routine. However over SPI those accesses
>> require a sleepable context, since a mutex is used in the concerning
>> functions.
>> For this reason request a threaded interrupt handler which is running in
>> (sleepable) process context.
>>
>> 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 e7d1eab..0959559 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -781,8 +781,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
>>
>
> Why?
>
> https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670
>
> I don't see anything that sleeps there.
>
> /Jarkko1
>

The problem are the register read/write functions which we use to access the status register in
the interrupt handler. In case of SPI they result in taking the spi_bus_lock which is a mutex.

E.g tpm_tis_spi_read32: tpm_tis_spi_read_bytes->tpm_tis_spi_transfer->spi_bus_lock->mutex_lock

Using a threaded interrupt handler seemed to me the easiest way to avoid this issue.

Regards,
Lino

Re: [PATCH v2 2/4] tpm: Simplify locality handling

[Lino Sanfilippo]

Hi,

On 28.04.21 at 01:49, Jarkko Sakkinen wrote:
>> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Hi, what hardware? Just if I could find something comparable lying around.
>

I tested these patches on an Infineon OPTIGA SLB 9670. This is an SPI device with
TPM 2.0 support.

Regards,
Lino

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

[Jarkko Sakkinen]

On Thu, Apr 29, 2021 at 12:37:40AM +0200, Lino Sanfilippo wrote:
> 
> Hi,
> 
> On 28.04.21 at 01:53, Jarkko Sakkinen wrote:
> > On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote:
> >> Interrupt handling at least includes reading and writing the interrupt
> >> status register from the interrupt routine. However over SPI those accesses
> >> require a sleepable context, since a mutex is used in the concerning
> >> functions.
> >> For this reason request a threaded interrupt handler which is running in
> >> (sleepable) process context.
> >>
> >> 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 e7d1eab..0959559 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -781,8 +781,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
> >>
> >
> > Why?
> >
> > https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670
> >
> > I don't see anything that sleeps there.
> >
> > /Jarkko1
> >
> 
> The problem are the register read/write functions which we use to access the status register in
> the interrupt handler. In case of SPI they result in taking the spi_bus_lock which is a mutex.
> 
> E.g tpm_tis_spi_read32: tpm_tis_spi_read_bytes->tpm_tis_spi_transfer->spi_bus_lock->mutex_lock
> 
> Using a threaded interrupt handler seemed to me the easiest way to avoid this issue.
> 
> Regards,
> Lino
> 
> 
> 

This is a sentence that you should delete:

"However over SPI those accesses require a sleepable context, since a
mutex is used in the concerning functions.  "

It neither explains anything who and why sort of stuff.

Why don't you put intead something like

"Inside tpm_int_handler(), tpm_tis_read32() and tpm_tis_write32() are
invoked. The SPI subsystem requires mutex for I/O, which means that the
calls ought not to be used inside interrupt context."

(I did not check typos). Generally speaking, commit message is as, if not
more important than the code change.

/Jarkko

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

[Lino Sanfilippo]

Hi,


On 29.04.21 at 08:58, Jarkko Sakkinen wrote:
>
> This is a sentence that you should delete:
>
> "However over SPI those accesses require a sleepable context, since a
> mutex is used in the concerning functions.  "
>
> It neither explains anything who and why sort of stuff.
>
> Why don't you put intead something like
>
> "Inside tpm_int_handler(), tpm_tis_read32() and tpm_tis_write32() are
> invoked. The SPI subsystem requires mutex for I/O, which means that the
> calls ought not to be used inside interrupt context."
>
> (I did not check typos). Generally speaking, commit message is as, if not
> more important than the code change.
>
> /Jarkko
>

ok, I will rephrase this in the next patch version.

Regards,
Lino