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
4).
Patch 5 takes into account that according to the TPM Interface
Specification stsValid and commandRead interrupts might not be supported
by the hardware. For this reason the supported interrupts are first queried
and stored. Then wait_for_tpm_stat() is adjusted to not wait for status
changes that are not reported by interrupts.
Patch 6 moves the interrupt flag checks into an own function as suggested
by Jarkko.
Patch 7 Removes the possibility that tpm_tis_data->locality can be changed
at driver runtime so this variable can be read without the need to protect
it against concurrent modification.
Patch 8 addresses the issue with concurrent locality handling:
Since the interrupt handler writes the interrupt status registers it needs
to hold the locality. However it runs concurrently to the thread which
triggered the interrupt (e.g. by reading or writing data to the TPM). So
it must take care when claiming and releasing the locality itself,
because it may race with the concurrent running thread which also claims
and releases the locality.
To avoid that both interrupt and concurrent running thread interfere with
each other a locality counter is used which guarantees that at any time
the locality is held as long as it is required by one of both execution
paths.
Patch 9 implements the request of a threaded interrupt handler. This is
needed since SPI uses a mutex for data transmission and since we access the
interrupt status register via SPI in the irq handler we need a sleepable
context.
Patch 10 makes sure that writes to the interrupt register are effective if
done in the interrupt handler.
Patch 11 makes sure that writes to the interrupt and INTERRUPT_VECTOR
and INTERRUPT_ENABLE registers are effective by holding the locality.
Patch 12 enables the test for interrupts by setting the required flag
before the test is executed.
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: Request threaded interrupt handler) 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 (12):
tpm, tpm_tis: Avoid cache incoherency in test for interrupts
tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register
tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed
tpm, tmp_tis: Claim locality before writing interrupt registers
tpm, tpm_tis: Only handle supported interrupts
tpm, tpm_tis: Move interrupt mask checks into own function
tpm, tpm_tis: do not check for the active locality in interrupt
handler
tpm, tpm: Implement usage counter for locality
tpm, tpm_tis: Request threaded interrupt handler
tpm, tpm_tis: Claim locality in interrupt handler
tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
tpm, tpm_tis: Enable interrupt test
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_core.c | 279 ++++++++++++++++++++------------
drivers/char/tpm/tpm_tis_core.h | 5 +-
3 files changed, 185 insertions(+), 101 deletions(-)
base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763
--
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 181c291b0bb8..4336f7ea8c2b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -728,7 +728,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
u32 interrupt;
- int i, rc;
+ int rc;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
if (rc < 0)
@@ -740,10 +740,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
- if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
- for (i = 0; i < 5; i++)
- if (check_locality(chip, i))
- break;
+
if (interrupt &
(TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
TPM_INTF_CMD_READY_INT))
--
2.36.1
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 | 75 ++++++++++++++++++++++-----------
drivers/char/tpm/tpm_tis_core.h | 2 +
2 files changed, 53 insertions(+), 24 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4336f7ea8c2b..ecde684a8f80 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;
}
@@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
cap_t cap;
int ret;
- ret = request_locality(chip, 0);
+ ret = tpm_tis_request_locality(chip, 0);
if (ret < 0)
return ret;
@@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
else
ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
- release_locality(chip, 0);
+ tpm_tis_relinquish_locality(chip, 0);
return ret;
}
@@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
}
priv->irq = irq;
- rc = request_locality(chip, 0);
+ rc = tpm_tis_request_locality(chip, 0);
if (rc < 0)
return rc;
rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
&original_int_vec);
if (rc < 0) {
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
if (rc < 0) {
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
if (rc < 0) {
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
/* Clear all existing */
rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
if (rc < 0) {
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
@@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
intmask | TPM_GLOBAL_INT_ENABLE);
if (rc < 0) {
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
return rc;
}
- release_locality(chip, priv->locality);
+ tpm_tis_relinquish_locality(chip, priv->locality);
clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
/* Generate an interrupt by having the core call through to
@@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = {
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
- .request_locality = request_locality,
- .relinquish_locality = release_locality,
+ .request_locality = tpm_tis_request_locality,
+ .relinquish_locality = tpm_tis_relinquish_locality,
.clk_enable = tpm_tis_clkrun_enable,
};
@@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
priv->phy_ops = phy_ops;
+ priv->locality_count = 0;
+ mutex_init(&priv->locality_count_mutex);
dev_set_drvdata(&chip->dev, priv);
@@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
intmask &= ~TPM_GLOBAL_INT_ENABLE;
- rc = request_locality(chip, 0);
+ rc = tpm_tis_request_locality(chip, 0);
if (rc < 0) {
rc = -ENODEV;
goto out_err;
}
tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
- release_locality(chip, 0);
+ tpm_tis_relinquish_locality(chip, 0);
rc = tpm_chip_start(chip);
if (rc)
@@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
* proper timeouts for the driver.
*/
- rc = request_locality(chip, 0);
+ rc = tpm_tis_request_locality(chip, 0);
if (rc < 0)
goto out_err;
rc = tpm_get_timeouts(chip);
- release_locality(chip, 0);
+ tpm_tis_relinquish_locality(chip, 0);
if (rc) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
@@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
- rc = request_locality(chip, 0);
+ rc = tpm_tis_request_locality(chip, 0);
if (rc < 0)
goto out_err;
disable_interrupts(chip);
- release_locality(chip, 0);
+ tpm_tis_relinquish_locality(chip, 0);
}
}
@@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev)
* an error code but for unknown reason it isn't handled.
*/
if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
- ret = request_locality(chip, 0);
+ ret = tpm_tis_request_locality(chip, 0);
if (ret < 0)
return ret;
tpm1_do_selftest(chip);
- release_locality(chip, 0);
+ tpm_tis_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]>
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.
Signed-off-by: Lino Sanfilippo <[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 a0a1b1a3ddc6..469a1db95941 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1241,28 +1241,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