From: Lino Sanfilippo <[email protected]>
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.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-lkp/[email protected]/
Suggested-by: Lukas Wunner <[email protected]>
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 117 ++++++++++++++++++++++++++++----
drivers/char/tpm/tpm_tis_core.h | 4 ++
2 files changed, 106 insertions(+), 15 deletions(-)
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..d42537b985c5 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,71 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
return status == TPM_STS_COMMAND_READY;
}
+static irqreturn_t tpm_tis_reenable_polling(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);
+
+ /*
+ * devm_free_irq() must not be called from within the interrupt handler,
+ * since this function waits for running handlers to finish and thus it
+ * would deadlock. Instead trigger a worker that takes care of the
+ * unregistration.
+ */
+ schedule_work(&priv->free_irq_work);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t tpm_tis_check_for_interrupt_storm(struct tpm_chip *chip)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+ irqreturn_t irqret = IRQ_HANDLED;
+
+ /*
+ * 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 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_reenable_polling(chip);
+
+ /*
+ * Prevent the genirq code from starting its own interrupt storm
+ * handling by always reporting that the interrupt was handled.
+ */
+ return irqret;
+}
+
static irqreturn_t tis_int_handler(int dummy, void *dev_id)
{
struct tpm_chip *chip = dev_id;
@@ -761,10 +833,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 +852,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 unhandled;
tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
return IRQ_HANDLED;
+
+unhandled:
+ return tpm_tis_check_for_interrupt_storm(chip);
}
static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
@@ -804,6 +879,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 +900,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 +1003,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 +1107,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 +1266,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: 45a3e24f65e90a047bef86f927ebdc4c710edaa1
--
2.40.1
BTW, you should do next time:
git format-patch -v4 to get "[PATCH v4]", which defacto way to mark up
patch set versions.
On Mon Jun 19, 2023 at 12:22 PM EEST, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> 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.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]/
> Suggested-by: Lukas Wunner <[email protected]>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 117 ++++++++++++++++++++++++++++----
> drivers/char/tpm/tpm_tis_core.h | 4 ++
> 2 files changed, 106 insertions(+), 15 deletions(-)
>
> 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..d42537b985c5 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;
> }
These look pretty good.
>
> /*
> @@ -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,71 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> return status == TPM_STS_COMMAND_READY;
> }
>
> +static irqreturn_t tpm_tis_reenable_polling(struct tpm_chip *chip)
I'd rename this to tpm_tis_revert_interrupts(), as it reverts enabling
the interrupts. Polling was never enabled in a fully initialized driver
so the function name is implying something that never happened.
> +{
> + 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);
> +
> + /*
> + * devm_free_irq() must not be called from within the interrupt handler,
> + * since this function waits for running handlers to finish and thus it
> + * would deadlock. Instead trigger a worker that takes care of the
> + * unregistration.
> + */
Way too complex description. This should do:
/* Defer devm_free_irq() outside the interrupt context: */
> + schedule_work(&priv->free_irq_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t tpm_tis_check_for_interrupt_storm(struct tpm_chip *chip)
What does checking interrupt storm mean, anyway?
> +{
> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> + irqreturn_t irqret = IRQ_HANDLED;
> +
> + /*
> + * The worker to free the TPM interrupt (free_irq_work) may already
> + * be scheduled, so make sure it is not scheduled again.
> + */
I don't understand the text in the comment. It is not even a proper
sentence ("to work to free").
> + 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_reenable_polling(chip);
> +
> + /*
> + * Prevent the genirq code from starting its own interrupt storm
> + * handling by always reporting that the interrupt was handled.
> + */
Ditto, textual content is confusing.
You can either make them more informative, or add a short description
before the function.
> + return irqret;
> +}
> +
> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> {
> struct tpm_chip *chip = dev_id;
> @@ -761,10 +833,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;
s/unhandled/err/g
>
> 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 +852,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 unhandled;
>
> tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
> return IRQ_HANDLED;
> +
> +unhandled:
> + return tpm_tis_check_for_interrupt_storm(chip);
> }
>
> static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> @@ -804,6 +879,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 +900,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 +1003,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 +1107,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 +1266,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: 45a3e24f65e90a047bef86f927ebdc4c710edaa1
> --
> 2.40.1
BR, Jarkko
Hi,
On 11.07.23 02:50, Jarkko Sakkinen wrote:
>
> BTW, you should do next time:
>
> git format-patch -v4 to get "[PATCH v4]", which defacto way to mark up
> patch set versions.
>
Right, will do so the next time.
> On Mon Jun 19, 2023 at 12:22 PM EEST, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <[email protected]>
>>
>> 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.
>>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-lkp/[email protected]/
>> Suggested-by: Lukas Wunner <[email protected]>
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 117 ++++++++++++++++++++++++++++----
>> drivers/char/tpm/tpm_tis_core.h | 4 ++
>> 2 files changed, 106 insertions(+), 15 deletions(-)
>>
>> 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..d42537b985c5 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;
>> }
>
> These look pretty good.
>
>>
>> /*
>> @@ -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,71 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
>> return status == TPM_STS_COMMAND_READY;
>> }
>>
>> +static irqreturn_t tpm_tis_reenable_polling(struct tpm_chip *chip)
>
>
> I'd rename this to tpm_tis_revert_interrupts(), as it reverts enabling
> the interrupts. Polling was never enabled in a fully initialized driver
> so the function name is implying something that never happened.
Ok.
>
>> +{
>> + 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);
>> +
>> + /*
>> + * devm_free_irq() must not be called from within the interrupt handler,
>> + * since this function waits for running handlers to finish and thus it
>> + * would deadlock. Instead trigger a worker that takes care of the
>> + * unregistration.
>> + */
>
> Way too complex description. This should do:
>
> /* Defer devm_free_irq() outside the interrupt context: */
Hmm ok. I thought a little description about why devm_free_irq() has to be deferred
would be helpful. But maybe it is obvious and one sentence is enough.
>
>> + schedule_work(&priv->free_irq_work);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t tpm_tis_check_for_interrupt_storm(struct tpm_chip *chip)
>
> What does checking interrupt storm mean, anyway?
>
This function is supposed to check if an interrupt storm has occured.
So tpm_tis_check_for_interrupt_storm seems to be proper name.
>> +{
>> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> + irqreturn_t irqret = IRQ_HANDLED;
>> +
>> + /*
>> + * The worker to free the TPM interrupt (free_irq_work) may already
>> + * be scheduled, so make sure it is not scheduled again.
>> + */
>
> I don't understand the text in the comment. It is not even a proper
> sentence ("to work to free").
Right, there are some words missing. I think this should be something
like "...the worker that is supposed to free..."
>
>> + 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_reenable_polling(chip);
>> +
>> + /*
>> + * Prevent the genirq code from starting its own interrupt storm
>> + * handling by always reporting that the interrupt was handled.
>> + */
>
> Ditto, textual content is confusing.
>
> You can either make them more informative, or add a short description
> before the function.
>
Ok, what about "In case of success return IRQ_HANDLED to prevent the genirq
code from starting its own interrupt storm handling".
>> + return irqret;
>> +}
>> +
>> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>> {
>> struct tpm_chip *chip = dev_id;
>> @@ -761,10 +833,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;
>
> s/unhandled/err/g
ok
>
>>
>> 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 +852,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 unhandled;
>>
>> tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
>> return IRQ_HANDLED;
>> +
>> +unhandled:
>> + return tpm_tis_check_for_interrupt_storm(chip);
>> }
>>
>> static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
>> @@ -804,6 +879,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 +900,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 +1003,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 +1107,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 +1266,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;
>>
Regards,
Lino