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

[Jerry Snitselaar <jsnitsel@redhat.com>]

James Bottomley @ 2020-12-01 14:06 MST:

> On Tue, 2020-12-01 at 12:49 -0700, Jerry Snitselaar wrote:
>> 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.
>
> But since we have a discriminator I'll try to use it and see if we can
> tidy up the messages.  I think the condition just becomes
>
> if (rc == 1 && (chip->flags & TPM_CHIP_FLAG_IRQ))
>
> because you'll have reset that if you found a storm?
>
> James


Yes, the worker calls disable_interrupts() and that clears the flag.

Jerry