linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Fixes for TPM interrupt handling
@ 2021-05-01 13:57 Lino Sanfilippo
  2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
                   ` (3 more replies)
  0 siblings, 4 replies; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-01 13:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel, LinoSanfilippo

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 commit 9f67672a817e ("Merge tag 'ext4_for_linus'
of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4") and tested on
on a SLB 9670 which is connected 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 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 (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(-)


base-commit: 9f67672a817ec046f7554a885f0fe0d60e1bf99f
-- 
2.31.1


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v3 1/4] tpm: Use a threaded interrupt handler
  2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
@ 2021-05-01 13:57 ` Lino Sanfilippo
  2021-05-03 15:14   ` Jarkko Sakkinen
  2021-05-01 13:57 ` [PATCH v3 2/4] tpm: Simplify locality handling Lino Sanfilippo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-01 13:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel, LinoSanfilippo, Stefan Berger

The interrupt handler uses tpm_tis_read32() and tpm_tis_write32() to access
the interrupt status register. In case of SPI those accesses are done with
the spi_bus_lock mutex held. This means that the status register cannot
be read or written in interrupt context.

For this reason request a threaded interrupt handler so that the required
accesses can be done in process context.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 a2e0395cbe61..a12992ae2a3e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -737,8 +737,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.31.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v3 2/4] tpm: Simplify locality handling
  2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
  2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
@ 2021-05-01 13:57 ` Lino Sanfilippo
  2021-05-03 15:50   ` Jarkko Sakkinen
  2021-05-01 13:57 ` [PATCH v3 3/4] tpm: Fix test for interrupts Lino Sanfilippo
  2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
  3 siblings, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-01 13:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel, LinoSanfilippo

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 ddaeceb7e109..a09b6523261e 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 a12992ae2a3e..f892b1ae46f2 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -626,9 +626,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;
@@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality);
 
 	return rc;
 }
@@ -707,22 +703,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
@@ -836,6 +823,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)
@@ -963,6 +951,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)
@@ -973,9 +969,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)
@@ -1036,15 +1029,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 aa11fe323c56..7a68832b14bb 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -167,9 +167,6 @@ struct tpm_chip {
 	u32 last_cc;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
-
-	/* active locality */
-	int locality;
 };
 
 #define TPM_HEADER_SIZE		10
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v3 3/4] tpm: Fix test for interrupts
  2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
  2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
  2021-05-01 13:57 ` [PATCH v3 2/4] tpm: Simplify locality handling Lino Sanfilippo
@ 2021-05-01 13:57 ` Lino Sanfilippo
  2021-05-03 15:52   ` Jarkko Sakkinen
  2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
  3 siblings, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-01 13:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel, LinoSanfilippo

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 f892b1ae46f2..9615234054aa 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -420,7 +420,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;
@@ -453,29 +453,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;
@@ -677,7 +654,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)
@@ -734,45 +712,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;
 
@@ -1039,7 +1016,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 9b2d32a59f67..dc5f92b18dca 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 7a68832b14bb..c57d0f0395f0 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -133,7 +133,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.31.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v3 4/4] tpm: Only enable supported irqs
  2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
                   ` (2 preceding siblings ...)
  2021-05-01 13:57 ` [PATCH v3 3/4] tpm: Fix test for interrupts Lino Sanfilippo
@ 2021-05-01 13:57 ` Lino Sanfilippo
  2021-05-01 19:09   ` Stefan Berger
  2021-05-03 15:52   ` Jarkko Sakkinen
  3 siblings, 2 replies; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-01 13:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel, LinoSanfilippo

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 9615234054aa..757498a63f57 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -936,13 +936,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);
 
@@ -971,32 +1005,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);
@@ -1066,9 +1074,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 dc5f92b18dca..8ff62213d8fc 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.31.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 4/4] tpm: Only enable supported irqs
  2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
@ 2021-05-01 19:09   ` Stefan Berger
  2021-05-02  3:15     ` Lino Sanfilippo
  2021-05-03 15:52   ` Jarkko Sakkinen
  1 sibling, 1 reply; 27+ messages in thread
From: Stefan Berger @ 2021-05-01 19:09 UTC (permalink / raw)
  To: Lino Sanfilippo, peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel


On 5/1/21 9:57 AM, 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>



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 4/4] tpm: Only enable supported irqs
  2021-05-01 19:09   ` Stefan Berger
@ 2021-05-02  3:15     ` Lino Sanfilippo
  0 siblings, 0 replies; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-02  3:15 UTC (permalink / raw)
  To: Stefan Berger, peterhuewe, jarkko, jgg
  Cc: stefanb, James.Bottomley, keescook, jsnitsel, ml.linux,
	linux-integrity, linux-kernel

On 01.05.21 at 21:09, Stefan Berger wrote:
>
> On 5/1/21 9:57 AM, 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>
>
>

Ah sorry, I forgot to add your Reviewed-By for this patch. Will do so in the
next version.

Thx,
Lino

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 1/4] tpm: Use a threaded interrupt handler
  2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
@ 2021-05-03 15:14   ` Jarkko Sakkinen
  2021-05-04 22:54     ` Lino Sanfilippo
  0 siblings, 1 reply; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-03 15:14 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel, Stefan Berger

On Sat, May 01, 2021 at 03:57:24PM +0200, Lino Sanfilippo wrote:
> The interrupt handler uses tpm_tis_read32() and tpm_tis_write32() to access
> the interrupt status register. In case of SPI those accesses are done with
> the spi_bus_lock mutex held. This means that the status register cannot
> be read or written in interrupt context.
> 
> For this reason request a threaded interrupt handler so that the required
> accesses can be done in process context.
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

No fixes tag.

The short summary scopes now the whole TPM subsystem ("tpm:"), but the fix
is targetted *only* for tpm_tis_spi. How about "tpm, tpm_tis_spi: Allow to
sleep in the interrupt handler"?

This also changes the semantics tpm_tis_*, not just tpm_tis_spi, which is
not acceptable. We cannot backport a fix like this.

Probably you should just add a parameter to tpm_tis_core_init() to hint
that threaded IRQ is required, and then only conditionally do so.

/Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 2/4] tpm: Simplify locality handling
  2021-05-01 13:57 ` [PATCH v3 2/4] tpm: Simplify locality handling Lino Sanfilippo
@ 2021-05-03 15:50   ` Jarkko Sakkinen
  2021-05-04 23:15     ` Lino Sanfilippo
  0 siblings, 1 reply; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-03 15:50 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel

What the heck is "simplification" and what that has to do with fixing
anything? I don't understand your terminology. 

On Sat, May 01, 2021 at 03:57:25PM +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.

Please rather create fix that fixes the issue exactly in the code path.
I don't want to worry what other things this might do "as a side-effect".

Also, lacks the explanation why this patch fixes the issue.

> 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 ddaeceb7e109..a09b6523261e 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 a12992ae2a3e..f892b1ae46f2 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -626,9 +626,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;
> @@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality);
>  
>  	return rc;
>  }
> @@ -707,22 +703,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
> @@ -836,6 +823,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)
> @@ -963,6 +951,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)
> @@ -973,9 +969,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)
> @@ -1036,15 +1029,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 aa11fe323c56..7a68832b14bb 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -167,9 +167,6 @@ struct tpm_chip {
>  	u32 last_cc;
>  	u32 nr_commands;
>  	u32 *cc_attrs_tbl;
> -
> -	/* active locality */
> -	int locality;
>  };
>  
>  #define TPM_HEADER_SIZE		10
> -- 
> 2.31.1
> 

/Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 3/4] tpm: Fix test for interrupts
  2021-05-01 13:57 ` [PATCH v3 3/4] tpm: Fix test for interrupts Lino Sanfilippo
@ 2021-05-03 15:52   ` Jarkko Sakkinen
  2021-05-04 23:18     ` Lino Sanfilippo
  0 siblings, 1 reply; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-03 15:52 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel

On Sat, May 01, 2021 at 03:57:26PM +0200, 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.

Unnecessary complicated is again terminolgy that I don't decipher,
unfortunately. I get "something that works" or "has a regression".
We don't polish things for nothing.

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

Not sure why to take this patch.

> ---
>  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 f892b1ae46f2..9615234054aa 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -420,7 +420,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;
> @@ -453,29 +453,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;
> @@ -677,7 +654,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)
> @@ -734,45 +712,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;
>  
> @@ -1039,7 +1016,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 9b2d32a59f67..dc5f92b18dca 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 7a68832b14bb..c57d0f0395f0 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -133,7 +133,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.31.1
> 

/Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 4/4] tpm: Only enable supported irqs
  2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
  2021-05-01 19:09   ` Stefan Berger
@ 2021-05-03 15:52   ` Jarkko Sakkinen
  1 sibling, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-03 15:52 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel

On Sat, May 01, 2021 at 03:57:27PM +0200, 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>

Zero reasoning again, why.


> ---
>  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 9615234054aa..757498a63f57 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -936,13 +936,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);
>  
> @@ -971,32 +1005,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);
> @@ -1066,9 +1074,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 dc5f92b18dca..8ff62213d8fc 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.31.1

/Jarkko
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 1/4] tpm: Use a threaded interrupt handler
  2021-05-03 15:14   ` Jarkko Sakkinen
@ 2021-05-04 22:54     ` Lino Sanfilippo
  2021-05-06  1:46       ` Jarkko Sakkinen
  0 siblings, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-04 22:54 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel, Stefan Berger


Hi,


On 03.05.21 at 17:14, Jarkko Sakkinen wrote:
> On Sat, May 01, 2021 at 03:57:24PM +0200, Lino Sanfilippo wrote:
>> The interrupt handler uses tpm_tis_read32() and tpm_tis_write32() to access
>> the interrupt status register. In case of SPI those accesses are done with
>> the spi_bus_lock mutex held. This means that the status register cannot
>> be read or written in interrupt context.
>>
>> For this reason request a threaded interrupt handler so that the required
>> accesses can be done in process context.
>>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>
> No fixes tag.
>
> The short summary scopes now the whole TPM subsystem ("tpm:"), but the fix
> is targetted *only* for tpm_tis_spi. How about "tpm, tpm_tis_spi: Allow to
> sleep in the interrupt handler"?
>
> This also changes the semantics tpm_tis_*, not just tpm_tis_spi, which is
> not acceptable. We cannot backport a fix like this.
>
> Probably you should just add a parameter to tpm_tis_core_init() to hint
> that threaded IRQ is required, and then only conditionally do so.
>

Sure, this is doable although to be honest I dont see the issue with also the
non-SPI code running in the threaded interrupt handler. The functionality should
not change (especially since interrupts are not even working right now) and it would
save us a special treatment of the SPI case.


Regards,
Lino

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 2/4] tpm: Simplify locality handling
  2021-05-03 15:50   ` Jarkko Sakkinen
@ 2021-05-04 23:15     ` Lino Sanfilippo
  2021-05-06  1:47       ` Jarkko Sakkinen
  0 siblings, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-04 23:15 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel

Hi,

On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> What the heck is "simplification" and what that has to do with fixing
> anything? I don't understand your terminology.


The intention for this patch is not to fix anything. Please read the cover
letter and the commit message.
This patch is about making the locality handling easier by not claiming/releasing
it multiple times over the driver life time, but claiming it once at driver
startup and only releasing it at driver shutdown.

Right now we have locality request/release combos in

- probe_itpm()
- tpm_tis_gen_interrupt()
- tpm_tis_core_init()
- tpm_chip_start()

and there is still one combo missing for

- tpm2_get_timeouts()

which is the reason why we get the "TPM returned invalid status" bug in case
of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see
below).

And if we are going to enable interrupts, we have to introduce yet another combo,
for accessing the status register in the interrupt handler, since TPM 2.0
requires holding the locality for writing to the status register. That makes
6 different code places in which we take and release the locality.

With this patch applied we only take the locality at one place. Furthermore
with interrupts enabled we dont have to claim the locality for each handler
execution, saving us countless claim/release combinations at runtime.

Hence the term "simplification" which is perfectly justified IMO.

So again, this patch is "only" in preparation for the next patch when interrupts
are actually enabled and we would have to take the locality in the interrupt
handler without this patch.



> On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote:

>> 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.
>
> Please rather create fix that fixes the issue exactly in the code path.
> I don't want to worry what other things this might do "as a side-effect".

As explained above this patch is not meant to fix a bug in the first place
but rather fixes the bug above incidentally by the locality handling reimplementation.

> Also, lacks the explanation why this patch fixes the issue.
>

The explanation is there:

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

I really dont know how to describe this more clear.


Lino

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 3/4] tpm: Fix test for interrupts
  2021-05-03 15:52   ` Jarkko Sakkinen
@ 2021-05-04 23:18     ` Lino Sanfilippo
  0 siblings, 0 replies; 27+ messages in thread
From: Lino Sanfilippo @ 2021-05-04 23:18 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel, LinoSanfilippo

On 03.05.21 at 17:52, Jarkko Sakkinen wrote:
> On Sat, May 01, 2021 at 03:57:26PM +0200, 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.
>
> Unnecessary complicated is again terminolgy that I don't decipher,
> unfortunately. I get "something that works" or "has a regression".
> We don't polish things for nothing.

In this case "unnecessary complicated" means that we can achieve the same
effect (test for interrupts) with fewer code and fewer runtime impact.
Note that in the current code we do the same check for irq for each transmission:

if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
		return tpm_tis_send_main(chip, buf, len);

With this patch the check for irqs has already be done at this time, so we dont have to
do this over and over again for each transmission.


>> 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>
>
> Not sure why to take this patch.

All I can say is that this patch is supposed to

- fix one bug:
"...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..."

- simplify the irq test

- provide interrupts instead of polling

If this is worth taking is up to you, of course.

Regards,
Lino


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 1/4] tpm: Use a threaded interrupt handler
  2021-05-04 22:54     ` Lino Sanfilippo
@ 2021-05-06  1:46       ` Jarkko Sakkinen
  0 siblings, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-06  1:46 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel, Stefan Berger

On Wed, May 05, 2021 at 12:54:37AM +0200, Lino Sanfilippo wrote:
> 
> Hi,
> 
> 
> On 03.05.21 at 17:14, Jarkko Sakkinen wrote:
> > On Sat, May 01, 2021 at 03:57:24PM +0200, Lino Sanfilippo wrote:
> >> The interrupt handler uses tpm_tis_read32() and tpm_tis_write32() to access
> >> the interrupt status register. In case of SPI those accesses are done with
> >> the spi_bus_lock mutex held. This means that the status register cannot
> >> be read or written in interrupt context.
> >>
> >> For this reason request a threaded interrupt handler so that the required
> >> accesses can be done in process context.
> >>
> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> >
> > No fixes tag.
> >
> > The short summary scopes now the whole TPM subsystem ("tpm:"), but the fix
> > is targetted *only* for tpm_tis_spi. How about "tpm, tpm_tis_spi: Allow to
> > sleep in the interrupt handler"?
> >
> > This also changes the semantics tpm_tis_*, not just tpm_tis_spi, which is
> > not acceptable. We cannot backport a fix like this.
> >
> > Probably you should just add a parameter to tpm_tis_core_init() to hint
> > that threaded IRQ is required, and then only conditionally do so.
> >
> 
> Sure, this is doable although to be honest I dont see the issue with also the
> non-SPI code running in the threaded interrupt handler. The functionality should
> not change (especially since interrupts are not even working right now) and it would
> save us a special treatment of the SPI case.

It's violation of "3) Separate your changes" [*].

E.g. we do not want to introduce "improvements" or "simplifications" to
stable kernels on purpose.

[*] https://www.kernel.org/doc/html/v5.11/process/submitting-patches.html> 

/Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 2/4] tpm: Simplify locality handling
  2021-05-04 23:15     ` Lino Sanfilippo
@ 2021-05-06  1:47       ` Jarkko Sakkinen
  2022-03-24 17:04         ` [PATCH v3 0/4] Fixes for TPM interrupt handling Michael Niewöhner
  0 siblings, 1 reply; 27+ messages in thread
From: Jarkko Sakkinen @ 2021-05-06  1:47 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, James.Bottomley, keescook, jsnitsel,
	ml.linux, linux-integrity, linux-kernel

On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> Hi,
> 
> On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > What the heck is "simplification" and what that has to do with fixing
> > anything? I don't understand your terminology.
> 
> 
> The intention for this patch is not to fix anything. Please read the cover
> letter and the commit message.
> This patch is about making the locality handling easier by not claiming/releasing
> it multiple times over the driver life time, but claiming it once at driver
> startup and only releasing it at driver shutdown.
> 
> Right now we have locality request/release combos in
> 
> - probe_itpm()
> - tpm_tis_gen_interrupt()
> - tpm_tis_core_init()
> - tpm_chip_start()
> 
> and there is still one combo missing for
> 
> - tpm2_get_timeouts()
> 
> which is the reason why we get the "TPM returned invalid status" bug in case
> of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see
> below).
> 
> And if we are going to enable interrupts, we have to introduce yet another combo,
> for accessing the status register in the interrupt handler, since TPM 2.0
> requires holding the locality for writing to the status register. That makes
> 6 different code places in which we take and release the locality.
> 
> With this patch applied we only take the locality at one place. Furthermore
> with interrupts enabled we dont have to claim the locality for each handler
> execution, saving us countless claim/release combinations at runtime.
> 
> Hence the term "simplification" which is perfectly justified IMO.
> 
> So again, this patch is "only" in preparation for the next patch when interrupts
> are actually enabled and we would have to take the locality in the interrupt
> handler without this patch.

So: what problem this patch does solve?

/Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2021-05-06  1:47       ` Jarkko Sakkinen
@ 2022-03-24 17:04         ` Michael Niewöhner
  2022-03-25  2:14           ` Jarkko Sakkinen
  0 siblings, 1 reply; 27+ messages in thread
From: Michael Niewöhner @ 2022-03-24 17:04 UTC (permalink / raw)
  To: Jarkko Sakkinen, Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak

Hi guys,

On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > Hi,
> > 
> > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > What the heck is "simplification" and what that has to do with fixing
> > > anything? I don't understand your terminology.
> > 
> > 
> > The intention for this patch is not to fix anything. Please read the cover
> > letter and the commit message.
> > This patch is about making the locality handling easier by not
> > claiming/releasing
> > it multiple times over the driver life time, but claiming it once at driver
> > startup and only releasing it at driver shutdown.
> > 
> > Right now we have locality request/release combos in
> > 
> > - probe_itpm()
> > - tpm_tis_gen_interrupt()
> > - tpm_tis_core_init()
> > - tpm_chip_start()
> > 
> > and there is still one combo missing for
> > 
> > - tpm2_get_timeouts()
> > 
> > which is the reason why we get the "TPM returned invalid status" bug in case
> > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch,
> > see
> > below).
> > 
> > And if we are going to enable interrupts, we have to introduce yet another
> > combo,
> > for accessing the status register in the interrupt handler, since TPM 2.0
> > requires holding the locality for writing to the status register. That makes
> > 6 different code places in which we take and release the locality.
> > 
> > With this patch applied we only take the locality at one place. Furthermore
> > with interrupts enabled we dont have to claim the locality for each handler
> > execution, saving us countless claim/release combinations at runtime.
> > 
> > Hence the term "simplification" which is perfectly justified IMO.
> > 
> > So again, this patch is "only" in preparation for the next patch when
> > interrupts
> > are actually enabled and we would have to take the locality in the interrupt
> > handler without this patch.
> 
> So: what problem this patch does solve?
> 
> /Jarkko
> 

first, thank you very much, Lino, for working on this! I've been debugging
issues with the tis driver in the last days and was about to start with the same
approach as yours when I luckily discovered your patch!

Jarkko, while I agree, that the commit message is not optimal, Lino tried hard
to explain what the problems with the current code are and how they are / can be
fixed. Further, I too don't see why simplification / optimization is such a bad
thing. This driver is actually a very good example. I had a hard time, too,
figuring out what's going on there. A clean rewrite is a very valid approach
here IMO. It's not "polishing for nothing", as you described it, but actually
solving problems.

Interrupt detection is broken for years now and finally a volunteer worked on a
solution. Don't you think this should be valued? Let's get this problem sorted
out :-)

Lino, I'd be happy to test the patches, when you have time and interest to work
on this again!

Thanks, Michael



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-24 17:04         ` [PATCH v3 0/4] Fixes for TPM interrupt handling Michael Niewöhner
@ 2022-03-25  2:14           ` Jarkko Sakkinen
  2022-03-25 12:32             ` Michael Niewöhner
  0 siblings, 1 reply; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-03-25  2:14 UTC (permalink / raw)
  To: Michael Niewöhner
  Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, stefanb,
	James.Bottomley, keescook, jsnitsel, ml.linux, linux-integrity,
	linux-kernel, twawrzynczak

On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote:
> Hi guys,
> 
> On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > Hi,
> > > 
> > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > What the heck is "simplification" and what that has to do with fixing
> > > > anything? I don't understand your terminology.
> > > 
> > > 
> > > The intention for this patch is not to fix anything. Please read the cover
> > > letter and the commit message.
> > > This patch is about making the locality handling easier by not
> > > claiming/releasing
> > > it multiple times over the driver life time, but claiming it once at driver
> > > startup and only releasing it at driver shutdown.
> > > 
> > > Right now we have locality request/release combos in
> > > 
> > > - probe_itpm()
> > > - tpm_tis_gen_interrupt()
> > > - tpm_tis_core_init()
> > > - tpm_chip_start()
> > > 
> > > and there is still one combo missing for
> > > 
> > > - tpm2_get_timeouts()
> > > 
> > > which is the reason why we get the "TPM returned invalid status" bug in case
> > > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch,
> > > see
> > > below).
> > > 
> > > And if we are going to enable interrupts, we have to introduce yet another
> > > combo,
> > > for accessing the status register in the interrupt handler, since TPM 2.0
> > > requires holding the locality for writing to the status register. That makes
> > > 6 different code places in which we take and release the locality.
> > > 
> > > With this patch applied we only take the locality at one place. Furthermore
> > > with interrupts enabled we dont have to claim the locality for each handler
> > > execution, saving us countless claim/release combinations at runtime.
> > > 
> > > Hence the term "simplification" which is perfectly justified IMO.
> > > 
> > > So again, this patch is "only" in preparation for the next patch when
> > > interrupts
> > > are actually enabled and we would have to take the locality in the interrupt
> > > handler without this patch.
> > 
> > So: what problem this patch does solve?
> > 
> > /Jarkko
> > 
> 
> first, thank you very much, Lino, for working on this! I've been debugging
> issues with the tis driver in the last days and was about to start with the same
> approach as yours when I luckily discovered your patch!
> 
> Jarkko, while I agree, that the commit message is not optimal, Lino tried hard
> to explain what the problems with the current code are and how they are / can be
> fixed. Further, I too don't see why simplification / optimization is such a bad
> thing. This driver is actually a very good example. I had a hard time, too,
> figuring out what's going on there. A clean rewrite is a very valid approach
> here IMO. It's not "polishing for nothing", as you described it, but actually
> solving problems.
> 
> Interrupt detection is broken for years now and finally a volunteer worked on a
> solution. Don't you think this should be valued? Let's get this problem sorted
> out :-)
> 
> Lino, I'd be happy to test the patches, when you have time and interest to work
> on this again!
> 
> Thanks, Michael

It's quite easy to test them out. Both fixes are in the mainline GIT tree.
E.g. give a shot rc1, and please report if any issues persists to:

  linux-integrity@vger.kernel.org 

BR, Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-25  2:14           ` Jarkko Sakkinen
@ 2022-03-25 12:32             ` Michael Niewöhner
  2022-03-26  3:24               ` Lino Sanfilippo
  2022-03-30 15:18               ` Jarkko Sakkinen
  0 siblings, 2 replies; 27+ messages in thread
From: Michael Niewöhner @ 2022-03-25 12:32 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, stefanb,
	James.Bottomley, keescook, jsnitsel, ml.linux, linux-integrity,
	linux-kernel, twawrzynczak

On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote:
> > Hi guys,
> > 
> > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > > Hi,
> > > > 
> > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > > What the heck is "simplification" and what that has to do with fixing
> > > > > anything? I don't understand your terminology.
> > > > 
> > > > 
> > > > The intention for this patch is not to fix anything. Please read the
> > > > cover
> > > > letter and the commit message.
> > > > This patch is about making the locality handling easier by not
> > > > claiming/releasing
> > > > it multiple times over the driver life time, but claiming it once at
> > > > driver
> > > > startup and only releasing it at driver shutdown.
> > > > 
> > > > Right now we have locality request/release combos in
> > > > 
> > > > - probe_itpm()
> > > > - tpm_tis_gen_interrupt()
> > > > - tpm_tis_core_init()
> > > > - tpm_chip_start()
> > > > 
> > > > and there is still one combo missing for
> > > > 
> > > > - tpm2_get_timeouts()
> > > > 
> > > > which is the reason why we get the "TPM returned invalid status" bug in
> > > > case
> > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this
> > > > patch,
> > > > see
> > > > below).
> > > > 
> > > > And if we are going to enable interrupts, we have to introduce yet
> > > > another
> > > > combo,
> > > > for accessing the status register in the interrupt handler, since TPM
> > > > 2.0
> > > > requires holding the locality for writing to the status register. That
> > > > makes
> > > > 6 different code places in which we take and release the locality.
> > > > 
> > > > With this patch applied we only take the locality at one place.
> > > > Furthermore
> > > > with interrupts enabled we dont have to claim the locality for each
> > > > handler
> > > > execution, saving us countless claim/release combinations at runtime.
> > > > 
> > > > Hence the term "simplification" which is perfectly justified IMO.
> > > > 
> > > > So again, this patch is "only" in preparation for the next patch when
> > > > interrupts
> > > > are actually enabled and we would have to take the locality in the
> > > > interrupt
> > > > handler without this patch.
> > > 
> > > So: what problem this patch does solve?
> > > 
> > > /Jarkko
> > > 
> > 
> > first, thank you very much, Lino, for working on this! I've been debugging
> > issues with the tis driver in the last days and was about to start with the
> > same
> > approach as yours when I luckily discovered your patch!
> > 
> > Jarkko, while I agree, that the commit message is not optimal, Lino tried
> > hard
> > to explain what the problems with the current code are and how they are /
> > can be
> > fixed. Further, I too don't see why simplification / optimization is such a
> > bad
> > thing. This driver is actually a very good example. I had a hard time, too,
> > figuring out what's going on there. A clean rewrite is a very valid approach
> > here IMO. It's not "polishing for nothing", as you described it, but
> > actually
> > solving problems.
> > 
> > Interrupt detection is broken for years now and finally a volunteer worked
> > on a
> > solution. Don't you think this should be valued? Let's get this problem
> > sorted
> > out :-)
> > 
> > Lino, I'd be happy to test the patches, when you have time and interest to
> > work
> > on this again!
> > 
> > Thanks, Michael
> 
> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> E.g. give a shot rc1, and please report if any issues persists to:
> 
>   linux-integrity@vger.kernel.org 
> 
> BR, Jarkko

I don't see Linos patches on mainline. Also, the series included four patches:
[PATCH v3 0/4] Fixes for TPM interrupt handling
[PATCH v3 1/4] tpm: Use a threaded interrupt handler
[PATCH v3 2/4] tpm: Simplify locality handling
[PATCH v3 3/4] tpm: Fix test for interrupts
[PATCH v3 4/4] tpm: Only enable supported irqs

Three of them are relevant for the interrupt problem, which is still present in
mainline, as these patches were refused:
[PATCH v3 1/4] tpm: Use a threaded interrupt handler
[PATCH v3 2/4] tpm: Simplify locality handling
[PATCH v3 3/4] tpm: Fix test for interrupts

Michael


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-25 12:32             ` Michael Niewöhner
@ 2022-03-26  3:24               ` Lino Sanfilippo
  2022-03-26  8:59                 ` Michael Niewöhner
                                   ` (2 more replies)
  2022-03-30 15:18               ` Jarkko Sakkinen
  1 sibling, 3 replies; 27+ messages in thread
From: Lino Sanfilippo @ 2022-03-26  3:24 UTC (permalink / raw)
  To: Michael Niewöhner, Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak


Hi Michael,

On 25.03.22 at 13:32, Michael Niewöhner wrote:
>>>
>>> Lino, I'd be happy to test the patches, when you have time and interest to
>>> work
>>> on this again!
>>>
>>> Thanks, Michael
>>
>> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
>> E.g. give a shot rc1, and please report if any issues persists to:
>>
>>   linux-integrity@vger.kernel.org 
>>
>> BR, Jarkko
>
> I don't see Linos patches on mainline. Also, the series included four patches:
> [PATCH v3 0/4] Fixes for TPM interrupt handling
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
> [PATCH v3 4/4] tpm: Only enable supported irqs
>
> Three of them are relevant for the interrupt problem, which is still present in
> mainline, as these patches were refused:
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
>
> Michael
>

You are right, the interrupts are still not working in the mainline kernel.
I would gladly make another attempt to fix this but rather step by step
than in a series that tries to fix (different) things at once.

A first step could be to have a sleepable context for the interrupt handling,
since in case of SPI the accesses to the irq status register may sleep.

I sent a patch for this purpose once, but it seems to have gone lost:

https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/


Best regards,
Lino






^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-26  3:24               ` Lino Sanfilippo
@ 2022-03-26  8:59                 ` Michael Niewöhner
  2022-03-30 15:19                 ` Jarkko Sakkinen
  2022-04-20  5:30                 ` Jarkko Sakkinen
  2 siblings, 0 replies; 27+ messages in thread
From: Michael Niewöhner @ 2022-03-26  8:59 UTC (permalink / raw)
  To: Lino Sanfilippo, Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak

Hi Lino,

On Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> 
> Hi Michael,
> 
> On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > 
> > > > Lino, I'd be happy to test the patches, when you have time and interest
> > > > to
> > > > work
> > > > on this again!
> > > > 
> > > > Thanks, Michael
> > > 
> > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > E.g. give a shot rc1, and please report if any issues persists to:
> > > 
> > >   linux-integrity@vger.kernel.org 
> > > 
> > > BR, Jarkko
> > 
> > I don't see Linos patches on mainline. Also, the series included four
> > patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> > 
> > Three of them are relevant for the interrupt problem, which is still present
> > in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > 
> > Michael
> > 
> 
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.

IMHO a series is perfectly fine, as it's easier to show *why* single changes are
required (like already done in v3 0/4). One just has to actually *read* what's
written there. Ahem. It's up to you, though.

> 
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
> 
> I sent a patch for this purpose once, but it seems to have gone lost:
> 
> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> 

Jarkko, looks like you've already tested that patch on your NUC?

> 
> Best regards,
> Lino
> 


Michael

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-25 12:32             ` Michael Niewöhner
  2022-03-26  3:24               ` Lino Sanfilippo
@ 2022-03-30 15:18               ` Jarkko Sakkinen
  1 sibling, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-03-30 15:18 UTC (permalink / raw)
  To: Michael Niewöhner
  Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, stefanb,
	James.Bottomley, keescook, jsnitsel, ml.linux, linux-integrity,
	linux-kernel, twawrzynczak

On Fri, Mar 25, 2022 at 01:32:25PM +0100, Michael Niewöhner wrote:
> On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote:
> > > Hi guys,
> > > 
> > > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > > > Hi,
> > > > > 
> > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > > > What the heck is "simplification" and what that has to do with fixing
> > > > > > anything? I don't understand your terminology.
> > > > > 
> > > > > 
> > > > > The intention for this patch is not to fix anything. Please read the
> > > > > cover
> > > > > letter and the commit message.
> > > > > This patch is about making the locality handling easier by not
> > > > > claiming/releasing
> > > > > it multiple times over the driver life time, but claiming it once at
> > > > > driver
> > > > > startup and only releasing it at driver shutdown.
> > > > > 
> > > > > Right now we have locality request/release combos in
> > > > > 
> > > > > - probe_itpm()
> > > > > - tpm_tis_gen_interrupt()
> > > > > - tpm_tis_core_init()
> > > > > - tpm_chip_start()
> > > > > 
> > > > > and there is still one combo missing for
> > > > > 
> > > > > - tpm2_get_timeouts()
> > > > > 
> > > > > which is the reason why we get the "TPM returned invalid status" bug in
> > > > > case
> > > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this
> > > > > patch,
> > > > > see
> > > > > below).
> > > > > 
> > > > > And if we are going to enable interrupts, we have to introduce yet
> > > > > another
> > > > > combo,
> > > > > for accessing the status register in the interrupt handler, since TPM
> > > > > 2.0
> > > > > requires holding the locality for writing to the status register. That
> > > > > makes
> > > > > 6 different code places in which we take and release the locality.
> > > > > 
> > > > > With this patch applied we only take the locality at one place.
> > > > > Furthermore
> > > > > with interrupts enabled we dont have to claim the locality for each
> > > > > handler
> > > > > execution, saving us countless claim/release combinations at runtime.
> > > > > 
> > > > > Hence the term "simplification" which is perfectly justified IMO.
> > > > > 
> > > > > So again, this patch is "only" in preparation for the next patch when
> > > > > interrupts
> > > > > are actually enabled and we would have to take the locality in the
> > > > > interrupt
> > > > > handler without this patch.
> > > > 
> > > > So: what problem this patch does solve?
> > > > 
> > > > /Jarkko
> > > > 
> > > 
> > > first, thank you very much, Lino, for working on this! I've been debugging
> > > issues with the tis driver in the last days and was about to start with the
> > > same
> > > approach as yours when I luckily discovered your patch!
> > > 
> > > Jarkko, while I agree, that the commit message is not optimal, Lino tried
> > > hard
> > > to explain what the problems with the current code are and how they are /
> > > can be
> > > fixed. Further, I too don't see why simplification / optimization is such a
> > > bad
> > > thing. This driver is actually a very good example. I had a hard time, too,
> > > figuring out what's going on there. A clean rewrite is a very valid approach
> > > here IMO. It's not "polishing for nothing", as you described it, but
> > > actually
> > > solving problems.
> > > 
> > > Interrupt detection is broken for years now and finally a volunteer worked
> > > on a
> > > solution. Don't you think this should be valued? Let's get this problem
> > > sorted
> > > out :-)
> > > 
> > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > work
> > > on this again!
> > > 
> > > Thanks, Michael
> > 
> > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > E.g. give a shot rc1, and please report if any issues persists to:
> > 
> >   linux-integrity@vger.kernel.org 
> > 
> > BR, Jarkko
> 
> I don't see Linos patches on mainline. Also, the series included four patches:
> [PATCH v3 0/4] Fixes for TPM interrupt handling
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
> [PATCH v3 4/4] tpm: Only enable supported irqs
> 
> Three of them are relevant for the interrupt problem, which is still present in
> mainline, as these patches were refused:
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
> 
> Michael

There was some unaddressed feedback, most of not that hard to address.
I'm waiting for v4.

BR, Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-26  3:24               ` Lino Sanfilippo
  2022-03-26  8:59                 ` Michael Niewöhner
@ 2022-03-30 15:19                 ` Jarkko Sakkinen
  2022-04-20  5:30                 ` Jarkko Sakkinen
  2 siblings, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-03-30 15:19 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Michael Niewöhner, peterhuewe, jgg, stefanb, stefanb,
	James.Bottomley, keescook, jsnitsel, ml.linux, linux-integrity,
	linux-kernel, twawrzynczak

On Sat, Mar 26, 2022 at 04:24:55AM +0100, Lino Sanfilippo wrote:
> 
> Hi Michael,
> 
> On 25.03.22 at 13:32, Michael Niewöhner wrote:
> >>>
> >>> Lino, I'd be happy to test the patches, when you have time and interest to
> >>> work
> >>> on this again!
> >>>
> >>> Thanks, Michael
> >>
> >> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> >> E.g. give a shot rc1, and please report if any issues persists to:
> >>
> >>   linux-integrity@vger.kernel.org 
> >>
> >> BR, Jarkko
> >
> > I don't see Linos patches on mainline. Also, the series included four patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> >
> > Three of them are relevant for the interrupt problem, which is still present in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> >
> > Michael
> >
> 
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
> 
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
> 
> I sent a patch for this purpose once, but it seems to have gone lost:
> 
> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> 
> 
> Best regards,
> Lino

This is clearly my bad I'll test this ASAP.

BR, Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-03-26  3:24               ` Lino Sanfilippo
  2022-03-26  8:59                 ` Michael Niewöhner
  2022-03-30 15:19                 ` Jarkko Sakkinen
@ 2022-04-20  5:30                 ` Jarkko Sakkinen
  2022-04-20  5:32                   ` Jarkko Sakkinen
  2022-04-24  2:22                   ` Lino Sanfilippo
  2 siblings, 2 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-04-20  5:30 UTC (permalink / raw)
  To: Lino Sanfilippo, Michael Niewöhner
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak

n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> 
> Hi Michael,
> 
> On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > 
> > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > work
> > > > on this again!
> > > > 
> > > > Thanks, Michael
> > > 
> > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > E.g. give a shot rc1, and please report if any issues persists to:
> > > 
> > >   linux-integrity@vger.kernel.org 
> > > 
> > > BR, Jarkko
> > 
> > I don't see Linos patches on mainline. Also, the series included four patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> > 
> > Three of them are relevant for the interrupt problem, which is still present in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > 
> > Michael
> > 
> 
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
> 
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
> 
> I sent a patch for this purpose once, but it seems to have gone lost:
> 
> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> 
> 
> Best regards,
> Lino

I went these through one by one.

# Above linked patch 

Boolean parameters are considered bad. I.e. use named flags
instead. This is for above linked patch.

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

1. Please remove "unnecessarily complicated" sentence because
   it cannot be evaluated. It's your opinion, which might perhaps
   be correct, but it is irrelevant for any possible patch
   description.
2. There's no such thing as "fix by re-implementation". Please
   explain instead code change is relevant for the bug fix.
3. If set_bit() et al necessarily to fix a possible race condition
   you need to have a separate patch for that.

To move forward, start with a better summary such as

"tpm: move interrupt test to tpm_tis_probe_irq_single()"

I'd also either revert the change for flags, or alternatively
move it to separate patch explaining race condition. Otherwise,
there's no argument of saying that using set_bit() is more 
proper. This will make the change more localized.


# [PATCH v3 2/4] tpm: Simplify locality handling

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

Generally speaking, the simplifications should be done on top of code
that does not have known bugs, even if the simplification renders out
the bug. This is because then we have code that have potentially unknown
unknown bugs.

I hope you see my point. The problem with these patches were then
and is still that they intermix bug fixes and other modifications and
thus cannot be taken in.

BR, Jarkko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-04-20  5:30                 ` Jarkko Sakkinen
@ 2022-04-20  5:32                   ` Jarkko Sakkinen
  2022-04-24  2:22                   ` Lino Sanfilippo
  1 sibling, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-04-20  5:32 UTC (permalink / raw)
  To: Lino Sanfilippo, Michael Niewöhner
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak

On Wed, 2022-04-20 at 08:30 +0300, Jarkko Sakkinen wrote:
> n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> > 
> > Hi Michael,
> > 
> > On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > > 
> > > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > > work
> > > > > on this again!
> > > > > 
> > > > > Thanks, Michael
> > > > 
> > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > > E.g. give a shot rc1, and please report if any issues persists to:
> > > > 
> > > >   linux-integrity@vger.kernel.org 
> > > > 
> > > > BR, Jarkko
> > > 
> > > I don't see Linos patches on mainline. Also, the series included four patches:
> > > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > [PATCH v3 4/4] tpm: Only enable supported irqs
> > > 
> > > Three of them are relevant for the interrupt problem, which is still present in
> > > mainline, as these patches were refused:
> > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > 
> > > Michael
> > > 
> > 
> > You are right, the interrupts are still not working in the mainline kernel.
> > I would gladly make another attempt to fix this but rather step by step
> > than in a series that tries to fix (different) things at once.
> > 
> > A first step could be to have a sleepable context for the interrupt handling,
> > since in case of SPI the accesses to the irq status register may sleep.
> > 
> > I sent a patch for this purpose once, but it seems to have gone lost:
> > 
> > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> > 
> > 
> > Best regards,
> > Lino
> 
> I went these through one by one.
> 
> # Above linked patch 
> 
> Boolean parameters are considered bad. I.e. use named flags
> instead. This is for above linked patch.
> 
> # [PATCH v3 3/4] tpm: Fix test for interrupts
> 
> 1. Please remove "unnecessarily complicated" sentence because
>    it cannot be evaluated. It's your opinion, which might perhaps
>    be correct, but it is irrelevant for any possible patch
>    description.
> 2. There's no such thing as "fix by re-implementation". Please
>    explain instead code change is relevant for the bug fix.
> 3. If set_bit() et al necessarily to fix a possible race condition
>    you need to have a separate patch for that.
> 
> To move forward, start with a better summary such as
> 
> "tpm: move interrupt test to tpm_tis_probe_irq_single()"
> 
> I'd also either revert the change for flags, or alternatively
> move it to separate patch explaining race condition. Otherwise,
> there's no argument of saying that using set_bit() is more 
> proper. This will make the change more localized.
> 
> 
> # [PATCH v3 2/4] tpm: Simplify locality handling
> 
> "As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:"
> 
> Generally speaking, the simplifications should be done on top of code
> that does not have known bugs, even if the simplification renders out
> the bug. This is because then we have code that have potentially unknown
> unknown bugs.
> 
> I hope you see my point. The problem with these patches were then
> and is still that they intermix bug fixes and other modifications and
> thus cannot be taken in.

I.e. to move forward create first localized fixes, and only after those
clean ups if there is point. Removing code (like in 2/4) is not a bug
fix fo anything. This not to say that some changes would be illegit, I'm
only saying that the patches are badly scoped.

BR, Jarkko


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-04-20  5:30                 ` Jarkko Sakkinen
  2022-04-20  5:32                   ` Jarkko Sakkinen
@ 2022-04-24  2:22                   ` Lino Sanfilippo
  2022-04-25 13:57                     ` Jarkko Sakkinen
  1 sibling, 1 reply; 27+ messages in thread
From: Lino Sanfilippo @ 2022-04-24  2:22 UTC (permalink / raw)
  To: Jarkko Sakkinen, Michael Niewöhner
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak,
	Lukas Wunner, Philipp Rosenberger


Hi,

On 20.04.22 at 07:30, Jarkko Sakkinen wrote:
> n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
>>
>> Hi Michael,
>>
>> On 25.03.22 at 13:32, Michael Niewöhner wrote:
>>>>>
>>>>> Lino, I'd be happy to test the patches, when you have time and interest to
>>>>> work
>>>>> on this again!
>>>>>
>>>>> Thanks, Michael
>>>>
>>>> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
>>>> E.g. give a shot rc1, and please report if any issues persists to:
>>>>
>>>>   linux-integrity@vger.kernel.org 
>>>>
>>>> BR, Jarkko
>>>
>>> I don't see Linos patches on mainline. Also, the series included four patches:
>>> [PATCH v3 0/4] Fixes for TPM interrupt handling
>>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
>>> [PATCH v3 2/4] tpm: Simplify locality handling
>>> [PATCH v3 3/4] tpm: Fix test for interrupts
>>> [PATCH v3 4/4] tpm: Only enable supported irqs
>>>
>>> Three of them are relevant for the interrupt problem, which is still present in
>>> mainline, as these patches were refused:
>>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
>>> [PATCH v3 2/4] tpm: Simplify locality handling
>>> [PATCH v3 3/4] tpm: Fix test for interrupts
>>>
>>> Michael
>>>
>>
>> You are right, the interrupts are still not working in the mainline kernel.
>> I would gladly make another attempt to fix this but rather step by step
>> than in a series that tries to fix (different) things at once.
>>
>> A first step could be to have a sleepable context for the interrupt handling,
>> since in case of SPI the accesses to the irq status register may sleep.
>>
>> I sent a patch for this purpose once, but it seems to have gone lost:
>>
>> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
>>
>>
>> Best regards,
>> Lino
>
> I went these through one by one>
> # Above linked patch
>
> Boolean parameters are considered bad. I.e. use named flags
> instead. This is for above linked patch.

Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ"
for this.

>
> # [PATCH v3 3/4] tpm: Fix test for interrupts
>
> 1. Please remove "unnecessarily complicated" sentence because
>    it cannot be evaluated. It's your opinion, which might perhaps
>    be correct, but it is irrelevant for any possible patch
>    description.
> 2. There's no such thing as "fix by re-implementation". Please
>    explain instead code change is relevant for the bug fix.
> 3. If set_bit() et al necessarily to fix a possible race condition
>    you need to have a separate patch for that.
>
> To move forward, start with a better summary such as
>
> "tpm: move interrupt test to tpm_tis_probe_irq_single()"
>
> I'd also either revert the change for flags, or alternatively
> move it to separate patch explaining race condition. Otherwise,
> there's no argument of saying that using set_bit() is more
> proper. This will make the change more localized.
>

Ok, I will split the fix for the irq test into two patches then.

>
> # [PATCH v3 2/4] tpm: Simplify locality handling
>
> "As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:"
>
> Generally speaking, the simplifications should be done on top of code
> that does not have known bugs, even if the simplification renders out
> the bug. This is because then we have code that have potentially unknown
> unknown bugs.
>
> I hope you see my point. The problem with these patches were then
> and is still that they intermix bug fixes and other modifications and
> thus cannot be taken in.
>

Yes, I can see that point.

> BR, Jarkko
>

Thanks a lot for the review. I will prepare new patches with the suggested
changes.

Best regards,
Lino


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
  2022-04-24  2:22                   ` Lino Sanfilippo
@ 2022-04-25 13:57                     ` Jarkko Sakkinen
  0 siblings, 0 replies; 27+ messages in thread
From: Jarkko Sakkinen @ 2022-04-25 13:57 UTC (permalink / raw)
  To: Lino Sanfilippo, Michael Niewöhner
  Cc: peterhuewe, jgg, stefanb, stefanb, James.Bottomley, keescook,
	jsnitsel, ml.linux, linux-integrity, linux-kernel, twawrzynczak,
	Lukas Wunner, Philipp Rosenberger

On Sun, 2022-04-24 at 04:22 +0200, Lino Sanfilippo wrote:
> 
> Hi,
> 
> On 20.04.22 at 07:30, Jarkko Sakkinen wrote:
> > n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> > > 
> > > Hi Michael,
> > > 
> > > On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > > > 
> > > > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > > > work
> > > > > > on this again!
> > > > > > 
> > > > > > Thanks, Michael
> > > > > 
> > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > > > E.g. give a shot rc1, and please report if any issues persists to:
> > > > > 
> > > > >   linux-integrity@vger.kernel.org 
> > > > > 
> > > > > BR, Jarkko
> > > > 
> > > > I don't see Linos patches on mainline. Also, the series included four patches:
> > > > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > > [PATCH v3 4/4] tpm: Only enable supported irqs
> > > > 
> > > > Three of them are relevant for the interrupt problem, which is still present in
> > > > mainline, as these patches were refused:
> > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > > 
> > > > Michael
> > > > 
> > > 
> > > You are right, the interrupts are still not working in the mainline kernel.
> > > I would gladly make another attempt to fix this but rather step by step
> > > than in a series that tries to fix (different) things at once.
> > > 
> > > A first step could be to have a sleepable context for the interrupt handling,
> > > since in case of SPI the accesses to the irq status register may sleep.
> > > 
> > > I sent a patch for this purpose once, but it seems to have gone lost:
> > > 
> > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> > > 
> > > 
> > > Best regards,
> > > Lino
> > 
> > I went these through one by one>
> > # Above linked patch
> > 
> > Boolean parameters are considered bad. I.e. use named flags
> > instead. This is for above linked patch.
> 
> Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ"
> for this.
> 
> > 
> > # [PATCH v3 3/4] tpm: Fix test for interrupts
> > 
> > 1. Please remove "unnecessarily complicated" sentence because
> >    it cannot be evaluated. It's your opinion, which might perhaps
> >    be correct, but it is irrelevant for any possible patch
> >    description.
> > 2. There's no such thing as "fix by re-implementation". Please
> >    explain instead code change is relevant for the bug fix.
> > 3. If set_bit() et al necessarily to fix a possible race condition
> >    you need to have a separate patch for that.
> > 
> > To move forward, start with a better summary such as
> > 
> > "tpm: move interrupt test to tpm_tis_probe_irq_single()"
> > 
> > I'd also either revert the change for flags, or alternatively
> > move it to separate patch explaining race condition. Otherwise,
> > there's no argument of saying that using set_bit() is more
> > proper. This will make the change more localized.
> > 
> 
> Ok, I will split the fix for the irq test into two patches then.
> 
> > 
> > # [PATCH v3 2/4] tpm: Simplify locality handling
> > 
> > "As a side-effect these modifications fix a bug which results in the
> > following warning when using TPM 2:"
> > 
> > Generally speaking, the simplifications should be done on top of code
> > that does not have known bugs, even if the simplification renders out
> > the bug. This is because then we have code that have potentially unknown
> > unknown bugs.
> > 
> > I hope you see my point. The problem with these patches were then
> > and is still that they intermix bug fixes and other modifications and
> > thus cannot be taken in.
> > 
> 
> Yes, I can see that point.
> 
> > BR, Jarkko
> > 
> 
> Thanks a lot for the review. I will prepare new patches with the suggested
> changes.

Yeah, I mean the point being: it's OK to suggest clean ups but with bug fixes
you should aim for the lowest common denominator as far as you possibly can.

BR, Jarkko


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2022-04-25 13:56 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
2021-05-03 15:14   ` Jarkko Sakkinen
2021-05-04 22:54     ` Lino Sanfilippo
2021-05-06  1:46       ` Jarkko Sakkinen
2021-05-01 13:57 ` [PATCH v3 2/4] tpm: Simplify locality handling Lino Sanfilippo
2021-05-03 15:50   ` Jarkko Sakkinen
2021-05-04 23:15     ` Lino Sanfilippo
2021-05-06  1:47       ` Jarkko Sakkinen
2022-03-24 17:04         ` [PATCH v3 0/4] Fixes for TPM interrupt handling Michael Niewöhner
2022-03-25  2:14           ` Jarkko Sakkinen
2022-03-25 12:32             ` Michael Niewöhner
2022-03-26  3:24               ` Lino Sanfilippo
2022-03-26  8:59                 ` Michael Niewöhner
2022-03-30 15:19                 ` Jarkko Sakkinen
2022-04-20  5:30                 ` Jarkko Sakkinen
2022-04-20  5:32                   ` Jarkko Sakkinen
2022-04-24  2:22                   ` Lino Sanfilippo
2022-04-25 13:57                     ` Jarkko Sakkinen
2022-03-30 15:18               ` Jarkko Sakkinen
2021-05-01 13:57 ` [PATCH v3 3/4] tpm: Fix test for interrupts Lino Sanfilippo
2021-05-03 15:52   ` Jarkko Sakkinen
2021-05-04 23:18     ` Lino Sanfilippo
2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
2021-05-01 19:09   ` Stefan Berger
2021-05-02  3:15     ` Lino Sanfilippo
2021-05-03 15:52   ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).