From: Lino Sanfilippo <[email protected]>
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
5).
Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
by Jarkko.
Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
at driver runtime so this variable can be read without the need to protect
it against concurrent modification.
Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
done in the interrupt handler.
Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
and INTERRUPT_ENABLE registers are effective by holding the locality.
Patch 13 makes sure that the TPM is properly initialized after power cycle
before the irq test is done.
Patch 14 enables the test for interrupts by setting the required flag
before the test is executed.
Changes in v11:
- adjust patches 4,5 and 14 slightly to void the consecutive removal and
re-addition of the "ret" variable in tpm_tis_gen_interrupt()
Changes in v10:
- fix typo in subject line as pointed out by Jason Andryuk
- improve patch "tpm, tpm_tis: Claim locality before writing interrupt
registers" by extending the scope with held locality". For this reason
the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
been removed.
- add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
(PATCH 13)
- add fix to restore the old interrupt vector at error (PATCH 4)
Changes in v9:
- add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
- improve the commit message for patch 8 as requested by Jarkko
- improved functions naming
- changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
Jarko has been removed
Changes in v8:
- tpm_tis_data->locality is not changed at runtime any more so that it can
be read without any protection against concurrent modification.
- add missing brackets as pointed out by Jason Andryuk
Changes in v7:
- moved interrupt flag checks into an own function as suggested by Jarkko
- added "Tested-by" tags for Tests from Michael Niewöhner
- fixed one comment
Changes in v6:
- set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
in an own bitfield
- improve commit messages
- use int_mask instead of irqs_in_use as variable name
- use sts_mask instead of active_irqs as variable name
- squash patch 5 and 6
- prefix functions with tpm_tis_
- remove "fixes" tag
Changes in v5:
- improve commit message of patch 1 as requested by Jarko
- drop patch that makes locality handling simpler by only claiming it at
driver startup and releasing it at driver shutdown (requested by Jarko)
- drop patch that moves the interrupt test from tpm_tis_send()
to tmp_tis_probe_irq_single() as requested by Jarko
- add patch to make locality handling threadsafe so that it can also be
done by the irq handler
- separate logical changes into own patches
- always request threaded interrupt handler
Changes in v4:
- only request threaded irq in case of SPI as requested by Jarko.
- reimplement patch 2 to limit locality handling changes to the TIS core.
- separate fixes from cleanups as requested by Jarko.
- rephrase commit messages
Changes in v3:
- fixed compiler error reported by kernel test robot
- rephrased commit message as suggested by Jarko Sakkinen
- added Reviewed-by tag
Changes in v2:
- rebase against 5.12
- free irq on error path
Lino Sanfilippo (14):
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, tpm_tis: Do not skip reset of original interrupt vector
tpm, tpm_tis: Claim locality before writing interrupt registers
tpm, tpm_tis: Only handle supported interrupts
tpm, tpm_tis: Move interrupt mask checks into own function
tpm, tpm_tis: do not check for the active locality in interrupt
handler
tpm, tpm: Implement usage counter for locality
tpm, tpm_tis: Request threaded interrupt handler
tpm, tpm_tis: Claim locality in interrupt handler
tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
tpm, tpm_tis: startup chip before testing for interrupts
tpm, tpm_tis: Enable interrupt test
drivers/char/tpm/tpm-chip.c | 38 ++--
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
drivers/char/tpm/tpm_tis_core.h | 5 +-
5 files changed, 214 insertions(+), 131 deletions(-)
base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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 <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 120 +++++++++++++++++++-------------
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 73 insertions(+), 48 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 49848268f547..2694b1bb77de 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -53,41 +53,63 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
long rc;
u8 status;
bool canceled = false;
+ u8 sts_mask = 0;
+ int ret = 0;
/* check current status */
status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;
- stop = jiffies + timeout;
+ /* check what status changes can be handled by irqs */
+ if (priv->int_mask & TPM_INTF_STS_VALID_INT)
+ sts_mask |= TPM_STS_VALID;
- if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+ if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
+ sts_mask |= TPM_STS_DATA_AVAIL;
+
+ if (priv->int_mask & TPM_INTF_CMD_READY_INT)
+ sts_mask |= TPM_STS_COMMAND_READY;
+
+ sts_mask &= mask;
+
+ stop = jiffies + timeout;
+ /* process status changes with irq support */
+ if (sts_mask) {
+ ret = -ETIME;
again:
timeout = stop - jiffies;
if ((long)timeout <= 0)
return -ETIME;
rc = wait_event_interruptible_timeout(*queue,
- wait_for_tpm_stat_cond(chip, mask, check_cancel,
+ wait_for_tpm_stat_cond(chip, sts_mask, check_cancel,
&canceled),
timeout);
if (rc > 0) {
if (canceled)
return -ECANCELED;
- return 0;
+ ret = 0;
}
if (rc == -ERESTARTSYS && freezing(current)) {
clear_thread_flag(TIF_SIGPENDING);
goto again;
}
- } else {
- do {
- usleep_range(priv->timeout_min,
- priv->timeout_max);
- status = chip->ops->status(chip);
- if ((status & mask) == mask)
- return 0;
- } while (time_before(jiffies, stop));
}
+
+ if (ret)
+ return ret;
+
+ mask &= ~sts_mask;
+ if (!mask) /* all done */
+ return 0;
+ /* process status changes without irq support */
+ do {
+ status = chip->ops->status(chip);
+ if ((status & mask) == mask)
+ return 0;
+ usleep_range(priv->timeout_min,
+ priv->timeout_max);
+ } while (time_before(jiffies, stop));
return -ETIME;
}
@@ -1001,8 +1023,40 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (rc < 0)
goto out_err;
- intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
- TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+ /* Figure out the capabilities */
+ rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
+ if (rc < 0)
+ goto out_err;
+
+ dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
+ intfcaps);
+ if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
+ dev_dbg(dev, "\tBurst Count Static\n");
+ if (intfcaps & TPM_INTF_CMD_READY_INT) {
+ intmask |= TPM_INTF_CMD_READY_INT;
+ dev_dbg(dev, "\tCommand Ready Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
+ dev_dbg(dev, "\tInterrupt Edge Falling\n");
+ if (intfcaps & TPM_INTF_INT_EDGE_RISING)
+ dev_dbg(dev, "\tInterrupt Edge Rising\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
+ dev_dbg(dev, "\tInterrupt Level Low\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
+ dev_dbg(dev, "\tInterrupt Level High\n");
+ if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT) {
+ intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
+ dev_dbg(dev, "\tLocality Change Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_STS_VALID_INT) {
+ intmask |= TPM_INTF_STS_VALID_INT;
+ dev_dbg(dev, "\tSts Valid Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+ intmask |= TPM_INTF_DATA_AVAIL_INT;
+ dev_dbg(dev, "\tData Avail Int Support\n");
+ }
+
intmask &= ~TPM_GLOBAL_INT_ENABLE;
rc = request_locality(chip, 0);
@@ -1036,32 +1090,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);
@@ -1092,7 +1120,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");
@@ -1139,13 +1169,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
if (rc < 0)
goto out;
- rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
- if (rc < 0)
- goto out;
-
- intmask |= TPM_INTF_CMD_READY_INT
- | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
- | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
+ intmask = priv->int_mask | TPM_GLOBAL_INT_ENABLE;
tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 695a2516dce0..2deef11c88db 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -93,6 +93,7 @@ struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
+ unsigned int int_mask;
unsigned long flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.36.1
From: Lino Sanfilippo <[email protected]>
In tpm_tis_resume() make sure that the locality has been claimed when
tpm_tis_reenable_interrupts() is called. Otherwise the writings to the
register might not have any effect.
Fixes: 45baa1d1fa39 ("tpm_tis: Re-enable interrupts upon (S3) resume")
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index eda3b122e540..ddaf362e62c1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1221,28 +1221,27 @@ int tpm_tis_resume(struct device *dev)
struct tpm_chip *chip = dev_get_drvdata(dev);
int ret;
+ ret = tpm_tis_request_locality(chip, 0);
+ if (ret < 0)
+ return ret;
+
if (chip->flags & TPM_CHIP_FLAG_IRQ)
tpm_tis_reenable_interrupts(chip);
ret = tpm_pm_resume(dev);
if (ret)
- return ret;
+ goto out;
/*
* TPM 1.2 requires self-test on resume. This function actually returns
* an error code but for unknown reason it isn't handled.
*/
- if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
- ret = tpm_tis_request_locality(chip, 0);
- if (ret < 0)
- return ret;
-
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
tpm1_do_selftest(chip);
+out:
+ tpm_tis_relinquish_locality(chip, 0);
- tpm_tis_relinquish_locality(chip, 0);
- }
-
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(tpm_tis_resume);
#endif
--
2.36.1
From: Lino Sanfilippo <[email protected]>
If in tpm_tis_probe_irq_single() an error occurs after the original
interrupt vector has been read, restore the interrupts before the error is
returned.
Since the caller does not check the error value, return -1 in any case that
the TPM_CHIP_FLAG_IRQ flag is not set. Since the return value of function
tpm_tis_gen_interrupt() is not longer used, make it a void function.
Fixes: 1107d065fdf1 ("tpm_tis: Introduce intermediate layer for TPM access")
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 603b82ca56da..81b9726d3ed2 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -729,7 +729,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
return IRQ_HANDLED;
}
-static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
+static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
{
const char *desc = "attempting to generate an interrupt";
u32 cap2;
@@ -738,7 +738,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
ret = request_locality(chip, 0);
if (ret < 0)
- return ret;
+ return;
if (chip->flags & TPM_CHIP_FLAG_TPM2)
ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
@@ -746,8 +746,6 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
release_locality(chip, 0);
-
- return ret;
}
/* Register the IRQ and issue a command that will cause an interrupt. If an
@@ -777,42 +775,37 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
if (rc < 0)
- return rc;
+ goto restore_irqs;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
if (rc < 0)
- return rc;
+ goto restore_irqs;
/* Clear all existing */
rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
if (rc < 0)
- return rc;
-
+ goto restore_irqs;
/* Turn on */
rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
intmask | TPM_GLOBAL_INT_ENABLE);
if (rc < 0)
- return rc;
+ goto restore_irqs;
clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
/* Generate an interrupt by having the core call through to
* tpm_tis_send
*/
- rc = tpm_tis_gen_interrupt(chip);
- if (rc < 0)
- return rc;
+ tpm_tis_gen_interrupt(chip);
+restore_irqs:
/* tpm_tis_send will either confirm the interrupt is working or it
* will call disable_irq which undoes all of the above.
*/
if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
- rc = tpm_tis_write8(priv, original_int_vec,
- TPM_INT_VECTOR(priv->locality));
- if (rc < 0)
- return rc;
-
- return 1;
+ tpm_tis_write8(priv, original_int_vec,
+ TPM_INT_VECTOR(priv->locality));
+ return -1;
}
return 0;
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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.
Since now tpm_tis_gen_interrupt() is called with the locality already
claimed remove locality request and release from this function.
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 81b9726d3ed2..49848268f547 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -736,16 +736,10 @@ static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
cap_t cap;
int ret;
- ret = request_locality(chip, 0);
- if (ret < 0)
- return;
-
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);
-
- release_locality(chip, 0);
}
/* Register the IRQ and issue a command that will cause an interrupt. If an
@@ -768,10 +762,16 @@ 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)
@@ -805,10 +805,12 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
tpm_tis_write8(priv, original_int_vec,
TPM_INT_VECTOR(priv->locality));
- return -1;
+ rc = -1;
}
- return 0;
+ release_locality(chip, priv->locality);
+
+ return rc;
}
/* Try to find the IRQ the TPM is using. This is for legacy x86 systems that
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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 <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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 94a2bfb244b3..602ca4bb8e2f 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -789,10 +789,15 @@ static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
cap_t cap;
int 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;
}
/* Register the IRQ and issue a command that will cause an interrupt. If an
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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 <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
---
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 3891499f9877..eda3b122e540 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -772,7 +772,9 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
wake_up_interruptible(&priv->int_queue);
/* Clear interrupts handled with TPM_EOI */
+ tpm_tis_request_locality(chip, 0);
rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), interrupt);
+ tpm_tis_relinquish_locality(chip, 0);
if (rc < 0)
return IRQ_NONE;
--
2.36.1
From: Lino Sanfilippo <[email protected]>
Clean up wait_for_tpm_stat() by moving multiple similar interrupt mask
checks into an own function.
Signed-off-by: Lino Sanfilippo <[email protected]>
Suggested-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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 2694b1bb77de..65d1ec7eb4c3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -44,6 +44,20 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
return false;
}
+static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
+{
+ if (!(int_mask & TPM_INTF_STS_VALID_INT))
+ sts_mask &= ~TPM_STS_VALID;
+
+ if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
+ sts_mask &= ~TPM_STS_DATA_AVAIL;
+
+ if (!(int_mask & TPM_INTF_CMD_READY_INT))
+ sts_mask &= ~TPM_STS_COMMAND_READY;
+
+ return sts_mask;
+}
+
static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
unsigned long timeout, wait_queue_head_t *queue,
bool check_cancel)
@@ -53,7 +67,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
long rc;
u8 status;
bool canceled = false;
- u8 sts_mask = 0;
+ u8 sts_mask;
int ret = 0;
/* check current status */
@@ -61,17 +75,10 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
if ((status & mask) == mask)
return 0;
+ sts_mask = mask & (TPM_STS_VALID | TPM_STS_DATA_AVAIL |
+ TPM_STS_COMMAND_READY);
/* check what status changes can be handled by irqs */
- if (priv->int_mask & TPM_INTF_STS_VALID_INT)
- sts_mask |= TPM_STS_VALID;
-
- if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
- sts_mask |= TPM_STS_DATA_AVAIL;
-
- if (priv->int_mask & TPM_INTF_CMD_READY_INT)
- sts_mask |= TPM_STS_COMMAND_READY;
-
- sts_mask &= mask;
+ sts_mask = tpm_tis_filter_sts_mask(priv->int_mask, sts_mask);
stop = jiffies + timeout;
/* process status changes with irq support */
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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 <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d2c9c9d76893..603b82ca56da 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1091,21 +1091,21 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
goto out_err;
}
- if (irq) {
+ if (irq)
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
irq);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
- dev_err(&chip->dev, FW_BUG
+ else
+ tpm_tis_probe_irq(chip, intmask);
+
+ if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+ dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
- rc = request_locality(chip, 0);
- if (rc < 0)
- goto out_err;
- disable_interrupts(chip);
- release_locality(chip, 0);
- }
- } else {
- tpm_tis_probe_irq(chip, intmask);
+ rc = request_locality(chip, 0);
+ if (rc < 0)
+ goto out_err;
+ disable_interrupts(chip);
+ release_locality(chip, 0);
}
}
--
2.36.1
From: Lino Sanfilippo <[email protected]>
In tpm_tis_gen_interrupt() a request for a property value is sent to the
TPM to test if interrupts are generated. However after a power cycle the
TPM responds with TPM_RC_INITIALIZE which indicates that the TPM is not
yet properly initialized.
Fix this by first starting the TPM up before the request is sent. For this
the startup implementation is removed from tpm_chip_register() and put
into the new function tpm_chip_startup() which is called before the
interrupts are tested.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 38 +++++++++++++++++++++------------
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm_tis_core.c | 5 +++++
3 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 783d65fc71f0..370aa1f529f2 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -543,6 +543,30 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
return rc;
}
+/*
+ * tpm_chip_startup() - performs auto startup and allocates the PCRs
+ * @chip: TPM chip to use.
+ */
+int tpm_chip_startup(struct tpm_chip *chip)
+{
+ int rc;
+
+ rc = tpm_chip_start(chip);
+ if (rc)
+ return rc;
+
+ rc = tpm_auto_startup(chip);
+ if (rc)
+ goto stop;
+
+ rc = tpm_get_pcr_allocation(chip);
+stop:
+ tpm_chip_stop(chip);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_chip_startup);
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -558,20 +582,6 @@ int tpm_chip_register(struct tpm_chip *chip)
{
int rc;
- rc = tpm_chip_start(chip);
- if (rc)
- return rc;
- rc = tpm_auto_startup(chip);
- if (rc) {
- tpm_chip_stop(chip);
- return rc;
- }
-
- rc = tpm_get_pcr_allocation(chip);
- tpm_chip_stop(chip);
- if (rc)
- return rc;
-
tpm_sysfs_add_device(chip);
tpm_bios_log_setup(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 24ee4e1cc452..919bb0b88b12 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -190,6 +190,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
delay_msec * 1000);
};
+int tpm_chip_startup(struct tpm_chip *chip);
int tpm_chip_start(struct tpm_chip *chip);
void tpm_chip_stop(struct tpm_chip *chip);
struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index ddaf362e62c1..94a2bfb244b3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1129,6 +1129,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
/* INTERRUPT Setup */
init_waitqueue_head(&priv->read_queue);
init_waitqueue_head(&priv->int_queue);
+
+ rc = tpm_chip_startup(chip);
+ if (rc)
+ goto out_err;
+
if (irq != -1) {
/*
* Before doing irq testing issue a command to the TPM in polling mode
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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. Since in case of SPI the register
accesses are locked by means of the SPI bus mutex use a sleepable lock
(i.e. also a mutex) to ensure thread-safety of the counter which may be
accessed by both a userspace thread and the interrupt handler.
By doing this refactor the names of the amended functions to use a more
appropriate prefix.
Signed-off-by: Lino Sanfilippo <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 63 +++++++++++++++++++++++----------
drivers/char/tpm/tpm_tis_core.h | 2 ++
2 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d705dfb64cf9..f00f1057fd27 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_relinquish_locality(struct tpm_tis_data *priv, int l)
+{
+ tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
+
+ return 0;
+}
+
+static int tpm_tis_relinquish_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_relinquish_locality(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(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(chip, l);
+ if (!ret)
+ priv->locality_count++;
+ mutex_unlock(&priv->locality_count_mutex);
+ return ret;
+}
+
static u8 tpm_tis_status(struct tpm_chip *chip)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
@@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
if (vendor != TPM_VID_INTEL)
return 0;
- if (request_locality(chip, 0) != 0)
+ if (tpm_tis_request_locality(chip, 0) != 0)
return -EBUSY;
rc = tpm_tis_send_data(chip, cmd_getticks, len);
@@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
out:
tpm_tis_ready(chip);
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
@@ -788,14 +813,14 @@ 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_relinquish_locality(chip, priv->locality);
return rc;
}
@@ -834,7 +859,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
rc = -1;
}
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
@@ -950,8 +975,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_relinquish_locality,
.clk_enable = tpm_tis_clkrun_enable,
};
@@ -985,6 +1010,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);
@@ -1063,14 +1090,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_relinquish_locality(chip, 0);
rc = tpm_chip_start(chip);
if (rc)
@@ -1104,13 +1131,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_relinquish_locality(chip, 0);
if (rc) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
@@ -1130,11 +1157,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_relinquish_locality(chip, 0);
}
}
@@ -1201,13 +1228,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_relinquish_locality(chip, 0);
}
return 0;
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 2deef11c88db..13bdcf38e56f 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -91,6 +91,8 @@ enum tpm_tis_flags {
struct tpm_tis_data {
u16 manufacturer_id;
+ struct mutex locality_count_mutex;
+ unsigned int locality_count;
int locality;
int irq;
unsigned int int_mask;
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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 <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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 f00f1057fd27..3891499f9877 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -805,8 +805,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
int rc;
u32 int_status;
- if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
- dev_name(&chip->dev), chip) != 0) {
+
+ rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
+ tis_int_handler, IRQF_ONESHOT | flags,
+ dev_name(&chip->dev), chip);
+ if (rc) {
dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
irq);
return -1;
--
2.36.1
From: Lino Sanfilippo <[email protected]>
After driver initialization tpm_tis_data->locality may only be modified in
case of a LOCALITY CHANGE interrupt. In this case the interrupt handler
iterates over all localities only to assign the active one to
tpm_tis_data->locality.
However this information is never used any more, so the assignment is not
needed.
Furthermore without the assignment tpm_tis_data->locality cannot change any
more at driver runtime, and thus no protection against concurrent
modification is required when the variable is read at other places.
So remove this iteration entirely.
Signed-off-by: Lino Sanfilippo <[email protected]>
Acked-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 65d1ec7eb4c3..d705dfb64cf9 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -728,7 +728,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
u32 interrupt;
- int i, rc;
+ int rc;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
if (rc < 0)
@@ -740,10 +740,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
- if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
- for (i = 0; i < 5; i++)
- if (check_locality(chip, i))
- break;
+
if (interrupt &
(TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
TPM_INTF_CMD_READY_INT))
--
2.36.1
From: Lino Sanfilippo <[email protected]>
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
tpm_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 <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
---
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_core.c | 21 +++++++++++----------
drivers/char/tpm/tpm_tis_core.h | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index bcff6429e0b4..ce43412eb398 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -226,7 +226,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
irq = tpm_info->irq;
if (itpm || is_itpm(ACPI_COMPANION(dev)))
- phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
+ set_bit(TPM_TIS_ITPM_WORKAROUND, &phy->priv.flags);
return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
ACPI_HANDLE(dev));
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 757623bacfd5..c0008efb95dc 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -351,7 +351,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc, status, burstcnt;
size_t count = 0;
- bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
+ bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -484,7 +484,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
int rc, irq;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+ if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
+ test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
return tpm_tis_send_main(chip, buf, len);
/* Verify receipt of the expected IRQ */
@@ -494,11 +495,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
rc = tpm_tis_send_main(chip, buf, len);
priv->irq = irq;
chip->flags |= TPM_CHIP_FLAG_IRQ;
- if (!priv->irq_tested)
+ if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
tpm_msleep(1);
- if (!priv->irq_tested)
+ if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
disable_interrupts(chip);
- priv->irq_tested = true;
+ set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
return rc;
}
@@ -641,7 +642,7 @@ static int probe_itpm(struct tpm_chip *chip)
size_t len = sizeof(cmd_getticks);
u16 vendor;
- if (priv->flags & TPM_TIS_ITPM_WORKAROUND)
+ if (test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags))
return 0;
rc = tpm_tis_read16(priv, TPM_DID_VID(0), &vendor);
@@ -661,13 +662,13 @@ static int probe_itpm(struct tpm_chip *chip)
tpm_tis_ready(chip);
- priv->flags |= TPM_TIS_ITPM_WORKAROUND;
+ set_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
rc = tpm_tis_send_data(chip, cmd_getticks, len);
if (rc == 0)
dev_info(&chip->dev, "Detected an iTPM.\n");
else {
- priv->flags &= ~TPM_TIS_ITPM_WORKAROUND;
+ clear_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
rc = -EFAULT;
}
@@ -707,7 +708,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
if (interrupt == 0)
return IRQ_NONE;
- priv->irq_tested = true;
+ set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -793,7 +794,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
if (rc < 0)
return rc;
- priv->irq_tested = false;
+ clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
/* Generate an interrupt by having the core call through to
* tpm_tis_send
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 66a5a13cd1df..695a2516dce0 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,13 +86,13 @@ enum tis_defaults {
enum tpm_tis_flags {
TPM_TIS_ITPM_WORKAROUND = BIT(0),
TPM_TIS_INVALID_STATUS = BIT(1),
+ TPM_TIS_IRQ_TESTED = BIT(2),
};
struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
- bool irq_tested;
unsigned long flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.36.1
Hi Lino,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 30a0b95b1335e12efef89dd78518ed3e4a71a763]
url: https://github.com/intel-lab-lkp/linux/commits/Lino-Sanfilippo/TPM-IRQ-fixes/20221124-215932
base: 30a0b95b1335e12efef89dd78518ed3e4a71a763
patch link: https://lore.kernel.org/r/20221124135538.31020-6-LinoSanfilippo%40gmx.de
patch subject: [PATCH v11 05/14] tpm, tpm_tis: Claim locality before writing interrupt registers
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aff4f2aead15a8dd374e43112b120603e7008156
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Lino-Sanfilippo/TPM-IRQ-fixes/20221124-215932
git checkout aff4f2aead15a8dd374e43112b120603e7008156
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/char/tpm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/char/tpm/tpm_tis_core.c: In function 'tpm_tis_gen_interrupt':
>> drivers/char/tpm/tpm_tis_core.c:737:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
737 | int ret;
| ^~~
vim +/ret +737 drivers/char/tpm/tpm_tis_core.c
41a5e1cf1fe151 Christophe Ricard 2016-05-19 731
c80ceaa1aa1671 Lino Sanfilippo 2022-11-24 732 static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
eb5854e764b91a Jarkko Sakkinen 2016-06-12 733 {
eb5854e764b91a Jarkko Sakkinen 2016-06-12 734 const char *desc = "attempting to generate an interrupt";
eb5854e764b91a Jarkko Sakkinen 2016-06-12 735 u32 cap2;
eb5854e764b91a Jarkko Sakkinen 2016-06-12 736 cap_t cap;
d53a6adfb55396 Lukasz Majczak 2021-02-16 @737 int ret;
eb5854e764b91a Jarkko Sakkinen 2016-06-12 738
e630af7dfb450d Jarkko Sakkinen 2021-05-10 739 if (chip->flags & TPM_CHIP_FLAG_TPM2)
e630af7dfb450d Jarkko Sakkinen 2021-05-10 740 ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
e630af7dfb450d Jarkko Sakkinen 2021-05-10 741 else
d53a6adfb55396 Lukasz Majczak 2021-02-16 742 ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
eb5854e764b91a Jarkko Sakkinen 2016-06-12 743 }
eb5854e764b91a Jarkko Sakkinen 2016-06-12 744
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Thu, Nov 24, 2022 at 02:55:28PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> If in tpm_tis_probe_irq_single() an error occurs after the original
> interrupt vector has been read, restore the interrupts before the error is
> returned.
>
> Since the caller does not check the error value, return -1 in any case that
> the TPM_CHIP_FLAG_IRQ flag is not set. Since the return value of function
> tpm_tis_gen_interrupt() is not longer used, make it a void function.
>
> Fixes: 1107d065fdf1 ("tpm_tis: Introduce intermediate layer for TPM access")
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 603b82ca56da..81b9726d3ed2 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -729,7 +729,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
> +static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> {
> const char *desc = "attempting to generate an interrupt";
> u32 cap2;
> @@ -738,7 +738,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>
> ret = request_locality(chip, 0);
> if (ret < 0)
> - return ret;
> + return;
>
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
> ret = tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
> @@ -746,8 +746,6 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
> ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>
> release_locality(chip, 0);
> -
> - return ret;
> }
>
> /* Register the IRQ and issue a command that will cause an interrupt. If an
> @@ -777,42 +775,37 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>
> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
> if (rc < 0)
> - return rc;
> + goto restore_irqs;
>
> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
> if (rc < 0)
> - return rc;
> + goto restore_irqs;
>
> /* Clear all existing */
> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
> if (rc < 0)
> - return rc;
> -
> + goto restore_irqs;
> /* Turn on */
> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> intmask | TPM_GLOBAL_INT_ENABLE);
> if (rc < 0)
> - return rc;
> + goto restore_irqs;
>
> clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
>
> /* Generate an interrupt by having the core call through to
> * tpm_tis_send
> */
> - rc = tpm_tis_gen_interrupt(chip);
> - if (rc < 0)
> - return rc;
> + tpm_tis_gen_interrupt(chip);
>
> +restore_irqs:
> /* tpm_tis_send will either confirm the interrupt is working or it
> * will call disable_irq which undoes all of the above.
> */
> if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> - rc = tpm_tis_write8(priv, original_int_vec,
> - TPM_INT_VECTOR(priv->locality));
> - if (rc < 0)
> - return rc;
> -
> - return 1;
> + tpm_tis_write8(priv, original_int_vec,
> + TPM_INT_VECTOR(priv->locality));
> + return -1;
> }
>
> return 0;
> --
> 2.36.1
>
Reviewed-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
On Thu, Nov 24, 2022 at 02:55:33PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> 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. Since in case of SPI the register
> accesses are locked by means of the SPI bus mutex use a sleepable lock
> (i.e. also a mutex) to ensure thread-safety of the counter which may be
> accessed by both a userspace thread and the interrupt handler.
>
> By doing this refactor the names of the amended functions to use a more
> appropriate prefix.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> Tested-by: Michael Niew?hner <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 63 +++++++++++++++++++++++----------
> drivers/char/tpm/tpm_tis_core.h | 2 ++
> 2 files changed, 47 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index d705dfb64cf9..f00f1057fd27 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_relinquish_locality(struct tpm_tis_data *priv, int l)
> +{
> + tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
> +
> + return 0;
> +}
> +
> +static int tpm_tis_relinquish_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_relinquish_locality(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(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(chip, l);
> + if (!ret)
> + priv->locality_count++;
> + mutex_unlock(&priv->locality_count_mutex);
> + return ret;
> +}
> +
> static u8 tpm_tis_status(struct tpm_chip *chip)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip)
> if (vendor != TPM_VID_INTEL)
> return 0;
>
> - if (request_locality(chip, 0) != 0)
> + if (tpm_tis_request_locality(chip, 0) != 0)
> return -EBUSY;
>
> rc = tpm_tis_send_data(chip, cmd_getticks, len);
> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip)
>
> out:
> tpm_tis_ready(chip);
> - release_locality(chip, priv->locality);
> + tpm_tis_relinquish_locality(chip, priv->locality);
>
> return rc;
> }
> @@ -788,14 +813,14 @@ 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_relinquish_locality(chip, priv->locality);
> return rc;
> }
>
> @@ -834,7 +859,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> rc = -1;
> }
>
> - release_locality(chip, priv->locality);
> + tpm_tis_relinquish_locality(chip, priv->locality);
>
> return rc;
> }
> @@ -950,8 +975,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_relinquish_locality,
> .clk_enable = tpm_tis_clkrun_enable,
> };
>
> @@ -985,6 +1010,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);
>
> @@ -1063,14 +1090,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_relinquish_locality(chip, 0);
>
> rc = tpm_chip_start(chip);
> if (rc)
> @@ -1104,13 +1131,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_relinquish_locality(chip, 0);
>
> if (rc) {
> dev_err(dev, "Could not get TPM timeouts and durations\n");
> @@ -1130,11 +1157,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_relinquish_locality(chip, 0);
> }
> }
>
> @@ -1201,13 +1228,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_relinquish_locality(chip, 0);
> }
>
> return 0;
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 2deef11c88db..13bdcf38e56f 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -91,6 +91,8 @@ enum tpm_tis_flags {
>
> struct tpm_tis_data {
> u16 manufacturer_id;
> + struct mutex locality_count_mutex;
> + unsigned int locality_count;
> int locality;
> int irq;
> unsigned int int_mask;
> --
> 2.36.1
>
Reviewed-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
On 24.11.22 14:55, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> In tpm_tis_gen_interrupt() a request for a property value is sent to the
> TPM to test if interrupts are generated. However after a power cycle the
> TPM responds with TPM_RC_INITIALIZE which indicates that the TPM is not
> yet properly initialized.
> Fix this by first starting the TPM up before the request is sent. For this
> the startup implementation is removed from tpm_chip_register() and put
> into the new function tpm_chip_startup() which is called before the
> interrupts are tested.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> drivers/char/tpm/tpm-chip.c | 38 +++++++++++++++++++++------------
> drivers/char/tpm/tpm.h | 1 +
> drivers/char/tpm/tpm_tis_core.c | 5 +++++
> 3 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 783d65fc71f0..370aa1f529f2 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -543,6 +543,30 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> return rc;
> }
>
> +/*
> + * tpm_chip_startup() - performs auto startup and allocates the PCRs
> + * @chip: TPM chip to use.
> + */
> +int tpm_chip_startup(struct tpm_chip *chip)
> +{
> + int rc;
> +
> + rc = tpm_chip_start(chip);
> + if (rc)
> + return rc;
> +
> + rc = tpm_auto_startup(chip);
> + if (rc)
> + goto stop;
> +
> + rc = tpm_get_pcr_allocation(chip);
> +stop:
> + tpm_chip_stop(chip);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_startup);
> +
> /*
> * tpm_chip_register() - create a character device for the TPM chip
> * @chip: TPM chip to use.
> @@ -558,20 +582,6 @@ int tpm_chip_register(struct tpm_chip *chip)
> {
> int rc;
>
> - rc = tpm_chip_start(chip);
> - if (rc)
> - return rc;
> - rc = tpm_auto_startup(chip);
> - if (rc) {
> - tpm_chip_stop(chip);
> - return rc;
> - }
> -
> - rc = tpm_get_pcr_allocation(chip);
> - tpm_chip_stop(chip);
> - if (rc)
> - return rc;
> -
> tpm_sysfs_add_device(chip);
>
> tpm_bios_log_setup(chip);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 24ee4e1cc452..919bb0b88b12 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -190,6 +190,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
> delay_msec * 1000);
> };
>
> +int tpm_chip_startup(struct tpm_chip *chip);
> int tpm_chip_start(struct tpm_chip *chip);
> void tpm_chip_stop(struct tpm_chip *chip);
> struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index ddaf362e62c1..94a2bfb244b3 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1129,6 +1129,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> /* INTERRUPT Setup */
> init_waitqueue_head(&priv->read_queue);
> init_waitqueue_head(&priv->int_queue);
> +
> + rc = tpm_chip_startup(chip);
> + if (rc)
> + goto out_err;
> +
> if (irq != -1) {
> /*
> * Before doing irq testing issue a command to the TPM in polling mode
Jarko, thanks for the review so far. What about this patch, are there any concerns from your side?
Regards,
Lino
On Thu, Nov 24, 2022 at 02:55:37PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> In tpm_tis_gen_interrupt() a request for a property value is sent to the
> TPM to test if interrupts are generated. However after a power cycle the
> TPM responds with TPM_RC_INITIALIZE which indicates that the TPM is not
> yet properly initialized.
> Fix this by first starting the TPM up before the request is sent. For this
> the startup implementation is removed from tpm_chip_register() and put
> into the new function tpm_chip_startup() which is called before the
> interrupts are tested.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> drivers/char/tpm/tpm-chip.c | 38 +++++++++++++++++++++------------
> drivers/char/tpm/tpm.h | 1 +
> drivers/char/tpm/tpm_tis_core.c | 5 +++++
> 3 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 783d65fc71f0..370aa1f529f2 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -543,6 +543,30 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> return rc;
> }
>
> +/*
> + * tpm_chip_startup() - performs auto startup and allocates the PCRs
> + * @chip: TPM chip to use.
> + */
> +int tpm_chip_startup(struct tpm_chip *chip)
> +{
> + int rc;
> +
> + rc = tpm_chip_start(chip);
> + if (rc)
> + return rc;
> +
> + rc = tpm_auto_startup(chip);
> + if (rc)
> + goto stop;
> +
> + rc = tpm_get_pcr_allocation(chip);
> +stop:
> + tpm_chip_stop(chip);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_startup);
> +
> /*
> * tpm_chip_register() - create a character device for the TPM chip
> * @chip: TPM chip to use.
> @@ -558,20 +582,6 @@ int tpm_chip_register(struct tpm_chip *chip)
> {
> int rc;
>
> - rc = tpm_chip_start(chip);
> - if (rc)
> - return rc;
> - rc = tpm_auto_startup(chip);
> - if (rc) {
> - tpm_chip_stop(chip);
> - return rc;
> - }
> -
> - rc = tpm_get_pcr_allocation(chip);
> - tpm_chip_stop(chip);
> - if (rc)
> - return rc;
> -
> tpm_sysfs_add_device(chip);
>
> tpm_bios_log_setup(chip);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 24ee4e1cc452..919bb0b88b12 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -190,6 +190,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
> delay_msec * 1000);
> };
>
> +int tpm_chip_startup(struct tpm_chip *chip);
> int tpm_chip_start(struct tpm_chip *chip);
> void tpm_chip_stop(struct tpm_chip *chip);
> struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index ddaf362e62c1..94a2bfb244b3 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1129,6 +1129,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> /* INTERRUPT Setup */
> init_waitqueue_head(&priv->read_queue);
> init_waitqueue_head(&priv->int_queue);
> +
> + rc = tpm_chip_startup(chip);
> + if (rc)
> + goto out_err;
> +
> if (irq != -1) {
> /*
> * Before doing irq testing issue a command to the TPM in polling mode
> --
> 2.36.1
>
Reviewed-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
On Mon, Nov 28, 2022 at 01:53:35PM +0100, Lino Sanfilippo wrote:
>
> On 24.11.22 14:55, Lino Sanfilippo wrote:
> > From: Lino Sanfilippo <[email protected]>
> >
> > In tpm_tis_gen_interrupt() a request for a property value is sent to the
> > TPM to test if interrupts are generated. However after a power cycle the
> > TPM responds with TPM_RC_INITIALIZE which indicates that the TPM is not
> > yet properly initialized.
> > Fix this by first starting the TPM up before the request is sent. For this
> > the startup implementation is removed from tpm_chip_register() and put
> > into the new function tpm_chip_startup() which is called before the
> > interrupts are tested.
> >
> > Signed-off-by: Lino Sanfilippo <[email protected]>
> > ---
> > drivers/char/tpm/tpm-chip.c | 38 +++++++++++++++++++++------------
> > drivers/char/tpm/tpm.h | 1 +
> > drivers/char/tpm/tpm_tis_core.c | 5 +++++
> > 3 files changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 783d65fc71f0..370aa1f529f2 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -543,6 +543,30 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> > return rc;
> > }
> >
> > +/*
> > + * tpm_chip_startup() - performs auto startup and allocates the PCRs
> > + * @chip: TPM chip to use.
> > + */
> > +int tpm_chip_startup(struct tpm_chip *chip)
> > +{
> > + int rc;
> > +
> > + rc = tpm_chip_start(chip);
> > + if (rc)
> > + return rc;
> > +
> > + rc = tpm_auto_startup(chip);
> > + if (rc)
> > + goto stop;
> > +
> > + rc = tpm_get_pcr_allocation(chip);
> > +stop:
> > + tpm_chip_stop(chip);
> > +
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_chip_startup);
> > +
> > /*
> > * tpm_chip_register() - create a character device for the TPM chip
> > * @chip: TPM chip to use.
> > @@ -558,20 +582,6 @@ int tpm_chip_register(struct tpm_chip *chip)
> > {
> > int rc;
> >
> > - rc = tpm_chip_start(chip);
> > - if (rc)
> > - return rc;
> > - rc = tpm_auto_startup(chip);
> > - if (rc) {
> > - tpm_chip_stop(chip);
> > - return rc;
> > - }
> > -
> > - rc = tpm_get_pcr_allocation(chip);
> > - tpm_chip_stop(chip);
> > - if (rc)
> > - return rc;
> > -
> > tpm_sysfs_add_device(chip);
> >
> > tpm_bios_log_setup(chip);
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 24ee4e1cc452..919bb0b88b12 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -190,6 +190,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
> > delay_msec * 1000);
> > };
> >
> > +int tpm_chip_startup(struct tpm_chip *chip);
> > int tpm_chip_start(struct tpm_chip *chip);
> > void tpm_chip_stop(struct tpm_chip *chip);
> > struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index ddaf362e62c1..94a2bfb244b3 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -1129,6 +1129,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > /* INTERRUPT Setup */
> > init_waitqueue_head(&priv->read_queue);
> > init_waitqueue_head(&priv->int_queue);
> > +
> > + rc = tpm_chip_startup(chip);
> > + if (rc)
> > + goto out_err;
> > +
> > if (irq != -1) {
> > /*
> > * Before doing irq testing issue a command to the TPM in polling mode
>
> Jarko, thanks for the review so far. What about this patch, are there any concerns from your side?
No concerns on this one.
BR, Jarkko
On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> 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
> 5).
>
> Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> by Jarkko.
>
> Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> at driver runtime so this variable can be read without the need to protect
> it against concurrent modification.
>
> Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> done in the interrupt handler.
>
> Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> and INTERRUPT_ENABLE registers are effective by holding the locality.
>
> Patch 13 makes sure that the TPM is properly initialized after power cycle
> before the irq test is done.
>
> Patch 14 enables the test for interrupts by setting the required flag
> before the test is executed.
>
> Changes in v11:
> - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> re-addition of the "ret" variable in tpm_tis_gen_interrupt()
>
> Changes in v10:
> - fix typo in subject line as pointed out by Jason Andryuk
> - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> registers" by extending the scope with held locality". For this reason
> the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> been removed.
> - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> (PATCH 13)
> - add fix to restore the old interrupt vector at error (PATCH 4)
>
>
> Changes in v9:
> - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> - improve the commit message for patch 8 as requested by Jarkko
> - improved functions naming
> - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> Jarko has been removed
>
> Changes in v8:
> - tpm_tis_data->locality is not changed at runtime any more so that it can
> be read without any protection against concurrent modification.
> - add missing brackets as pointed out by Jason Andryuk
>
> Changes in v7:
> - moved interrupt flag checks into an own function as suggested by Jarkko
> - added "Tested-by" tags for Tests from Michael Niewöhner
> - fixed one comment
>
> Changes in v6:
> - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> in an own bitfield
> - improve commit messages
> - use int_mask instead of irqs_in_use as variable name
> - use sts_mask instead of active_irqs as variable name
> - squash patch 5 and 6
> - prefix functions with tpm_tis_
> - remove "fixes" tag
>
> Changes in v5:
> - improve commit message of patch 1 as requested by Jarko
> - drop patch that makes locality handling simpler by only claiming it at
> driver startup and releasing it at driver shutdown (requested by Jarko)
> - drop patch that moves the interrupt test from tpm_tis_send()
> to tmp_tis_probe_irq_single() as requested by Jarko
> - add patch to make locality handling threadsafe so that it can also be
> done by the irq handler
> - separate logical changes into own patches
> - always request threaded interrupt handler
>
> Changes in v4:
> - only request threaded irq in case of SPI as requested by Jarko.
> - reimplement patch 2 to limit locality handling changes to the TIS core.
> - separate fixes from cleanups as requested by Jarko.
> - rephrase commit messages
>
> Changes in v3:
> - fixed compiler error reported by kernel test robot
> - rephrased commit message as suggested by Jarko Sakkinen
> - added Reviewed-by tag
>
> Changes in v2:
> - rebase against 5.12
> - free irq on error path
>
>
> Lino Sanfilippo (14):
> 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, tpm_tis: Do not skip reset of original interrupt vector
> tpm, tpm_tis: Claim locality before writing interrupt registers
> tpm, tpm_tis: Only handle supported interrupts
> tpm, tpm_tis: Move interrupt mask checks into own function
> tpm, tpm_tis: do not check for the active locality in interrupt
> handler
> tpm, tpm: Implement usage counter for locality
> tpm, tpm_tis: Request threaded interrupt handler
> tpm, tpm_tis: Claim locality in interrupt handler
> tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> tpm, tpm_tis: startup chip before testing for interrupts
> tpm, tpm_tis: Enable interrupt test
>
> drivers/char/tpm/tpm-chip.c | 38 ++--
> drivers/char/tpm/tpm.h | 1 +
> drivers/char/tpm/tpm_tis.c | 2 +-
> drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> drivers/char/tpm/tpm_tis_core.h | 5 +-
> 5 files changed, 214 insertions(+), 131 deletions(-)
>
Hi Jarkko,
its been a while now since the review of this series has been completed. Will it be merged in the
near future? Or is there anything left to do (from my side)?
Regards,
Lino
On Fri, 2023-02-24 at 15:48 +0100, Lino Sanfilippo wrote:
> On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> > From: Lino Sanfilippo <[email protected]>
> >
> > 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
> > 5).
> >
> > Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> > by Jarkko.
> >
> > Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> > at driver runtime so this variable can be read without the need to protect
> > it against concurrent modification.
> >
> > Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> > done in the interrupt handler.
> >
> > Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> > and INTERRUPT_ENABLE registers are effective by holding the locality.
> >
> > Patch 13 makes sure that the TPM is properly initialized after power cycle
> > before the irq test is done.
> >
> > Patch 14 enables the test for interrupts by setting the required flag
> > before the test is executed.
> >
> > Changes in v11:
> > - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> > re-addition of the "ret" variable in tpm_tis_gen_interrupt()
> >
> > Changes in v10:
> > - fix typo in subject line as pointed out by Jason Andryuk
> > - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> > registers" by extending the scope with held locality". For this reason
> > the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> > been removed.
> > - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> > (PATCH 13)
> > - add fix to restore the old interrupt vector at error (PATCH 4)
> >
> >
> > Changes in v9:
> > - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> > - improve the commit message for patch 8 as requested by Jarkko
> > - improved functions naming
> > - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> > TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> > change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> > Jarko has been removed
> >
> > Changes in v8:
> > - tpm_tis_data->locality is not changed at runtime any more so that it can
> > be read without any protection against concurrent modification.
> > - add missing brackets as pointed out by Jason Andryuk
> >
> > Changes in v7:
> > - moved interrupt flag checks into an own function as suggested by Jarkko
> > - added "Tested-by" tags for Tests from Michael Niewöhner
> > - fixed one comment
> >
> > Changes in v6:
> > - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> > in an own bitfield
> > - improve commit messages
> > - use int_mask instead of irqs_in_use as variable name
> > - use sts_mask instead of active_irqs as variable name
> > - squash patch 5 and 6
> > - prefix functions with tpm_tis_
> > - remove "fixes" tag
> >
> > Changes in v5:
> > - improve commit message of patch 1 as requested by Jarko
> > - drop patch that makes locality handling simpler by only claiming it at
> > driver startup and releasing it at driver shutdown (requested by Jarko)
> > - drop patch that moves the interrupt test from tpm_tis_send()
> > to tmp_tis_probe_irq_single() as requested by Jarko
> > - add patch to make locality handling threadsafe so that it can also be
> > done by the irq handler
> > - separate logical changes into own patches
> > - always request threaded interrupt handler
> >
> > Changes in v4:
> > - only request threaded irq in case of SPI as requested by Jarko.
> > - reimplement patch 2 to limit locality handling changes to the TIS core.
> > - separate fixes from cleanups as requested by Jarko.
> > - rephrase commit messages
> >
> > Changes in v3:
> > - fixed compiler error reported by kernel test robot
> > - rephrased commit message as suggested by Jarko Sakkinen
> > - added Reviewed-by tag
> >
> > Changes in v2:
> > - rebase against 5.12
> > - free irq on error path
> >
> >
> > Lino Sanfilippo (14):
> > 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, tpm_tis: Do not skip reset of original interrupt vector
> > tpm, tpm_tis: Claim locality before writing interrupt registers
> > tpm, tpm_tis: Only handle supported interrupts
> > tpm, tpm_tis: Move interrupt mask checks into own function
> > tpm, tpm_tis: do not check for the active locality in interrupt
> > handler
> > tpm, tpm: Implement usage counter for locality
> > tpm, tpm_tis: Request threaded interrupt handler
> > tpm, tpm_tis: Claim locality in interrupt handler
> > tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> > tpm, tpm_tis: startup chip before testing for interrupts
> > tpm, tpm_tis: Enable interrupt test
> >
> > drivers/char/tpm/tpm-chip.c | 38 ++--
> > drivers/char/tpm/tpm.h | 1 +
> > drivers/char/tpm/tpm_tis.c | 2 +-
> > drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> > drivers/char/tpm/tpm_tis_core.h | 5 +-
> > 5 files changed, 214 insertions(+), 131 deletions(-)
> >
>
> Hi Jarkko,
>
> its been a while now since the review of this series has been completed. Will
> it be merged in the
> near future? Or is there anything left to do (from my side)?
>
> Regards,
> Lino
>
@Jarkko ping ;)
On Thu, Mar 16, 2023 at 06:18:11PM +0100, Michael Niew?hner wrote:
> On Fri, 2023-02-24 at 15:48 +0100, Lino Sanfilippo wrote:
> > On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> > > From: Lino Sanfilippo <[email protected]>
> > >
> > > 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
> > > 5).
> > >
> > > Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> > > by Jarkko.
> > >
> > > Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> > > at driver runtime so this variable can be read without the need to protect
> > > it against concurrent modification.
> > >
> > > Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> > > done in the interrupt handler.
> > >
> > > Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> > > and INTERRUPT_ENABLE registers are effective by holding the locality.
> > >
> > > Patch 13 makes sure that the TPM is properly initialized after power cycle
> > > before the irq test is done.
> > >
> > > Patch 14 enables the test for interrupts by setting the required flag
> > > before the test is executed.
> > >
> > > Changes in v11:
> > > - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> > > ? re-addition of the "ret" variable in tpm_tis_gen_interrupt()
> > >
> > > Changes in v10:
> > > - fix typo in subject line as pointed out by Jason Andryuk
> > > - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> > > ? registers" by extending the scope with held locality". For this reason
> > > ? the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> > > ? been removed.
> > > - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> > > ? (PATCH 13)
> > > - add fix to restore the old interrupt vector at error (PATCH 4)
> > >
> > >
> > > Changes in v9:
> > > - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> > > - improve the commit message for patch 8 as requested by Jarkko
> > > - improved functions naming
> > > - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> > > ? TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> > > ? change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> > > ? Jarko has been removed
> > >
> > > Changes in v8:
> > > - tpm_tis_data->locality is not changed at runtime any more so that it can
> > > be read without any protection against concurrent modification.
> > > - add missing brackets as pointed out by Jason Andryuk
> > >
> > > Changes in v7:
> > > - moved interrupt flag checks into an own function as suggested by Jarkko
> > > - added "Tested-by" tags for Tests from Michael Niew?hner
> > > - fixed one comment
> > >
> > > Changes in v6:
> > > - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> > > in an own bitfield
> > > - improve commit messages
> > > - use int_mask instead of irqs_in_use as variable name
> > > - use sts_mask instead of active_irqs as variable name
> > > - squash patch 5 and 6
> > > - prefix functions with tpm_tis_
> > > - remove "fixes" tag
> > >
> > > Changes in v5:
> > > - improve commit message of patch 1 as requested by Jarko
> > > - drop patch that makes locality handling simpler by only claiming it at
> > > ? driver startup and releasing it at driver shutdown (requested by Jarko)
> > > - drop patch that moves the interrupt test from tpm_tis_send()
> > > ? to tmp_tis_probe_irq_single() as requested by Jarko
> > > - add patch to make locality handling threadsafe so that it can also be
> > > ? done by the irq handler
> > > - separate logical changes into own patches
> > > - always request threaded interrupt handler
> > >
> > > Changes in v4:
> > > - only request threaded irq in case of SPI as requested by Jarko.
> > > - reimplement patch 2 to limit locality handling changes to the TIS core.
> > > - separate fixes from cleanups as requested by Jarko.
> > > - rephrase commit messages
> > >
> > > Changes in v3:
> > > - fixed compiler error reported by kernel test robot
> > > - rephrased commit message as suggested by Jarko Sakkinen
> > > - added Reviewed-by tag
> > >
> > > Changes in v2:
> > > - rebase against 5.12
> > > - free irq on error path
> > >
> > >
> > > Lino Sanfilippo (14):
> > > ? 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, tpm_tis: Do not skip reset of original interrupt vector
> > > ? tpm, tpm_tis: Claim locality before writing interrupt registers
> > > ? tpm, tpm_tis: Only handle supported interrupts
> > > ? tpm, tpm_tis: Move interrupt mask checks into own function
> > > ? tpm, tpm_tis: do not check for the active locality in interrupt
> > > ??? handler
> > > ? tpm, tpm: Implement usage counter for locality
> > > ? tpm, tpm_tis: Request threaded interrupt handler
> > > ? tpm, tpm_tis: Claim locality in interrupt handler
> > > ? tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> > > ? tpm, tpm_tis: startup chip before testing for interrupts
> > > ? tpm, tpm_tis: Enable interrupt test
> > >
> > > ?drivers/char/tpm/tpm-chip.c???? |? 38 ++--
> > > ?drivers/char/tpm/tpm.h????????? |?? 1 +
> > > ?drivers/char/tpm/tpm_tis.c????? |?? 2 +-
> > > ?drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> > > ?drivers/char/tpm/tpm_tis_core.h |?? 5 +-
> > > ?5 files changed, 214 insertions(+), 131 deletions(-)
> > >
> >
> > Hi Jarkko,
> >
> > its been a while now since the review of this series has been completed. Will
> > it be merged in the
> > near future? Or is there anything left to do (from my side)?
> >
> > Regards,
> > Lino
> >
>
> @Jarkko ping ;)
>
Thanks for reminding. I was wondering this week what was the situation
but latest version I had bookmarked from lore was 10.
Please ping earlier! I'll get on testing this, apologies.
BR, Jarkko
On Sun, Mar 19, 2023 at 03:53:38PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 16, 2023 at 06:18:11PM +0100, Michael Niew?hner wrote:
> > On Fri, 2023-02-24 at 15:48 +0100, Lino Sanfilippo wrote:
> > > On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> > > > From: Lino Sanfilippo <[email protected]>
> > > >
> > > > 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
> > > > 5).
> > > >
> > > > Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> > > > by Jarkko.
> > > >
> > > > Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> > > > at driver runtime so this variable can be read without the need to protect
> > > > it against concurrent modification.
> > > >
> > > > Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> > > > done in the interrupt handler.
> > > >
> > > > Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> > > > and INTERRUPT_ENABLE registers are effective by holding the locality.
> > > >
> > > > Patch 13 makes sure that the TPM is properly initialized after power cycle
> > > > before the irq test is done.
> > > >
> > > > Patch 14 enables the test for interrupts by setting the required flag
> > > > before the test is executed.
> > > >
> > > > Changes in v11:
> > > > - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> > > > ? re-addition of the "ret" variable in tpm_tis_gen_interrupt()
> > > >
> > > > Changes in v10:
> > > > - fix typo in subject line as pointed out by Jason Andryuk
> > > > - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> > > > ? registers" by extending the scope with held locality". For this reason
> > > > ? the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> > > > ? been removed.
> > > > - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> > > > ? (PATCH 13)
> > > > - add fix to restore the old interrupt vector at error (PATCH 4)
> > > >
> > > >
> > > > Changes in v9:
> > > > - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> > > > - improve the commit message for patch 8 as requested by Jarkko
> > > > - improved functions naming
> > > > - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> > > > ? TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> > > > ? change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> > > > ? Jarko has been removed
> > > >
> > > > Changes in v8:
> > > > - tpm_tis_data->locality is not changed at runtime any more so that it can
> > > > be read without any protection against concurrent modification.
> > > > - add missing brackets as pointed out by Jason Andryuk
> > > >
> > > > Changes in v7:
> > > > - moved interrupt flag checks into an own function as suggested by Jarkko
> > > > - added "Tested-by" tags for Tests from Michael Niew?hner
> > > > - fixed one comment
> > > >
> > > > Changes in v6:
> > > > - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> > > > in an own bitfield
> > > > - improve commit messages
> > > > - use int_mask instead of irqs_in_use as variable name
> > > > - use sts_mask instead of active_irqs as variable name
> > > > - squash patch 5 and 6
> > > > - prefix functions with tpm_tis_
> > > > - remove "fixes" tag
> > > >
> > > > Changes in v5:
> > > > - improve commit message of patch 1 as requested by Jarko
> > > > - drop patch that makes locality handling simpler by only claiming it at
> > > > ? driver startup and releasing it at driver shutdown (requested by Jarko)
> > > > - drop patch that moves the interrupt test from tpm_tis_send()
> > > > ? to tmp_tis_probe_irq_single() as requested by Jarko
> > > > - add patch to make locality handling threadsafe so that it can also be
> > > > ? done by the irq handler
> > > > - separate logical changes into own patches
> > > > - always request threaded interrupt handler
> > > >
> > > > Changes in v4:
> > > > - only request threaded irq in case of SPI as requested by Jarko.
> > > > - reimplement patch 2 to limit locality handling changes to the TIS core.
> > > > - separate fixes from cleanups as requested by Jarko.
> > > > - rephrase commit messages
> > > >
> > > > Changes in v3:
> > > > - fixed compiler error reported by kernel test robot
> > > > - rephrased commit message as suggested by Jarko Sakkinen
> > > > - added Reviewed-by tag
> > > >
> > > > Changes in v2:
> > > > - rebase against 5.12
> > > > - free irq on error path
> > > >
> > > >
> > > > Lino Sanfilippo (14):
> > > > ? 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, tpm_tis: Do not skip reset of original interrupt vector
> > > > ? tpm, tpm_tis: Claim locality before writing interrupt registers
> > > > ? tpm, tpm_tis: Only handle supported interrupts
> > > > ? tpm, tpm_tis: Move interrupt mask checks into own function
> > > > ? tpm, tpm_tis: do not check for the active locality in interrupt
> > > > ??? handler
> > > > ? tpm, tpm: Implement usage counter for locality
> > > > ? tpm, tpm_tis: Request threaded interrupt handler
> > > > ? tpm, tpm_tis: Claim locality in interrupt handler
> > > > ? tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> > > > ? tpm, tpm_tis: startup chip before testing for interrupts
> > > > ? tpm, tpm_tis: Enable interrupt test
> > > >
> > > > ?drivers/char/tpm/tpm-chip.c???? |? 38 ++--
> > > > ?drivers/char/tpm/tpm.h????????? |?? 1 +
> > > > ?drivers/char/tpm/tpm_tis.c????? |?? 2 +-
> > > > ?drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> > > > ?drivers/char/tpm/tpm_tis_core.h |?? 5 +-
> > > > ?5 files changed, 214 insertions(+), 131 deletions(-)
> > > >
> > >
> > > Hi Jarkko,
> > >
> > > its been a while now since the review of this series has been completed. Will
> > > it be merged in the
> > > near future? Or is there anything left to do (from my side)?
> > >
> > > Regards,
> > > Lino
> > >
> >
> > @Jarkko ping ;)
> >
>
> Thanks for reminding. I was wondering this week what was the situation
> but latest version I had bookmarked from lore was 10.
>
> Please ping earlier! I'll get on testing this, apologies.
I tested this with libvirt/QEMU/swtpm and did the following tests:
1. TPM 1.2 suspend/resume.
2. TPM 2.0 kselftest.
3. TPM 2.0 suspend/resume + kselftest.
I see no issues so I can pick this for my pull request.
Tests were performed on top of v6.3-rc7.
For all:
Tested-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
On Fri, Apr 21, 2023 at 07:50:14PM +0300, Jarkko Sakkinen wrote:
> On Sun, Mar 19, 2023 at 03:53:38PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 16, 2023 at 06:18:11PM +0100, Michael Niew?hner wrote:
> > > On Fri, 2023-02-24 at 15:48 +0100, Lino Sanfilippo wrote:
> > > > On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> > > > > From: Lino Sanfilippo <[email protected]>
> > > > >
> > > > > 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
> > > > > 5).
> > > > >
> > > > > Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> > > > > by Jarkko.
> > > > >
> > > > > Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> > > > > at driver runtime so this variable can be read without the need to protect
> > > > > it against concurrent modification.
> > > > >
> > > > > Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> > > > > done in the interrupt handler.
> > > > >
> > > > > Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> > > > > and INTERRUPT_ENABLE registers are effective by holding the locality.
> > > > >
> > > > > Patch 13 makes sure that the TPM is properly initialized after power cycle
> > > > > before the irq test is done.
> > > > >
> > > > > Patch 14 enables the test for interrupts by setting the required flag
> > > > > before the test is executed.
> > > > >
> > > > > Changes in v11:
> > > > > - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> > > > > ? re-addition of the "ret" variable in tpm_tis_gen_interrupt()
> > > > >
> > > > > Changes in v10:
> > > > > - fix typo in subject line as pointed out by Jason Andryuk
> > > > > - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> > > > > ? registers" by extending the scope with held locality". For this reason
> > > > > ? the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> > > > > ? been removed.
> > > > > - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> > > > > ? (PATCH 13)
> > > > > - add fix to restore the old interrupt vector at error (PATCH 4)
> > > > >
> > > > >
> > > > > Changes in v9:
> > > > > - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> > > > > - improve the commit message for patch 8 as requested by Jarkko
> > > > > - improved functions naming
> > > > > - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> > > > > ? TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> > > > > ? change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> > > > > ? Jarko has been removed
> > > > >
> > > > > Changes in v8:
> > > > > - tpm_tis_data->locality is not changed at runtime any more so that it can
> > > > > be read without any protection against concurrent modification.
> > > > > - add missing brackets as pointed out by Jason Andryuk
> > > > >
> > > > > Changes in v7:
> > > > > - moved interrupt flag checks into an own function as suggested by Jarkko
> > > > > - added "Tested-by" tags for Tests from Michael Niew?hner
> > > > > - fixed one comment
> > > > >
> > > > > Changes in v6:
> > > > > - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> > > > > in an own bitfield
> > > > > - improve commit messages
> > > > > - use int_mask instead of irqs_in_use as variable name
> > > > > - use sts_mask instead of active_irqs as variable name
> > > > > - squash patch 5 and 6
> > > > > - prefix functions with tpm_tis_
> > > > > - remove "fixes" tag
> > > > >
> > > > > Changes in v5:
> > > > > - improve commit message of patch 1 as requested by Jarko
> > > > > - drop patch that makes locality handling simpler by only claiming it at
> > > > > ? driver startup and releasing it at driver shutdown (requested by Jarko)
> > > > > - drop patch that moves the interrupt test from tpm_tis_send()
> > > > > ? to tmp_tis_probe_irq_single() as requested by Jarko
> > > > > - add patch to make locality handling threadsafe so that it can also be
> > > > > ? done by the irq handler
> > > > > - separate logical changes into own patches
> > > > > - always request threaded interrupt handler
> > > > >
> > > > > Changes in v4:
> > > > > - only request threaded irq in case of SPI as requested by Jarko.
> > > > > - reimplement patch 2 to limit locality handling changes to the TIS core.
> > > > > - separate fixes from cleanups as requested by Jarko.
> > > > > - rephrase commit messages
> > > > >
> > > > > Changes in v3:
> > > > > - fixed compiler error reported by kernel test robot
> > > > > - rephrased commit message as suggested by Jarko Sakkinen
> > > > > - added Reviewed-by tag
> > > > >
> > > > > Changes in v2:
> > > > > - rebase against 5.12
> > > > > - free irq on error path
> > > > >
> > > > >
> > > > > Lino Sanfilippo (14):
> > > > > ? 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, tpm_tis: Do not skip reset of original interrupt vector
> > > > > ? tpm, tpm_tis: Claim locality before writing interrupt registers
> > > > > ? tpm, tpm_tis: Only handle supported interrupts
> > > > > ? tpm, tpm_tis: Move interrupt mask checks into own function
> > > > > ? tpm, tpm_tis: do not check for the active locality in interrupt
> > > > > ??? handler
> > > > > ? tpm, tpm: Implement usage counter for locality
> > > > > ? tpm, tpm_tis: Request threaded interrupt handler
> > > > > ? tpm, tpm_tis: Claim locality in interrupt handler
> > > > > ? tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> > > > > ? tpm, tpm_tis: startup chip before testing for interrupts
> > > > > ? tpm, tpm_tis: Enable interrupt test
> > > > >
> > > > > ?drivers/char/tpm/tpm-chip.c???? |? 38 ++--
> > > > > ?drivers/char/tpm/tpm.h????????? |?? 1 +
> > > > > ?drivers/char/tpm/tpm_tis.c????? |?? 2 +-
> > > > > ?drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> > > > > ?drivers/char/tpm/tpm_tis_core.h |?? 5 +-
> > > > > ?5 files changed, 214 insertions(+), 131 deletions(-)
> > > > >
> > > >
> > > > Hi Jarkko,
> > > >
> > > > its been a while now since the review of this series has been completed. Will
> > > > it be merged in the
> > > > near future? Or is there anything left to do (from my side)?
> > > >
> > > > Regards,
> > > > Lino
> > > >
> > >
> > > @Jarkko ping ;)
> > >
> >
> > Thanks for reminding. I was wondering this week what was the situation
> > but latest version I had bookmarked from lore was 10.
> >
> > Please ping earlier! I'll get on testing this, apologies.
>
> I tested this with libvirt/QEMU/swtpm and did the following tests:
>
> 1. TPM 1.2 suspend/resume.
> 2. TPM 2.0 kselftest.
> 3. TPM 2.0 suspend/resume + kselftest.
>
> I see no issues so I can pick this for my pull request.
>
> Tests were performed on top of v6.3-rc7.
>
> For all:
>
> Tested-by: Jarkko Sakkinen <[email protected]>
Changes are in my tree and should be soon mirrored to linux-next.
Please check for any possible abnormalities, I tried to be careful.
BR, Jarkko
On Fri, Apr 21, 2023 at 10:34:21PM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 21, 2023 at 07:50:14PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Mar 19, 2023 at 03:53:38PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Mar 16, 2023 at 06:18:11PM +0100, Michael Niew?hner wrote:
> > > > On Fri, 2023-02-24 at 15:48 +0100, Lino Sanfilippo wrote:
> > > > > On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> > > > > > From: Lino Sanfilippo <[email protected]>
> > > > > >
> > > > > > 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
> > > > > > 5).
> > > > > >
> > > > > > Patch 6 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 7 moves the interrupt flag checks into an own function as suggested
> > > > > > by Jarkko.
> > > > > >
> > > > > > Patch 8 Removes the possibility that tpm_tis_data->locality can be changed
> > > > > > at driver runtime so this variable can be read without the need to protect
> > > > > > it against concurrent modification.
> > > > > >
> > > > > > Patch 9 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 10 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 11 makes sure that writes to the interrupt register are effective if
> > > > > > done in the interrupt handler.
> > > > > >
> > > > > > Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> > > > > > and INTERRUPT_ENABLE registers are effective by holding the locality.
> > > > > >
> > > > > > Patch 13 makes sure that the TPM is properly initialized after power cycle
> > > > > > before the irq test is done.
> > > > > >
> > > > > > Patch 14 enables the test for interrupts by setting the required flag
> > > > > > before the test is executed.
> > > > > >
> > > > > > Changes in v11:
> > > > > > - adjust patches 4,5 and 14 slightly to void the consecutive removal and
> > > > > > ? re-addition of the "ret" variable in tpm_tis_gen_interrupt()
> > > > > >
> > > > > > Changes in v10:
> > > > > > - fix typo in subject line as pointed out by Jason Andryuk
> > > > > > - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
> > > > > > ? registers" by extending the scope with held locality". For this reason
> > > > > > ? the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
> > > > > > ? been removed.
> > > > > > - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
> > > > > > ? (PATCH 13)
> > > > > > - add fix to restore the old interrupt vector at error (PATCH 4)
> > > > > >
> > > > > >
> > > > > > Changes in v9:
> > > > > > - add a fix for an issue when interrupts are reenabled on resume (PATCH 11)
> > > > > > - improve the commit message for patch 8 as requested by Jarkko
> > > > > > - improved functions naming
> > > > > > - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete the
> > > > > > ? TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to this
> > > > > > ? change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag from
> > > > > > ? Jarko has been removed
> > > > > >
> > > > > > Changes in v8:
> > > > > > - tpm_tis_data->locality is not changed at runtime any more so that it can
> > > > > > be read without any protection against concurrent modification.
> > > > > > - add missing brackets as pointed out by Jason Andryuk
> > > > > >
> > > > > > Changes in v7:
> > > > > > - moved interrupt flag checks into an own function as suggested by Jarkko
> > > > > > - added "Tested-by" tags for Tests from Michael Niew?hner
> > > > > > - fixed one comment
> > > > > >
> > > > > > Changes in v6:
> > > > > > - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct instead
> > > > > > in an own bitfield
> > > > > > - improve commit messages
> > > > > > - use int_mask instead of irqs_in_use as variable name
> > > > > > - use sts_mask instead of active_irqs as variable name
> > > > > > - squash patch 5 and 6
> > > > > > - prefix functions with tpm_tis_
> > > > > > - remove "fixes" tag
> > > > > >
> > > > > > Changes in v5:
> > > > > > - improve commit message of patch 1 as requested by Jarko
> > > > > > - drop patch that makes locality handling simpler by only claiming it at
> > > > > > ? driver startup and releasing it at driver shutdown (requested by Jarko)
> > > > > > - drop patch that moves the interrupt test from tpm_tis_send()
> > > > > > ? to tmp_tis_probe_irq_single() as requested by Jarko
> > > > > > - add patch to make locality handling threadsafe so that it can also be
> > > > > > ? done by the irq handler
> > > > > > - separate logical changes into own patches
> > > > > > - always request threaded interrupt handler
> > > > > >
> > > > > > Changes in v4:
> > > > > > - only request threaded irq in case of SPI as requested by Jarko.
> > > > > > - reimplement patch 2 to limit locality handling changes to the TIS core.
> > > > > > - separate fixes from cleanups as requested by Jarko.
> > > > > > - rephrase commit messages
> > > > > >
> > > > > > Changes in v3:
> > > > > > - fixed compiler error reported by kernel test robot
> > > > > > - rephrased commit message as suggested by Jarko Sakkinen
> > > > > > - added Reviewed-by tag
> > > > > >
> > > > > > Changes in v2:
> > > > > > - rebase against 5.12
> > > > > > - free irq on error path
> > > > > >
> > > > > >
> > > > > > Lino Sanfilippo (14):
> > > > > > ? 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, tpm_tis: Do not skip reset of original interrupt vector
> > > > > > ? tpm, tpm_tis: Claim locality before writing interrupt registers
> > > > > > ? tpm, tpm_tis: Only handle supported interrupts
> > > > > > ? tpm, tpm_tis: Move interrupt mask checks into own function
> > > > > > ? tpm, tpm_tis: do not check for the active locality in interrupt
> > > > > > ??? handler
> > > > > > ? tpm, tpm: Implement usage counter for locality
> > > > > > ? tpm, tpm_tis: Request threaded interrupt handler
> > > > > > ? tpm, tpm_tis: Claim locality in interrupt handler
> > > > > > ? tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
> > > > > > ? tpm, tpm_tis: startup chip before testing for interrupts
> > > > > > ? tpm, tpm_tis: Enable interrupt test
> > > > > >
> > > > > > ?drivers/char/tpm/tpm-chip.c???? |? 38 ++--
> > > > > > ?drivers/char/tpm/tpm.h????????? |?? 1 +
> > > > > > ?drivers/char/tpm/tpm_tis.c????? |?? 2 +-
> > > > > > ?drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
> > > > > > ?drivers/char/tpm/tpm_tis_core.h |?? 5 +-
> > > > > > ?5 files changed, 214 insertions(+), 131 deletions(-)
> > > > > >
> > > > >
> > > > > Hi Jarkko,
> > > > >
> > > > > its been a while now since the review of this series has been completed. Will
> > > > > it be merged in the
> > > > > near future? Or is there anything left to do (from my side)?
> > > > >
> > > > > Regards,
> > > > > Lino
> > > > >
> > > >
> > > > @Jarkko ping ;)
> > > >
> > >
> > > Thanks for reminding. I was wondering this week what was the situation
> > > but latest version I had bookmarked from lore was 10.
> > >
> > > Please ping earlier! I'll get on testing this, apologies.
> >
> > I tested this with libvirt/QEMU/swtpm and did the following tests:
> >
> > 1. TPM 1.2 suspend/resume.
> > 2. TPM 2.0 kselftest.
> > 3. TPM 2.0 suspend/resume + kselftest.
> >
> > I see no issues so I can pick this for my pull request.
> >
> > Tests were performed on top of v6.3-rc7.
> >
> > For all:
> >
> > Tested-by: Jarkko Sakkinen <[email protected]>
>
> Changes are in my tree and should be soon mirrored to linux-next.
>
> Please check for any possible abnormalities, I tried to be careful.
I'd also expect that there will appear some yet to be know issues
(if not, even better ofc) but there is full cycle to fix them.
BR, Jarkko
Hi,
On 21.04.23 18:50, Jarkko Sakkinen wrote:
>
> I tested this with libvirt/QEMU/swtpm and did the following tests:
>
> 1. TPM 1.2 suspend/resume.
> 2. TPM 2.0 kselftest.
> 3. TPM 2.0 suspend/resume + kselftest.
>
> I see no issues so I can pick this for my pull request.
>
> Tests were performed on top of v6.3-rc7.
>
> For all:
>
> Tested-by: Jarkko Sakkinen <[email protected]>
>
> BR, Jarkko
Thats great, thanks a lot for testing this!
Regards,
Lino
On Sat Apr 22, 2023 at 3:59 AM EEST, Lino Sanfilippo wrote:
> Hi,
>
> On 21.04.23 18:50, Jarkko Sakkinen wrote:
>
> >
> > I tested this with libvirt/QEMU/swtpm and did the following tests:
> >
> > 1. TPM 1.2 suspend/resume.
> > 2. TPM 2.0 kselftest.
> > 3. TPM 2.0 suspend/resume + kselftest.
> >
> > I see no issues so I can pick this for my pull request.
> >
> > Tests were performed on top of v6.3-rc7.
> >
> > For all:
> >
> > Tested-by: Jarkko Sakkinen <[email protected]>
> >
> > BR, Jarkko
>
> Thats great, thanks a lot for testing this!
Thanks for the patience! I'm sorry it took so long but at least all the
steps in v11 make perfect sense and I see nothing that would rise red
flags. So we can land this with good confidence I think.
BR, Jarkko
On Sun, 2023-04-23 at 17:15 +0300, Jarkko Sakkinen wrote:
> On Sat Apr 22, 2023 at 3:59 AM EEST, Lino Sanfilippo wrote:
> > Hi,
> >
> > On 21.04.23 18:50, Jarkko Sakkinen wrote:
> >
> > >
> > > I tested this with libvirt/QEMU/swtpm and did the following tests:
> > >
> > > 1. TPM 1.2 suspend/resume.
> > > 2. TPM 2.0 kselftest.
> > > 3. TPM 2.0 suspend/resume + kselftest.
> > >
> > > I see no issues so I can pick this for my pull request.
> > >
> > > Tests were performed on top of v6.3-rc7.
> > >
> > > For all:
> > >
> > > Tested-by: Jarkko Sakkinen <[email protected]>
> > >
> > > BR, Jarkko
> >
> > Thats great, thanks a lot for testing this!
>
> Thanks for the patience! I'm sorry it took so long but at least all the
> steps in v11 make perfect sense and I see nothing that would rise red
> flags. So we can land this with good confidence I think.
>
> BR, Jarkko
I wonder, if it makes sense to submit this patch series to longterm and/or at
least stable?
BR Michael
On Sun Apr 23, 2023 at 6:36 PM EEST, Michael Niewöhner wrote:
> On Sun, 2023-04-23 at 17:15 +0300, Jarkko Sakkinen wrote:
> > On Sat Apr 22, 2023 at 3:59 AM EEST, Lino Sanfilippo wrote:
> > > Hi,
> > >
> > > On 21.04.23 18:50, Jarkko Sakkinen wrote:
> > >
> > > >
> > > > I tested this with libvirt/QEMU/swtpm and did the following tests:
> > > >
> > > > 1. TPM 1.2 suspend/resume.
> > > > 2. TPM 2.0 kselftest.
> > > > 3. TPM 2.0 suspend/resume + kselftest.
> > > >
> > > > I see no issues so I can pick this for my pull request.
> > > >
> > > > Tests were performed on top of v6.3-rc7.
> > > >
> > > > For all:
> > > >
> > > > Tested-by: Jarkko Sakkinen <[email protected]>
> > > >
> > > > BR, Jarkko
> > >
> > > Thats great, thanks a lot for testing this!
> >
> > Thanks for the patience! I'm sorry it took so long but at least all the
> > steps in v11 make perfect sense and I see nothing that would rise red
> > flags. So we can land this with good confidence I think.
> >
> > BR, Jarkko
>
> I wonder, if it makes sense to submit this patch series to longterm and/or at
> least stable?
it's a feature, so I don't think so.
BR, Jarkko
On Sun, 2023-04-23 at 18:40 +0300, Jarkko Sakkinen wrote:
> On Sun Apr 23, 2023 at 6:36 PM EEST, Michael Niewöhner wrote:
> > On Sun, 2023-04-23 at 17:15 +0300, Jarkko Sakkinen wrote:
> > > On Sat Apr 22, 2023 at 3:59 AM EEST, Lino Sanfilippo wrote:
> > > > Hi,
> > > >
> > > > On 21.04.23 18:50, Jarkko Sakkinen wrote:
> > > >
> > > > >
> > > > > I tested this with libvirt/QEMU/swtpm and did the following tests:
> > > > >
> > > > > 1. TPM 1.2 suspend/resume.
> > > > > 2. TPM 2.0 kselftest.
> > > > > 3. TPM 2.0 suspend/resume + kselftest.
> > > > >
> > > > > I see no issues so I can pick this for my pull request.
> > > > >
> > > > > Tests were performed on top of v6.3-rc7.
> > > > >
> > > > > For all:
> > > > >
> > > > > Tested-by: Jarkko Sakkinen <[email protected]>
> > > > >
> > > > > BR, Jarkko
> > > >
> > > > Thats great, thanks a lot for testing this!
> > >
> > > Thanks for the patience! I'm sorry it took so long but at least all the
> > > steps in v11 make perfect sense and I see nothing that would rise red
> > > flags. So we can land this with good confidence I think.
> > >
> > > BR, Jarkko
> >
> > I wonder, if it makes sense to submit this patch series to longterm and/or
> > at
> > least stable?
>
> it's a feature, so I don't think so.
>
> BR, Jarkko
IMO it's a fix of a incomplete/broken implementation of that feature. I mean,
the code even tested for interrupts and printed an error. It was just missed to
enable them (TPM_CHIP_FLAG_IRQ).