| Message ID | 20210113120021.59045-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm/tpm_tis: Fix variable reset during IRQ probing | expand |
On Wed, Jan 13, 2021 at 08:00:21PM +0800, Tianjia Zhang wrote: > In tpm_tis_core_init(), tpm2_probe() will be called first, this > function will eventually call tpm_tis_send(), and then > tpm_tis_probe_irq_single() will detect whether the interrupt is > normal, mainly the installation interrupted, set `priv->irq_tested` > to false. The logic will eventually be executed to tpm_tis_send() > to trigger an interrupt. > > There is currently such a scenario, which will cause the IRQ probe > code to never be executed, so that the TPM device is in polling > mode: after setting irq_tested to false, an interrupt occurs > between entering the ttpm_tis_send() function, and the interrupt > will be first set irq_tested to true will cause the IRQ probe code > to never be executed. Can you describe the scenario more detail? > It seems that this interrupt comes from tpm2_probe(). Although the > interrupt has not been installed when tpm2_probe() is called, the > interrupt of tpm2_probe() is only received after IRQ detection. > > This patch solves this issue by introducing a new variable, which > is only used in interrupts, and irq_tested only marks whether the > interrupt test has been completed. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- I'm not sure I understand this patch. TPM should be in polling mode. This is also assumption before calling tpm_get_timeouts(): /* Before doing irq testing issue a command to the TPM in polling mode * to make sure it works. May as well use that command to set the * proper timeouts for the driver. */ if (tpm_get_timeouts(chip)) { dev_err(dev, "Could not get TPM timeouts and durations\n"); rc = -ENODEV; goto out_err; } /Jarkko
On 1/14/21 10:51 AM, Jarkko Sakkinen wrote: > On Wed, Jan 13, 2021 at 08:00:21PM +0800, Tianjia Zhang wrote: >> In tpm_tis_core_init(), tpm2_probe() will be called first, this >> function will eventually call tpm_tis_send(), and then >> tpm_tis_probe_irq_single() will detect whether the interrupt is >> normal, mainly the installation interrupted, set `priv->irq_tested` >> to false. The logic will eventually be executed to tpm_tis_send() >> to trigger an interrupt. >> >> There is currently such a scenario, which will cause the IRQ probe >> code to never be executed, so that the TPM device is in polling >> mode: after setting irq_tested to false, an interrupt occurs >> between entering the ttpm_tis_send() function, and the interrupt >> will be first set irq_tested to true will cause the IRQ probe code >> to never be executed. > > Can you describe the scenario more detail? > The problematic scenario we encountered is like this. The following figure shows the execution flow of tpm_tis_core_init(). An interrupt occurred before the IRQ probe. This interrupt was caused by tpm2_probe(), but it was triggered before the IRQ probe was executed, and the interrupt handler would set irq_tested to true, so the IRQ probe code can never execute, that is, the code marked 2 in the figure will never happen. IRQ tpm_tis_core_init() tpm2_probe() tpm_tis_send() -----------+ | tpm_tis_probe_irq_single() | | devm_request_irq() | 1 priv->irq_tested = false | tpm_tis_gen_interrupt() | | tpm_tis_send() | irq_tested = true | <------------------+ if (priv->irq_tested) return tpm_tis_send_main() /* probe IRQ */ tpm_tis_send_main() --------+ | 2 chip->flags |= FLAG_IRQ <-------+ priv->irq_tested = true Best regards, Tianjia
On Thu, Jan 14, 2021 at 12:12:16PM +0800, Tianjia Zhang wrote: > > > On 1/14/21 10:51 AM, Jarkko Sakkinen wrote: > > On Wed, Jan 13, 2021 at 08:00:21PM +0800, Tianjia Zhang wrote: > > > In tpm_tis_core_init(), tpm2_probe() will be called first, this > > > function will eventually call tpm_tis_send(), and then > > > tpm_tis_probe_irq_single() will detect whether the interrupt is > > > normal, mainly the installation interrupted, set `priv->irq_tested` > > > to false. The logic will eventually be executed to tpm_tis_send() > > > to trigger an interrupt. > > > > > > There is currently such a scenario, which will cause the IRQ probe > > > code to never be executed, so that the TPM device is in polling > > > mode: after setting irq_tested to false, an interrupt occurs > > > between entering the ttpm_tis_send() function, and the interrupt > > > will be first set irq_tested to true will cause the IRQ probe code > > > to never be executed. > > > > Can you describe the scenario more detail? > > > > The problematic scenario we encountered is like this. The following figure > shows the execution flow of tpm_tis_core_init(). An interrupt occurred > before the IRQ probe. This interrupt was caused by tpm2_probe(), but it was > triggered before the IRQ probe was executed, and the interrupt handler would > set irq_tested to true, so the IRQ probe code can never execute, that is, > the code marked 2 in the figure will never happen. TPM_INT_ENABLE is cleared on reset [*]. [*] Section 5.9.1 https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis/ /Jarkko
On 1/15/21 5:23 PM, Jarkko Sakkinen wrote: > On Thu, Jan 14, 2021 at 12:12:16PM +0800, Tianjia Zhang wrote: >> >> >> On 1/14/21 10:51 AM, Jarkko Sakkinen wrote: >>> On Wed, Jan 13, 2021 at 08:00:21PM +0800, Tianjia Zhang wrote: >>>> In tpm_tis_core_init(), tpm2_probe() will be called first, this >>>> function will eventually call tpm_tis_send(), and then >>>> tpm_tis_probe_irq_single() will detect whether the interrupt is >>>> normal, mainly the installation interrupted, set `priv->irq_tested` >>>> to false. The logic will eventually be executed to tpm_tis_send() >>>> to trigger an interrupt. >>>> >>>> There is currently such a scenario, which will cause the IRQ probe >>>> code to never be executed, so that the TPM device is in polling >>>> mode: after setting irq_tested to false, an interrupt occurs >>>> between entering the ttpm_tis_send() function, and the interrupt >>>> will be first set irq_tested to true will cause the IRQ probe code >>>> to never be executed. >>> >>> Can you describe the scenario more detail? >>> >> >> The problematic scenario we encountered is like this. The following figure >> shows the execution flow of tpm_tis_core_init(). An interrupt occurred >> before the IRQ probe. This interrupt was caused by tpm2_probe(), but it was >> triggered before the IRQ probe was executed, and the interrupt handler would >> set irq_tested to true, so the IRQ probe code can never execute, that is, >> the code marked 2 in the figure will never happen. > > TPM_INT_ENABLE is cleared on reset [*]. > > [*] Section 5.9.1 > https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis/ > > /Jarkko > Hi, I got it, this seems to be a firmware issue. Thanks for your reply. Best regards, Tianjia
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 92c51c6cfd1b..d7589b0b3e56 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -502,7 +502,7 @@ 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 (priv->irq_tested) return tpm_tis_send_main(chip, buf, len); /* Verify receipt of the expected IRQ */ @@ -512,9 +512,9 @@ 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 (!priv->int_count) tpm_msleep(1); - if (!priv->irq_tested) + if (!priv->int_count) disable_interrupts(chip); priv->irq_tested = true; return rc; @@ -725,7 +725,7 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id) if (interrupt == 0) return IRQ_NONE; - priv->irq_tested = true; + priv->int_count += 1; if (interrupt & TPM_INTF_DATA_AVAIL_INT) wake_up_interruptible(&priv->read_queue); if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT) diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 9b2d32a59f67..c6845672f6f7 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -90,6 +90,7 @@ struct tpm_tis_data { int locality; int irq; bool irq_tested; + unsigned int int_count; unsigned int flags; void __iomem *ilb_base_addr; u16 clkrun_enabled;
In tpm_tis_core_init(), tpm2_probe() will be called first, this function will eventually call tpm_tis_send(), and then tpm_tis_probe_irq_single() will detect whether the interrupt is normal, mainly the installation interrupted, set `priv->irq_tested` to false. The logic will eventually be executed to tpm_tis_send() to trigger an interrupt. There is currently such a scenario, which will cause the IRQ probe code to never be executed, so that the TPM device is in polling mode: after setting irq_tested to false, an interrupt occurs between entering the ttpm_tis_send() function, and the interrupt will be first set irq_tested to true will cause the IRQ probe code to never be executed. It seems that this interrupt comes from tpm2_probe(). Although the interrupt has not been installed when tpm2_probe() is called, the interrupt of tpm2_probe() is only received after IRQ detection. This patch solves this issue by introducing a new variable, which is only used in interrupts, and irq_tested only marks whether the interrupt test has been completed. Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> --- drivers/char/tpm/tpm_tis_core.c | 8 ++++---- drivers/char/tpm/tpm_tis_core.h | 1 + 2 files changed, 5 insertions(+), 4 deletions(-)