linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 00/11] TPM IRQ fixes
@ 2022-10-17 23:57 Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 01/11] tpm, tpm_tis: Avoid cache incoherency in test for interrupts Lino Sanfilippo
                   ` (10 more replies)
  0 siblings, 11 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

This series enables IRQ support for the TPM TIS core. For this reason a
number of bugfixes around the interrupt handling are required (patches 1 to
4).

Patch 5 takes into account that according to the TPM Interface
Specification stsValid and commandRead interrupts might not be supported
by the hardware. For this reason the supported interrupts are first queried
and stored. Then wait_for_tpm_stat() is adjusted to not wait for status
changes that are not reported by interrupts.

Patch 6 moves the interrupt flag checks into an own function as suggested
by Jarkko.

Patch 7 Removes the possibility that tpm_tis_data->locality can be changed
at driver runtime so this variable can be read without the need to protect
it against concurrent modification. 

Patch 8 addresses the issue with concurrent locality handling:
Since the interrupt handler writes the interrupt status registers it needs
to hold the locality. However it runs concurrently to the thread which
triggered the interrupt (e.g. by reading or writing data to the TPM). So
it must take care when claiming and releasing the locality itself,
because it may race with the concurrent running thread which also claims
and releases the locality.
To avoid that both interrupt and concurrent running thread interfere with
each other a locality counter is used which guarantees that at any time
the locality is held as long as it is required by one of both execution
paths.

Patch 9 implements the request of a threaded interrupt handler. This is
needed since SPI uses a mutex for data transmission and since we access the
interrupt status register via SPI in the irq handler we need a sleepable
context.

Patch 10 makes sure that writes to the interrupt register are effective if
done in the interrupt handler.

Patch 11 enables the test for interrupts by setting the required flag
before the test is executed.

Changes in v8:
- tpm_tis_data->locality is not changed at runtime any more so that it can
be read without any protection against concurrent modification.
- add missing brackets as pointed out by Jason Andryuk

Changes in v7:
- moved interrupt flag checks into an own function as suggested by Jarkko
- added "Tested-by" tags for Tests from Michael Niewöhner
- fixed one comment

Changes in v6:
- set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
in an own bitfield 
- improve commit messages
- use int_mask instead of irqs_in_use as variable name
- use sts_mask instead of active_irqs as variable name
- squash patch 5 and 6
- prefix functions with tpm_tis_
- remove "fixes" tag

Changes in v5:
- improve commit message of patch 1 as requested by Jarko
- drop patch that makes locality handling simpler by only claiming it at
  driver startup and releasing it at driver shutdown (requested by Jarko)
- drop patch that moves the interrupt test from tpm_tis_send()
  to tmp_tis_probe_irq_single() as requested by Jarko
- add patch to make locality handling threadsafe so that it can also be
  done by the irq handler
- separate logical changes into own patches
- always request threaded interrupt handler

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

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

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


Lino Sanfilippo (11):
  tpm, tpm_tis: Avoid cache incoherency in test for interrupts
  tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register
  tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed
  tpm, tmp_tis: Claim locality before writing interrupt registers
  tpm, tpm_tis: Only handle supported interrupts
  tpm, tpm_tis: Move interrupt mask checks into own function
  tpm, tpm_tis: do not check for the active locality in interrupt
    handler
  tpm, tpm: Implement usage counter for locality
  tpm, tpm_tis: Request threaded interrupt handler
  tpm, tpm_tis: Claim locality in interrupt handler
  tpm, tpm_tis: Enable interrupt test

 drivers/char/tpm/tpm_tis.c      |   2 +-
 drivers/char/tpm/tpm_tis_core.c | 267 +++++++++++++++++++++-----------
 drivers/char/tpm/tpm_tis_core.h |   5 +-
 3 files changed, 181 insertions(+), 93 deletions(-)


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.36.1


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

* [PATCH v8 01/11] tpm, tpm_tis: Avoid cache incoherency in test for interrupts
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 02/11] tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register Lino Sanfilippo
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

The interrupt handler that sets the boolean variable irq_tested may run on
another CPU as the thread that checks irq_tested as part of the irq test in
tmp_tis_send().

Since nothing guarantees cache coherency between CPUs for unsynchronized
accesses to boolean variables the testing thread might not perceive the
value change done in the interrupt handler.

Avoid this issue by setting the bit TPM_TIS_IRQ_TESTED in the flags field
of the tpm_tis_data struct and by accessing this field with the bit
manipulating functions that provide cache coherency.

Also convert all other existing sites to use the proper macros when
accessing this bitfield.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis.c      |  2 +-
 drivers/char/tpm/tpm_tis_core.c | 21 +++++++++++----------
 drivers/char/tpm/tpm_tis_core.h |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index bcff6429e0b4..ce43412eb398 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -226,7 +226,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
 		irq = tpm_info->irq;
 
 	if (itpm || is_itpm(ACPI_COMPANION(dev)))
-		phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
+		set_bit(TPM_TIS_ITPM_WORKAROUND, &phy->priv.flags);
 
 	return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
 				 ACPI_HANDLE(dev));
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 757623bacfd5..c0008efb95dc 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -351,7 +351,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	int rc, status, burstcnt;
 	size_t count = 0;
-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
+	bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -484,7 +484,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	int rc, irq;
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
+	     test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
 		return tpm_tis_send_main(chip, buf, len);
 
 	/* Verify receipt of the expected IRQ */
@@ -494,11 +495,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	rc = tpm_tis_send_main(chip, buf, len);
 	priv->irq = irq;
 	chip->flags |= TPM_CHIP_FLAG_IRQ;
-	if (!priv->irq_tested)
+	if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
 		tpm_msleep(1);
-	if (!priv->irq_tested)
+	if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
 		disable_interrupts(chip);
-	priv->irq_tested = true;
+	set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 	return rc;
 }
 
@@ -641,7 +642,7 @@ static int probe_itpm(struct tpm_chip *chip)
 	size_t len = sizeof(cmd_getticks);
 	u16 vendor;
 
-	if (priv->flags & TPM_TIS_ITPM_WORKAROUND)
+	if (test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags))
 		return 0;
 
 	rc = tpm_tis_read16(priv, TPM_DID_VID(0), &vendor);
@@ -661,13 +662,13 @@ static int probe_itpm(struct tpm_chip *chip)
 
 	tpm_tis_ready(chip);
 
-	priv->flags |= TPM_TIS_ITPM_WORKAROUND;
+	set_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
 
 	rc = tpm_tis_send_data(chip, cmd_getticks, len);
 	if (rc == 0)
 		dev_info(&chip->dev, "Detected an iTPM.\n");
 	else {
-		priv->flags &= ~TPM_TIS_ITPM_WORKAROUND;
+		clear_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
 		rc = -EFAULT;
 	}
 
@@ -707,7 +708,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	if (interrupt == 0)
 		return IRQ_NONE;
 
-	priv->irq_tested = true;
+	set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&priv->read_queue);
 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -793,7 +794,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
-	priv->irq_tested = false;
+	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 66a5a13cd1df..695a2516dce0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,13 +86,13 @@ enum tis_defaults {
 enum tpm_tis_flags {
 	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
 	TPM_TIS_INVALID_STATUS		= BIT(1),
+	TPM_TIS_IRQ_TESTED		= BIT(2),
 };
 
 struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
-	bool irq_tested;
 	unsigned long flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
-- 
2.36.1


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

* [PATCH v8 02/11] tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 01/11] tpm, tpm_tis: Avoid cache incoherency in test for interrupts Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 03/11] tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed Lino Sanfilippo
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

In disable_interrupts() the TPM_GLOBAL_INT_ENABLE bit is unset in the
TPM_INT_ENABLE register to shut the interrupts off. However modifying the
register is only possible with a held locality. So claim the locality
before disable_interrupts() is called.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c0008efb95dc..d2c9c9d76893 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1098,7 +1098,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
+				rc = request_locality(chip, 0);
+				if (rc < 0)
+					goto out_err;
 				disable_interrupts(chip);
+				release_locality(chip, 0);
 			}
 		} else {
 			tpm_tis_probe_irq(chip, intmask);
-- 
2.36.1


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

* [PATCH v8 03/11] tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 01/11] tpm, tpm_tis: Avoid cache incoherency in test for interrupts Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 02/11] tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 04/11] tpm, tmp_tis: Claim locality before writing interrupt registers Lino Sanfilippo
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

Both functions tpm_tis_probe_irq_single() and tpm_tis_probe_irq() may setup
the interrupts and then return with an error. This case is indicated by a
missing TPM_CHIP_FLAG_IRQ flag in chip->flags.
Currently the interrupt setup is only undone if tpm_tis_probe_irq_single()
fails. Undo the setup also if tpm_tis_probe_irq() fails.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2c9c9d76893..603b82ca56da 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1091,21 +1091,21 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 			goto out_err;
 		}
 
-		if (irq) {
+		if (irq)
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
 						 irq);
-			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
-				dev_err(&chip->dev, FW_BUG
+		else
+			tpm_tis_probe_irq(chip, intmask);
+
+		if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+			dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
-				rc = request_locality(chip, 0);
-				if (rc < 0)
-					goto out_err;
-				disable_interrupts(chip);
-				release_locality(chip, 0);
-			}
-		} else {
-			tpm_tis_probe_irq(chip, intmask);
+			rc = request_locality(chip, 0);
+			if (rc < 0)
+				goto out_err;
+			disable_interrupts(chip);
+			release_locality(chip, 0);
 		}
 	}
 
-- 
2.36.1


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

* [PATCH v8 04/11] tpm, tmp_tis: Claim locality before writing interrupt registers
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (2 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 03/11] tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 05/11] tpm, tpm_tis: Only handle supported interrupts Lino Sanfilippo
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

In tpm_tis_probe_single_irq() interrupt registers TPM_INT_VECTOR,
TPM_INT_STATUS and TPM_INT_ENABLE are modified to setup the interrupts.
Currently these modifications are done without holding a locality thus they
have no effect. Fix this by claiming the (default) locality before the
registers are written.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 603b82ca56da..52205a1fee9e 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -770,30 +770,45 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	}
 	priv->irq = irq;
 
+	rc = request_locality(chip, 0);
+	if (rc < 0)
+		return rc;
+
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
 			   &original_int_vec);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, priv->locality);
 		return rc;
+	}
 
 	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, priv->locality);
 		return rc;
+	}
 
 	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, priv->locality);
 		return rc;
+	}
 
 	/* Clear all existing */
 	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, priv->locality);
 		return rc;
+	}
 
 	/* Turn on */
 	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
 			     intmask | TPM_GLOBAL_INT_ENABLE);
-	if (rc < 0)
+	if (rc < 0) {
+		release_locality(chip, priv->locality);
 		return rc;
+	}
 
+	release_locality(chip, priv->locality);
 	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 
 	/* Generate an interrupt by having the core call through to
-- 
2.36.1


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

* [PATCH v8 05/11] tpm, tpm_tis: Only handle supported interrupts
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (3 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 04/11] tpm, tmp_tis: Claim locality before writing interrupt registers Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 06/11] tpm, tpm_tis: Move interrupt mask checks into own function Lino Sanfilippo
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

According to the TPM Interface Specification (TIS) support for "stsValid"
and "commandReady" interrupts is only optional.
This has to be taken into account when handling the interrupts in functions
like wait_for_tpm_stat(). To determine the supported interrupts use the
capability query.

Also adjust wait_for_tpm_stat() to only wait for interrupt reported status
changes. After that process all the remaining status changes by polling
the status register.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm_tis_core.c | 120 +++++++++++++++++++-------------
 drivers/char/tpm/tpm_tis_core.h |   1 +
 2 files changed, 73 insertions(+), 48 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 52205a1fee9e..d07debc3182c 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -53,41 +53,63 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 	long rc;
 	u8 status;
 	bool canceled = false;
+	u8 sts_mask = 0;
+	int ret = 0;
 
 	/* check current status */
 	status = chip->ops->status(chip);
 	if ((status & mask) == mask)
 		return 0;
 
-	stop = jiffies + timeout;
+	/* check what status changes can be handled by irqs */
+	if (priv->int_mask & TPM_INTF_STS_VALID_INT)
+		sts_mask |= TPM_STS_VALID;
 
-	if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+	if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
+		sts_mask |= TPM_STS_DATA_AVAIL;
+
+	if (priv->int_mask & TPM_INTF_CMD_READY_INT)
+		sts_mask |= TPM_STS_COMMAND_READY;
+
+	sts_mask &= mask;
+
+	stop = jiffies + timeout;
+	/* process status changes with irq support */
+	if (sts_mask) {
+		ret = -ETIME;
 again:
 		timeout = stop - jiffies;
 		if ((long)timeout <= 0)
 			return -ETIME;
 		rc = wait_event_interruptible_timeout(*queue,
-			wait_for_tpm_stat_cond(chip, mask, check_cancel,
+			wait_for_tpm_stat_cond(chip, sts_mask, check_cancel,
 					       &canceled),
 			timeout);
 		if (rc > 0) {
 			if (canceled)
 				return -ECANCELED;
-			return 0;
+			ret = 0;
 		}
 		if (rc == -ERESTARTSYS && freezing(current)) {
 			clear_thread_flag(TIF_SIGPENDING);
 			goto again;
 		}
-	} else {
-		do {
-			usleep_range(priv->timeout_min,
-				     priv->timeout_max);
-			status = chip->ops->status(chip);
-			if ((status & mask) == mask)
-				return 0;
-		} while (time_before(jiffies, stop));
 	}
+
+	if (ret)
+		return ret;
+
+	mask &= ~sts_mask;
+	if (!mask) /* all done */
+		return 0;
+	/* process status changes without irq support */
+	do {
+		status = chip->ops->status(chip);
+		if ((status & mask) == mask)
+			return 0;
+		usleep_range(priv->timeout_min,
+			     priv->timeout_max);
+	} while (time_before(jiffies, stop));
 	return -ETIME;
 }
 
@@ -1021,8 +1043,40 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	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;
+	/* 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) {
+		intmask |= 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) {
+		intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
+		dev_dbg(dev, "\tLocality Change Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_STS_VALID_INT) {
+		intmask |= TPM_INTF_STS_VALID_INT;
+		dev_dbg(dev, "\tSts Valid Int Support\n");
+	}
+	if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+		intmask |= TPM_INTF_DATA_AVAIL_INT;
+		dev_dbg(dev, "\tData Avail Int Support\n");
+	}
+
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 
 	rc = request_locality(chip, 0);
@@ -1056,32 +1110,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);
@@ -1112,7 +1140,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		else
 			tpm_tis_probe_irq(chip, intmask);
 
-		if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+		if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+			priv->int_mask = intmask;
+		} else {
 			dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
@@ -1159,13 +1189,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 	if (rc < 0)
 		goto out;
 
-	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
-	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->int_mask | 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 695a2516dce0..2deef11c88db 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -93,6 +93,7 @@ struct tpm_tis_data {
 	u16 manufacturer_id;
 	int locality;
 	int irq;
+	unsigned int int_mask;
 	unsigned long flags;
 	void __iomem *ilb_base_addr;
 	u16 clkrun_enabled;
-- 
2.36.1


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

* [PATCH v8 06/11] tpm, tpm_tis: Move interrupt mask checks into own function
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (4 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 05/11] tpm, tpm_tis: Only handle supported interrupts Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler Lino Sanfilippo
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

Clean up wait_for_tpm_stat() by moving multiple similar interrupt mask
checks into an own function.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm_tis_core.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d07debc3182c..181c291b0bb8 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -44,6 +44,20 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
 	return false;
 }
 
+static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
+{
+	if (!(int_mask & TPM_INTF_STS_VALID_INT))
+		sts_mask &= ~TPM_STS_VALID;
+
+	if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
+		sts_mask &= ~TPM_STS_DATA_AVAIL;
+
+	if (!(int_mask & TPM_INTF_CMD_READY_INT))
+		sts_mask &= ~TPM_STS_COMMAND_READY;
+
+	return sts_mask;
+}
+
 static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		unsigned long timeout, wait_queue_head_t *queue,
 		bool check_cancel)
@@ -53,7 +67,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 	long rc;
 	u8 status;
 	bool canceled = false;
-	u8 sts_mask = 0;
+	u8 sts_mask;
 	int ret = 0;
 
 	/* check current status */
@@ -61,17 +75,10 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 	if ((status & mask) == mask)
 		return 0;
 
+	sts_mask = mask & (TPM_STS_VALID | TPM_STS_DATA_AVAIL |
+			   TPM_STS_COMMAND_READY);
 	/* check what status changes can be handled by irqs */
-	if (priv->int_mask & TPM_INTF_STS_VALID_INT)
-		sts_mask |= TPM_STS_VALID;
-
-	if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
-		sts_mask |= TPM_STS_DATA_AVAIL;
-
-	if (priv->int_mask & TPM_INTF_CMD_READY_INT)
-		sts_mask |= TPM_STS_COMMAND_READY;
-
-	sts_mask &= mask;
+	sts_mask = tpm_tis_filter_sts_mask(priv->int_mask, sts_mask);
 
 	stop = jiffies + timeout;
 	/* process status changes with irq support */
-- 
2.36.1


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

* [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (5 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 06/11] tpm, tpm_tis: Move interrupt mask checks into own function Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-23  4:32   ` Jarkko Sakkinen
  2022-10-17 23:57 ` [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Lino Sanfilippo
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

After driver initialization tpm_tis_data->locality may only be modified in
case of a LOCALITY CHANGE interrupt. In this case the interrupt handler
iterates over all localities only to assign the active one to
tpm_tis_data->locality.

However this information is never used any more, so the assignment is not
needed.
Furthermore without the assignment tpm_tis_data->locality cannot change any
more at driver runtime, and thus no protection against concurrent
modification is required when the variable is read at other places.

So remove this iteration entirely.

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

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 181c291b0bb8..4336f7ea8c2b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -728,7 +728,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	struct tpm_chip *chip = dev_id;
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	u32 interrupt;
-	int i, rc;
+	int rc;
 
 	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
 	if (rc < 0)
@@ -740,10 +740,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 	set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&priv->read_queue);
-	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
-		for (i = 0; i < 5; i++)
-			if (check_locality(chip, i))
-				break;
+
 	if (interrupt &
 	    (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
 	     TPM_INTF_CMD_READY_INT))
-- 
2.36.1


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

* [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (6 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-18  6:25   ` Lukas Wunner
  2022-10-23  4:40   ` Jarkko Sakkinen
  2022-10-17 23:57 ` [PATCH v8 09/11] tpm, tpm_tis: Request threaded interrupt handler Lino Sanfilippo
                   ` (2 subsequent siblings)
  10 siblings, 2 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

Implement a usage counter for the (default) locality used by the TPM TIS
driver:
Request the locality from the TPM if it has not been claimed yet, otherwise
only increment the counter. Also release the locality if the counter is 0
otherwise only decrement the counter. Ensure thread-safety by protecting
the counter with a mutex.

This allows to request and release the locality from a thread and the
interrupt handler at the same time without the danger to interfere with
each other.

By doing this refactor the names of the amended functions to use the proper
prefix.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++-----------
 drivers/char/tpm/tpm_tis_core.h |  2 +
 2 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4336f7ea8c2b..79dfab65976f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l)
 	return false;
 }
 
-static int release_locality(struct tpm_chip *chip, int l)
+static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l)
+{
+	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
+
+	return 0;
+}
+
+static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
+	mutex_lock(&priv->locality_count_mutex);
+	priv->locality_count--;
+	if (priv->locality_count == 0)
+		tpm_tis_release_locality_locked(priv, l);
+	mutex_unlock(&priv->locality_count_mutex);
 
 	return 0;
 }
 
-static int request_locality(struct tpm_chip *chip, int l)
+static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	unsigned long stop, timeout;
@@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l)
 	return -1;
 }
 
+static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
+{
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	int ret = 0;
+
+	mutex_lock(&priv->locality_count_mutex);
+	if (priv->locality_count == 0)
+		ret = tpm_tis_request_locality_locked(chip, l);
+	if (!ret)
+		priv->locality_count++;
+	mutex_unlock(&priv->locality_count_mutex);
+	return ret;
+}
+
 static u8 tpm_tis_status(struct tpm_chip *chip)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
@@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
 	if (vendor != TPM_VID_INTEL)
 		return 0;
 
-	if (request_locality(chip, 0) != 0)
+	if (tpm_tis_request_locality(chip, 0) != 0)
 		return -EBUSY;
 
 	rc = tpm_tis_send_data(chip, cmd_getticks, len);
@@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, priv->locality);
+	tpm_tis_release_locality(chip, priv->locality);
 
 	return rc;
 }
@@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
 	cap_t cap;
 	int ret;
 
-	ret = request_locality(chip, 0);
+	ret = tpm_tis_request_locality(chip, 0);
 	if (ret < 0)
 		return ret;
 
@@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
 	else
 		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
 
-	release_locality(chip, 0);
+	tpm_tis_release_locality(chip, 0);
 
 	return ret;
 }
@@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	}
 	priv->irq = irq;
 
-	rc = request_locality(chip, 0);
+	rc = tpm_tis_request_locality(chip, 0);
 	if (rc < 0)
 		return rc;
 
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
 			   &original_int_vec);
 	if (rc < 0) {
-		release_locality(chip, priv->locality);
+		tpm_tis_release_locality(chip, priv->locality);
 		return rc;
 	}
 
 	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
 	if (rc < 0) {
-		release_locality(chip, priv->locality);
+		tpm_tis_release_locality(chip, priv->locality);
 		return rc;
 	}
 
 	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
 	if (rc < 0) {
-		release_locality(chip, priv->locality);
+		tpm_tis_release_locality(chip, priv->locality);
 		return rc;
 	}
 
 	/* Clear all existing */
 	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
 	if (rc < 0) {
-		release_locality(chip, priv->locality);
+		tpm_tis_release_locality(chip, priv->locality);
 		return rc;
 	}
 
@@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
 			     intmask | TPM_GLOBAL_INT_ENABLE);
 	if (rc < 0) {
-		release_locality(chip, priv->locality);
+		tpm_tis_release_locality(chip, priv->locality);
 		return rc;
 	}
 
-	release_locality(chip, priv->locality);
+	tpm_tis_release_locality(chip, priv->locality);
 	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
 
 	/* Generate an interrupt by having the core call through to
@@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = {
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_canceled = tpm_tis_req_canceled,
-	.request_locality = request_locality,
-	.relinquish_locality = release_locality,
+	.request_locality = tpm_tis_request_locality,
+	.relinquish_locality = tpm_tis_release_locality,
 	.clk_enable = tpm_tis_clkrun_enable,
 };
 
@@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
 	priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
 	priv->phy_ops = phy_ops;
+	priv->locality_count = 0;
+	mutex_init(&priv->locality_count_mutex);
 
 	dev_set_drvdata(&chip->dev, priv);
 
@@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 	intmask &= ~TPM_GLOBAL_INT_ENABLE;
 
-	rc = request_locality(chip, 0);
+	rc = tpm_tis_request_locality(chip, 0);
 	if (rc < 0) {
 		rc = -ENODEV;
 		goto out_err;
 	}
 
 	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
-	release_locality(chip, 0);
+	tpm_tis_release_locality(chip, 0);
 
 	rc = tpm_chip_start(chip);
 	if (rc)
@@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		 * proper timeouts for the driver.
 		 */
 
-		rc = request_locality(chip, 0);
+		rc = tpm_tis_request_locality(chip, 0);
 		if (rc < 0)
 			goto out_err;
 
 		rc = tpm_get_timeouts(chip);
 
-		release_locality(chip, 0);
+		tpm_tis_release_locality(chip, 0);
 
 		if (rc) {
 			dev_err(dev, "Could not get TPM timeouts and durations\n");
@@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 			dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
 
-			rc = request_locality(chip, 0);
+			rc = tpm_tis_request_locality(chip, 0);
 			if (rc < 0)
 				goto out_err;
 			disable_interrupts(chip);
-			release_locality(chip, 0);
+			tpm_tis_release_locality(chip, 0);
 		}
 	}
 
@@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev)
 	 * an error code but for unknown reason it isn't handled.
 	 */
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
-		ret = request_locality(chip, 0);
+		ret = tpm_tis_request_locality(chip, 0);
 		if (ret < 0)
 			return ret;
 
 		tpm1_do_selftest(chip);
 
-		release_locality(chip, 0);
+		tpm_tis_release_locality(chip, 0);
 	}
 
 	return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 2deef11c88db..13bdcf38e56f 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -91,6 +91,8 @@ enum tpm_tis_flags {
 
 struct tpm_tis_data {
 	u16 manufacturer_id;
+	struct mutex locality_count_mutex;
+	unsigned int locality_count;
 	int locality;
 	int irq;
 	unsigned int int_mask;
-- 
2.36.1


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

* [PATCH v8 09/11] tpm, tpm_tis: Request threaded interrupt handler
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (7 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 10/11] tpm, tpm_tis: Claim locality in " Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test Lino Sanfilippo
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

The TIS interrupt handler at least has to read and write the interrupt
status register. In case of SPI both operations result in a call to
tpm_tis_spi_transfer() which uses the bus_lock_mutex of the spi device
and thus must only be called from a sleepable context.

To ensure this request a threaded interrupt handler.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm_tis_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 79dfab65976f..1e065e7ac460 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -813,8 +813,11 @@ 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) {
+
+	rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
+				       tis_int_handler, IRQF_ONESHOT | flags,
+				       dev_name(&chip->dev), chip);
+	if (rc) {
 		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
 			 irq);
 		return -1;
-- 
2.36.1


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

* [PATCH v8 10/11] tpm, tpm_tis: Claim locality in interrupt handler
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (8 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 09/11] tpm, tpm_tis: Request threaded interrupt handler Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-17 23:57 ` [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test Lino Sanfilippo
  10 siblings, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

Writing the TPM_INT_STATUS register in the interrupt handler to clear the
interrupts only has effect if a locality is held. Since this is not
guaranteed at the time the interrupt is fired, claim the locality
explicitly in the handler.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 1e065e7ac460..42f628e52cde 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -772,7 +772,9 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 		wake_up_interruptible(&priv->int_queue);
 
 	/* Clear interrupts handled with TPM_EOI */
+	tpm_tis_request_locality(chip, 0);
 	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), interrupt);
+	tpm_tis_release_locality(chip, 0);
 	if (rc < 0)
 		return IRQ_NONE;
 
-- 
2.36.1


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

* [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test
  2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
                   ` (9 preceding siblings ...)
  2022-10-17 23:57 ` [PATCH v8 10/11] tpm, tpm_tis: Claim locality in " Lino Sanfilippo
@ 2022-10-17 23:57 ` Lino Sanfilippo
  2022-10-23  4:33   ` Jarkko Sakkinen
  2022-10-23  4:34   ` Jarkko Sakkinen
  10 siblings, 2 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-17 23:57 UTC (permalink / raw)
  To: peterhuewe, jarkko, jgg
  Cc: stefanb, linux, linux-integrity, linux-kernel, jandryuk, pmenzel,
	l.sanfilippo, LinoSanfilippo, lukas, p.rosenberger

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

The test for interrupts in tpm_tis_send() is skipped if the flag
TPM_CHIP_FLAG_IRQ is not set. Since the current code never sets the flag
initially the test is never executed.

Fix this by setting the flag in tpm_tis_gen_interrupt() right after
interrupts have been enabled and before the test is executed.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Tested-by: Michael Niewöhner <linux@mniewoehner.de>
---
 drivers/char/tpm/tpm_tis_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 42f628e52cde..9778860e1598 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -793,11 +793,16 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
 	if (ret < 0)
 		return ret;
 
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
 	else
 		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
 
+	if (ret)
+		chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+
 	tpm_tis_release_locality(chip, 0);
 
 	return ret;
-- 
2.36.1


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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-17 23:57 ` [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Lino Sanfilippo
@ 2022-10-18  6:25   ` Lukas Wunner
  2022-10-18  7:42     ` Lino Sanfilippo
  2022-10-23  5:26     ` Jarkko Sakkinen
  2022-10-23  4:40   ` Jarkko Sakkinen
  1 sibling, 2 replies; 25+ messages in thread
From: Lukas Wunner @ 2022-10-18  6:25 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jarkko, jgg, stefanb, linux, linux-integrity,
	linux-kernel, jandryuk, pmenzel, l.sanfilippo, p.rosenberger

On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
> Implement a usage counter for the (default) locality used by the TPM TIS
> driver:
> Request the locality from the TPM if it has not been claimed yet, otherwise
> only increment the counter. Also release the locality if the counter is 0
> otherwise only decrement the counter. Ensure thread-safety by protecting
> the counter with a mutex.
> 
> This allows to request and release the locality from a thread and the
> interrupt handler at the same time without the danger to interfere with
> each other.
[...]
> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  
> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> +	mutex_lock(&priv->locality_count_mutex);
> +	priv->locality_count--;
> +	if (priv->locality_count == 0)
> +		tpm_tis_release_locality_locked(priv, l);
> +	mutex_unlock(&priv->locality_count_mutex);
>  
>  	return 0;
>  }

Hm, any reason not to use struct kref for the locality counter?
Provides correct memory ordering (no mutex needed) and allows for
calling a release function too upon reaching 0.

Thanks,

Lukas

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-18  6:25   ` Lukas Wunner
@ 2022-10-18  7:42     ` Lino Sanfilippo
  2022-10-23  5:26     ` Jarkko Sakkinen
  1 sibling, 0 replies; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-18  7:42 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: peterhuewe, jarkko, jgg, stefanb, linux, linux-integrity,
	linux-kernel, jandryuk, pmenzel, l.sanfilippo, p.rosenberger



On 18.10.22 08:25, Lukas Wunner wrote:
> Hm, any reason not to use struct kref for the locality counter?
> Provides correct memory ordering (no mutex needed) and allows for
> calling a release function too upon reaching 0.
>

I already tried this but krefs turned out to be not very usable
in this case. See my post here:

https://lore.kernel.org/lkml/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/

Regards,
Lino


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

* Re: [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler
  2022-10-17 23:57 ` [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler Lino Sanfilippo
@ 2022-10-23  4:32   ` Jarkko Sakkinen
  0 siblings, 0 replies; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-10-23  4:32 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger

On Tue, Oct 18, 2022 at 01:57:28AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> After driver initialization tpm_tis_data->locality may only be modified in
> case of a LOCALITY CHANGE interrupt. In this case the interrupt handler
> iterates over all localities only to assign the active one to
> tpm_tis_data->locality.
> 
> However this information is never used any more, so the assignment is not
> needed.
> Furthermore without the assignment tpm_tis_data->locality cannot change any
> more at driver runtime, and thus no protection against concurrent
> modification is required when the variable is read at other places.
> 
> So remove this iteration entirely.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

> ---
>  drivers/char/tpm/tpm_tis_core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 181c291b0bb8..4336f7ea8c2b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -728,7 +728,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>  	struct tpm_chip *chip = dev_id;
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	u32 interrupt;
> -	int i, rc;
> +	int rc;
>  
>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
>  	if (rc < 0)
> @@ -740,10 +740,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>  	set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>  		wake_up_interruptible(&priv->read_queue);
> -	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> -		for (i = 0; i < 5; i++)
> -			if (check_locality(chip, i))
> -				break;
> +
>  	if (interrupt &
>  	    (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
>  	     TPM_INTF_CMD_READY_INT))
> -- 
> 2.36.1
> 

BR, Jarkko

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

* Re: [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test
  2022-10-17 23:57 ` [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test Lino Sanfilippo
@ 2022-10-23  4:33   ` Jarkko Sakkinen
  2022-10-23  4:34   ` Jarkko Sakkinen
  1 sibling, 0 replies; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-10-23  4:33 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger

On Tue, Oct 18, 2022 at 01:57:32AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> The test for interrupts in tpm_tis_send() is skipped if the flag
> TPM_CHIP_FLAG_IRQ is not set. Since the current code never sets the flag
> initially the test is never executed.
> 
> Fix this by setting the flag in tpm_tis_gen_interrupt() right after
> interrupts have been enabled and before the test is executed.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> Tested-by: Michael Niewöhner <linux@mniewoehner.de>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 42f628e52cde..9778860e1598 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -793,11 +793,16 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
>  	else
>  		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>  
> +	if (ret)
> +		chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +
>  	tpm_tis_release_locality(chip, 0);
>  
>  	return ret;
> -- 
> 2.36.1
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

* Re: [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test
  2022-10-17 23:57 ` [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test Lino Sanfilippo
  2022-10-23  4:33   ` Jarkko Sakkinen
@ 2022-10-23  4:34   ` Jarkko Sakkinen
  1 sibling, 0 replies; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-10-23  4:34 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger

On Tue, Oct 18, 2022 at 01:57:32AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> The test for interrupts in tpm_tis_send() is skipped if the flag
> TPM_CHIP_FLAG_IRQ is not set. Since the current code never sets the flag
> initially the test is never executed.
> 
> Fix this by setting the flag in tpm_tis_gen_interrupt() right after
> interrupts have been enabled and before the test is executed.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> Tested-by: Michael Niewöhner <linux@mniewoehner.de>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 42f628e52cde..9778860e1598 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -793,11 +793,16 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
>  	else
>  		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>  
> +	if (ret)
> +		chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +
>  	tpm_tis_release_locality(chip, 0);
>  
>  	return ret;
> -- 
> 2.36.1
> 


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-17 23:57 ` [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Lino Sanfilippo
  2022-10-18  6:25   ` Lukas Wunner
@ 2022-10-23  4:40   ` Jarkko Sakkinen
  2022-10-25  0:15     ` Lino Sanfilippo
  1 sibling, 1 reply; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-10-23  4:40 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger

On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Implement a usage counter for the (default) locality used by the TPM TIS
> driver:
> Request the locality from the TPM if it has not been claimed yet, otherwise
> only increment the counter. Also release the locality if the counter is 0
> otherwise only decrement the counter. Ensure thread-safety by protecting
> the counter with a mutex.
> 
> This allows to request and release the locality from a thread and the
> interrupt handler at the same time without the danger to interfere with
> each other.
> 
> By doing this refactor the names of the amended functions to use the proper
> prefix.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> Tested-by: Michael Niewöhner <linux@mniewoehner.de>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++-----------
>  drivers/char/tpm/tpm_tis_core.h |  2 +
>  2 files changed, 53 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4336f7ea8c2b..79dfab65976f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l)
>  	return false;
>  }
>  
> -static int release_locality(struct tpm_chip *chip, int l)
> +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l)

Nit: usually you would actually use "unlocked" here, not locked.

Probably best name would be __tpm_tis_release_locality().

> +{
> +	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> +
> +	return 0;
> +}
> +
> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  
> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> +	mutex_lock(&priv->locality_count_mutex);
> +	priv->locality_count--;
> +	if (priv->locality_count == 0)
> +		tpm_tis_release_locality_locked(priv, l);
> +	mutex_unlock(&priv->locality_count_mutex);
>  
>  	return 0;
>  }

Since the function pointer has the word "relinquish" and not "release",
perhaps these should also use that word for consistency.

>  
> -static int request_locality(struct tpm_chip *chip, int l)
> +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	unsigned long stop, timeout;
> @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l)
>  	return -1;
>  }
>  
> +static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	int ret = 0;
> +
> +	mutex_lock(&priv->locality_count_mutex);
> +	if (priv->locality_count == 0)
> +		ret = tpm_tis_request_locality_locked(chip, l);
> +	if (!ret)
> +		priv->locality_count++;
> +	mutex_unlock(&priv->locality_count_mutex);
> +	return ret;
> +}
> +
>  static u8 tpm_tis_status(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
>  	if (vendor != TPM_VID_INTEL)
>  		return 0;
>  
> -	if (request_locality(chip, 0) != 0)
> +	if (tpm_tis_request_locality(chip, 0) != 0)
>  		return -EBUSY;
>  
>  	rc = tpm_tis_send_data(chip, cmd_getticks, len);
> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality);
> +	tpm_tis_release_locality(chip, priv->locality);
>  
>  	return rc;
>  }
> @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	cap_t cap;
>  	int ret;
>  
> -	ret = request_locality(chip, 0);
> +	ret = tpm_tis_request_locality(chip, 0);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	else
>  		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>  
> -	release_locality(chip, 0);
> +	tpm_tis_release_locality(chip, 0);
>  
>  	return ret;
>  }
> @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  	}
>  	priv->irq = irq;
>  
> -	rc = request_locality(chip, 0);
> +	rc = tpm_tis_request_locality(chip, 0);
>  	if (rc < 0)
>  		return rc;
>  
>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>  			   &original_int_vec);
>  	if (rc < 0) {
> -		release_locality(chip, priv->locality);
> +		tpm_tis_release_locality(chip, priv->locality);
>  		return rc;
>  	}
>  
>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>  	if (rc < 0) {
> -		release_locality(chip, priv->locality);
> +		tpm_tis_release_locality(chip, priv->locality);
>  		return rc;
>  	}
>  
>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>  	if (rc < 0) {
> -		release_locality(chip, priv->locality);
> +		tpm_tis_release_locality(chip, priv->locality);
>  		return rc;
>  	}
>  
>  	/* Clear all existing */
>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>  	if (rc < 0) {
> -		release_locality(chip, priv->locality);
> +		tpm_tis_release_locality(chip, priv->locality);
>  		return rc;
>  	}
>  
> @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>  			     intmask | TPM_GLOBAL_INT_ENABLE);
>  	if (rc < 0) {
> -		release_locality(chip, priv->locality);
> +		tpm_tis_release_locality(chip, priv->locality);
>  		return rc;
>  	}
>  
> -	release_locality(chip, priv->locality);
> +	tpm_tis_release_locality(chip, priv->locality);
>  	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
>  
>  	/* Generate an interrupt by having the core call through to
> @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = {
>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>  	.req_canceled = tpm_tis_req_canceled,
> -	.request_locality = request_locality,
> -	.relinquish_locality = release_locality,
> +	.request_locality = tpm_tis_request_locality,
> +	.relinquish_locality = tpm_tis_release_locality,
>  	.clk_enable = tpm_tis_clkrun_enable,
>  };
>  
> @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
>  	priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
>  	priv->phy_ops = phy_ops;
> +	priv->locality_count = 0;
> +	mutex_init(&priv->locality_count_mutex);
>  
>  	dev_set_drvdata(&chip->dev, priv);
>  
> @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  
>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>  
> -	rc = request_locality(chip, 0);
> +	rc = tpm_tis_request_locality(chip, 0);
>  	if (rc < 0) {
>  		rc = -ENODEV;
>  		goto out_err;
>  	}
>  
>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> -	release_locality(chip, 0);
> +	tpm_tis_release_locality(chip, 0);
>  
>  	rc = tpm_chip_start(chip);
>  	if (rc)
> @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		 * proper timeouts for the driver.
>  		 */
>  
> -		rc = request_locality(chip, 0);
> +		rc = tpm_tis_request_locality(chip, 0);
>  		if (rc < 0)
>  			goto out_err;
>  
>  		rc = tpm_get_timeouts(chip);
>  
> -		release_locality(chip, 0);
> +		tpm_tis_release_locality(chip, 0);
>  
>  		if (rc) {
>  			dev_err(dev, "Could not get TPM timeouts and durations\n");
> @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  			dev_err(&chip->dev, FW_BUG
>  					"TPM interrupt not working, polling instead\n");
>  
> -			rc = request_locality(chip, 0);
> +			rc = tpm_tis_request_locality(chip, 0);
>  			if (rc < 0)
>  				goto out_err;
>  			disable_interrupts(chip);
> -			release_locality(chip, 0);
> +			tpm_tis_release_locality(chip, 0);
>  		}
>  	}
>  
> @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev)
>  	 * an error code but for unknown reason it isn't handled.
>  	 */
>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> -		ret = request_locality(chip, 0);
> +		ret = tpm_tis_request_locality(chip, 0);
>  		if (ret < 0)
>  			return ret;
>  
>  		tpm1_do_selftest(chip);
>  
> -		release_locality(chip, 0);
> +		tpm_tis_release_locality(chip, 0);
>  	}
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 2deef11c88db..13bdcf38e56f 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -91,6 +91,8 @@ enum tpm_tis_flags {
>  
>  struct tpm_tis_data {
>  	u16 manufacturer_id;
> +	struct mutex locality_count_mutex;

BTW, why mutex and not spinlock?

Hmm.. also I think you might have given feedback already on this
but could the lock cover the whole struct instead of a counter?
I tried to dig lore for earlier response but could not find. I'm
sorry if I'm asking the same question again.

You could probably use rcu for that.

> +	unsigned int locality_count;
>  	int locality;
>  	int irq;
>  	unsigned int int_mask;
> -- 
> 2.36.1
>

BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-18  6:25   ` Lukas Wunner
  2022-10-18  7:42     ` Lino Sanfilippo
@ 2022-10-23  5:26     ` Jarkko Sakkinen
  2022-10-25  0:25       ` Lino Sanfilippo
  1 sibling, 1 reply; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-10-23  5:26 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, linux,
	linux-integrity, linux-kernel, jandryuk, pmenzel, l.sanfilippo,
	p.rosenberger

On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote:
> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
> > Implement a usage counter for the (default) locality used by the TPM TIS
> > driver:
> > Request the locality from the TPM if it has not been claimed yet, otherwise
> > only increment the counter. Also release the locality if the counter is 0
> > otherwise only decrement the counter. Ensure thread-safety by protecting
> > the counter with a mutex.
> > 
> > This allows to request and release the locality from a thread and the
> > interrupt handler at the same time without the danger to interfere with
> > each other.
> [...]
> > +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
> >  {
> >  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >  
> > -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> > +	mutex_lock(&priv->locality_count_mutex);
> > +	priv->locality_count--;
> > +	if (priv->locality_count == 0)
> > +		tpm_tis_release_locality_locked(priv, l);
> > +	mutex_unlock(&priv->locality_count_mutex);
> >  
> >  	return 0;
> >  }
> 
> Hm, any reason not to use struct kref for the locality counter?
> Provides correct memory ordering (no mutex needed) and allows for
> calling a release function too upon reaching 0.

I proposed for last version kref. I have no idea why this is still
using mutex. And now I apparently have proposed rcu for the whole
struct (forgot what I had put my feedback for earlier version).

This keeps being confusing patch as the commit message does not
really go to the bottom line why mutex is really the best possible
choice here.

BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-23  4:40   ` Jarkko Sakkinen
@ 2022-10-25  0:15     ` Lino Sanfilippo
  2022-11-01  1:06       ` Jarkko Sakkinen
  0 siblings, 1 reply; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-25  0:15 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger


On 23.10.22 06:40, Jarkko Sakkinen wrote:
> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Implement a usage counter for the (default) locality used by the TPM TIS
>> driver:
>> Request the locality from the TPM if it has not been claimed yet, otherwise
>> only increment the counter. Also release the locality if the counter is 0
>> otherwise only decrement the counter. Ensure thread-safety by protecting
>> the counter with a mutex.
>>
>> This allows to request and release the locality from a thread and the
>> interrupt handler at the same time without the danger to interfere with
>> each other.
>>
>> By doing this refactor the names of the amended functions to use the proper
>> prefix.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> Tested-by: Michael Niewöhner <linux@mniewoehner.de>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++-----------
>>  drivers/char/tpm/tpm_tis_core.h |  2 +
>>  2 files changed, 53 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 4336f7ea8c2b..79dfab65976f 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l)
>>  	return false;
>>  }
>>
>> -static int release_locality(struct tpm_chip *chip, int l)
>> +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l)
>
> Nit: usually you would actually use "unlocked" here, not locked.
>
> Probably best name would be __tpm_tis_release_locality().


Agreed. This is also consistent with the naming scheme used for many other
kernel functions.

>
>> +{
>> +	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
>>  {
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>
>> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>> +	mutex_lock(&priv->locality_count_mutex);
>> +	priv->locality_count--;
>> +	if (priv->locality_count == 0)
>> +		tpm_tis_release_locality_locked(priv, l);
>> +	mutex_unlock(&priv->locality_count_mutex);
>>
>>  	return 0;
>>  }
>
> Since the function pointer has the word "relinquish" and not "release",
> perhaps these should also use that word for consistency.
>
>>
>> -static int request_locality(struct tpm_chip *chip, int l)
>> +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l)
>>  {
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>  	unsigned long stop, timeout;
>> @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l)
>>  	return -1;
>>  }
>>
>> +static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>> +{
>> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&priv->locality_count_mutex);
>> +	if (priv->locality_count == 0)
>> +		ret = tpm_tis_request_locality_locked(chip, l);
>> +	if (!ret)
>> +		priv->locality_count++;
>> +	mutex_unlock(&priv->locality_count_mutex);
>> +	return ret;
>> +}
>> +
>>  static u8 tpm_tis_status(struct tpm_chip *chip)
>>  {
>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
>>  	if (vendor != TPM_VID_INTEL)
>>  		return 0;
>>
>> -	if (request_locality(chip, 0) != 0)
>> +	if (tpm_tis_request_locality(chip, 0) != 0)
>>  		return -EBUSY;
>>
>>  	rc = tpm_tis_send_data(chip, cmd_getticks, len);
>> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
>>
>>  out:
>>  	tpm_tis_ready(chip);
>> -	release_locality(chip, priv->locality);
>> +	tpm_tis_release_locality(chip, priv->locality);
>>
>>  	return rc;
>>  }
>> @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>>  	cap_t cap;
>>  	int ret;
>>
>> -	ret = request_locality(chip, 0);
>> +	ret = tpm_tis_request_locality(chip, 0);
>>  	if (ret < 0)
>>  		return ret;
>>
>> @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>>  	else
>>  		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>>
>> -	release_locality(chip, 0);
>> +	tpm_tis_release_locality(chip, 0);
>>
>>  	return ret;
>>  }
>> @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	}
>>  	priv->irq = irq;
>>
>> -	rc = request_locality(chip, 0);
>> +	rc = tpm_tis_request_locality(chip, 0);
>>  	if (rc < 0)
>>  		return rc;
>>
>>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>>  			   &original_int_vec);
>>  	if (rc < 0) {
>> -		release_locality(chip, priv->locality);
>> +		tpm_tis_release_locality(chip, priv->locality);
>>  		return rc;
>>  	}
>>
>>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>>  	if (rc < 0) {
>> -		release_locality(chip, priv->locality);
>> +		tpm_tis_release_locality(chip, priv->locality);
>>  		return rc;
>>  	}
>>
>>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
>>  	if (rc < 0) {
>> -		release_locality(chip, priv->locality);
>> +		tpm_tis_release_locality(chip, priv->locality);
>>  		return rc;
>>  	}
>>
>>  	/* Clear all existing */
>>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
>>  	if (rc < 0) {
>> -		release_locality(chip, priv->locality);
>> +		tpm_tis_release_locality(chip, priv->locality);
>>  		return rc;
>>  	}
>>
>> @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
>>  			     intmask | TPM_GLOBAL_INT_ENABLE);
>>  	if (rc < 0) {
>> -		release_locality(chip, priv->locality);
>> +		tpm_tis_release_locality(chip, priv->locality);
>>  		return rc;
>>  	}
>>
>> -	release_locality(chip, priv->locality);
>> +	tpm_tis_release_locality(chip, priv->locality);
>>  	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
>>
>>  	/* Generate an interrupt by having the core call through to
>> @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = {
>>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>>  	.req_canceled = tpm_tis_req_canceled,
>> -	.request_locality = request_locality,
>> -	.relinquish_locality = release_locality,
>> +	.request_locality = tpm_tis_request_locality,
>> +	.relinquish_locality = tpm_tis_release_locality,
>>  	.clk_enable = tpm_tis_clkrun_enable,
>>  };
>>
>> @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  	priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
>>  	priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
>>  	priv->phy_ops = phy_ops;
>> +	priv->locality_count = 0;
>> +	mutex_init(&priv->locality_count_mutex);
>>
>>  	dev_set_drvdata(&chip->dev, priv);
>>
>> @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>
>>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>>
>> -	rc = request_locality(chip, 0);
>> +	rc = tpm_tis_request_locality(chip, 0);
>>  	if (rc < 0) {
>>  		rc = -ENODEV;
>>  		goto out_err;
>>  	}
>>
>>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> -	release_locality(chip, 0);
>> +	tpm_tis_release_locality(chip, 0);
>>
>>  	rc = tpm_chip_start(chip);
>>  	if (rc)
>> @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  		 * proper timeouts for the driver.
>>  		 */
>>
>> -		rc = request_locality(chip, 0);
>> +		rc = tpm_tis_request_locality(chip, 0);
>>  		if (rc < 0)
>>  			goto out_err;
>>
>>  		rc = tpm_get_timeouts(chip);
>>
>> -		release_locality(chip, 0);
>> +		tpm_tis_release_locality(chip, 0);
>>
>>  		if (rc) {
>>  			dev_err(dev, "Could not get TPM timeouts and durations\n");
>> @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>  			dev_err(&chip->dev, FW_BUG
>>  					"TPM interrupt not working, polling instead\n");
>>
>> -			rc = request_locality(chip, 0);
>> +			rc = tpm_tis_request_locality(chip, 0);
>>  			if (rc < 0)
>>  				goto out_err;
>>  			disable_interrupts(chip);
>> -			release_locality(chip, 0);
>> +			tpm_tis_release_locality(chip, 0);
>>  		}
>>  	}
>>
>> @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev)
>>  	 * an error code but for unknown reason it isn't handled.
>>  	 */
>>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
>> -		ret = request_locality(chip, 0);
>> +		ret = tpm_tis_request_locality(chip, 0);
>>  		if (ret < 0)
>>  			return ret;
>>
>>  		tpm1_do_selftest(chip);
>>
>> -		release_locality(chip, 0);
>> +		tpm_tis_release_locality(chip, 0);
>>  	}
>>
>>  	return 0;
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 2deef11c88db..13bdcf38e56f 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -91,6 +91,8 @@ enum tpm_tis_flags {
>>
>>  struct tpm_tis_data {
>>  	u16 manufacturer_id;
>> +	struct mutex locality_count_mutex;
>
> BTW, why mutex and not spinlock?
>
> Hmm.. also I think you might have given feedback already on this
> but could the lock cover the whole struct instead of a counter?
> I tried to dig lore for earlier response but could not find. I'm
> sorry if I'm asking the same question again.
>

Actually thats on me, since it took me much too long to send the v8 after the v7 review.

However the reason that we need a mutex here is that we not only increase or decrease
the locality_counter under the mutex, but also do the locality request and release by
writing to the ACCESS register. Since in the SPI case each communication over the spi bus
is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing
the register accesses.

Concerning covering the whole tpm_tis_data struct:
Most structure elements are set once at driver startup but never changed at driver
runtime. So no locking needed for these. The only exception is "flags" and "locality_count"
whereby "flags" is accessed by atomic bit manipulating functions and thus
does not need extra locking. So "locality_count" is AFAICS the only element that needs to be
protected by the mutex.


> You could probably use rcu for that.
>
>> +	unsigned int locality_count;

Note that we do not only have to increase or decrease the counter atomically, but the first
increment (when the counter goes from 1 to 0) AND requesting the locality by writing to the
ACCESS register has to be atomic.
Likewise the last decrement (when the counter goes from 1 to 0) AND releasing the locality
has to be done atomically:

	mutex_lock(&priv->locality_count_mutex);
	priv->locality_count--;
	if (priv->locality_count == 0)
		tpm_tis_release_locality_locked(priv, l);
	mutex_unlock(&priv->locality_count_mutex);

I dont know if this is possible with RCU, especially since AFAIK RCU works with pointers instead of
integral data types.


Regards,
Lino





>>  	int locality;
>>  	int irq;
>>  	unsigned int int_mask;
>> --
>> 2.36.1
>>
>
> BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-23  5:26     ` Jarkko Sakkinen
@ 2022-10-25  0:25       ` Lino Sanfilippo
  2022-11-01  1:06         ` Jarkko Sakkinen
  0 siblings, 1 reply; 25+ messages in thread
From: Lino Sanfilippo @ 2022-10-25  0:25 UTC (permalink / raw)
  To: Jarkko Sakkinen, Lukas Wunner
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, p.rosenberger



On 23.10.22 07:26, Jarkko Sakkinen wrote:
> On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote:
>> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
>>> Implement a usage counter for the (default) locality used by the TPM TIS
>>> driver:
>>> Request the locality from the TPM if it has not been claimed yet, otherwise
>>> only increment the counter. Also release the locality if the counter is 0
>>> otherwise only decrement the counter. Ensure thread-safety by protecting
>>> the counter with a mutex.
>>>
>>> This allows to request and release the locality from a thread and the
>>> interrupt handler at the same time without the danger to interfere with
>>> each other.
>> [...]
>>> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
>>>  {
>>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>
>>> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>>> +	mutex_lock(&priv->locality_count_mutex);
>>> +	priv->locality_count--;
>>> +	if (priv->locality_count == 0)
>>> +		tpm_tis_release_locality_locked(priv, l);
>>> +	mutex_unlock(&priv->locality_count_mutex);
>>>
>>>  	return 0;
>>>  }
>>
>> Hm, any reason not to use struct kref for the locality counter?
>> Provides correct memory ordering (no mutex needed) and allows for
>> calling a release function too upon reaching 0.
>
> I proposed for last version kref. I have no idea why this is still
> using mutex. And now I apparently have proposed rcu for the whole
> struct (forgot what I had put my feedback for earlier version).
>
> This keeps being confusing patch as the commit message does not
> really go to the bottom line why mutex is really the best possible
> choice here.
>


I actually tried to implement this via kref but then came to the
conclusion it is rather not a good choice for our case. Please
see my response to your former request to implement this via kref:

https://lore.kernel.org/all/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/

Regards,
Lino


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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-25  0:15     ` Lino Sanfilippo
@ 2022-11-01  1:06       ` Jarkko Sakkinen
  2022-11-04 16:18         ` Lino Sanfilippo
  0 siblings, 1 reply; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-11-01  1:06 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, l.sanfilippo, lukas, p.rosenberger

On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote:
> 
> On 23.10.22 06:40, Jarkko Sakkinen wrote:
> > On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >>
> >> Implement a usage counter for the (default) locality used by the TPM TIS
> >> driver:
> >> Request the locality from the TPM if it has not been claimed yet, otherwise
> >> only increment the counter. Also release the locality if the counter is 0
> >> otherwise only decrement the counter. Ensure thread-safety by protecting
> >> the counter with a mutex.
> >>
> >> This allows to request and release the locality from a thread and the
> >> interrupt handler at the same time without the danger to interfere with
> >> each other.
> >>
> >> By doing this refactor the names of the amended functions to use the proper
> >> prefix.
> >>
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >> Tested-by: Michael Niewöhner <linux@mniewoehner.de>
> >> ---
> >>  drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++-----------
> >>  drivers/char/tpm/tpm_tis_core.h |  2 +
> >>  2 files changed, 53 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index 4336f7ea8c2b..79dfab65976f 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l)
> >>  	return false;
> >>  }
> >>
> >> -static int release_locality(struct tpm_chip *chip, int l)
> >> +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l)
> >
> > Nit: usually you would actually use "unlocked" here, not locked.
> >
> > Probably best name would be __tpm_tis_release_locality().
> 
> 
> Agreed. This is also consistent with the naming scheme used for many other
> kernel functions.
> 
> >
> >> +{
> >> +	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
> >>  {
> >>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>
> >> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> >> +	mutex_lock(&priv->locality_count_mutex);
> >> +	priv->locality_count--;
> >> +	if (priv->locality_count == 0)
> >> +		tpm_tis_release_locality_locked(priv, l);
> >> +	mutex_unlock(&priv->locality_count_mutex);
> >>
> >>  	return 0;
> >>  }
> >
> > Since the function pointer has the word "relinquish" and not "release",
> > perhaps these should also use that word for consistency.
> >
> >>
> >> -static int request_locality(struct tpm_chip *chip, int l)
> >> +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l)
> >>  {
> >>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>  	unsigned long stop, timeout;
> >> @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l)
> >>  	return -1;
> >>  }
> >>
> >> +static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
> >> +{
> >> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> +	int ret = 0;
> >> +
> >> +	mutex_lock(&priv->locality_count_mutex);
> >> +	if (priv->locality_count == 0)
> >> +		ret = tpm_tis_request_locality_locked(chip, l);
> >> +	if (!ret)
> >> +		priv->locality_count++;
> >> +	mutex_unlock(&priv->locality_count_mutex);
> >> +	return ret;
> >> +}
> >> +
> >>  static u8 tpm_tis_status(struct tpm_chip *chip)
> >>  {
> >>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
> >>  	if (vendor != TPM_VID_INTEL)
> >>  		return 0;
> >>
> >> -	if (request_locality(chip, 0) != 0)
> >> +	if (tpm_tis_request_locality(chip, 0) != 0)
> >>  		return -EBUSY;
> >>
> >>  	rc = tpm_tis_send_data(chip, cmd_getticks, len);
> >> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
> >>
> >>  out:
> >>  	tpm_tis_ready(chip);
> >> -	release_locality(chip, priv->locality);
> >> +	tpm_tis_release_locality(chip, priv->locality);
> >>
> >>  	return rc;
> >>  }
> >> @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
> >>  	cap_t cap;
> >>  	int ret;
> >>
> >> -	ret = request_locality(chip, 0);
> >> +	ret = tpm_tis_request_locality(chip, 0);
> >>  	if (ret < 0)
> >>  		return ret;
> >>
> >> @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
> >>  	else
> >>  		ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
> >>
> >> -	release_locality(chip, 0);
> >> +	tpm_tis_release_locality(chip, 0);
> >>
> >>  	return ret;
> >>  }
> >> @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> >>  	}
> >>  	priv->irq = irq;
> >>
> >> -	rc = request_locality(chip, 0);
> >> +	rc = tpm_tis_request_locality(chip, 0);
> >>  	if (rc < 0)
> >>  		return rc;
> >>
> >>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> >>  			   &original_int_vec);
> >>  	if (rc < 0) {
> >> -		release_locality(chip, priv->locality);
> >> +		tpm_tis_release_locality(chip, priv->locality);
> >>  		return rc;
> >>  	}
> >>
> >>  	rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
> >>  	if (rc < 0) {
> >> -		release_locality(chip, priv->locality);
> >> +		tpm_tis_release_locality(chip, priv->locality);
> >>  		return rc;
> >>  	}
> >>
> >>  	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
> >>  	if (rc < 0) {
> >> -		release_locality(chip, priv->locality);
> >> +		tpm_tis_release_locality(chip, priv->locality);
> >>  		return rc;
> >>  	}
> >>
> >>  	/* Clear all existing */
> >>  	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
> >>  	if (rc < 0) {
> >> -		release_locality(chip, priv->locality);
> >> +		tpm_tis_release_locality(chip, priv->locality);
> >>  		return rc;
> >>  	}
> >>
> >> @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> >>  	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> >>  			     intmask | TPM_GLOBAL_INT_ENABLE);
> >>  	if (rc < 0) {
> >> -		release_locality(chip, priv->locality);
> >> +		tpm_tis_release_locality(chip, priv->locality);
> >>  		return rc;
> >>  	}
> >>
> >> -	release_locality(chip, priv->locality);
> >> +	tpm_tis_release_locality(chip, priv->locality);
> >>  	clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
> >>
> >>  	/* Generate an interrupt by having the core call through to
> >> @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = {
> >>  	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >>  	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >>  	.req_canceled = tpm_tis_req_canceled,
> >> -	.request_locality = request_locality,
> >> -	.relinquish_locality = release_locality,
> >> +	.request_locality = tpm_tis_request_locality,
> >> +	.relinquish_locality = tpm_tis_release_locality,
> >>  	.clk_enable = tpm_tis_clkrun_enable,
> >>  };
> >>
> >> @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >>  	priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
> >>  	priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
> >>  	priv->phy_ops = phy_ops;
> >> +	priv->locality_count = 0;
> >> +	mutex_init(&priv->locality_count_mutex);
> >>
> >>  	dev_set_drvdata(&chip->dev, priv);
> >>
> >> @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >>
> >>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> >>
> >> -	rc = request_locality(chip, 0);
> >> +	rc = tpm_tis_request_locality(chip, 0);
> >>  	if (rc < 0) {
> >>  		rc = -ENODEV;
> >>  		goto out_err;
> >>  	}
> >>
> >>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> >> -	release_locality(chip, 0);
> >> +	tpm_tis_release_locality(chip, 0);
> >>
> >>  	rc = tpm_chip_start(chip);
> >>  	if (rc)
> >> @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >>  		 * proper timeouts for the driver.
> >>  		 */
> >>
> >> -		rc = request_locality(chip, 0);
> >> +		rc = tpm_tis_request_locality(chip, 0);
> >>  		if (rc < 0)
> >>  			goto out_err;
> >>
> >>  		rc = tpm_get_timeouts(chip);
> >>
> >> -		release_locality(chip, 0);
> >> +		tpm_tis_release_locality(chip, 0);
> >>
> >>  		if (rc) {
> >>  			dev_err(dev, "Could not get TPM timeouts and durations\n");
> >> @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >>  			dev_err(&chip->dev, FW_BUG
> >>  					"TPM interrupt not working, polling instead\n");
> >>
> >> -			rc = request_locality(chip, 0);
> >> +			rc = tpm_tis_request_locality(chip, 0);
> >>  			if (rc < 0)
> >>  				goto out_err;
> >>  			disable_interrupts(chip);
> >> -			release_locality(chip, 0);
> >> +			tpm_tis_release_locality(chip, 0);
> >>  		}
> >>  	}
> >>
> >> @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev)
> >>  	 * an error code but for unknown reason it isn't handled.
> >>  	 */
> >>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> >> -		ret = request_locality(chip, 0);
> >> +		ret = tpm_tis_request_locality(chip, 0);
> >>  		if (ret < 0)
> >>  			return ret;
> >>
> >>  		tpm1_do_selftest(chip);
> >>
> >> -		release_locality(chip, 0);
> >> +		tpm_tis_release_locality(chip, 0);
> >>  	}
> >>
> >>  	return 0;
> >> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> >> index 2deef11c88db..13bdcf38e56f 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.h
> >> +++ b/drivers/char/tpm/tpm_tis_core.h
> >> @@ -91,6 +91,8 @@ enum tpm_tis_flags {
> >>
> >>  struct tpm_tis_data {
> >>  	u16 manufacturer_id;
> >> +	struct mutex locality_count_mutex;
> >
> > BTW, why mutex and not spinlock?
> >
> > Hmm.. also I think you might have given feedback already on this
> > but could the lock cover the whole struct instead of a counter?
> > I tried to dig lore for earlier response but could not find. I'm
> > sorry if I'm asking the same question again.
> >
> 
> Actually thats on me, since it took me much too long to send the v8 after the v7 review.
> 
> However the reason that we need a mutex here is that we not only increase or decrease
> the locality_counter under the mutex, but also do the locality request and release by
> writing to the ACCESS register. Since in the SPI case each communication over the spi bus
> is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing
> the register accesses.
> 
> Concerning covering the whole tpm_tis_data struct:
> Most structure elements are set once at driver startup but never changed at driver
> runtime. So no locking needed for these. The only exception is "flags" and "locality_count"
> whereby "flags" is accessed by atomic bit manipulating functions and thus
> does not need extra locking. So "locality_count" is AFAICS the only element that needs to be
> protected by the mutex.

OK, but you should should still address this in commit message, e.g.
by mentioning that in the case of SPI bus mutex is required because
the bus itself needs to be locked in the mutex.

I.e. this a claim, definitely not an argument: "Ensure thread-safety by
protecting the counter with a mutex."

BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-10-25  0:25       ` Lino Sanfilippo
@ 2022-11-01  1:06         ` Jarkko Sakkinen
  0 siblings, 0 replies; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-11-01  1:06 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Lukas Wunner, peterhuewe, jgg, stefanb, linux, linux-integrity,
	linux-kernel, jandryuk, pmenzel, l.sanfilippo, p.rosenberger

On Tue, Oct 25, 2022 at 02:25:39AM +0200, Lino Sanfilippo wrote:
> 
> 
> On 23.10.22 07:26, Jarkko Sakkinen wrote:
> > On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote:
> >> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote:
> >>> Implement a usage counter for the (default) locality used by the TPM TIS
> >>> driver:
> >>> Request the locality from the TPM if it has not been claimed yet, otherwise
> >>> only increment the counter. Also release the locality if the counter is 0
> >>> otherwise only decrement the counter. Ensure thread-safety by protecting
> >>> the counter with a mutex.
> >>>
> >>> This allows to request and release the locality from a thread and the
> >>> interrupt handler at the same time without the danger to interfere with
> >>> each other.
> >> [...]
> >>> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l)
> >>>  {
> >>>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>>
> >>> -	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> >>> +	mutex_lock(&priv->locality_count_mutex);
> >>> +	priv->locality_count--;
> >>> +	if (priv->locality_count == 0)
> >>> +		tpm_tis_release_locality_locked(priv, l);
> >>> +	mutex_unlock(&priv->locality_count_mutex);
> >>>
> >>>  	return 0;
> >>>  }
> >>
> >> Hm, any reason not to use struct kref for the locality counter?
> >> Provides correct memory ordering (no mutex needed) and allows for
> >> calling a release function too upon reaching 0.
> >
> > I proposed for last version kref. I have no idea why this is still
> > using mutex. And now I apparently have proposed rcu for the whole
> > struct (forgot what I had put my feedback for earlier version).
> >
> > This keeps being confusing patch as the commit message does not
> > really go to the bottom line why mutex is really the best possible
> > choice here.
> >
> 
> 
> I actually tried to implement this via kref but then came to the
> conclusion it is rather not a good choice for our case. Please
> see my response to your former request to implement this via kref:
> 
> https://lore.kernel.org/all/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/

OK, my bad I missed this, sorry.

BR, Jarkko

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-11-01  1:06       ` Jarkko Sakkinen
@ 2022-11-04 16:18         ` Lino Sanfilippo
  2022-11-07 16:03           ` Jarkko Sakkinen
  0 siblings, 1 reply; 25+ messages in thread
From: Lino Sanfilippo @ 2022-11-04 16:18 UTC (permalink / raw)
  To: Jarkko Sakkinen, Lino Sanfilippo
  Cc: peterhuewe, jgg, stefanb, linux, linux-integrity, linux-kernel,
	jandryuk, pmenzel, lukas, p.rosenberger



On 01.11.22 02:06, Jarkko Sakkinen wrote:
> On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote:

>> Actually thats on me, since it took me much too long to send the v8 after the v7 review.
>>
>> However the reason that we need a mutex here is that we not only increase or decrease
>> the locality_counter under the mutex, but also do the locality request and release by
>> writing to the ACCESS register. Since in the SPI case each communication over the spi bus
>> is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing
>> the register accesses.
>>
>> Concerning covering the whole tpm_tis_data struct:
>> Most structure elements are set once at driver startup but never changed at driver
>> runtime. So no locking needed for these. The only exception is "flags" and "locality_count"
>> whereby "flags" is accessed by atomic bit manipulating functions and thus
>> does not need extra locking. So "locality_count" is AFAICS the only element that needs to be
>> protected by the mutex.
> 
> OK, but you should should still address this in commit message, e.g.
> by mentioning that in the case of SPI bus mutex is required because
> the bus itself needs to be locked in the mutex.
> 
> I.e. this a claim, definitely not an argument: "Ensure thread-safety by
> protecting the counter with a mutex."
> 

Ok, I will rephrase the commit message accordingly. 
Thanks for the review!

Regards,
Lino 

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

* Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
  2022-11-04 16:18         ` Lino Sanfilippo
@ 2022-11-07 16:03           ` Jarkko Sakkinen
  0 siblings, 0 replies; 25+ messages in thread
From: Jarkko Sakkinen @ 2022-11-07 16:03 UTC (permalink / raw)
  To: Lino Sanfilippo
  Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, linux,
	linux-integrity, linux-kernel, jandryuk, pmenzel, lukas,
	p.rosenberger

On Fri, Nov 04, 2022 at 05:18:21PM +0100, Lino Sanfilippo wrote:
> 
> 
> On 01.11.22 02:06, Jarkko Sakkinen wrote:
> > On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote:
> 
> >> Actually thats on me, since it took me much too long to send the v8 after the v7 review.
> >>
> >> However the reason that we need a mutex here is that we not only increase or decrease
> >> the locality_counter under the mutex, but also do the locality request and release by
> >> writing to the ACCESS register. Since in the SPI case each communication over the spi bus
> >> is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing
> >> the register accesses.
> >>
> >> Concerning covering the whole tpm_tis_data struct:
> >> Most structure elements are set once at driver startup but never changed at driver
> >> runtime. So no locking needed for these. The only exception is "flags" and "locality_count"
> >> whereby "flags" is accessed by atomic bit manipulating functions and thus
> >> does not need extra locking. So "locality_count" is AFAICS the only element that needs to be
> >> protected by the mutex.
> > 
> > OK, but you should should still address this in commit message, e.g.
> > by mentioning that in the case of SPI bus mutex is required because
> > the bus itself needs to be locked in the mutex.
> > 
> > I.e. this a claim, definitely not an argument: "Ensure thread-safety by
> > protecting the counter with a mutex."
> > 
> 
> Ok, I will rephrase the commit message accordingly. 
> Thanks for the review!

Yeah, np. I.e. I understand your reasoning but it is easy to intuitively
think it as not the right solution. Thus, it deserves a remark, right?
:-)

BR, Jarkko

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

end of thread, other threads:[~2022-11-07 16:04 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-17 23:57 [PATCH v8 00/11] TPM IRQ fixes Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 01/11] tpm, tpm_tis: Avoid cache incoherency in test for interrupts Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 02/11] tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 03/11] tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 04/11] tpm, tmp_tis: Claim locality before writing interrupt registers Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 05/11] tpm, tpm_tis: Only handle supported interrupts Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 06/11] tpm, tpm_tis: Move interrupt mask checks into own function Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 07/11] tpm, tpm_tis: do not check for the active locality in interrupt handler Lino Sanfilippo
2022-10-23  4:32   ` Jarkko Sakkinen
2022-10-17 23:57 ` [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Lino Sanfilippo
2022-10-18  6:25   ` Lukas Wunner
2022-10-18  7:42     ` Lino Sanfilippo
2022-10-23  5:26     ` Jarkko Sakkinen
2022-10-25  0:25       ` Lino Sanfilippo
2022-11-01  1:06         ` Jarkko Sakkinen
2022-10-23  4:40   ` Jarkko Sakkinen
2022-10-25  0:15     ` Lino Sanfilippo
2022-11-01  1:06       ` Jarkko Sakkinen
2022-11-04 16:18         ` Lino Sanfilippo
2022-11-07 16:03           ` Jarkko Sakkinen
2022-10-17 23:57 ` [PATCH v8 09/11] tpm, tpm_tis: Request threaded interrupt handler Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 10/11] tpm, tpm_tis: Claim locality in " Lino Sanfilippo
2022-10-17 23:57 ` [PATCH v8 11/11] tpm, tpm_tis: Enable interrupt test Lino Sanfilippo
2022-10-23  4:33   ` Jarkko Sakkinen
2022-10-23  4:34   ` 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).