Re: [PATCH v2 3/5] tpm_tis: Fix interrupts for TIS TPMs without legacy cycles

[Jerry Snitselaar <jsnitsel@redhat.com>]

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@HansenPartnership.com>
>>
>> ---
>>
>> 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.