[PATCH v7 00/10] TPM IRQ fixes

[PATCH v7 00/10] TPM IRQ fixes

[Lino Sanfilippo]

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.

Patch 7 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 8 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 9 makes sure that writes to the interrupt register are effective if
done in the interrupt handler.

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


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 (10):
  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
  tmp, tmp_tis: 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 | 259 +++++++++++++++++++++-----------
 drivers/char/tpm/tpm_tis_core.h |   5 +-
 3 files changed, 178 insertions(+), 88 deletions(-)


base-commit: 941e3e7912696b9fbe3586083a7c2e102cee7a87
-- 
2.25.1

[PATCH v7 01/10] tpm, tpm_tis: Avoid cache incoherency in test for interrupts

[Lino Sanfilippo]

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 dc56b976d816..b5fd4ff46666 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -343,7 +343,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) {
@@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	int rc, irq;
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
-	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
+	     test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
 		return tpm_tis_send_main(chip, buf, len);
 
 	/* Verify receipt of the expected IRQ */
@@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	rc = tpm_tis_send_main(chip, buf, len);
 	priv->irq = irq;
 	chip->flags |= TPM_CHIP_FLAG_IRQ;
-	if (!priv->irq_tested)
+	if (!test_bit(TPM_TIS_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;
 }
 
@@ -627,7 +628,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);
@@ -647,13 +648,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;
 	}
 
@@ -693,7 +694,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)
@@ -779,7 +780,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 6c203f36b8a1..bf07379dea42 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.25.1

[PATCH v7 02/10] tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register

[Lino Sanfilippo]

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 b5fd4ff46666..0e68e4502a56 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1084,7 +1084,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.25.1

[PATCH v7 03/10] tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed

[Lino Sanfilippo]

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 0e68e4502a56..d32e93c86f48 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1077,21 +1077,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.25.1

[PATCH v7 04/10] tpm, tmp_tis: Claim locality before writing interrupt registers

[Lino Sanfilippo]

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 d32e93c86f48..09d8f04cbc81 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -756,30 +756,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.25.1

[PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

[Lino Sanfilippo]

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>
---
 drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
 drivers/char/tpm/tpm_tis_core.h |   1 +
 2 files changed, 72 insertions(+), 48 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09d8f04cbc81..c13599e94ab6 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 which 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;
 }
 
@@ -1007,8 +1029,39 @@ 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);
@@ -1042,32 +1095,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);
@@ -1098,7 +1125,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");
 
@@ -1145,13 +1174,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 bf07379dea42..e005eb99480e 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.25.1

[PATCH v7 06/10] tpm, tpm_tis: Move interrupt mask checks into own function

[Lino Sanfilippo]

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>
---
 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 c13599e94ab6..bd4eeb0b2192 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 which 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.25.1

[PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

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 bd4eeb0b2192..e50a2c78de9f 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);
@@ -668,7 +693,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);
@@ -689,7 +714,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;
 }
@@ -751,7 +776,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;
 
@@ -760,7 +785,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;
 }
@@ -785,33 +810,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;
 	}
 
@@ -819,11 +844,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
@@ -959,8 +984,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,
 };
 
@@ -994,6 +1019,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);
 
@@ -1071,14 +1098,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)
@@ -1112,13 +1139,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");
@@ -1138,11 +1165,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);
 		}
 	}
 
@@ -1209,13 +1236,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 e005eb99480e..7c6c14707e31 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.25.1

[PATCH v7 08/10] tpm, tpm_tis: Request threaded interrupt handler

[Lino Sanfilippo]

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>
---
 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 e50a2c78de9f..83b31c25e55c 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -802,8 +802,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.25.1

[PATCH v7 09/10] tpm, tpm_tis: Claim locality in interrupt handler

[Lino Sanfilippo]

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 83b31c25e55c..307a7a3a55c6 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -761,7 +761,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.25.1

[PATCH v7 10/10] tpm, tpm_tis: Enable interrupt test

[Lino Sanfilippo]

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 307a7a3a55c6..e0889c152909 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -782,11 +782,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.25.1

Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

[Jarkko Sakkinen]

On Thu, Jun 30, 2022 at 01:26:48AM +0200, Lino Sanfilippo wrote:
> 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>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
>  drivers/char/tpm/tpm_tis_core.h |   1 +
>  2 files changed, 72 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 09d8f04cbc81..c13599e94ab6 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 which 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;
>  }
>  
> @@ -1007,8 +1029,39 @@ 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);
> @@ -1042,32 +1095,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);
> @@ -1098,7 +1125,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");
>  
> @@ -1145,13 +1174,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 bf07379dea42..e005eb99480e 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.25.1
> 


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

BR, Jarkko

Re: [PATCH v7 06/10] tpm, tpm_tis: Move interrupt mask checks into own function

[Jarkko Sakkinen]

On Thu, Jun 30, 2022 at 01:26:49AM +0200, Lino Sanfilippo wrote:
> 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>
> ---
>  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 c13599e94ab6..bd4eeb0b2192 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 which 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.25.1
> 

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

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Thu, Jun 30, 2022 at 01:26:50AM +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 bd4eeb0b2192..e50a2c78de9f 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);
> @@ -668,7 +693,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);
> @@ -689,7 +714,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;
>  }
> @@ -751,7 +776,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;
>  
> @@ -760,7 +785,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;
>  }
> @@ -785,33 +810,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;
>  	}
>  
> @@ -819,11 +844,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
> @@ -959,8 +984,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,
>  };
>  
> @@ -994,6 +1019,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);
>  
> @@ -1071,14 +1098,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)
> @@ -1112,13 +1139,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");
> @@ -1138,11 +1165,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);
>  		}
>  	}
>  
> @@ -1209,13 +1236,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 e005eb99480e..7c6c14707e31 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.25.1
> 

I'm kind of thinking that should tpm_tis_data have a lock for its
contents?

I kind of doubt that we would ever need more than one lock for it,
and it would give some more ensurance to not be race, especially
when re-enabling interrupts this feels important to be "extra safe".

I looked at this commit, and did not see anything that would prevent
using a spin lock instead of mutex. With a spin lock priv can be
accessed also in the interrupt context.

So instead prepend this patch with a patch that adds:

        struct spin_lock lock;

And something like:

        static inline struct tpm_tis_data *tpm_tis_priv_get(struct tpm_chip *chip)
        {
                struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);

                spin_lock(&priv->lock); 
                return priv; 
        }

        static inline void tpm_tis_priv_put(struct tpm_tis_data *priv)
        {
                spin_unlock(&priv->lock);
        }

And change the sites where priv is used to acquire the instance with this.

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Fri, Jul 01, 2022 at 02:29:47AM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 30, 2022 at 01:26:50AM +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 bd4eeb0b2192..e50a2c78de9f 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);
> > @@ -668,7 +693,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);
> > @@ -689,7 +714,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;
> >  }
> > @@ -751,7 +776,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;
> >  
> > @@ -760,7 +785,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;
> >  }
> > @@ -785,33 +810,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;
> >  	}
> >  
> > @@ -819,11 +844,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
> > @@ -959,8 +984,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,
> >  };
> >  
> > @@ -994,6 +1019,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);
> >  
> > @@ -1071,14 +1098,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)
> > @@ -1112,13 +1139,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");
> > @@ -1138,11 +1165,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);
> >  		}
> >  	}
> >  
> > @@ -1209,13 +1236,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 e005eb99480e..7c6c14707e31 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.25.1
> > 
> 
> I'm kind of thinking that should tpm_tis_data have a lock for its
> contents?
> 
> I kind of doubt that we would ever need more than one lock for it,
> and it would give some more ensurance to not be race, especially
> when re-enabling interrupts this feels important to be "extra safe".
> 
> I looked at this commit, and did not see anything that would prevent
> using a spin lock instead of mutex. With a spin lock priv can be
> accessed also in the interrupt context.
> 
> So instead prepend this patch with a patch that adds:
> 
>         struct spin_lock lock;
> 
> And something like:
> 
>         static inline struct tpm_tis_data *tpm_tis_priv_get(struct tpm_chip *chip)
>         {
>                 struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> 
>                 spin_lock(&priv->lock); 
>                 return priv; 
>         }
> 
>         static inline void tpm_tis_priv_put(struct tpm_tis_data *priv)
>         {
>                 spin_unlock(&priv->lock);
>         }
> 
> And change the sites where priv is used to acquire the instance with this.

I.e.

        struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);

becomes:

        struct tpm_tis_data *priv = tpm_tis_priv_get(&chip);

In some simes most likely the acquirance must be done later, e.g.
because of locking order with chip's lock (perhaps).

BR, Jarkko

Re: [PATCH v7 08/10] tpm, tpm_tis: Request threaded interrupt handler

[Jarkko Sakkinen]

On Thu, Jun 30, 2022 at 01:26:51AM +0200, Lino Sanfilippo wrote:
> 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>
> ---
>  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 e50a2c78de9f..83b31c25e55c 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -802,8 +802,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.25.1
> 

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

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

On 01.07.22 01:29, Jarkko Sakkinen wrote:

>
> I'm kind of thinking that should tpm_tis_data have a lock for its
> contents?

Most of the tpm_tis_data structure elements are set once during init and
then never changed but only read. So no need for locking for these. The
exceptions I see are

- flags
- locality_count
- locality


whereby "flags" is accessed by atomic bit manipulating functions and thus
does not need extra locking. "locality_count" is protected by the locality_count_mutex.
"locality" is only set in check_locality() which is called from tpm_tis_request_locality_locked()
which holds the locality_count_mutex. So check_locality() is also protected by the locality_count_mutex
(which for this reason should probably rather be called locality_mutex since it protects both the "locality_count"
and the "locality" variable).

There is one other place check_locality() is called from, namely the interrupt handler. This is also the only
place in which "locality" could be assigned another value than 0 (aka the default). In this case there
is no lock, so this could indeed by racy.

The solution I see for this is:
1. remove the entire loop that checks for the current locality, i.e. this code:

	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
		for (i = 0; i < 5; i++)
			if (check_locality(chip, i))
				break;

So we avoid "locality" from being changed to something that is not the default.


2. grab the locality_count_mutex and protect "locality":

if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
	mutex_lock(&priv->locality_count_mutex);
		for (i = 0; i < 5; i++)
			if (check_locality(chip, i))
				break;
	mutex_unlock(&priv->locality_count_mutex);


I dont see the reason why we should store which locality is the active one, since the only thing
that ever would change it from 0 (i.e. the default which we use) to something else is some external instance.

So I would vote for option 1.



>
> I kind of doubt that we would ever need more than one lock for it,
> and it would give some more ensurance to not be race, especially
> when re-enabling interrupts this feels important to be "extra safe".
>
> I looked at this commit, and did not see anything that would prevent
> using a spin lock instead of mutex. With a spin lock priv can be
> accessed also in the interrupt context.
>
> So instead prepend this patch with a patch that adds:
>
>         struct spin_lock lock;
>
> And something like:
>
>         static inline struct tpm_tis_data *tpm_tis_priv_get(struct tpm_chip *chip)
>         {
>                 struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
>                 spin_lock(&priv->lock);
>                 return priv;
>         }
>
>         static inline void tpm_tis_priv_put(struct tpm_tis_data *priv)
>         {
>                 spin_unlock(&priv->lock);
>         }
>
> And change the sites where priv is used to acquire the instance with this.
>

In this patch we need the mutex to protect the locality counter. We have to hold the mutex
while we do a register access that requires a locality (to make sure that the locality is not
released by another thread shortly before we do the access).

We cannot do the register access while holding a spinlock, since for SPI the (SPI) bus
lock mutex is used which needs a sleepable context. That is not given while holding a spinlock,
so I think we have no choice here unfortunately.

Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
> 
> 
> On 01.07.22 01:29, Jarkko Sakkinen wrote:
> 
> >
> > I'm kind of thinking that should tpm_tis_data have a lock for its
> > contents?
> 
> Most of the tpm_tis_data structure elements are set once during init and
> then never changed but only read. So no need for locking for these. The
> exceptions I see are
> 
> - flags
> - locality_count
> - locality

I'd still go for single data struct lock, since this lock would
be taken in every transmit flow. It makes the whole thing easier
to maintain over time, and does not really affect scalability.

This brings me to another question: what does this lock protect
against given that tpm_try_get_ops() already takes tpm_mutex?
It's not clear and that should be somehow reasoned in the commit
message.

Anyway, *if* a lock is needed the granularity should be the whole
struct.

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jason Andryuk]

Hi,

This patch subject has a typo "tmp, tmp_tis" -> "tpm, tpm_tis"

On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> 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 bd4eeb0b2192..e50a2c78de9f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c

> @@ -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;
> +}
> +

This function should check that the requested locality matches the
current locality otherwise this sequence would seemingly succeed
though locality 0 is the one acquired.

tpm_tis_request_locality(chip, 0);
tpm_tis_request_locality(chip, 1);

Regards,
Jason

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

On 11.07.22 04:50, Jarkko Sakkinen wrote:
> On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
>>
>>
>> On 01.07.22 01:29, Jarkko Sakkinen wrote:
>>
>>>
>>> I'm kind of thinking that should tpm_tis_data have a lock for its
>>> contents?
>>
>> Most of the tpm_tis_data structure elements are set once during init and
>> then never changed but only read. So no need for locking for these. The
>> exceptions I see are
>>
>> - flags
>> - locality_count
>> - locality
>
> I'd still go for single data struct lock, since this lock would
> be taken in every transmit flow.

Well in both cases, transmit and receive, we end up in wait_for_tmp_stat().
Whatever lock we hold at this time cannot be taken in the interrupt
handler, since this would deadlock (wait_for_tmp_stat() waits for the interrupt
handler to complete but holds the lock that the interrupt handler needs to proceed).

So in the interrupt handler we need something that is not held during the whole
transmit/receive flow.

This is the reason why the locality_count_mutex only protects the one thing we
have to take care of in the interrupt handler, namely the locality counter.


> It makes the whole thing easier
> to maintain over time, and does not really affect scalability>
> This brings me to another question: what does this lock protect
> against given that tpm_try_get_ops() already takes tpm_mutex?
> It's not clear and that should be somehow reasoned in the commit
> message.

See above, we cannot take the tpm mutex in the interrupt handler for the same
reason.

> Anyway, *if* a lock is needed the granularity should be the whole
> struct.
>
> BR, Jarkko

Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

Hi,

On 11.07.22 21:39, Jason Andryuk wrote:
> Hi,
>
> This patch subject has a typo "tmp, tmp_tis" -> "tpm, tpm_tis"
>

Right, thanks for the hint!

> On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> 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 bd4eeb0b2192..e50a2c78de9f 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>
>> @@ -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;
>> +}
>> +
>
> This function should check that the requested locality matches the
> current locality otherwise this sequence would seemingly succeed
> though locality 0 is the one acquired.
>
> tpm_tis_request_locality(chip, 0);
> tpm_tis_request_locality(chip, 1);

This should not really be an issue since the TPM TIS driver only uses
locality 0.

>
> Regards,
> Jason

Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Mon, Jul 11, 2022 at 11:03:05PM +0200, Lino Sanfilippo wrote:
> 
> On 11.07.22 04:50, Jarkko Sakkinen wrote:
> > On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
> >>
> >>
> >> On 01.07.22 01:29, Jarkko Sakkinen wrote:
> >>
> >>>
> >>> I'm kind of thinking that should tpm_tis_data have a lock for its
> >>> contents?
> >>
> >> Most of the tpm_tis_data structure elements are set once during init and
> >> then never changed but only read. So no need for locking for these. The
> >> exceptions I see are
> >>
> >> - flags
> >> - locality_count
> >> - locality
> >
> > I'd still go for single data struct lock, since this lock would
> > be taken in every transmit flow.
> 
> Well in both cases, transmit and receive, we end up in wait_for_tmp_stat().
> Whatever lock we hold at this time cannot be taken in the interrupt
> handler, since this would deadlock (wait_for_tmp_stat() waits for the interrupt
> handler to complete but holds the lock that the interrupt handler needs to proceed).
> 
> So in the interrupt handler we need something that is not held during the whole
> transmit/receive flow.
> 
> This is the reason why the locality_count_mutex only protects the one thing we
> have to take care of in the interrupt handler, namely the locality counter.
> 
> 
> > It makes the whole thing easier
> > to maintain over time, and does not really affect scalability>
> > This brings me to another question: what does this lock protect
> > against given that tpm_try_get_ops() already takes tpm_mutex?
> > It's not clear and that should be somehow reasoned in the commit
> > message.
> 
> See above, we cannot take the tpm mutex in the interrupt handler for the same
> reason.

You should squash this then with the following patch.

Also, I'm not sure why you don't use kref for this.

> > Anyway, *if* a lock is needed the granularity should be the whole
> > struct.
> >
> > BR, Jarkko
> 
> Regards,
> Lino

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

On 11.07.22 04:50, Jarkko Sakkinen wrote:
> On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
>>
>>
>> On 01.07.22 01:29, Jarkko Sakkinen wrote:
>>
>>>
>>> I'm kind of thinking that should tpm_tis_data have a lock for its
>>> contents?
>>
>> Most of the tpm_tis_data structure elements are set once during init and
>> then never changed but only read. So no need for locking for these. The
>> exceptions I see are
>>
>> - flags
>> - locality_count
>> - locality
>
> I'd still go for single data struct lock, since this lock would
> be taken in every transmit flow. It makes the whole thing easier
> to maintain over time, and does not really affect scalability.
>

This means switching to a complete new locking scheme which affects many
parts of the TIS core code. It is also not directly related to what this patch series
is about, namely activating the interrupts for TPM TIS.

I suggest to first finish polishing this series especially since there have
only been minor issues in the last versions. Once the interrupts work we
still can think of implementing another lock handling in a follow up series.


> This brings me to another question: what does this lock protect
> against given that tpm_try_get_ops() already takes tpm_mutex?
> It's not clear and that should be somehow reasoned in the commit
> message.
>
> Anyway, *if* a lock is needed the granularity should be the whole
> struct.
>
> BR, Jarkko

Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Wed, Jul 27, 2022 at 02:16:56PM +0200, Lino Sanfilippo wrote:
> 
> 
> On 11.07.22 04:50, Jarkko Sakkinen wrote:
> > On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
> >>
> >>
> >> On 01.07.22 01:29, Jarkko Sakkinen wrote:
> >>
> >>>
> >>> I'm kind of thinking that should tpm_tis_data have a lock for its
> >>> contents?
> >>
> >> Most of the tpm_tis_data structure elements are set once during init and
> >> then never changed but only read. So no need for locking for these. The
> >> exceptions I see are
> >>
> >> - flags
> >> - locality_count
> >> - locality
> >
> > I'd still go for single data struct lock, since this lock would
> > be taken in every transmit flow. It makes the whole thing easier
> > to maintain over time, and does not really affect scalability.
> >
> 
> This means switching to a complete new locking scheme which affects many
> parts of the TIS core code. It is also not directly related to what this patch series
> is about, namely activating the interrupts for TPM TIS.
> 
> I suggest to first finish polishing this series especially since there have
> only been minor issues in the last versions. Once the interrupts work we
> still can think of implementing another lock handling in a follow up series.

So what if you would use kref instead here?

On surface this looks like ad-hoc kref but I could wrong too (as always).

BR, Jarkko

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

On 28.07.22 10:15, Jarkko Sakkinen wrote:
> On Wed, Jul 27, 2022 at 02:16:56PM +0200, Lino Sanfilippo wrote:
>>
>>
>> On 11.07.22 04:50, Jarkko Sakkinen wrote:
>>> On Mon, Jul 04, 2022 at 07:45:12PM +0200, Lino Sanfilippo wrote:
>>>>
>>>>
>>>> On 01.07.22 01:29, Jarkko Sakkinen wrote:
>>>>
>>>>>
>>>>> I'm kind of thinking that should tpm_tis_data have a lock for its
>>>>> contents?
>>>>
>>>> Most of the tpm_tis_data structure elements are set once during init and
>>>> then never changed but only read. So no need for locking for these. The
>>>> exceptions I see are
>>>>
>>>> - flags
>>>> - locality_count
>>>> - locality
>>>
>>> I'd still go for single data struct lock, since this lock would
>>> be taken in every transmit flow. It makes the whole thing easier
>>> to maintain over time, and does not really affect scalability.
>>>
>>
>> This means switching to a complete new locking scheme which affects many
>> parts of the TIS core code. It is also not directly related to what this patch series
>> is about, namely activating the interrupts for TPM TIS.
>>
>> I suggest to first finish polishing this series especially since there have
>> only been minor issues in the last versions. Once the interrupts work we
>> still can think of implementing another lock handling in a follow up series.
>
> So what if you would use kref instead here?
>

Sure, that should not be too difficult to do. I will implement this for the next version.

Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

On 04.07.22 19:45, Lino Sanfilippo wrote:
> 
> 
> On 01.07.22 01:29, Jarkko Sakkinen wrote:
> 
>>
>> I'm kind of thinking that should tpm_tis_data have a lock for its
>> contents?
> 
> Most of the tpm_tis_data structure elements are set once during init and
> then never changed but only read. So no need for locking for these. The
> exceptions I see are
> 
> - flags
> - locality_count
> - locality
> 
> 
> whereby "flags" is accessed by atomic bit manipulating functions and thus
> does not need extra locking. "locality_count" is protected by the locality_count_mutex.
> "locality" is only set in check_locality() which is called from tpm_tis_request_locality_locked()
> which holds the locality_count_mutex. So check_locality() is also protected by the locality_count_mutex
> (which for this reason should probably rather be called locality_mutex since it protects both the "locality_count"
> and the "locality" variable).
> 
> There is one other place check_locality() is called from, namely the interrupt handler. This is also the only
> place in which "locality" could be assigned another value than 0 (aka the default). In this case there
> is no lock, so this could indeed by racy.
> 
> The solution I see for this is:
> 1. remove the entire loop that checks for the current locality, i.e. this code:
> 
> 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> 		for (i = 0; i < 5; i++)
> 			if (check_locality(chip, i))
> 				break;
> 
> So we avoid "locality" from being changed to something that is not the default.
> 
> 

I wonder if we need tpm_tis_data->locality at all: the claimed locality is already tracked in
chip->locality and in TPM TIS we never use anything else than locality 0 so it never changes.

Is there any good reason not to remove it?



Regards,
Lino

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Jarkko Sakkinen]

On Thu, Jul 28, 2022 at 07:36:19PM +0200, Lino Sanfilippo wrote:
> 
> 
> On 04.07.22 19:45, Lino Sanfilippo wrote:
> > 
> > 
> > On 01.07.22 01:29, Jarkko Sakkinen wrote:
> > 
> >>
> >> I'm kind of thinking that should tpm_tis_data have a lock for its
> >> contents?
> > 
> > Most of the tpm_tis_data structure elements are set once during init and
> > then never changed but only read. So no need for locking for these. The
> > exceptions I see are
> > 
> > - flags
> > - locality_count
> > - locality
> > 
> > 
> > whereby "flags" is accessed by atomic bit manipulating functions and thus
> > does not need extra locking. "locality_count" is protected by the locality_count_mutex.
> > "locality" is only set in check_locality() which is called from tpm_tis_request_locality_locked()
> > which holds the locality_count_mutex. So check_locality() is also protected by the locality_count_mutex
> > (which for this reason should probably rather be called locality_mutex since it protects both the "locality_count"
> > and the "locality" variable).
> > 
> > There is one other place check_locality() is called from, namely the interrupt handler. This is also the only
> > place in which "locality" could be assigned another value than 0 (aka the default). In this case there
> > is no lock, so this could indeed by racy.
> > 
> > The solution I see for this is:
> > 1. remove the entire loop that checks for the current locality, i.e. this code:
> > 
> > 	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> > 		for (i = 0; i < 5; i++)
> > 			if (check_locality(chip, i))
> > 				break;
> > 
> > So we avoid "locality" from being changed to something that is not the default.
> > 
> > 
> 
> I wonder if we need tpm_tis_data->locality at all: the claimed locality is already tracked in
> chip->locality and in TPM TIS we never use anything else than locality 0 so it never changes.
> 
> Is there any good reason not to remove it?

I think it would be a great idea to unify them.

BR, Jarkko

Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

[Jason Andryuk]

On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:

> @@ -1007,8 +1029,39 @@ 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)

Hi, you may already have fixed this, but I just saw:

error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
 1144 |         if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
      |         ^~

> +               intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
> +               dev_dbg(dev, "\tLocality Change Int Support\n");

You need { } for the block.

Regards,
Jason

Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

[Lino Sanfilippo]

Hi Jason,

On 26.08.22 19:43, Jason Andryuk wrote:
> On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:
> 
>> @@ -1007,8 +1029,39 @@ 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)
> 
> Hi, you may already have fixed this, but I just saw:
> 
> error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
>  1144 |         if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>       |         ^~
> 
>> +               intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
>> +               dev_dbg(dev, "\tLocality Change Int Support\n");
> 
> You need { } for the block.
> 

thanks for pointing at this, I will fix it in the next version of this series.

Regards,
Lino


> Regards,
> Jason

Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

[Lino Sanfilippo]

Hi Jason,

On 26.08.22 at 19:43, Jason Andryuk wrote:
> On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:
>
>> @@ -1007,8 +1029,39 @@ 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)
>
> Hi, you may already have fixed this, but I just saw:
>
> error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
>  1144 |         if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>       |         ^~
>
>> +               intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
>> +               dev_dbg(dev, "\tLocality Change Int Support\n");
>
> You need { } for the block.
>

Thanks for pointing at this. I will fix it in the next version of the series.

Best Regards,
Lino


> Regards,
> Jason
>

Re: [PATCH v7 07/10] tmp, tmp_tis: Implement usage counter for locality

[Lino Sanfilippo]

Hi Jarkko,

On 28.07.22 at 17:45, Lino Sanfilippo wrote:

>>>
>>> This means switching to a complete new locking scheme which affects many
>>> parts of the TIS core code. It is also not directly related to what this patch series
>>> is about, namely activating the interrupts for TPM TIS.
>>>
>>> I suggest to first finish polishing this series especially since there have
>>> only been minor issues in the last versions. Once the interrupts work we
>>> still can think of implementing another lock handling in a follow up series.
>>
>> So what if you would use kref instead here?
>>
>
> Sure, that should not be too difficult to do. I will implement this for the next version.
>
> Regards,
> Lino
>

First of all, sorry for this very late reply. Unfortunately in the last weeks I was
not able to work further on this series due to my private situation.
Nevertheless I tried to implement your suggestion (using krefs for the locality counting)
meanwhile. However krefs turned out to be a rather bad fit for this task.

The reason is that for the locality handling we have to perform a certain action (i.e.
writing to the access register) on two occasions:

1. When the locality is requested while no locality is active
2. When the locality has been released the number of times it has been requested before

Since a kref is designed to track the lifetime of an object which is freed as soon as the
kref counter hits 0, it starts with a counter of 1 when it is created, not with a counter 0
(as we would need it, since at the beginning nothing has claimed the locality yet).
Furthermore while kref provides a built-in mechanism to execute a function when the counter
hits 0 it does not provide anything similar for the case that the counter is increased the
first time (i.e when we want to claim the locality by writing to the access register).

So although certainly doable I do not see much gain from using krefs in this case. Again,
sorry for this late reply.

Regards,
Lino