Jerry Snitselaar @ 2020-12-01 11:12 MST:
> James Bottomley @ 2020-10-01 11:09 MST:
>
>> If a TIS TPM doesn't have legacy cycles, any write to the interrupt
>> registers is ignored unless a locality is active. This means even to
>> set up the interrupt vectors a locality must first be activated. Fix
>> this by activating the 0 locality in the interrupt probe setup.
>>
>> Since the TPM_EOI signalling also requires an active locality, the
>> interrupt routine cannot end an interrupt if the locality is released.
>> This can lead to a situation where the TPM goes command ready after
>> locality release and since the interrupt cannot be ended it refires
>> continuously. Fix this by disabling all interrupts except locality
>> change when a locality is released (this only fires when a locality
>> becomes active, meaning the TPM_EOI should work).
>>
>> Finally, since we now disable all status based interrupts in the
>> locality release, they must be re-enabled before waiting to check the
>> condition, so add interrupt enabling to the status wait.
>>
>> Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
>>
>> ---
>>
>> v2: renamed functions
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------
>> 1 file changed, 101 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 431919d5f48a..0c07da8cd680 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -29,6 +29,46 @@
>>
>> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>>
>> +static void tpm_tis_enable_interrupt(struct tpm_chip *chip, u8 mask)
>> +{
>> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> + u32 intmask;
>> +
>> + /* Take control of the TPM's interrupt hardware and shut it off */
>> + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>> +
>> + intmask |= mask | TPM_GLOBAL_INT_ENABLE;
>> +
>> + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> +}
>> +
>> +static void tpm_tis_disable_interrupt(struct tpm_chip *chip, u8 mask)
>> +{
>> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> + u32 intmask;
>> +
>> + /* Take control of the TPM's interrupt hardware and shut it off */
>> + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>> +
>> + intmask &= ~mask;
>> +
>> + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> +}
>> +
>> +static void tpm_tis_enable_stat_interrupts(struct tpm_chip *chip, u8 stat)
>> +{
>> + u32 mask = 0;
>> +
>> + if (stat & TPM_STS_COMMAND_READY)
>> + mask |= TPM_INTF_CMD_READY_INT;
>> + if (stat & TPM_STS_VALID)
>> + mask |= TPM_INTF_STS_VALID_INT;
>> + if (stat & TPM_STS_DATA_AVAIL)
>> + mask |= TPM_INTF_DATA_AVAIL_INT;
>> +
>> + tpm_tis_enable_interrupt(chip, mask);
>> +}
>> +
>> static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>> bool check_cancel, bool *canceled)
>> {
>> @@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>> timeout = stop - jiffies;
>> if ((long)timeout <= 0)
>> return -ETIME;
>> + tpm_tis_enable_stat_interrupts(chip, mask);
>> rc = wait_event_interruptible_timeout(*queue,
>> wait_for_tpm_stat_cond(chip, mask, check_cancel,
>> &canceled),
>> timeout);
>> if (rc > 0) {
>> + if (rc == 1)
>> + dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n");
>
> With my proposed patch to check for the interrupt storm condition I
> sometimes see this message. Do you think it would make sense to have a
> check here and in the request_locality location to see that
> TPM_CHIP_FLAG is still enabled? It will print a message about the
> interrupt storm being detected, and switching to polling, so I don't
> know if this will cause confusion for people to have this show up as
> well in that case.
>
I guess it wouldn't be too confusing since the messages will appear
close together.
