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 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 7 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 8 makes sure that writes to the interrupt register are effective if
done in the interrupt handler.
Patch 9 enables the test for interrupts by setting the required flag before
the test is executed.
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 (9):
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
tmp, tmp_tis: Implement usage counter for locality
tpm, tpm_tis: Request threaded interrupt handler
tpm, tpm_tis: Claim locality in interrupt handler
tpm, tpm_tis: Enable interrupt test
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/tpm_tis_core.c | 252 +++++++++++++++++++++-----------
drivers/char/tpm/tpm_tis_core.h | 5 +-
3 files changed, 171 insertions(+), 88 deletions(-)
base-commit: 78ca55889a549a9a194c6ec666836329b774ab6d
--
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]>
---
drivers/char/tpm/tpm_tis_core.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 0e68e4502a56..d32e93c86f48 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1077,21 +1077,21 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
goto out_err;
}
- if (irq) {
+ if (irq)
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
irq);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
- dev_err(&chip->dev, FW_BUG
+ else
+ tpm_tis_probe_irq(chip, intmask);
+
+ if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+ dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
- rc = request_locality(chip, 0);
- if (rc < 0)
- goto out_err;
- disable_interrupts(chip);
- release_locality(chip, 0);
- }
- } else {
- tpm_tis_probe_irq(chip, intmask);
+ rc = request_locality(chip, 0);
+ if (rc < 0)
+ goto out_err;
+ disable_interrupts(chip);
+ release_locality(chip, 0);
}
}
--
2.36.1
From: Lino Sanfilippo <[email protected]>
In disable_interrupts() the TPM_GLOBAL_INT_ENABLE bit is unset in the
TPM_INT_ENABLE register to shut the interrupts off. However modifying the
register is only possible with a held locality. So claim the locality
before disable_interrupts() is called.
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index b5fd4ff46666..0e68e4502a56 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1084,7 +1084,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
+ rc = request_locality(chip, 0);
+ if (rc < 0)
+ goto out_err;
disable_interrupts(chip);
+ release_locality(chip, 0);
}
} else {
tpm_tis_probe_irq(chip, intmask);
--
2.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]>
---
drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 72 insertions(+), 48 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09d8f04cbc81..cb469591373a 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;
}
@@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (rc < 0)
goto out_err;
- intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
- TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+ /* Figure out the capabilities */
+ rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
+ if (rc < 0)
+ goto out_err;
+
+ dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
+ intfcaps);
+ if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
+ dev_dbg(dev, "\tBurst Count Static\n");
+ if (intfcaps & TPM_INTF_CMD_READY_INT) {
+ intmask |= TPM_INTF_CMD_READY_INT;
+ dev_dbg(dev, "\tCommand Ready Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
+ dev_dbg(dev, "\tInterrupt Edge Falling\n");
+ if (intfcaps & TPM_INTF_INT_EDGE_RISING)
+ dev_dbg(dev, "\tInterrupt Edge Rising\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
+ dev_dbg(dev, "\tInterrupt Level Low\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
+ dev_dbg(dev, "\tInterrupt Level High\n");
+ if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
+ intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
+ dev_dbg(dev, "\tLocality Change Int Support\n");
+ if (intfcaps & TPM_INTF_STS_VALID_INT) {
+ intmask |= TPM_INTF_STS_VALID_INT;
+ dev_dbg(dev, "\tSts Valid Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+ intmask |= TPM_INTF_DATA_AVAIL_INT;
+ dev_dbg(dev, "\tData Avail Int Support\n");
+ }
+
intmask &= ~TPM_GLOBAL_INT_ENABLE;
rc = request_locality(chip, 0);
@@ -1042,32 +1095,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
goto out_err;
}
- /* Figure out the capabilities */
- rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
- if (rc < 0)
- goto out_err;
-
- dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
- intfcaps);
- if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
- dev_dbg(dev, "\tBurst Count Static\n");
- if (intfcaps & TPM_INTF_CMD_READY_INT)
- dev_dbg(dev, "\tCommand Ready Int Support\n");
- if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
- dev_dbg(dev, "\tInterrupt Edge Falling\n");
- if (intfcaps & TPM_INTF_INT_EDGE_RISING)
- dev_dbg(dev, "\tInterrupt Edge Rising\n");
- if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
- dev_dbg(dev, "\tInterrupt Level Low\n");
- if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
- dev_dbg(dev, "\tInterrupt Level High\n");
- if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
- dev_dbg(dev, "\tLocality Change Int Support\n");
- if (intfcaps & TPM_INTF_STS_VALID_INT)
- dev_dbg(dev, "\tSts Valid Int Support\n");
- if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
- dev_dbg(dev, "\tData Avail Int Support\n");
-
/* INTERRUPT Setup */
init_waitqueue_head(&priv->read_queue);
init_waitqueue_head(&priv->int_queue);
@@ -1098,7 +1125,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
else
tpm_tis_probe_irq(chip, intmask);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+ if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+ priv->int_mask = intmask;
+ } else {
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
@@ -1145,13 +1174,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
if (rc < 0)
goto out;
- rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
- if (rc < 0)
- goto out;
-
- intmask |= TPM_INTF_CMD_READY_INT
- | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
- | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
+ intmask = priv->int_mask | TPM_GLOBAL_INT_ENABLE;
tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index bf07379dea42..e005eb99480e 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -93,6 +93,7 @@ struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
+ unsigned int int_mask;
unsigned long flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.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
tmp_tis_send().
Since nothing guarantees cache coherency between CPUs for unsynchronized
accesses to boolean variables the testing thread might not perceive the
value change done in the interrupt handler.
Avoid this issue by setting the bit TPM_TIS_IRQ_TESTED in the flags field
of the tpm_tis_data struct and by accessing this field with the bit
manipulating functions that provide cache coherency.
Also convert all other existing sites to use the proper macros when
accessing this bitfield.
Signed-off-by: Lino Sanfilippo <[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 dc56b976d816..b5fd4ff46666 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -343,7 +343,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc, status, burstcnt;
size_t count = 0;
- bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
+ bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
int rc, irq;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
+ if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
+ test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
return tpm_tis_send_main(chip, buf, len);
/* Verify receipt of the expected IRQ */
@@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
rc = tpm_tis_send_main(chip, buf, len);
priv->irq = irq;
chip->flags |= TPM_CHIP_FLAG_IRQ;
- if (!priv->irq_tested)
+ if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
tpm_msleep(1);
- if (!priv->irq_tested)
+ if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
disable_interrupts(chip);
- priv->irq_tested = true;
+ set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
return rc;
}
@@ -627,7 +628,7 @@ static int probe_itpm(struct tpm_chip *chip)
size_t len = sizeof(cmd_getticks);
u16 vendor;
- if (priv->flags & TPM_TIS_ITPM_WORKAROUND)
+ if (test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags))
return 0;
rc = tpm_tis_read16(priv, TPM_DID_VID(0), &vendor);
@@ -647,13 +648,13 @@ static int probe_itpm(struct tpm_chip *chip)
tpm_tis_ready(chip);
- priv->flags |= TPM_TIS_ITPM_WORKAROUND;
+ set_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
rc = tpm_tis_send_data(chip, cmd_getticks, len);
if (rc == 0)
dev_info(&chip->dev, "Detected an iTPM.\n");
else {
- priv->flags &= ~TPM_TIS_ITPM_WORKAROUND;
+ clear_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
rc = -EFAULT;
}
@@ -693,7 +694,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
if (interrupt == 0)
return IRQ_NONE;
- priv->irq_tested = true;
+ set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -779,7 +780,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
if (rc < 0)
return rc;
- priv->irq_tested = false;
+ clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
/* Generate an interrupt by having the core call through to
* tpm_tis_send
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 6c203f36b8a1..bf07379dea42 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,13 +86,13 @@ enum tis_defaults {
enum tpm_tis_flags {
TPM_TIS_ITPM_WORKAROUND = BIT(0),
TPM_TIS_INVALID_STATUS = BIT(1),
+ TPM_TIS_IRQ_TESTED = BIT(2),
};
struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
- bool irq_tested;
unsigned long flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.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]>
---
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 639838c5cfe3..08a4e82f76a1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -795,8 +795,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
On Tue, Jun 21, 2022 at 03:24:39PM +0200, Lino Sanfilippo wrote:
> 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
> tmp_tis_send().
>
> Since nothing guarantees cache coherency between CPUs for unsynchronized
> accesses to boolean variables the testing thread might not perceive the
> value change done in the interrupt handler.
>
> Avoid this issue by setting the bit TPM_TIS_IRQ_TESTED in the flags field
> of the tpm_tis_data struct and by accessing this field with the bit
> manipulating functions that provide cache coherency.
>
> Also convert all other existing sites to use the proper macros when
> accessing this bitfield.
>
> Signed-off-by: Lino Sanfilippo <[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 dc56b976d816..b5fd4ff46666 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -343,7 +343,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> int rc, status, burstcnt;
> size_t count = 0;
> - bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> + bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
>
> status = tpm_tis_status(chip);
> if ((status & TPM_STS_COMMAND_READY) == 0) {
> @@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> int rc, irq;
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> + test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
> return tpm_tis_send_main(chip, buf, len);
>
> /* Verify receipt of the expected IRQ */
> @@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> rc = tpm_tis_send_main(chip, buf, len);
> priv->irq = irq;
> chip->flags |= TPM_CHIP_FLAG_IRQ;
> - if (!priv->irq_tested)
> + if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
> tpm_msleep(1);
> - if (!priv->irq_tested)
> + if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags))
> disable_interrupts(chip);
> - priv->irq_tested = true;
> + set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
> return rc;
> }
>
> @@ -627,7 +628,7 @@ static int probe_itpm(struct tpm_chip *chip)
> size_t len = sizeof(cmd_getticks);
> u16 vendor;
>
> - if (priv->flags & TPM_TIS_ITPM_WORKAROUND)
> + if (test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags))
> return 0;
>
> rc = tpm_tis_read16(priv, TPM_DID_VID(0), &vendor);
> @@ -647,13 +648,13 @@ static int probe_itpm(struct tpm_chip *chip)
>
> tpm_tis_ready(chip);
>
> - priv->flags |= TPM_TIS_ITPM_WORKAROUND;
> + set_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
>
> rc = tpm_tis_send_data(chip, cmd_getticks, len);
> if (rc == 0)
> dev_info(&chip->dev, "Detected an iTPM.\n");
> else {
> - priv->flags &= ~TPM_TIS_ITPM_WORKAROUND;
> + clear_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
> rc = -EFAULT;
> }
>
> @@ -693,7 +694,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> if (interrupt == 0)
> return IRQ_NONE;
>
> - priv->irq_tested = true;
> + set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
> if (interrupt & TPM_INTF_DATA_AVAIL_INT)
> wake_up_interruptible(&priv->read_queue);
> if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> @@ -779,7 +780,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> if (rc < 0)
> return rc;
>
> - priv->irq_tested = false;
> + clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
>
> /* Generate an interrupt by having the core call through to
> * tpm_tis_send
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 6c203f36b8a1..bf07379dea42 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -86,13 +86,13 @@ enum tis_defaults {
> enum tpm_tis_flags {
> TPM_TIS_ITPM_WORKAROUND = BIT(0),
> TPM_TIS_INVALID_STATUS = BIT(1),
> + TPM_TIS_IRQ_TESTED = BIT(2),
> };
>
> struct tpm_tis_data {
> u16 manufacturer_id;
> int locality;
> int irq;
> - bool irq_tested;
> unsigned long flags;
> void __iomem *ilb_base_addr;
> u16 clkrun_enabled;
> --
> 2.36.1
>
Reviewed-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
On Tue, Jun 21, 2022 at 03:24:43PM +0200, Lino Sanfilippo wrote:
> 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]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
> drivers/char/tpm/tpm_tis_core.h | 1 +
> 2 files changed, 72 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 09d8f04cbc81..cb469591373a 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;
I would instead mask out bits and write a helper function
taking care of this:
static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
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;
}
Less operations and imho somewhat cleaner structure.
Add suggested-by if you want.
BR, Jarkko
On Sun, Jun 26, 2022 at 09:40:43AM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 21, 2022 at 03:24:43PM +0200, Lino Sanfilippo wrote:
> > 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]>
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
> > drivers/char/tpm/tpm_tis_core.h | 1 +
> > 2 files changed, 72 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 09d8f04cbc81..cb469591373a 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;
>
> I would instead mask out bits and write a helper function
> taking care of this:
>
> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
Ignore this line (wrote this out of top of my head).
> 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;
> }
>
> Less operations and imho somewhat cleaner structure.
>
> Add suggested-by if you want.
>
> BR, Jarkko
On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
>
> I would instead mask out bits and write a helper function
> taking care of this:
>
> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
> 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;
> }
>
> Less operations and imho somewhat cleaner structure.
>
> Add suggested-by if you want.
I thought of a helper like this before but then decided to
not introduce another function to keep the code changes minimal. But yes,
it is indeed cleaner. I will do the change and resubmit the series.
Thanks for the review!
Regards,
Lino
On Sun, Jun 26, 2022 at 02:18:17PM +0200, Lino Sanfilippo wrote:
> On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
> >
> > I would instead mask out bits and write a helper function
> > taking care of this:
> >
> > static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> > {
> > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >
> > 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;
> > }
> >
> > Less operations and imho somewhat cleaner structure.
> >
> > Add suggested-by if you want.
>
> I thought of a helper like this before but then decided to
> not introduce another function to keep the code changes minimal. But yes,
> it is indeed cleaner. I will do the change and resubmit the series.
>
> Thanks for the review!
>
> Regards,
> Lino
Yeah, please don't add suggested-by, it's such a minor detail
in the overall patch :-) Thanks for taking time to fix these
glitches and also taking all the feedback into account (and
also being patient).
BR, Jarkko
On 28.06.22 01:09, Jarkko Sakkinen wrote:
> On Sun, Jun 26, 2022 at 02:18:17PM +0200, Lino Sanfilippo wrote:
>> On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
>>>
>>> I would instead mask out bits and write a helper function
>>> taking care of this:
>>>
>>> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
>>> {
>>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>
>>> 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;
>>> }
>>>
>>> Less operations and imho somewhat cleaner structure.
>>>
>>> Add suggested-by if you want.
>>
>> I thought of a helper like this before but then decided to
>> not introduce another function to keep the code changes minimal. But yes,
>> it is indeed cleaner. I will do the change and resubmit the series.
>>
>> Thanks for the review!
>>
>> Regards,
>> Lino
>
> Yeah, please don't add suggested-by, it's such a minor detail
> in the overall patch :-)
I already created a separate patch which only contains moving the bit checks into the
helper function. For that patch the Suggested-by is fully justified IMHO.
Thanks for taking time to fix these
> glitches and also taking all the feedback into account (and
> also being patient).
>
No problem. Its always good to have some feedback from people that have a deeper insight
into the code. Especially when it is as complex as the TPM subsystem and drivers.
Best regards,
Lino