[PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[Tianjia Zhang]

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(-)

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;
-- 
2.19.1.3.ge56e4f7

Re: [PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[Tianjia Zhang]

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

Re: [PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Fix variable reset during IRQ probing

[Tianjia Zhang]

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