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