This series enables interrupts for TPM. For this some obstacles had to be
removed first, like the interrupt handler running in interrupt context and
thus not allowing to access registers over SPI. Also the locality handling
has been simplified to make a complicated synchronization between threads
and irq handler unnecessary. As a side effect of this simplification a bug
is fixed in which a TMP command is issued without a claimed locality in
case of TPM 2.
Another fix concerns the interrupt test which currently is broken.
Finally the results of the capability query at startup is used to only set
the interrupts which are actually supported by the hardware.
These patches are based on commit 9f67672a817e ("Merge tag 'ext4_for_linus'
of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4") and tested on
on a SLB 9670 which is connected via SPI.
Of course any further testing is highly appreciated.
PATCH 1: The SPI implementation of the functions to read/write to/from
registers uses mutexes and thus require a sleepable context. For this
reason request a threaded interrupt handler.
PATCH 2: Simplify locality handling by taking the driver locality (0) at
driver startup and releasing it at driver shutdown. This also fixes a bug
in case of TMP 2.
PATCH 3: Fix and simplify the test for interrupts.
PATCH 4: Only set the interrupts which are reported as being available.
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 (4):
tpm: Use a threaded interrupt handler
tpm: Simplify locality handling
tpm: Fix test for interrupts
tpm: Only enable supported irqs
drivers/char/tpm/tpm-chip.c | 40 --------
drivers/char/tpm/tpm_tis_core.c | 170 +++++++++++++-------------------
drivers/char/tpm/tpm_tis_core.h | 2 +-
include/linux/tpm.h | 5 +-
4 files changed, 72 insertions(+), 145 deletions(-)
base-commit: 9f67672a817ec046f7554a885f0fe0d60e1bf99f
--
2.31.1
The current test for functional interrupts is broken in multiple ways:
1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
executed.
2. The test includes the check for two variables and the check is done for
each transmission which is unnecessarily complicated.
3. Part of the test is setting a bool variable in an interrupt handler and
then check the value in the main thread. However there is nothing that
guarantees the visibility of the value set in the interrupt handler for
any other thread. Some kind of synchronization primitive is required for
this purpose.
Fix all these issues by a reimplementation:
Instead of doing the test in tpm_tis_send() which is called for each
transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
use proper accessor functions like get_bit()/set_bit() which include the
required synchronization primitives to guarantee visibility between the
interrupt handler and threads.
Finally remove one function which is no longer needed.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 61 ++++++++++-----------------------
drivers/char/tpm/tpm_tis_core.h | 1 -
include/linux/tpm.h | 2 +-
3 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index f892b1ae46f2..9615234054aa 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -420,7 +420,7 @@ static void disable_interrupts(struct tpm_chip *chip)
* tpm.c can skip polling for the data to be available as the interrupt is
* waited for here
*/
-static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
+static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc;
@@ -453,29 +453,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
return rc;
}
-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)
- return tpm_tis_send_main(chip, buf, len);
-
- /* Verify receipt of the expected IRQ */
- irq = priv->irq;
- priv->irq = 0;
- chip->flags &= ~TPM_CHIP_FLAG_IRQ;
- rc = tpm_tis_send_main(chip, buf, len);
- priv->irq = irq;
- chip->flags |= TPM_CHIP_FLAG_IRQ;
- if (!priv->irq_tested)
- tpm_msleep(1);
- if (!priv->irq_tested)
- disable_interrupts(chip);
- priv->irq_tested = true;
- return rc;
-}
-
struct tis_vendor_durations_override {
u32 did_vid;
struct tpm1_version version;
@@ -677,7 +654,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
if (interrupt == 0)
return IRQ_NONE;
- priv->irq_tested = true;
+ set_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
+
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -734,45 +712,44 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
}
priv->irq = irq;
+ clear_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
+
rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
&original_int_vec);
if (rc < 0)
- return rc;
+ goto out;
rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
if (rc < 0)
- return rc;
+ goto out;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
if (rc < 0)
- return rc;
+ goto out;
/* Clear all existing */
rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
if (rc < 0)
- return rc;
+ goto out;
/* Turn on */
rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
intmask | TPM_GLOBAL_INT_ENABLE);
if (rc < 0)
- return rc;
-
- priv->irq_tested = false;
+ goto out;
- /* Generate an interrupt by having the core call through to
- * tpm_tis_send
- */
+ /* Generate an interrupt by transmitting a command to the chip */
rc = tpm_tis_gen_interrupt(chip);
if (rc < 0)
- return rc;
+ goto out;
+
+ tpm_msleep(1);
+out:
+ if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
+ disable_interrupts(chip);
- /* 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));
+ TPM_INT_VECTOR(priv->locality));
if (rc < 0)
return rc;
@@ -1039,7 +1016,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
irq);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+ if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a59f67..dc5f92b18dca 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,7 +89,6 @@ struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
- bool irq_tested;
unsigned int flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 7a68832b14bb..c57d0f0395f0 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -133,7 +133,7 @@ struct tpm_chip {
struct tpm_chip_seqops bin_log_seqops;
struct tpm_chip_seqops ascii_log_seqops;
- unsigned int flags;
+ unsigned long flags;
int dev_num; /* /dev/tpm# */
unsigned long is_open; /* only one allowed */
--
2.31.1
Do not set interrupts which are not supported by the hardware. Instead use
the information from the capability query and activate only the reported
interrupts.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 68 ++++++++++++++++++---------------
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 9615234054aa..757498a63f57 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -936,13 +936,47 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (rc)
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) {
+ priv->supported_irqs |= 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) {
+ priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
+ dev_dbg(dev, "\tLocality Change Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_STS_VALID_INT) {
+ priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
+ dev_dbg(dev, "\tSts Valid Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+ priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
+ dev_dbg(dev, "\tData Avail Int Support\n");
+ }
+
/* Take control of the TPM's interrupt hardware and shut it off */
rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
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;
+ intmask |= priv->supported_irqs;
+
intmask &= ~TPM_GLOBAL_INT_ENABLE;
tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
@@ -971,32 +1005,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);
@@ -1066,9 +1074,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
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->supported_irqs | 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 dc5f92b18dca..8ff62213d8fc 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -89,6 +89,7 @@ struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
+ unsigned int supported_irqs;
unsigned int flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.31.1
Currently the TPM (default) locality is claimed and released for each
access to the TPM registers which require a claimed locality. This results
in locality claim/release combos at various code places. For interrupt
handling we also need such a combo in the interrupt handler (for clearing
the interrupts) which makes the locality handling even more complicated
since now we also have to synchronize concurrent accesses in process and in
interrupt context.
Since currently the driver uses only one locality anyway, avoid the
increasing complexity by claiming it once at driver startup and only
releasing it at driver shutdown.
Due to the simplifications the functions tpm_request_locality() and
tpm_relinquish_locality() are not needed any more an can be removed.
As a side-effect these modifications fix a bug which results in the
following warning when using TPM 2:
TPM returned invalid status
WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1
Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
Workqueue: events_unbound async_run_entry_fn
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
sp : ffffffc0127e38f0
x29: ffffffc0127e38f0 x28: ffffffc011238000
x27: 0000000000000016 x26: 000000000000017a
x25: 0000000000000014 x24: ffffff8047f60000
x23: 0000000000000000 x22: 0000000000000016
x21: ffffff8044e8a480 x20: 0000000000000000
x19: ffffffc011238000 x18: ffffffc011238948
x17: 0000000000000000 x16: 0000000000000000
x15: ffffffc01141be81 x14: ffffffffffffffff
x13: ffffffc01141be7e x12: ffffffffffffffff
x11: ffffff807fb797f0 x10: 00000000000019b0
x9 : ffffffc0127e38f0 x8 : 766e692064656e72
x7 : 0000000000000000 x6 : ffffffc011239000
x5 : ffffff807fb628b8 x4 : 0000000000000000
x3 : 0000000000000027 x2 : 0000000000000000
x1 : 6818b6f22fdef800 x0 : 0000000000000000
Call trace:
tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
tpm_tis_send+0x4c/0xe0 [tpm_tis_core]
tpm_transmit+0xd0/0x350 [tpm]
tpm_transmit_cmd+0x3c/0xc0 [tpm]
tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core]
tpm_tis_core_init+0x304/0x520 [tpm_tis_core]
tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
spi_probe+0x84/0xf0
really_probe+0x118/0x420
driver_probe_device+0x5c/0xc0
__driver_attach_async_helper+0x64/0x68
async_run_entry_fn+0x48/0x150
process_one_work+0x15c/0x4d0
worker_thread+0x50/0x490
kthread+0x118/0x150
ret_from_fork+0x10/0x1c
The reason for this issue is that in case of TPM 2 function
tpm2_get_timeouts() which executes a TPM command is called without a
claimed locality. Since with this patch the locality is taken once at
driver startup and never released before shutdown the issue does not occur
any more.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 40 ---------------------------------
drivers/char/tpm/tpm_tis_core.c | 35 +++++++++--------------------
include/linux/tpm.h | 3 ---
3 files changed, 10 insertions(+), 68 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb7e109..a09b6523261e 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -32,35 +32,6 @@ struct class *tpm_class;
struct class *tpmrm_class;
dev_t tpm_devt;
-static int tpm_request_locality(struct tpm_chip *chip)
-{
- int rc;
-
- if (!chip->ops->request_locality)
- return 0;
-
- rc = chip->ops->request_locality(chip, 0);
- if (rc < 0)
- return rc;
-
- chip->locality = rc;
- return 0;
-}
-
-static void tpm_relinquish_locality(struct tpm_chip *chip)
-{
- int rc;
-
- if (!chip->ops->relinquish_locality)
- return;
-
- rc = chip->ops->relinquish_locality(chip, chip->locality);
- if (rc)
- dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
-
- chip->locality = -1;
-}
-
static int tpm_cmd_ready(struct tpm_chip *chip)
{
if (!chip->ops->cmd_ready)
@@ -103,17 +74,8 @@ int tpm_chip_start(struct tpm_chip *chip)
tpm_clk_enable(chip);
- if (chip->locality == -1) {
- ret = tpm_request_locality(chip);
- if (ret) {
- tpm_clk_disable(chip);
- return ret;
- }
- }
-
ret = tpm_cmd_ready(chip);
if (ret) {
- tpm_relinquish_locality(chip);
tpm_clk_disable(chip);
return ret;
}
@@ -133,7 +95,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
void tpm_chip_stop(struct tpm_chip *chip)
{
tpm_go_idle(chip);
- tpm_relinquish_locality(chip);
tpm_clk_disable(chip);
}
EXPORT_SYMBOL_GPL(tpm_chip_stop);
@@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
goto out;
}
- chip->locality = -1;
return chip;
out:
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index a12992ae2a3e..f892b1ae46f2 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -626,9 +626,6 @@ static int probe_itpm(struct tpm_chip *chip)
if (vendor != TPM_VID_INTEL)
return 0;
- if (request_locality(chip, 0) != 0)
- return -EBUSY;
-
rc = tpm_tis_send_data(chip, cmd_getticks, len);
if (rc == 0)
goto out;
@@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip)
out:
tpm_tis_ready(chip);
- release_locality(chip, priv->locality);
return rc;
}
@@ -707,22 +703,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
const char *desc = "attempting to generate an interrupt";
u32 cap2;
cap_t cap;
- int ret;
/* TPM 2.0 */
if (chip->flags & TPM_CHIP_FLAG_TPM2)
return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
/* TPM 1.2 */
- ret = request_locality(chip, 0);
- if (ret < 0)
- return ret;
-
- ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
-
- release_locality(chip, 0);
-
- return ret;
+ return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
}
/* Register the IRQ and issue a command that will cause an interrupt. If an
@@ -836,6 +823,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
+ release_locality(chip, 0);
tpm_tis_clkrun_enable(chip, false);
if (priv->ilb_base_addr)
@@ -963,6 +951,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
goto out_err;
}
+ rc = request_locality(chip, 0);
+ if (rc)
+ goto out_err;
+
+ rc = tpm_chip_start(chip);
+ if (rc)
+ goto out_err;
+
/* Take control of the TPM's interrupt hardware and shut it off */
rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
if (rc < 0)
@@ -973,9 +969,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
intmask &= ~TPM_GLOBAL_INT_ENABLE;
tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
- rc = tpm_chip_start(chip);
- if (rc)
- goto out_err;
rc = tpm2_probe(chip);
tpm_chip_stop(chip);
if (rc)
@@ -1036,15 +1029,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
* to make sure it works. May as well use that command to set the
* proper timeouts for the driver.
*/
-
- rc = request_locality(chip, 0);
- if (rc < 0)
- goto out_err;
-
rc = tpm_get_timeouts(chip);
-
- release_locality(chip, 0);
-
if (rc) {
dev_err(dev, "Could not get TPM timeouts and durations\n");
rc = -ENODEV;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index aa11fe323c56..7a68832b14bb 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -167,9 +167,6 @@ struct tpm_chip {
u32 last_cc;
u32 nr_commands;
u32 *cc_attrs_tbl;
-
- /* active locality */
- int locality;
};
#define TPM_HEADER_SIZE 10
--
2.31.1
On 5/1/21 9:57 AM, Lino Sanfilippo wrote:
> Do not set interrupts which are not supported by the hardware. Instead use
> the information from the capability query and activate only the reported
> interrupts.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
On 01.05.21 at 21:09, Stefan Berger wrote:
>
> On 5/1/21 9:57 AM, Lino Sanfilippo wrote:
>> Do not set interrupts which are not supported by the hardware. Instead use
>> the information from the capability query and activate only the reported
>> interrupts.
>>
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>
>
> Reviewed-by: Stefan Berger <[email protected]>
>
>
Ah sorry, I forgot to add your Reviewed-By for this patch. Will do so in the
next version.
Thx,
Lino
What the heck is "simplification" and what that has to do with fixing
anything? I don't understand your terminology.
On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote:
> Currently the TPM (default) locality is claimed and released for each
> access to the TPM registers which require a claimed locality. This results
> in locality claim/release combos at various code places. For interrupt
> handling we also need such a combo in the interrupt handler (for clearing
> the interrupts) which makes the locality handling even more complicated
> since now we also have to synchronize concurrent accesses in process and in
> interrupt context.
>
> Since currently the driver uses only one locality anyway, avoid the
> increasing complexity by claiming it once at driver startup and only
> releasing it at driver shutdown.
>
> Due to the simplifications the functions tpm_request_locality() and
> tpm_relinquish_locality() are not needed any more an can be removed.
>
> As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:
>
> TPM returned invalid status
> WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1
> Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
> Workqueue: events_unbound async_run_entry_fn
> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> sp : ffffffc0127e38f0
> x29: ffffffc0127e38f0 x28: ffffffc011238000
> x27: 0000000000000016 x26: 000000000000017a
> x25: 0000000000000014 x24: ffffff8047f60000
> x23: 0000000000000000 x22: 0000000000000016
> x21: ffffff8044e8a480 x20: 0000000000000000
> x19: ffffffc011238000 x18: ffffffc011238948
> x17: 0000000000000000 x16: 0000000000000000
> x15: ffffffc01141be81 x14: ffffffffffffffff
> x13: ffffffc01141be7e x12: ffffffffffffffff
> x11: ffffff807fb797f0 x10: 00000000000019b0
> x9 : ffffffc0127e38f0 x8 : 766e692064656e72
> x7 : 0000000000000000 x6 : ffffffc011239000
> x5 : ffffff807fb628b8 x4 : 0000000000000000
> x3 : 0000000000000027 x2 : 0000000000000000
> x1 : 6818b6f22fdef800 x0 : 0000000000000000
> Call trace:
> tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
> tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
> tpm_tis_send+0x4c/0xe0 [tpm_tis_core]
> tpm_transmit+0xd0/0x350 [tpm]
> tpm_transmit_cmd+0x3c/0xc0 [tpm]
> tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
> tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core]
> tpm_tis_core_init+0x304/0x520 [tpm_tis_core]
> tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
> tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
> tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
> spi_probe+0x84/0xf0
> really_probe+0x118/0x420
> driver_probe_device+0x5c/0xc0
> __driver_attach_async_helper+0x64/0x68
> async_run_entry_fn+0x48/0x150
> process_one_work+0x15c/0x4d0
> worker_thread+0x50/0x490
> kthread+0x118/0x150
> ret_from_fork+0x10/0x1c
>
> The reason for this issue is that in case of TPM 2 function
> tpm2_get_timeouts() which executes a TPM command is called without a
> claimed locality. Since with this patch the locality is taken once at
> driver startup and never released before shutdown the issue does not occur
> any more.
Please rather create fix that fixes the issue exactly in the code path.
I don't want to worry what other things this might do "as a side-effect".
Also, lacks the explanation why this patch fixes the issue.
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> drivers/char/tpm/tpm-chip.c | 40 ---------------------------------
> drivers/char/tpm/tpm_tis_core.c | 35 +++++++++--------------------
> include/linux/tpm.h | 3 ---
> 3 files changed, 10 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb7e109..a09b6523261e 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -32,35 +32,6 @@ struct class *tpm_class;
> struct class *tpmrm_class;
> dev_t tpm_devt;
>
> -static int tpm_request_locality(struct tpm_chip *chip)
> -{
> - int rc;
> -
> - if (!chip->ops->request_locality)
> - return 0;
> -
> - rc = chip->ops->request_locality(chip, 0);
> - if (rc < 0)
> - return rc;
> -
> - chip->locality = rc;
> - return 0;
> -}
> -
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> -{
> - int rc;
> -
> - if (!chip->ops->relinquish_locality)
> - return;
> -
> - rc = chip->ops->relinquish_locality(chip, chip->locality);
> - if (rc)
> - dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
> -
> - chip->locality = -1;
> -}
> -
> static int tpm_cmd_ready(struct tpm_chip *chip)
> {
> if (!chip->ops->cmd_ready)
> @@ -103,17 +74,8 @@ int tpm_chip_start(struct tpm_chip *chip)
>
> tpm_clk_enable(chip);
>
> - if (chip->locality == -1) {
> - ret = tpm_request_locality(chip);
> - if (ret) {
> - tpm_clk_disable(chip);
> - return ret;
> - }
> - }
> -
> ret = tpm_cmd_ready(chip);
> if (ret) {
> - tpm_relinquish_locality(chip);
> tpm_clk_disable(chip);
> return ret;
> }
> @@ -133,7 +95,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
> void tpm_chip_stop(struct tpm_chip *chip)
> {
> tpm_go_idle(chip);
> - tpm_relinquish_locality(chip);
> tpm_clk_disable(chip);
> }
> EXPORT_SYMBOL_GPL(tpm_chip_stop);
> @@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> goto out;
> }
>
> - chip->locality = -1;
> return chip;
>
> out:
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index a12992ae2a3e..f892b1ae46f2 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -626,9 +626,6 @@ static int probe_itpm(struct tpm_chip *chip)
> if (vendor != TPM_VID_INTEL)
> return 0;
>
> - if (request_locality(chip, 0) != 0)
> - return -EBUSY;
> -
> rc = tpm_tis_send_data(chip, cmd_getticks, len);
> if (rc == 0)
> goto out;
> @@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip)
>
> out:
> tpm_tis_ready(chip);
> - release_locality(chip, priv->locality);
>
> return rc;
> }
> @@ -707,22 +703,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
> const char *desc = "attempting to generate an interrupt";
> u32 cap2;
> cap_t cap;
> - int ret;
>
> /* TPM 2.0 */
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
> return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
>
> /* TPM 1.2 */
> - ret = request_locality(chip, 0);
> - if (ret < 0)
> - return ret;
> -
> - ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
> -
> - release_locality(chip, 0);
> -
> - return ret;
> + return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
> }
>
> /* Register the IRQ and issue a command that will cause an interrupt. If an
> @@ -836,6 +823,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
>
> tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>
> + release_locality(chip, 0);
> tpm_tis_clkrun_enable(chip, false);
>
> if (priv->ilb_base_addr)
> @@ -963,6 +951,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> goto out_err;
> }
>
> + rc = request_locality(chip, 0);
> + if (rc)
> + goto out_err;
> +
> + rc = tpm_chip_start(chip);
> + if (rc)
> + goto out_err;
> +
> /* Take control of the TPM's interrupt hardware and shut it off */
> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> if (rc < 0)
> @@ -973,9 +969,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> intmask &= ~TPM_GLOBAL_INT_ENABLE;
> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>
> - rc = tpm_chip_start(chip);
> - if (rc)
> - goto out_err;
> rc = tpm2_probe(chip);
> tpm_chip_stop(chip);
> if (rc)
> @@ -1036,15 +1029,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> * to make sure it works. May as well use that command to set the
> * proper timeouts for the driver.
> */
> -
> - rc = request_locality(chip, 0);
> - if (rc < 0)
> - goto out_err;
> -
> rc = tpm_get_timeouts(chip);
> -
> - release_locality(chip, 0);
> -
> if (rc) {
> dev_err(dev, "Could not get TPM timeouts and durations\n");
> rc = -ENODEV;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index aa11fe323c56..7a68832b14bb 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -167,9 +167,6 @@ struct tpm_chip {
> u32 last_cc;
> u32 nr_commands;
> u32 *cc_attrs_tbl;
> -
> - /* active locality */
> - int locality;
> };
>
> #define TPM_HEADER_SIZE 10
> --
> 2.31.1
>
/Jarkko
On Sat, May 01, 2021 at 03:57:26PM +0200, Lino Sanfilippo wrote:
> The current test for functional interrupts is broken in multiple ways:
> 1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
> executed.
> 2. The test includes the check for two variables and the check is done for
> each transmission which is unnecessarily complicated.
Unnecessary complicated is again terminolgy that I don't decipher,
unfortunately. I get "something that works" or "has a regression".
We don't polish things for nothing.
> 3. Part of the test is setting a bool variable in an interrupt handler and
> then check the value in the main thread. However there is nothing that
> guarantees the visibility of the value set in the interrupt handler for
> any other thread. Some kind of synchronization primitive is required for this purpose.
>
> Fix all these issues by a reimplementation:
> Instead of doing the test in tpm_tis_send() which is called for each
> transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
> use proper accessor functions like get_bit()/set_bit() which include the
> required synchronization primitives to guarantee visibility between the
> interrupt handler and threads.
> Finally remove one function which is no longer needed.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
Not sure why to take this patch.
> ---
> drivers/char/tpm/tpm_tis_core.c | 61 ++++++++++-----------------------
> drivers/char/tpm/tpm_tis_core.h | 1 -
> include/linux/tpm.h | 2 +-
> 3 files changed, 20 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index f892b1ae46f2..9615234054aa 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -420,7 +420,7 @@ static void disable_interrupts(struct tpm_chip *chip)
> * tpm.c can skip polling for the data to be available as the interrupt is
> * waited for here
> */
> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> int rc;
> @@ -453,29 +453,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
> return rc;
> }
>
> -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)
> - return tpm_tis_send_main(chip, buf, len);
> -
> - /* Verify receipt of the expected IRQ */
> - irq = priv->irq;
> - priv->irq = 0;
> - chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> - rc = tpm_tis_send_main(chip, buf, len);
> - priv->irq = irq;
> - chip->flags |= TPM_CHIP_FLAG_IRQ;
> - if (!priv->irq_tested)
> - tpm_msleep(1);
> - if (!priv->irq_tested)
> - disable_interrupts(chip);
> - priv->irq_tested = true;
> - return rc;
> -}
> -
> struct tis_vendor_durations_override {
> u32 did_vid;
> struct tpm1_version version;
> @@ -677,7 +654,8 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> if (interrupt == 0)
> return IRQ_NONE;
>
> - priv->irq_tested = true;
> + set_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
> +
> if (interrupt & TPM_INTF_DATA_AVAIL_INT)
> wake_up_interruptible(&priv->read_queue);
> if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> @@ -734,45 +712,44 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> }
> priv->irq = irq;
>
> + clear_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
> +
> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> &original_int_vec);
> if (rc < 0)
> - return rc;
> + goto out;
>
> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
> if (rc < 0)
> - return rc;
> + goto out;
>
> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
> if (rc < 0)
> - return rc;
> + goto out;
>
> /* Clear all existing */
> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
> if (rc < 0)
> - return rc;
> + goto out;
>
> /* Turn on */
> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> intmask | TPM_GLOBAL_INT_ENABLE);
> if (rc < 0)
> - return rc;
> -
> - priv->irq_tested = false;
> + goto out;
>
> - /* Generate an interrupt by having the core call through to
> - * tpm_tis_send
> - */
> + /* Generate an interrupt by transmitting a command to the chip */
> rc = tpm_tis_gen_interrupt(chip);
> if (rc < 0)
> - return rc;
> + goto out;
> +
> + tpm_msleep(1);
> +out:
> + if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
> + disable_interrupts(chip);
>
> - /* 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));
> + TPM_INT_VECTOR(priv->locality));
> if (rc < 0)
> return rc;
>
> @@ -1039,7 +1016,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> if (irq) {
> tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
> irq);
> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> + if (!test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
> dev_err(&chip->dev, FW_BUG
> "TPM interrupt not working, polling instead\n");
>
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 9b2d32a59f67..dc5f92b18dca 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -89,7 +89,6 @@ struct tpm_tis_data {
> u16 manufacturer_id;
> int locality;
> int irq;
> - bool irq_tested;
> unsigned int flags;
> void __iomem *ilb_base_addr;
> u16 clkrun_enabled;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 7a68832b14bb..c57d0f0395f0 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -133,7 +133,7 @@ struct tpm_chip {
> struct tpm_chip_seqops bin_log_seqops;
> struct tpm_chip_seqops ascii_log_seqops;
>
> - unsigned int flags;
> + unsigned long flags;
>
> int dev_num; /* /dev/tpm# */
> unsigned long is_open; /* only one allowed */
> --
> 2.31.1
>
/Jarkko
On Sat, May 01, 2021 at 03:57:27PM +0200, Lino Sanfilippo wrote:
> Do not set interrupts which are not supported by the hardware. Instead use
> the information from the capability query and activate only the reported
> interrupts.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
Zero reasoning again, why.
> ---
> drivers/char/tpm/tpm_tis_core.c | 68 ++++++++++++++++++---------------
> drivers/char/tpm/tpm_tis_core.h | 1 +
> 2 files changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 9615234054aa..757498a63f57 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -936,13 +936,47 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> if (rc)
> 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) {
> + priv->supported_irqs |= 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) {
> + priv->supported_irqs |= TPM_INTF_LOCALITY_CHANGE_INT;
> + dev_dbg(dev, "\tLocality Change Int Support\n");
> + }
> + if (intfcaps & TPM_INTF_STS_VALID_INT) {
> + priv->supported_irqs |= TPM_INTF_STS_VALID_INT;
> + dev_dbg(dev, "\tSts Valid Int Support\n");
> + }
> + if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
> + priv->supported_irqs |= TPM_INTF_DATA_AVAIL_INT;
> + dev_dbg(dev, "\tData Avail Int Support\n");
> + }
> +
> /* Take control of the TPM's interrupt hardware and shut it off */
> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> 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;
> + intmask |= priv->supported_irqs;
> +
> intmask &= ~TPM_GLOBAL_INT_ENABLE;
> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>
> @@ -971,32 +1005,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);
> @@ -1066,9 +1074,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
> 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->supported_irqs | 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 dc5f92b18dca..8ff62213d8fc 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -89,6 +89,7 @@ struct tpm_tis_data {
> u16 manufacturer_id;
> int locality;
> int irq;
> + unsigned int supported_irqs;
> unsigned int flags;
> void __iomem *ilb_base_addr;
> u16 clkrun_enabled;
> --
> 2.31.1
/Jarkko
>
On 03.05.21 at 17:52, Jarkko Sakkinen wrote:
> On Sat, May 01, 2021 at 03:57:26PM +0200, Lino Sanfilippo wrote:
>> The current test for functional interrupts is broken in multiple ways:
>> 1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
>> executed.
>> 2. The test includes the check for two variables and the check is done for
>> each transmission which is unnecessarily complicated.
>
> Unnecessary complicated is again terminolgy that I don't decipher,
> unfortunately. I get "something that works" or "has a regression".
> We don't polish things for nothing.
In this case "unnecessary complicated" means that we can achieve the same
effect (test for interrupts) with fewer code and fewer runtime impact.
Note that in the current code we do the same check for irq for each transmission:
if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
return tpm_tis_send_main(chip, buf, len);
With this patch the check for irqs has already be done at this time, so we dont have to
do this over and over again for each transmission.
>> 3. Part of the test is setting a bool variable in an interrupt handler and
>> then check the value in the main thread. However there is nothing that
>> guarantees the visibility of the value set in the interrupt handler for
>> any other thread. Some kind of synchronization primitive is required for this purpose.
>>
>> Fix all these issues by a reimplementation:
>> Instead of doing the test in tpm_tis_send() which is called for each
>> transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
>> use proper accessor functions like get_bit()/set_bit() which include the
>> required synchronization primitives to guarantee visibility between the
>> interrupt handler and threads.
>> Finally remove one function which is no longer needed.
>>
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>
> Not sure why to take this patch.
All I can say is that this patch is supposed to
- fix one bug:
"...Part of the test is setting a bool variable in an interrupt handler and
then check the value in the main thread. However there is nothing that
guarantees the visibility of the value set in the interrupt handler for
any other thread. Some kind of synchronization primitive is required for this purpose..."
- simplify the irq test
- provide interrupts instead of polling
If this is worth taking is up to you, of course.
Regards,
Lino
Hi,
On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> What the heck is "simplification" and what that has to do with fixing
> anything? I don't understand your terminology.
The intention for this patch is not to fix anything. Please read the cover
letter and the commit message.
This patch is about making the locality handling easier by not claiming/releasing
it multiple times over the driver life time, but claiming it once at driver
startup and only releasing it at driver shutdown.
Right now we have locality request/release combos in
- probe_itpm()
- tpm_tis_gen_interrupt()
- tpm_tis_core_init()
- tpm_chip_start()
and there is still one combo missing for
- tpm2_get_timeouts()
which is the reason why we get the "TPM returned invalid status" bug in case
of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see
below).
And if we are going to enable interrupts, we have to introduce yet another combo,
for accessing the status register in the interrupt handler, since TPM 2.0
requires holding the locality for writing to the status register. That makes
6 different code places in which we take and release the locality.
With this patch applied we only take the locality at one place. Furthermore
with interrupts enabled we dont have to claim the locality for each handler
execution, saving us countless claim/release combinations at runtime.
Hence the term "simplification" which is perfectly justified IMO.
So again, this patch is "only" in preparation for the next patch when interrupts
are actually enabled and we would have to take the locality in the interrupt
handler without this patch.
> On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote:
>> WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
>> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1
>> Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
>> Workqueue: events_unbound async_run_entry_fn
>> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>> pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> sp : ffffffc0127e38f0
>> x29: ffffffc0127e38f0 x28: ffffffc011238000
>> x27: 0000000000000016 x26: 000000000000017a
>> x25: 0000000000000014 x24: ffffff8047f60000
>> x23: 0000000000000000 x22: 0000000000000016
>> x21: ffffff8044e8a480 x20: 0000000000000000
>> x19: ffffffc011238000 x18: ffffffc011238948
>> x17: 0000000000000000 x16: 0000000000000000
>> x15: ffffffc01141be81 x14: ffffffffffffffff
>> x13: ffffffc01141be7e x12: ffffffffffffffff
>> x11: ffffff807fb797f0 x10: 00000000000019b0
>> x9 : ffffffc0127e38f0 x8 : 766e692064656e72
>> x7 : 0000000000000000 x6 : ffffffc011239000
>> x5 : ffffff807fb628b8 x4 : 0000000000000000
>> x3 : 0000000000000027 x2 : 0000000000000000
>> x1 : 6818b6f22fdef800 x0 : 0000000000000000
>> Call trace:
>> tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
>> tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
>> tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
>> tpm_tis_send+0x4c/0xe0 [tpm_tis_core]
>> tpm_transmit+0xd0/0x350 [tpm]
>> tpm_transmit_cmd+0x3c/0xc0 [tpm]
>> tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
>> tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core]
>> tpm_tis_core_init+0x304/0x520 [tpm_tis_core]
>> tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
>> tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
>> tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
>> spi_probe+0x84/0xf0
>> really_probe+0x118/0x420
>> driver_probe_device+0x5c/0xc0
>> __driver_attach_async_helper+0x64/0x68
>> async_run_entry_fn+0x48/0x150
>> process_one_work+0x15c/0x4d0
>> worker_thread+0x50/0x490
>> kthread+0x118/0x150
>> ret_from_fork+0x10/0x1c
>>
>> The reason for this issue is that in case of TPM 2 function
>> tpm2_get_timeouts() which executes a TPM command is called without a
>> claimed locality. Since with this patch the locality is taken once at
>> driver startup and never released before shutdown the issue does not occur
>> any more.
>
> Please rather create fix that fixes the issue exactly in the code path.
> I don't want to worry what other things this might do "as a side-effect".
As explained above this patch is not meant to fix a bug in the first place
but rather fixes the bug above incidentally by the locality handling reimplementation.
> Also, lacks the explanation why this patch fixes the issue.
>
The explanation is there:
"The reason for this issue is that in case of TPM 2 function
tpm2_get_timeouts() which executes a TPM command is called without a
claimed locality. Since with this patch the locality is taken once at
driver startup and never released before shutdown the issue does not occur
any more."
I really dont know how to describe this more clear.
Lino
On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> Hi,
>
> On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > What the heck is "simplification" and what that has to do with fixing
> > anything? I don't understand your terminology.
>
>
> The intention for this patch is not to fix anything. Please read the cover
> letter and the commit message.
> This patch is about making the locality handling easier by not claiming/releasing
> it multiple times over the driver life time, but claiming it once at driver
> startup and only releasing it at driver shutdown.
>
> Right now we have locality request/release combos in
>
> - probe_itpm()
> - tpm_tis_gen_interrupt()
> - tpm_tis_core_init()
> - tpm_chip_start()
>
> and there is still one combo missing for
>
> - tpm2_get_timeouts()
>
> which is the reason why we get the "TPM returned invalid status" bug in case
> of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see
> below).
>
> And if we are going to enable interrupts, we have to introduce yet another combo,
> for accessing the status register in the interrupt handler, since TPM 2.0
> requires holding the locality for writing to the status register. That makes
> 6 different code places in which we take and release the locality.
>
> With this patch applied we only take the locality at one place. Furthermore
> with interrupts enabled we dont have to claim the locality for each handler
> execution, saving us countless claim/release combinations at runtime.
>
> Hence the term "simplification" which is perfectly justified IMO.
>
> So again, this patch is "only" in preparation for the next patch when interrupts
> are actually enabled and we would have to take the locality in the interrupt
> handler without this patch.
So: what problem this patch does solve?
/Jarkko
Hi guys,
On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > Hi,
> >
> > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > What the heck is "simplification" and what that has to do with fixing
> > > anything? I don't understand your terminology.
> >
> >
> > The intention for this patch is not to fix anything. Please read the cover
> > letter and the commit message.
> > This patch is about making the locality handling easier by not
> > claiming/releasing
> > it multiple times over the driver life time, but claiming it once at driver
> > startup and only releasing it at driver shutdown.
> >
> > Right now we have locality request/release combos in
> >
> > - probe_itpm()
> > - tpm_tis_gen_interrupt()
> > - tpm_tis_core_init()
> > - tpm_chip_start()
> >
> > and there is still one combo missing for
> >
> > - tpm2_get_timeouts()
> >
> > which is the reason why we get the "TPM returned invalid status" bug in case
> > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch,
> > see
> > below).
> >
> > And if we are going to enable interrupts, we have to introduce yet another
> > combo,
> > for accessing the status register in the interrupt handler, since TPM 2.0
> > requires holding the locality for writing to the status register. That makes
> > 6 different code places in which we take and release the locality.
> >
> > With this patch applied we only take the locality at one place. Furthermore
> > with interrupts enabled we dont have to claim the locality for each handler
> > execution, saving us countless claim/release combinations at runtime.
> >
> > Hence the term "simplification" which is perfectly justified IMO.
> >
> > So again, this patch is "only" in preparation for the next patch when
> > interrupts
> > are actually enabled and we would have to take the locality in the interrupt
> > handler without this patch.
>
> So: what problem this patch does solve?
>
> /Jarkko
>
first, thank you very much, Lino, for working on this! I've been debugging
issues with the tis driver in the last days and was about to start with the same
approach as yours when I luckily discovered your patch!
Jarkko, while I agree, that the commit message is not optimal, Lino tried hard
to explain what the problems with the current code are and how they are / can be
fixed. Further, I too don't see why simplification / optimization is such a bad
thing. This driver is actually a very good example. I had a hard time, too,
figuring out what's going on there. A clean rewrite is a very valid approach
here IMO. It's not "polishing for nothing", as you described it, but actually
solving problems.
Interrupt detection is broken for years now and finally a volunteer worked on a
solution. Don't you think this should be valued? Let's get this problem sorted
out :-)
Lino, I'd be happy to test the patches, when you have time and interest to work
on this again!
Thanks, Michael
On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niew?hner wrote:
> Hi guys,
>
> On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > Hi,
> > >
> > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > What the heck is "simplification" and what that has to do with fixing
> > > > anything? I don't understand your terminology.
> > >
> > >
> > > The intention for this patch is not to fix anything. Please read the cover
> > > letter and the commit message.
> > > This patch is about making the locality handling easier by not
> > > claiming/releasing
> > > it multiple times over the driver life time, but claiming it once at driver
> > > startup and only releasing it at driver shutdown.
> > >
> > > Right now we have locality request/release combos in
> > >
> > > - probe_itpm()
> > > - tpm_tis_gen_interrupt()
> > > - tpm_tis_core_init()
> > > - tpm_chip_start()
> > >
> > > and there is still one combo missing for
> > >
> > > - tpm2_get_timeouts()
> > >
> > > which is the reason why we get the "TPM returned invalid status" bug in case
> > > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch,
> > > see
> > > below).
> > >
> > > And if we are going to enable interrupts, we have to introduce yet another
> > > combo,
> > > for accessing the status register in the interrupt handler, since TPM 2.0
> > > requires holding the locality for writing to the status register. That makes
> > > 6 different code places in which we take and release the locality.
> > >
> > > With this patch applied we only take the locality at one place. Furthermore
> > > with interrupts enabled we dont have to claim the locality for each handler
> > > execution, saving us countless claim/release combinations at runtime.
> > >
> > > Hence the term "simplification" which is perfectly justified IMO.
> > >
> > > So again, this patch is "only" in preparation for the next patch when
> > > interrupts
> > > are actually enabled and we would have to take the locality in the interrupt
> > > handler without this patch.
> >
> > So: what problem this patch does solve?
> >
> > /Jarkko
> >
>
> first, thank you very much, Lino, for working on this! I've been debugging
> issues with the tis driver in the last days and was about to start with the same
> approach as yours when I luckily discovered your patch!
>
> Jarkko, while I agree, that the commit message is not optimal, Lino tried hard
> to explain what the problems with the current code are and how they are / can be
> fixed. Further, I too don't see why simplification / optimization is such a bad
> thing. This driver is actually a very good example. I had a hard time, too,
> figuring out what's going on there. A clean rewrite is a very valid approach
> here IMO. It's not "polishing for nothing", as you described it, but actually
> solving problems.
>
> Interrupt detection is broken for years now and finally a volunteer worked on a
> solution. Don't you think this should be valued? Let's get this problem sorted
> out :-)
>
> Lino, I'd be happy to test the patches, when you have time and interest to work
> on this again!
>
> Thanks, Michael
It's quite easy to test them out. Both fixes are in the mainline GIT tree.
E.g. give a shot rc1, and please report if any issues persists to:
[email protected]
BR, Jarkko
On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote:
> > Hi guys,
> >
> > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > > Hi,
> > > >
> > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > > What the heck is "simplification" and what that has to do with fixing
> > > > > anything? I don't understand your terminology.
> > > >
> > > >
> > > > The intention for this patch is not to fix anything. Please read the
> > > > cover
> > > > letter and the commit message.
> > > > This patch is about making the locality handling easier by not
> > > > claiming/releasing
> > > > it multiple times over the driver life time, but claiming it once at
> > > > driver
> > > > startup and only releasing it at driver shutdown.
> > > >
> > > > Right now we have locality request/release combos in
> > > >
> > > > - probe_itpm()
> > > > - tpm_tis_gen_interrupt()
> > > > - tpm_tis_core_init()
> > > > - tpm_chip_start()
> > > >
> > > > and there is still one combo missing for
> > > >
> > > > - tpm2_get_timeouts()
> > > >
> > > > which is the reason why we get the "TPM returned invalid status" bug in
> > > > case
> > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this
> > > > patch,
> > > > see
> > > > below).
> > > >
> > > > And if we are going to enable interrupts, we have to introduce yet
> > > > another
> > > > combo,
> > > > for accessing the status register in the interrupt handler, since TPM
> > > > 2.0
> > > > requires holding the locality for writing to the status register. That
> > > > makes
> > > > 6 different code places in which we take and release the locality.
> > > >
> > > > With this patch applied we only take the locality at one place.
> > > > Furthermore
> > > > with interrupts enabled we dont have to claim the locality for each
> > > > handler
> > > > execution, saving us countless claim/release combinations at runtime.
> > > >
> > > > Hence the term "simplification" which is perfectly justified IMO.
> > > >
> > > > So again, this patch is "only" in preparation for the next patch when
> > > > interrupts
> > > > are actually enabled and we would have to take the locality in the
> > > > interrupt
> > > > handler without this patch.
> > >
> > > So: what problem this patch does solve?
> > >
> > > /Jarkko
> > >
> >
> > first, thank you very much, Lino, for working on this! I've been debugging
> > issues with the tis driver in the last days and was about to start with the
> > same
> > approach as yours when I luckily discovered your patch!
> >
> > Jarkko, while I agree, that the commit message is not optimal, Lino tried
> > hard
> > to explain what the problems with the current code are and how they are /
> > can be
> > fixed. Further, I too don't see why simplification / optimization is such a
> > bad
> > thing. This driver is actually a very good example. I had a hard time, too,
> > figuring out what's going on there. A clean rewrite is a very valid approach
> > here IMO. It's not "polishing for nothing", as you described it, but
> > actually
> > solving problems.
> >
> > Interrupt detection is broken for years now and finally a volunteer worked
> > on a
> > solution. Don't you think this should be valued? Let's get this problem
> > sorted
> > out :-)
> >
> > Lino, I'd be happy to test the patches, when you have time and interest to
> > work
> > on this again!
> >
> > Thanks, Michael
>
> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> E.g. give a shot rc1, and please report if any issues persists to:
>
> [email protected]
>
> BR, Jarkko
I don't see Linos patches on mainline. Also, the series included four patches:
[PATCH v3 0/4] Fixes for TPM interrupt handling
[PATCH v3 1/4] tpm: Use a threaded interrupt handler
[PATCH v3 2/4] tpm: Simplify locality handling
[PATCH v3 3/4] tpm: Fix test for interrupts
[PATCH v3 4/4] tpm: Only enable supported irqs
Three of them are relevant for the interrupt problem, which is still present in
mainline, as these patches were refused:
[PATCH v3 1/4] tpm: Use a threaded interrupt handler
[PATCH v3 2/4] tpm: Simplify locality handling
[PATCH v3 3/4] tpm: Fix test for interrupts
Michael
Hi Michael,
On 25.03.22 at 13:32, Michael Niewöhner wrote:
>>>
>>> Lino, I'd be happy to test the patches, when you have time and interest to
>>> work
>>> on this again!
>>>
>>> Thanks, Michael
>>
>> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
>> E.g. give a shot rc1, and please report if any issues persists to:
>>
>> [email protected]
>>
>> BR, Jarkko
>
> I don't see Linos patches on mainline. Also, the series included four patches:
> [PATCH v3 0/4] Fixes for TPM interrupt handling
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
> [PATCH v3 4/4] tpm: Only enable supported irqs
>
> Three of them are relevant for the interrupt problem, which is still present in
> mainline, as these patches were refused:
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
>
> Michael
>
You are right, the interrupts are still not working in the mainline kernel.
I would gladly make another attempt to fix this but rather step by step
than in a series that tries to fix (different) things at once.
A first step could be to have a sleepable context for the interrupt handling,
since in case of SPI the accesses to the irq status register may sleep.
I sent a patch for this purpose once, but it seems to have gone lost:
https://lore.kernel.org/all/[email protected]/
Best regards,
Lino
Hi Lino,
On Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
>
> Hi Michael,
>
> On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > >
> > > > Lino, I'd be happy to test the patches, when you have time and interest
> > > > to
> > > > work
> > > > on this again!
> > > >
> > > > Thanks, Michael
> > >
> > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > E.g. give a shot rc1, and please report if any issues persists to:
> > >
> > > [email protected]
> > >
> > > BR, Jarkko
> >
> > I don't see Linos patches on mainline. Also, the series included four
> > patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> >
> > Three of them are relevant for the interrupt problem, which is still present
> > in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> >
> > Michael
> >
>
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
IMHO a series is perfectly fine, as it's easier to show *why* single changes are
required (like already done in v3 0/4). One just has to actually *read* what's
written there. Ahem. It's up to you, though.
>
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
>
> I sent a patch for this purpose once, but it seems to have gone lost:
>
> https://lore.kernel.org/all/[email protected]/
>
Jarkko, looks like you've already tested that patch on your NUC?
>
> Best regards,
> Lino
>
Michael
On Sat, Mar 26, 2022 at 04:24:55AM +0100, Lino Sanfilippo wrote:
>
> Hi Michael,
>
> On 25.03.22 at 13:32, Michael Niew?hner wrote:
> >>>
> >>> Lino, I'd be happy to test the patches, when you have time and interest to
> >>> work
> >>> on this again!
> >>>
> >>> Thanks, Michael
> >>
> >> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> >> E.g. give a shot rc1, and please report if any issues persists to:
> >>
> >> ? [email protected]?
> >>
> >> BR, Jarkko
> >
> > I don't see Linos patches on mainline. Also, the series included four patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> >
> > Three of them are relevant for the interrupt problem, which is still present in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> >
> > Michael
> >
>
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
>
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
>
> I sent a patch for this purpose once, but it seems to have gone lost:
>
> https://lore.kernel.org/all/[email protected]/
>
>
> Best regards,
> Lino
This is clearly my bad I'll test this ASAP.
BR, Jarkko
On Fri, Mar 25, 2022 at 01:32:25PM +0100, Michael Niew?hner wrote:
> On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niew?hner wrote:
> > > Hi guys,
> > >
> > > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > > > > Hi,
> > > > >
> > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > > > > What the heck is "simplification" and what that has to do with fixing
> > > > > > anything? I don't understand your terminology.
> > > > >
> > > > >
> > > > > The intention for this patch is not to fix anything. Please read the
> > > > > cover
> > > > > letter and the commit message.
> > > > > This patch is about making the locality handling easier by not
> > > > > claiming/releasing
> > > > > it multiple times over the driver life time, but claiming it once at
> > > > > driver
> > > > > startup and only releasing it at driver shutdown.
> > > > >
> > > > > Right now we have locality request/release combos in
> > > > >
> > > > > - probe_itpm()
> > > > > - tpm_tis_gen_interrupt()
> > > > > - tpm_tis_core_init()
> > > > > - tpm_chip_start()
> > > > >
> > > > > and there is still one combo missing for
> > > > >
> > > > > - tpm2_get_timeouts()
> > > > >
> > > > > which is the reason why we get the "TPM returned invalid status" bug in
> > > > > case
> > > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this
> > > > > patch,
> > > > > see
> > > > > below).
> > > > >
> > > > > And if we are going to enable interrupts, we have to introduce yet
> > > > > another
> > > > > combo,
> > > > > for accessing the status register in the interrupt handler, since TPM
> > > > > 2.0
> > > > > requires holding the locality for writing to the status register. That
> > > > > makes
> > > > > 6 different code places in which we take and release the locality.
> > > > >
> > > > > With this patch applied we only take the locality at one place.
> > > > > Furthermore
> > > > > with interrupts enabled we dont have to claim the locality for each
> > > > > handler
> > > > > execution, saving us countless claim/release combinations at runtime.
> > > > >
> > > > > Hence the term "simplification" which is perfectly justified IMO.
> > > > >
> > > > > So again, this patch is "only" in preparation for the next patch when
> > > > > interrupts
> > > > > are actually enabled and we would have to take the locality in the
> > > > > interrupt
> > > > > handler without this patch.
> > > >
> > > > So: what problem this patch does solve?
> > > >
> > > > /Jarkko
> > > >
> > >
> > > first, thank you very much, Lino, for working on this! I've been debugging
> > > issues with the tis driver in the last days and was about to start with the
> > > same
> > > approach as yours when I luckily discovered your patch!
> > >
> > > Jarkko, while I agree, that the commit message is not optimal, Lino tried
> > > hard
> > > to explain what the problems with the current code are and how they are /
> > > can be
> > > fixed. Further, I too don't see why simplification / optimization is such a
> > > bad
> > > thing. This driver is actually a very good example. I had a hard time, too,
> > > figuring out what's going on there. A clean rewrite is a very valid approach
> > > here IMO. It's not "polishing for nothing", as you described it, but
> > > actually
> > > solving problems.
> > >
> > > Interrupt detection is broken for years now and finally a volunteer worked
> > > on a
> > > solution. Don't you think this should be valued? Let's get this problem
> > > sorted
> > > out :-)
> > >
> > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > work
> > > on this again!
> > >
> > > Thanks, Michael
> >
> > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > E.g. give a shot rc1, and please report if any issues persists to:
> >
> > ? [email protected]?
> >
> > BR, Jarkko
>
> I don't see Linos patches on mainline. Also, the series included four patches:
> [PATCH v3 0/4] Fixes for TPM interrupt handling
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
> [PATCH v3 4/4] tpm: Only enable supported irqs
>
> Three of them are relevant for the interrupt problem, which is still present in
> mainline, as these patches were refused:
> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> [PATCH v3 2/4] tpm: Simplify locality handling
> [PATCH v3 3/4] tpm: Fix test for interrupts
>
> Michael
There was some unaddressed feedback, most of not that hard to address.
I'm waiting for v4.
BR, Jarkko
On Wed, 2022-04-20 at 08:30 +0300, Jarkko Sakkinen wrote:
> n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> >
> > Hi Michael,
> >
> > On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > >
> > > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > > work
> > > > > on this again!
> > > > >
> > > > > Thanks, Michael
> > > >
> > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > > E.g. give a shot rc1, and please report if any issues persists to:
> > > >
> > > > [email protected]
> > > >
> > > > BR, Jarkko
> > >
> > > I don't see Linos patches on mainline. Also, the series included four patches:
> > > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > [PATCH v3 4/4] tpm: Only enable supported irqs
> > >
> > > Three of them are relevant for the interrupt problem, which is still present in
> > > mainline, as these patches were refused:
> > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > >
> > > Michael
> > >
> >
> > You are right, the interrupts are still not working in the mainline kernel.
> > I would gladly make another attempt to fix this but rather step by step
> > than in a series that tries to fix (different) things at once.
> >
> > A first step could be to have a sleepable context for the interrupt handling,
> > since in case of SPI the accesses to the irq status register may sleep.
> >
> > I sent a patch for this purpose once, but it seems to have gone lost:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> >
> > Best regards,
> > Lino
>
> I went these through one by one.
>
> # Above linked patch
>
> Boolean parameters are considered bad. I.e. use named flags
> instead. This is for above linked patch.
>
> # [PATCH v3 3/4] tpm: Fix test for interrupts
>
> 1. Please remove "unnecessarily complicated" sentence because
> it cannot be evaluated. It's your opinion, which might perhaps
> be correct, but it is irrelevant for any possible patch
> description.
> 2. There's no such thing as "fix by re-implementation". Please
> explain instead code change is relevant for the bug fix.
> 3. If set_bit() et al necessarily to fix a possible race condition
> you need to have a separate patch for that.
>
> To move forward, start with a better summary such as
>
> "tpm: move interrupt test to tpm_tis_probe_irq_single()"
>
> I'd also either revert the change for flags, or alternatively
> move it to separate patch explaining race condition. Otherwise,
> there's no argument of saying that using set_bit() is more
> proper. This will make the change more localized.
>
>
> # [PATCH v3 2/4] tpm: Simplify locality handling
>
> "As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:"
>
> Generally speaking, the simplifications should be done on top of code
> that does not have known bugs, even if the simplification renders out
> the bug. This is because then we have code that have potentially unknown
> unknown bugs.
>
> I hope you see my point. The problem with these patches were then
> and is still that they intermix bug fixes and other modifications and
> thus cannot be taken in.
I.e. to move forward create first localized fixes, and only after those
clean ups if there is point. Removing code (like in 2/4) is not a bug
fix fo anything. This not to say that some changes would be illegit, I'm
only saying that the patches are badly scoped.
BR, Jarkko
n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
>
> Hi Michael,
>
> On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > >
> > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > work
> > > > on this again!
> > > >
> > > > Thanks, Michael
> > >
> > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > E.g. give a shot rc1, and please report if any issues persists to:
> > >
> > > [email protected]
> > >
> > > BR, Jarkko
> >
> > I don't see Linos patches on mainline. Also, the series included four patches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> >
> > Three of them are relevant for the interrupt problem, which is still present in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> >
> > Michael
> >
>
> You are right, the interrupts are still not working in the mainline kernel.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
>
> A first step could be to have a sleepable context for the interrupt handling,
> since in case of SPI the accesses to the irq status register may sleep.
>
> I sent a patch for this purpose once, but it seems to have gone lost:
>
> https://lore.kernel.org/all/[email protected]/
>
>
> Best regards,
> Lino
I went these through one by one.
# Above linked patch
Boolean parameters are considered bad. I.e. use named flags
instead. This is for above linked patch.
# [PATCH v3 3/4] tpm: Fix test for interrupts
1. Please remove "unnecessarily complicated" sentence because
it cannot be evaluated. It's your opinion, which might perhaps
be correct, but it is irrelevant for any possible patch
description.
2. There's no such thing as "fix by re-implementation". Please
explain instead code change is relevant for the bug fix.
3. If set_bit() et al necessarily to fix a possible race condition
you need to have a separate patch for that.
To move forward, start with a better summary such as
"tpm: move interrupt test to tpm_tis_probe_irq_single()"
I'd also either revert the change for flags, or alternatively
move it to separate patch explaining race condition. Otherwise,
there's no argument of saying that using set_bit() is more
proper. This will make the change more localized.
# [PATCH v3 2/4] tpm: Simplify locality handling
"As a side-effect these modifications fix a bug which results in the
following warning when using TPM 2:"
Generally speaking, the simplifications should be done on top of code
that does not have known bugs, even if the simplification renders out
the bug. This is because then we have code that have potentially unknown
unknown bugs.
I hope you see my point. The problem with these patches were then
and is still that they intermix bug fixes and other modifications and
thus cannot be taken in.
BR, Jarkko
Hi,
On 20.04.22 at 07:30, Jarkko Sakkinen wrote:
> n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
>>
>> Hi Michael,
>>
>> On 25.03.22 at 13:32, Michael Niewöhner wrote:
>>>>>
>>>>> Lino, I'd be happy to test the patches, when you have time and interest to
>>>>> work
>>>>> on this again!
>>>>>
>>>>> Thanks, Michael
>>>>
>>>> It's quite easy to test them out. Both fixes are in the mainline GIT tree.
>>>> E.g. give a shot rc1, and please report if any issues persists to:
>>>>
>>>> [email protected]
>>>>
>>>> BR, Jarkko
>>>
>>> I don't see Linos patches on mainline. Also, the series included four patches:
>>> [PATCH v3 0/4] Fixes for TPM interrupt handling
>>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
>>> [PATCH v3 2/4] tpm: Simplify locality handling
>>> [PATCH v3 3/4] tpm: Fix test for interrupts
>>> [PATCH v3 4/4] tpm: Only enable supported irqs
>>>
>>> Three of them are relevant for the interrupt problem, which is still present in
>>> mainline, as these patches were refused:
>>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler
>>> [PATCH v3 2/4] tpm: Simplify locality handling
>>> [PATCH v3 3/4] tpm: Fix test for interrupts
>>>
>>> Michael
>>>
>>
>> You are right, the interrupts are still not working in the mainline kernel.
>> I would gladly make another attempt to fix this but rather step by step
>> than in a series that tries to fix (different) things at once.
>>
>> A first step could be to have a sleepable context for the interrupt handling,
>> since in case of SPI the accesses to the irq status register may sleep.
>>
>> I sent a patch for this purpose once, but it seems to have gone lost:
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>>
>> Best regards,
>> Lino
>
> I went these through one by one>
> # Above linked patch
>
> Boolean parameters are considered bad. I.e. use named flags
> instead. This is for above linked patch.
Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ"
for this.
>
> # [PATCH v3 3/4] tpm: Fix test for interrupts
>
> 1. Please remove "unnecessarily complicated" sentence because
> it cannot be evaluated. It's your opinion, which might perhaps
> be correct, but it is irrelevant for any possible patch
> description.
> 2. There's no such thing as "fix by re-implementation". Please
> explain instead code change is relevant for the bug fix.
> 3. If set_bit() et al necessarily to fix a possible race condition
> you need to have a separate patch for that.
>
> To move forward, start with a better summary such as
>
> "tpm: move interrupt test to tpm_tis_probe_irq_single()"
>
> I'd also either revert the change for flags, or alternatively
> move it to separate patch explaining race condition. Otherwise,
> there's no argument of saying that using set_bit() is more
> proper. This will make the change more localized.
>
Ok, I will split the fix for the irq test into two patches then.
>
> # [PATCH v3 2/4] tpm: Simplify locality handling
>
> "As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:"
>
> Generally speaking, the simplifications should be done on top of code
> that does not have known bugs, even if the simplification renders out
> the bug. This is because then we have code that have potentially unknown
> unknown bugs.
>
> I hope you see my point. The problem with these patches were then
> and is still that they intermix bug fixes and other modifications and
> thus cannot be taken in.
>
Yes, I can see that point.
> BR, Jarkko
>
Thanks a lot for the review. I will prepare new patches with the suggested
changes.
Best regards,
Lino
On Sun, 2022-04-24 at 04:22 +0200, Lino Sanfilippo wrote:
>
> Hi,
>
> On 20.04.22 at 07:30, Jarkko Sakkinen wrote:
> > n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> > >
> > > Hi Michael,
> > >
> > > On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > > >
> > > > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > > > work
> > > > > > on this again!
> > > > > >
> > > > > > Thanks, Michael
> > > > >
> > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree.
> > > > > E.g. give a shot rc1, and please report if any issues persists to:
> > > > >
> > > > > [email protected]
> > > > >
> > > > > BR, Jarkko
> > > >
> > > > I don't see Linos patches on mainline. Also, the series included four patches:
> > > > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > > [PATCH v3 4/4] tpm: Only enable supported irqs
> > > >
> > > > Three of them are relevant for the interrupt problem, which is still present in
> > > > mainline, as these patches were refused:
> > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > > > [PATCH v3 2/4] tpm: Simplify locality handling
> > > > [PATCH v3 3/4] tpm: Fix test for interrupts
> > > >
> > > > Michael
> > > >
> > >
> > > You are right, the interrupts are still not working in the mainline kernel.
> > > I would gladly make another attempt to fix this but rather step by step
> > > than in a series that tries to fix (different) things at once.
> > >
> > > A first step could be to have a sleepable context for the interrupt handling,
> > > since in case of SPI the accesses to the irq status register may sleep.
> > >
> > > I sent a patch for this purpose once, but it seems to have gone lost:
> > >
> > > https://lore.kernel.org/all/[email protected]/
> > >
> > >
> > > Best regards,
> > > Lino
> >
> > I went these through one by one>
> > # Above linked patch
> >
> > Boolean parameters are considered bad. I.e. use named flags
> > instead. This is for above linked patch.
>
> Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ"
> for this.
>
> >
> > # [PATCH v3 3/4] tpm: Fix test for interrupts
> >
> > 1. Please remove "unnecessarily complicated" sentence because
> > it cannot be evaluated. It's your opinion, which might perhaps
> > be correct, but it is irrelevant for any possible patch
> > description.
> > 2. There's no such thing as "fix by re-implementation". Please
> > explain instead code change is relevant for the bug fix.
> > 3. If set_bit() et al necessarily to fix a possible race condition
> > you need to have a separate patch for that.
> >
> > To move forward, start with a better summary such as
> >
> > "tpm: move interrupt test to tpm_tis_probe_irq_single()"
> >
> > I'd also either revert the change for flags, or alternatively
> > move it to separate patch explaining race condition. Otherwise,
> > there's no argument of saying that using set_bit() is more
> > proper. This will make the change more localized.
> >
>
> Ok, I will split the fix for the irq test into two patches then.
>
> >
> > # [PATCH v3 2/4] tpm: Simplify locality handling
> >
> > "As a side-effect these modifications fix a bug which results in the
> > following warning when using TPM 2:"
> >
> > Generally speaking, the simplifications should be done on top of code
> > that does not have known bugs, even if the simplification renders out
> > the bug. This is because then we have code that have potentially unknown
> > unknown bugs.
> >
> > I hope you see my point. The problem with these patches were then
> > and is still that they intermix bug fixes and other modifications and
> > thus cannot be taken in.
> >
>
> Yes, I can see that point.
>
> > BR, Jarkko
> >
>
> Thanks a lot for the review. I will prepare new patches with the suggested
> changes.
Yeah, I mean the point being: it's OK to suggest clean ups but with bug fixes
you should aim for the lowest common denominator as far as you possibly can.
BR, Jarkko