| Message ID | 144f1c66-499e-2da9-c4c1-b5f26cb8841f@kunbus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,RESEND] tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs | expand |
On Thu Jul 13, 2023 at 7:01 PM UTC, Lino Sanfilippo wrote: > After activation of interrupts for TPM TIS drivers 0-day reports an > interrupt storm on an Inspur NF5180M6 server. > > Fix this by detecting the storm and falling back to polling: > Count the number of unhandled interrupts within a 10 ms time interval. In > case that more than 1000 were unhandled deactivate interrupts entirely, > deregister the handler and use polling instead. > > Also print a note to point to the tpm_tis_dmi_table. > > Since the interrupt deregistration function devm_free_irq() waits for all > interrupt handlers to finish, only trigger a worker in the interrupt > handler and do the unregistration in the worker to avoid a deadlock. > > Note: the storm detection logic equals the implementation in > note_interrupt() which uses timestamps and counters stored in struct > irq_desc. Since this structure is private to the generic interrupt core > the TPM TIS core uses its own timestamps and counters. Furthermore the TPM > interrupt handler always returns IRQ_HANDLED to prevent the generic > interrupt core from processing the interrupt storm. > > Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") > Reported-by: kernel test robot <yujie.liu@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202305041325.ae8b0c43-yujie.liu@intel.com/ > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/char/tpm/tpm_tis_core.c | 103 +++++++++++++++++++++++++++----- > drivers/char/tpm/tpm_tis_core.h | 4 ++ > 2 files changed, 92 insertions(+), 15 deletions(-) > > Resending the patch due to several bounce messages for the first attempt. > > Changes to v3 (all requested by Jarko): > - remove all inline comments > - rename tpm_tis_reenable_polling() to tpm_tis_revert_interrupts() > - rename tpm_tis_check_for_interrupt_storm() to tpm_tis_update_unhandled_irqs() > - rename label "unhandled" to "err" > - add Fixes: tag > > Changes to v2: > - use define for max number of unhandles irqs(requested by Jarko) > - rename intmask to int_mask (requested by Jarko) > - rephrased short summary (requested by Jarko) > - rename disable_interrupts to tpm_tis_disable_interrupts (requested by Jarko) > - print info message concerning adding an entry to tpm_tis_dmi_table > (suggested by Jerry) > - amended commit message > - handle failure of locality request by returning IRQ_NONE > - dont take and release locality in __tpm_tis_disable_interrupts but in its > caller > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 558144fa707a..88a5384c09c0 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -24,9 +24,12 @@ > #include <linux/wait.h> > #include <linux/acpi.h> > #include <linux/freezer.h> > +#include <linux/dmi.h> > #include "tpm.h" > #include "tpm_tis_core.h" > > +#define TPM_TIS_MAX_UNHANDLED_IRQS 1000 > + > static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); > > static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, > @@ -468,25 +471,29 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) > return rc; > } > > -static void disable_interrupts(struct tpm_chip *chip) > +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 int_mask = 0; > + > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &int_mask); > + int_mask &= ~TPM_GLOBAL_INT_ENABLE; > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), int_mask); > + > + chip->flags &= ~TPM_CHIP_FLAG_IRQ; > +} > + > +static void tpm_tis_disable_interrupts(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - u32 intmask; > - int rc; > > if (priv->irq == 0) > return; > > - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > - if (rc < 0) > - intmask = 0; > - > - intmask &= ~TPM_GLOBAL_INT_ENABLE; > - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > + __tpm_tis_disable_interrupts(chip); > > devm_free_irq(chip->dev.parent, priv->irq, chip); > priv->irq = 0; > - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > } > > /* > @@ -552,7 +559,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) > tpm_msleep(1); > if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) > - disable_interrupts(chip); > + tpm_tis_disable_interrupts(chip); > set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); > return rc; > } > @@ -752,6 +759,57 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) > return status == TPM_STS_COMMAND_READY; > } > > +static irqreturn_t tpm_tis_revert_interrupts(struct tpm_chip *chip) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + const char *product; > + const char *vendor; > + > + dev_warn(&chip->dev, FW_BUG > + "TPM interrupt storm detected, polling instead\n"); > + > + vendor = dmi_get_system_info(DMI_SYS_VENDOR); > + product = dmi_get_system_info(DMI_PRODUCT_VERSION); > + > + if (vendor && product) { > + dev_info(&chip->dev, > + "Consider adding the following entry to tpm_tis_dmi_table:\n"); > + dev_info(&chip->dev, "\tDMI_SYS_VENDOR: %s\n", vendor); > + dev_info(&chip->dev, "\tDMI_PRODUCT_VERSION: %s\n", product); > + } > + > + if (tpm_tis_request_locality(chip, 0) != 0) > + return IRQ_NONE; > + > + __tpm_tis_disable_interrupts(chip); > + tpm_tis_relinquish_locality(chip, 0); > + > + schedule_work(&priv->free_irq_work); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t tpm_tis_update_unhandled_irqs(struct tpm_chip *chip) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + irqreturn_t irqret = IRQ_HANDLED; > + > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > + return IRQ_HANDLED; > + > + 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 > TPM_TIS_MAX_UNHANDLED_IRQS) > + irqret = tpm_tis_revert_interrupts(chip); > + > + return irqret; > +} > + > static irqreturn_t tis_int_handler(int dummy, void *dev_id) > { > struct tpm_chip *chip = dev_id; > @@ -761,10 +819,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 err; > > if (interrupt == 0) > - return IRQ_NONE; > + goto err; > > set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); > if (interrupt & TPM_INTF_DATA_AVAIL_INT) > @@ -780,10 +838,13 @@ 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 err; > > tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt); > return IRQ_HANDLED; > + > +err: > + return tpm_tis_update_unhandled_irqs(chip); > } > > static void tpm_tis_gen_interrupt(struct tpm_chip *chip) > @@ -804,6 +865,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 +886,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 +989,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 +1093,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; > @@ -1179,7 +1252,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > rc = tpm_tis_request_locality(chip, 0); > if (rc < 0) > goto out_err; > - disable_interrupts(chip); > + tpm_tis_disable_interrupts(chip); > tpm_tis_relinquish_locality(chip, 0); > } > } > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > index 610bfadb6acf..b1a169d7d1ca 100644 > --- a/drivers/char/tpm/tpm_tis_core.h > +++ b/drivers/char/tpm/tpm_tis_core.h > @@ -91,11 +91,15 @@ 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; > 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; > > base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1 > -- > 2.25.1 Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> Looks very good, thank you. BR, Jarkko
On 14.07.23 05:39, Jarkko Sakkinen wrote: > On Thu Jul 13, 2023 at 7:01 PM UTC, Lino Sanfilippo wrote: >> After activation of interrupts for TPM TIS drivers 0-day reports an >> interrupt storm on an Inspur NF5180M6 server. >> >> Fix this by detecting the storm and falling back to polling: >> Count the number of unhandled interrupts within a 10 ms time interval. In >> case that more than 1000 were unhandled deactivate interrupts entirely, >> deregister the handler and use polling instead. >> >> Also print a note to point to the tpm_tis_dmi_table. >> >> Since the interrupt deregistration function devm_free_irq() waits for all >> interrupt handlers to finish, only trigger a worker in the interrupt >> handler and do the unregistration in the worker to avoid a deadlock. >> >> Note: the storm detection logic equals the implementation in >> note_interrupt() which uses timestamps and counters stored in struct >> irq_desc. Since this structure is private to the generic interrupt core >> the TPM TIS core uses its own timestamps and counters. Furthermore the TPM >> interrupt handler always returns IRQ_HANDLED to prevent the generic >> interrupt core from processing the interrupt storm. >> >> Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") >> Reported-by: kernel test robot <yujie.liu@intel.com> >> Closes: https://lore.kernel.org/oe-lkp/202305041325.ae8b0c43-yujie.liu@intel.com/ >> Suggested-by: Lukas Wunner <lukas@wunner.de> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/char/tpm/tpm_tis_core.c | 103 +++++++++++++++++++++++++++----- >> drivers/char/tpm/tpm_tis_core.h | 4 ++ >> 2 files changed, 92 insertions(+), 15 deletions(-) >> >> Resending the patch due to several bounce messages for the first attempt. >> >> Changes to v3 (all requested by Jarko): >> - remove all inline comments >> - rename tpm_tis_reenable_polling() to tpm_tis_revert_interrupts() >> - rename tpm_tis_check_for_interrupt_storm() to tpm_tis_update_unhandled_irqs() >> - rename label "unhandled" to "err" >> - add Fixes: tag >> >> Changes to v2: >> - use define for max number of unhandles irqs(requested by Jarko) >> - rename intmask to int_mask (requested by Jarko) >> - rephrased short summary (requested by Jarko) >> - rename disable_interrupts to tpm_tis_disable_interrupts (requested by Jarko) >> - print info message concerning adding an entry to tpm_tis_dmi_table >> (suggested by Jerry) >> - amended commit message >> - handle failure of locality request by returning IRQ_NONE >> - dont take and release locality in __tpm_tis_disable_interrupts but in its >> caller >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index 558144fa707a..88a5384c09c0 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -24,9 +24,12 @@ >> #include <linux/wait.h> >> #include <linux/acpi.h> >> #include <linux/freezer.h> >> +#include <linux/dmi.h> >> #include "tpm.h" >> #include "tpm_tis_core.h" >> >> +#define TPM_TIS_MAX_UNHANDLED_IRQS 1000 >> + >> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); >> >> static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, >> @@ -468,25 +471,29 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) >> return rc; >> } >> >> -static void disable_interrupts(struct tpm_chip *chip) >> +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> + u32 int_mask = 0; >> + >> + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &int_mask); >> + int_mask &= ~TPM_GLOBAL_INT_ENABLE; >> + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), int_mask); >> + >> + chip->flags &= ~TPM_CHIP_FLAG_IRQ; >> +} >> + >> +static void tpm_tis_disable_interrupts(struct tpm_chip *chip) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> - u32 intmask; >> - int rc; >> >> if (priv->irq == 0) >> return; >> >> - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); >> - if (rc < 0) >> - intmask = 0; >> - >> - intmask &= ~TPM_GLOBAL_INT_ENABLE; >> - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); >> + __tpm_tis_disable_interrupts(chip); >> >> devm_free_irq(chip->dev.parent, priv->irq, chip); >> priv->irq = 0; >> - chip->flags &= ~TPM_CHIP_FLAG_IRQ; >> } >> >> /* >> @@ -552,7 +559,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >> if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) >> tpm_msleep(1); >> if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) >> - disable_interrupts(chip); >> + tpm_tis_disable_interrupts(chip); >> set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); >> return rc; >> } >> @@ -752,6 +759,57 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) >> return status == TPM_STS_COMMAND_READY; >> } >> >> +static irqreturn_t tpm_tis_revert_interrupts(struct tpm_chip *chip) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> + const char *product; >> + const char *vendor; >> + >> + dev_warn(&chip->dev, FW_BUG >> + "TPM interrupt storm detected, polling instead\n"); >> + >> + vendor = dmi_get_system_info(DMI_SYS_VENDOR); >> + product = dmi_get_system_info(DMI_PRODUCT_VERSION); >> + >> + if (vendor && product) { >> + dev_info(&chip->dev, >> + "Consider adding the following entry to tpm_tis_dmi_table:\n"); >> + dev_info(&chip->dev, "\tDMI_SYS_VENDOR: %s\n", vendor); >> + dev_info(&chip->dev, "\tDMI_PRODUCT_VERSION: %s\n", product); >> + } >> + >> + if (tpm_tis_request_locality(chip, 0) != 0) >> + return IRQ_NONE; >> + >> + __tpm_tis_disable_interrupts(chip); >> + tpm_tis_relinquish_locality(chip, 0); >> + >> + schedule_work(&priv->free_irq_work); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t tpm_tis_update_unhandled_irqs(struct tpm_chip *chip) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> + irqreturn_t irqret = IRQ_HANDLED; >> + >> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >> + return IRQ_HANDLED; >> + >> + 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 > TPM_TIS_MAX_UNHANDLED_IRQS) >> + irqret = tpm_tis_revert_interrupts(chip); >> + >> + return irqret; >> +} >> + >> static irqreturn_t tis_int_handler(int dummy, void *dev_id) >> { >> struct tpm_chip *chip = dev_id; >> @@ -761,10 +819,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 err; >> >> if (interrupt == 0) >> - return IRQ_NONE; >> + goto err; >> >> set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); >> if (interrupt & TPM_INTF_DATA_AVAIL_INT) >> @@ -780,10 +838,13 @@ 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 err; >> >> tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt); >> return IRQ_HANDLED; >> + >> +err: >> + return tpm_tis_update_unhandled_irqs(chip); >> } >> >> static void tpm_tis_gen_interrupt(struct tpm_chip *chip) >> @@ -804,6 +865,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 +886,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 +989,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 +1093,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; >> @@ -1179,7 +1252,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> rc = tpm_tis_request_locality(chip, 0); >> if (rc < 0) >> goto out_err; >> - disable_interrupts(chip); >> + tpm_tis_disable_interrupts(chip); >> tpm_tis_relinquish_locality(chip, 0); >> } >> } >> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >> index 610bfadb6acf..b1a169d7d1ca 100644 >> --- a/drivers/char/tpm/tpm_tis_core.h >> +++ b/drivers/char/tpm/tpm_tis_core.h >> @@ -91,11 +91,15 @@ 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; >> 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; >> >> base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1 >> -- >> 2.25.1 > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > Looks very good, thank you. Great, thanks a lot for the review(s)! Regards, Lino
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 558144fa707a..88a5384c09c0 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -24,9 +24,12 @@ #include <linux/wait.h> #include <linux/acpi.h> #include <linux/freezer.h> +#include <linux/dmi.h> #include "tpm.h" #include "tpm_tis_core.h" +#define TPM_TIS_MAX_UNHANDLED_IRQS 1000 + static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, @@ -468,25 +471,29 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) return rc; } -static void disable_interrupts(struct tpm_chip *chip) +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 int_mask = 0; + + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &int_mask); + int_mask &= ~TPM_GLOBAL_INT_ENABLE; + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), int_mask); + + chip->flags &= ~TPM_CHIP_FLAG_IRQ; +} + +static void tpm_tis_disable_interrupts(struct tpm_chip *chip) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - u32 intmask; - int rc; if (priv->irq == 0) return; - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); - if (rc < 0) - intmask = 0; - - intmask &= ~TPM_GLOBAL_INT_ENABLE; - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); + __tpm_tis_disable_interrupts(chip); devm_free_irq(chip->dev.parent, priv->irq, chip); priv->irq = 0; - chip->flags &= ~TPM_CHIP_FLAG_IRQ; } /* @@ -552,7 +559,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) tpm_msleep(1); if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->flags)) - disable_interrupts(chip); + tpm_tis_disable_interrupts(chip); set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); return rc; } @@ -752,6 +759,57 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status) return status == TPM_STS_COMMAND_READY; } +static irqreturn_t tpm_tis_revert_interrupts(struct tpm_chip *chip) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + const char *product; + const char *vendor; + + dev_warn(&chip->dev, FW_BUG + "TPM interrupt storm detected, polling instead\n"); + + vendor = dmi_get_system_info(DMI_SYS_VENDOR); + product = dmi_get_system_info(DMI_PRODUCT_VERSION); + + if (vendor && product) { + dev_info(&chip->dev, + "Consider adding the following entry to tpm_tis_dmi_table:\n"); + dev_info(&chip->dev, "\tDMI_SYS_VENDOR: %s\n", vendor); + dev_info(&chip->dev, "\tDMI_PRODUCT_VERSION: %s\n", product); + } + + if (tpm_tis_request_locality(chip, 0) != 0) + return IRQ_NONE; + + __tpm_tis_disable_interrupts(chip); + tpm_tis_relinquish_locality(chip, 0); + + schedule_work(&priv->free_irq_work); + + return IRQ_HANDLED; +} + +static irqreturn_t tpm_tis_update_unhandled_irqs(struct tpm_chip *chip) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + irqreturn_t irqret = IRQ_HANDLED; + + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) + return IRQ_HANDLED; + + 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 > TPM_TIS_MAX_UNHANDLED_IRQS) + irqret = tpm_tis_revert_interrupts(chip); + + return irqret; +} + static irqreturn_t tis_int_handler(int dummy, void *dev_id) { struct tpm_chip *chip = dev_id; @@ -761,10 +819,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 err; if (interrupt == 0) - return IRQ_NONE; + goto err; set_bit(TPM_TIS_IRQ_TESTED, &priv->flags); if (interrupt & TPM_INTF_DATA_AVAIL_INT) @@ -780,10 +838,13 @@ 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 err; tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt); return IRQ_HANDLED; + +err: + return tpm_tis_update_unhandled_irqs(chip); } static void tpm_tis_gen_interrupt(struct tpm_chip *chip) @@ -804,6 +865,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 +886,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 +989,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 +1093,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; @@ -1179,7 +1252,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, rc = tpm_tis_request_locality(chip, 0); if (rc < 0) goto out_err; - disable_interrupts(chip); + tpm_tis_disable_interrupts(chip); tpm_tis_relinquish_locality(chip, 0); } } diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 610bfadb6acf..b1a169d7d1ca 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -91,11 +91,15 @@ 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; 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;
After activation of interrupts for TPM TIS drivers 0-day reports an interrupt storm on an Inspur NF5180M6 server. Fix this by detecting the storm and falling back to polling: Count the number of unhandled interrupts within a 10 ms time interval. In case that more than 1000 were unhandled deactivate interrupts entirely, deregister the handler and use polling instead. Also print a note to point to the tpm_tis_dmi_table. Since the interrupt deregistration function devm_free_irq() waits for all interrupt handlers to finish, only trigger a worker in the interrupt handler and do the unregistration in the worker to avoid a deadlock. Note: the storm detection logic equals the implementation in note_interrupt() which uses timestamps and counters stored in struct irq_desc. Since this structure is private to the generic interrupt core the TPM TIS core uses its own timestamps and counters. Furthermore the TPM interrupt handler always returns IRQ_HANDLED to prevent the generic interrupt core from processing the interrupt storm. Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Reported-by: kernel test robot <yujie.liu@intel.com> Closes: https://lore.kernel.org/oe-lkp/202305041325.ae8b0c43-yujie.liu@intel.com/ Suggested-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> --- drivers/char/tpm/tpm_tis_core.c | 103 +++++++++++++++++++++++++++----- drivers/char/tpm/tpm_tis_core.h | 4 ++ 2 files changed, 92 insertions(+), 15 deletions(-) Resending the patch due to several bounce messages for the first attempt. Changes to v3 (all requested by Jarko): - remove all inline comments - rename tpm_tis_reenable_polling() to tpm_tis_revert_interrupts() - rename tpm_tis_check_for_interrupt_storm() to tpm_tis_update_unhandled_irqs() - rename label "unhandled" to "err" - add Fixes: tag Changes to v2: - use define for max number of unhandles irqs(requested by Jarko) - rename intmask to int_mask (requested by Jarko) - rephrased short summary (requested by Jarko) - rename disable_interrupts to tpm_tis_disable_interrupts (requested by Jarko) - print info message concerning adding an entry to tpm_tis_dmi_table (suggested by Jerry) - amended commit message - handle failure of locality request by returning IRQ_NONE - dont take and release locality in __tpm_tis_disable_interrupts but in its caller base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1