diff mbox series

[3] tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs

Message ID 20230619092219.2600-1-LinoSanfilippo@gmx.de (mailing list archive)
State New, archived
Headers show
Series [3] tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs | expand

Commit Message

Lino Sanfilippo June 19, 2023, 9:22 a.m. UTC
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

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


base-commit: 45a3e24f65e90a047bef86f927ebdc4c710edaa1

Comments

Jarkko Sakkinen July 11, 2023, 12:50 a.m. UTC | #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 <l.sanfilippo@kunbus.com>
>
> 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 <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 | 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
Lino Sanfilippo July 13, 2023, 3:02 p.m. UTC | #2
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 <l.sanfilippo@kunbus.com>
>>
>> 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 <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 | 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
diff mbox series

Patch

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;