Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm

From: Jarkko Sakkinen
Date: Wed May 24 2023 - 00:00:16 EST

Next message: Yang Li: "[PATCH -next 04/13] drm/amd/display: remove unused definition"
Previous message: Yang Li: "[PATCH -next 03/13] drm/amd/display: remove unused definition"
Next in thread: Lukas Wunner: "Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed May 24, 2023 at 6:58 AM EEST, Jarkko Sakkinen wrote:
> Hi,
>
> Sorry, some minor glitches.
>
> On Mon May 22, 2023 at 5:31 PM EEST, Lino Sanfilippo wrote:
> > From: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> >
> > Commit e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") enabled
> > interrupts instead of polling on all capable TPMs. Unfortunately, on some
> > products the interrupt line is either never asserted or never deasserted.
>
> Use Reported-by and Closes tag and remove this paragraph.
>
> In Closes link instead from lore the email where the bug was reported.
>
> > The former causes interrupt timeouts and is detected by
> > tpm_tis_core_init(). The latter results in interrupt storms.
>
> Please describe instead the system where the bug was realized. Don't
> worry about speculative descriptions. We only deal with ones actually
> realized.
>
> > Recent reports concern the Lenovo ThinkStation P360 Tiny, Lenovo ThinkPad
> > L490 and Inspur NF5180M6:
> >
> > https://lore.kernel.org/linux-integrity/20230511005403.24689-1-jsnitsel@xxxxxxxxxx/
> > https://lore.kernel.org/linux-integrity/d80b180a569a9f068d3a2614f062cfa3a78af5a6.camel@xxxxxxxxxx/
>
> Please remove all of this, as the fixes have been handled. Let's keep
> the commit message focused.
>
> > The current approach to avoid those storms is to disable interrupts by
> > adding a DMI quirk for the concerned device.
> >
> > However this is a maintenance burden in the long run, so use a generic
> > approach:
> >
> > Detect an interrupt storm by counting the number of unhandled interrupts
> > within a 10 ms time interval. In case that more than 1000 were unhandled
> > deactivate interrupts, deregister the handler and fall back to polling.
> >
> > This equals the implementation that handles interrupt storms in
> > note_interrupt() by means of timestamps and counters in struct irq_desc.
> > However the function to access this structure is private so the logic has
> > to be reimplemented in the TPM TIS core.
>
> I only now found out that this is based on kernel/irq/spurious.c code
> partly. Why this was unmentioned?
>
> That would make this already more legitimate because it is based
> on field tested metrics.
>
> Then we only have to discuss about counter.
>
> > routine trigger a worker thread that executes the unregistration.
> >
> > Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> > Signed-off-by: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 71 +++++++++++++++++++++++++++++++--
> > drivers/char/tpm/tpm_tis_core.h | 6 +++
> > 2 files changed, 74 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 558144fa707a..458ebf8c2f16 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -752,6 +752,55 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> > return status == TPM_STS_COMMAND_READY;
> > }
> >
> > +static void tpm_tis_handle_irq_storm(struct tpm_chip *chip)
> > +{
> > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > + int intmask = 0;
> > +
> > + dev_err(&chip->dev, HW_ERR
> > + "TPM interrupt storm detected, polling instead\n");
>
> Degrading this to warn is fine because it is legit behaviour in a
> sense.
>
> > +
> > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> > +
> > + intmask &= ~TPM_GLOBAL_INT_ENABLE;
> > +
> > + tpm_tis_request_locality(chip, 0);
> > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> > + tpm_tis_relinquish_locality(chip, 0);
> > +
> > + chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > +
> > + /*
> > + * We must not call devm_free_irq() from within the interrupt handler,
>
> Never use "we" form. Always use either:
>
> 1. Imperative
> 2. Passive
>
> I.e. to address this, you would write instead "devm_free_irq() must not
> be called within the interrupt handler because ...".
>
> > + * since this function waits for running interrupt handlers to finish
> > + * and thus it would deadlock. Instead trigger a worker that does the
> > + * unregistration.
> > + */
> > + schedule_work(&priv->free_irq_work);
> > +}
> > +
> > +static void tpm_tis_process_unhandled_interrupt(struct tpm_chip *chip)
> > +{
> > + const unsigned int MAX_UNHANDLED_IRQS = 1000;
> > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
> Reverse order and add empty line.
>
> > + /*
> > + * The worker to free the TPM interrupt (free_irq_work) may already
> > + * be scheduled, so make sure it is not scheduled again.
> > + */
> > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > + return;
> > +
> > + if (time_after(jiffies, priv->last_unhandled_irq + HZ/10))
> > + priv->unhandled_irqs = 1;
> > + else
> > + priv->unhandled_irqs++;
> > +
> > + priv->last_unhandled_irq = jiffies;
> > +
> > + if (priv->unhandled_irqs > MAX_UNHANDLED_IRQS)
> > + tpm_tis_handle_irq_storm(chip);
>
> Why wouldn't we switch to polling mode even when there is a single
> unhandled IRQ?
>
> > +}
> > +
> > static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> > {
> > struct tpm_chip *chip = dev_id;
> > @@ -761,10 +810,10 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> >
> > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
> > if (rc < 0)
> > - return IRQ_NONE;
> > + goto unhandled;
> >
> > if (interrupt == 0)
> > - return IRQ_NONE;
> > + goto unhandled;
> >
> > set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
> > if (interrupt & TPM_INTF_DATA_AVAIL_INT)
> > @@ -780,10 +829,14 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), interrupt);
> > tpm_tis_relinquish_locality(chip, 0);
> > if (rc < 0)
> > - return IRQ_NONE;
> > + goto unhandled;
> >
> > tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
> > return IRQ_HANDLED;
> > +
> > +unhandled:
> > + tpm_tis_process_unhandled_interrupt(chip);
> > + return IRQ_HANDLED;
> > }
> >
> > static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> > @@ -804,6 +857,15 @@ static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> > chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > }
> >
> > +static void tpm_tis_free_irq_func(struct work_struct *work)
> > +{
> > + struct tpm_tis_data *priv = container_of(work, typeof(*priv), free_irq_work);
> > + struct tpm_chip *chip = priv->chip;
> > +
> > + devm_free_irq(chip->dev.parent, priv->irq, chip);
> > + priv->irq = 0;
> > +}
> > +
> > /* Register the IRQ and issue a command that will cause an interrupt. If an
> > * irq is seen then leave the chip setup for IRQ operation, otherwise reverse
> > * everything and leave in polling mode. Returns 0 on success.
> > @@ -816,6 +878,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> > int rc;
> > u32 int_status;
> >
> > + INIT_WORK(&priv->free_irq_work, tpm_tis_free_irq_func);
> >
> > rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
> > tis_int_handler, IRQF_ONESHOT | flags,
> > @@ -918,6 +981,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
> > interrupt = 0;
> >
> > tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
> > + flush_work(&priv->free_irq_work);
> >
> > tpm_tis_clkrun_enable(chip, false);
> >
> > @@ -1021,6 +1085,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
> > chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX);
> > chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX);
> > + priv->chip = chip;
> > priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
> > priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
> > priv->phy_ops = phy_ops;
> > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> > index e978f457fd4d..6fc86baa4398 100644
> > --- a/drivers/char/tpm/tpm_tis_core.h
> > +++ b/drivers/char/tpm/tpm_tis_core.h
> > @@ -91,12 +91,18 @@ enum tpm_tis_flags {
> > };
> >
> > struct tpm_tis_data {
> > + struct tpm_chip *chip;
> > u16 manufacturer_id;
> > struct mutex locality_count_mutex;
> > unsigned int locality_count;
> > int locality;
> > + /* Interrupts */
> > int irq;
> > + struct work_struct free_irq_work;
> > + unsigned long last_unhandled_irq;
> > + unsigned int unhandled_irqs;
> > unsigned int int_mask;
> > +
> > unsigned long flags;
> > void __iomem *ilb_base_addr;
> > u16 clkrun_enabled;
> >
> > base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
> > --
> > 2.40.1

I added 'irq-storm' branch where I have the latest fixes:

git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git

All known DMI table updates are now afaik in the mainline.

BR, Jarkko

Next message: Yang Li: "[PATCH -next 04/13] drm/amd/display: remove unused definition"
Previous message: Yang Li: "[PATCH -next 03/13] drm/amd/display: remove unused definition"
Next in thread: Lukas Wunner: "Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]