[PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler

[PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler

[Lino Sanfilippo]

Interrupt handling at least includes reading and writing the interrupt
status register within the interrupt routine. For accesses over SPI a mutex
is used in the concerning functions. Since this requires a sleepable
context request a threaded interrupt handler for this case.

Cc: stable@vger.kernel.org
Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
This patch has been tested on a SLB9670vq2.0.
The first version of this patch was part of patch series (see
https://marc.info/?l=linux-integrity&m=162016888020044&w=2)

v2:
- request threaded irq handler only in case of SPI as requested by Jarkko
  Sakkinen
- add "Fixes" tag
- add stable
- correct short summary

 drivers/char/tpm/tpm_tis.c           |  2 +-
 drivers/char/tpm/tpm_tis_core.c      | 32 ++++++++++++++++++++--------
 drivers/char/tpm/tpm_tis_core.h      |  2 +-
 drivers/char/tpm/tpm_tis_spi_main.c  |  3 ++-
 drivers/char/tpm/tpm_tis_synquacer.c |  2 +-
 5 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 4ed6e660273a..d27009b989fe 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -236,7 +236,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
 		phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
 
 	return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
-				 ACPI_HANDLE(dev));
+				 ACPI_HANDLE(dev), false);
 }
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 55b9d3965ae1..21abc2168d07 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -728,19 +728,31 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
  * everything and leave in polling mode. Returns 0 on success.
  */
 static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
-				    int flags, int irq)
+				    int flags, int irq, bool threaded_irq)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	u8 original_int_vec;
 	int rc;
 	u32 int_status;
 
-	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
-			     dev_name(&chip->dev), chip) != 0) {
+
+	if (threaded_irq) {
+		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
+					       tis_int_handler,
+					       IRQF_ONESHOT | flags,
+					       dev_name(&chip->dev),
+					       chip);
+	} else {
+		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
+				      flags, dev_name(&chip->dev), chip);
+	}
+
+	if (rc) {
 		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
 			 irq);
 		return -1;
 	}
+
 	priv->irq = irq;
 
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
@@ -795,7 +807,8 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
  * do not have ACPI/etc. We typically expect the interrupt to be declared if
  * present.
  */
-static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
+static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask,
+			      bool threaded_irq)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	u8 original_int_vec;
@@ -810,10 +823,11 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
 		if (IS_ENABLED(CONFIG_X86))
 			for (i = 3; i <= 15; i++)
 				if (!tpm_tis_probe_irq_single(chip, intmask, 0,
-							      i))
+							      i, threaded_irq))
 					return;
 	} else if (!tpm_tis_probe_irq_single(chip, intmask, 0,
-					     original_int_vec))
+					     original_int_vec,
+					     threaded_irq))
 		return;
 }
 
@@ -909,7 +923,7 @@ static const struct tpm_class_ops tpm_tis = {
 
 int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		      const struct tpm_tis_phy_ops *phy_ops,
-		      acpi_handle acpi_dev_handle)
+		      acpi_handle acpi_dev_handle, bool threaded_irq)
 {
 	u32 vendor;
 	u32 intfcaps;
@@ -1049,7 +1063,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 		if (irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
-						 irq);
+						 irq, threaded_irq);
 			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
@@ -1057,7 +1071,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 				disable_interrupts(chip);
 			}
 		} else {
-			tpm_tis_probe_irq(chip, intmask);
+			tpm_tis_probe_irq(chip, intmask, threaded_irq);
 		}
 	}
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a59f67..32d36e538208 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -161,7 +161,7 @@ static inline bool is_bsw(void)
 void tpm_tis_remove(struct tpm_chip *chip);
 int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		      const struct tpm_tis_phy_ops *phy_ops,
-		      acpi_handle acpi_dev_handle);
+		      acpi_handle acpi_dev_handle, bool use_threaded_irq);
 
 #ifdef CONFIG_PM_SLEEP
 int tpm_tis_resume(struct device *dev);
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index 3856f6ebcb34..2eb57c1cf9ba 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -199,7 +199,8 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
 
 	phy->spi_device = spi;
 
-	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL);
+	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL,
+				 true);
 }
 
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
diff --git a/drivers/char/tpm/tpm_tis_synquacer.c b/drivers/char/tpm/tpm_tis_synquacer.c
index e47bdd272704..6ee59a1fdf08 100644
--- a/drivers/char/tpm/tpm_tis_synquacer.c
+++ b/drivers/char/tpm/tpm_tis_synquacer.c
@@ -127,7 +127,7 @@ static int tpm_tis_synquacer_init(struct device *dev,
 		return PTR_ERR(phy->iobase);
 
 	return tpm_tis_core_init(dev, &phy->priv, tpm_info->irq, &tpm_tcg_bw,
-				 ACPI_HANDLE(dev));
+				 ACPI_HANDLE(dev), false);
 }
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_synquacer_pm, tpm_pm_suspend, tpm_tis_resume);

base-commit: 913ec3c22ef425d63dd0bc81fb008ce7f9bcb07b
-- 
2.31.1

Re: [PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler

[Jarkko Sakkinen]

On Sun, Jun 20, 2021 at 04:34:44AM +0200, Lino Sanfilippo wrote:
> Interrupt handling at least includes reading and writing the interrupt
> status register within the interrupt routine. For accesses over SPI a mutex
> is used in the concerning functions. Since this requires a sleepable
> context request a threaded interrupt handler for this case.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

I'll test this after rc1 PR (I have one NUC which uses tpm_tis_spi).

/Jarkko

> ---
> This patch has been tested on a SLB9670vq2.0.
> The first version of this patch was part of patch series (see
> https://marc.info/?l=linux-integrity&m=162016888020044&w=2)
> 
> v2:
> - request threaded irq handler only in case of SPI as requested by Jarkko
>   Sakkinen
> - add "Fixes" tag
> - add stable
> - correct short summary
> 
>  drivers/char/tpm/tpm_tis.c           |  2 +-
>  drivers/char/tpm/tpm_tis_core.c      | 32 ++++++++++++++++++++--------
>  drivers/char/tpm/tpm_tis_core.h      |  2 +-
>  drivers/char/tpm/tpm_tis_spi_main.c  |  3 ++-
>  drivers/char/tpm/tpm_tis_synquacer.c |  2 +-
>  5 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 4ed6e660273a..d27009b989fe 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -236,7 +236,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
>  		phy->priv.flags |= TPM_TIS_ITPM_WORKAROUND;
>  
>  	return tpm_tis_core_init(dev, &phy->priv, irq, &tpm_tcg,
> -				 ACPI_HANDLE(dev));
> +				 ACPI_HANDLE(dev), false);
>  }
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 55b9d3965ae1..21abc2168d07 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -728,19 +728,31 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>   * everything and leave in polling mode. Returns 0 on success.
>   */
>  static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> -				    int flags, int irq)
> +				    int flags, int irq, bool threaded_irq)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	u8 original_int_vec;
>  	int rc;
>  	u32 int_status;
>  
> -	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
> -			     dev_name(&chip->dev), chip) != 0) {
> +
> +	if (threaded_irq) {
> +		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
> +					       tis_int_handler,
> +					       IRQF_ONESHOT | flags,
> +					       dev_name(&chip->dev),
> +					       chip);
> +	} else {
> +		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
> +				      flags, dev_name(&chip->dev), chip);
> +	}
> +
> +	if (rc) {
>  		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
>  			 irq);
>  		return -1;
>  	}
> +
>  	priv->irq = irq;
>  
>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> @@ -795,7 +807,8 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>   * do not have ACPI/etc. We typically expect the interrupt to be declared if
>   * present.
>   */
> -static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
> +static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask,
> +			      bool threaded_irq)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	u8 original_int_vec;
> @@ -810,10 +823,11 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
>  		if (IS_ENABLED(CONFIG_X86))
>  			for (i = 3; i <= 15; i++)
>  				if (!tpm_tis_probe_irq_single(chip, intmask, 0,
> -							      i))
> +							      i, threaded_irq))
>  					return;
>  	} else if (!tpm_tis_probe_irq_single(chip, intmask, 0,
> -					     original_int_vec))
> +					     original_int_vec,
> +					     threaded_irq))
>  		return;
>  }
>  
> @@ -909,7 +923,7 @@ static const struct tpm_class_ops tpm_tis = {
>  
>  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		      const struct tpm_tis_phy_ops *phy_ops,
> -		      acpi_handle acpi_dev_handle)
> +		      acpi_handle acpi_dev_handle, bool threaded_irq)
>  {
>  	u32 vendor;
>  	u32 intfcaps;
> @@ -1049,7 +1063,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  
>  		if (irq) {
>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
> -						 irq);
> +						 irq, threaded_irq);
>  			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>  				dev_err(&chip->dev, FW_BUG
>  					"TPM interrupt not working, polling instead\n");
> @@ -1057,7 +1071,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  				disable_interrupts(chip);
>  			}
>  		} else {
> -			tpm_tis_probe_irq(chip, intmask);
> +			tpm_tis_probe_irq(chip, intmask, threaded_irq);
>  		}
>  	}
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 9b2d32a59f67..32d36e538208 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -161,7 +161,7 @@ static inline bool is_bsw(void)
>  void tpm_tis_remove(struct tpm_chip *chip);
>  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		      const struct tpm_tis_phy_ops *phy_ops,
> -		      acpi_handle acpi_dev_handle);
> +		      acpi_handle acpi_dev_handle, bool use_threaded_irq);
>  
>  #ifdef CONFIG_PM_SLEEP
>  int tpm_tis_resume(struct device *dev);
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index 3856f6ebcb34..2eb57c1cf9ba 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -199,7 +199,8 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
>  
>  	phy->spi_device = spi;
>  
> -	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL);
> +	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL,
> +				 true);
>  }
>  
>  static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
> diff --git a/drivers/char/tpm/tpm_tis_synquacer.c b/drivers/char/tpm/tpm_tis_synquacer.c
> index e47bdd272704..6ee59a1fdf08 100644
> --- a/drivers/char/tpm/tpm_tis_synquacer.c
> +++ b/drivers/char/tpm/tpm_tis_synquacer.c
> @@ -127,7 +127,7 @@ static int tpm_tis_synquacer_init(struct device *dev,
>  		return PTR_ERR(phy->iobase);
>  
>  	return tpm_tis_core_init(dev, &phy->priv, tpm_info->irq, &tpm_tcg_bw,
> -				 ACPI_HANDLE(dev));
> +				 ACPI_HANDLE(dev), false);
>  }
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_synquacer_pm, tpm_pm_suspend, tpm_tis_resume);
> 
> base-commit: 913ec3c22ef425d63dd0bc81fb008ce7f9bcb07b
> -- 
> 2.31.1
> 

Re: [PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler

[Lino Sanfilippo]

Hi,

On 23.06.21 at 15:34, Jarkko Sakkinen wrote:
> On Sun, Jun 20, 2021 at 04:34:44AM +0200, Lino Sanfilippo wrote:
>> Interrupt handling at least includes reading and writing the interrupt
>> status register within the interrupt routine. For accesses over SPI a mutex
>> is used in the concerning functions. Since this requires a sleepable
>> context request a threaded interrupt handler for this case.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>
> I'll test this after rc1 PR (I have one NUC which uses tpm_tis_spi).
>
> /Jarkko
>

Sounds great, thank you!

Regards,
Lino

Aw: Re: [PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler

[Lino Sanfilippo]

Hi Jarko,

> Gesendet: Mittwoch, 23. Juni 2021 um 15:34 Uhr
> Von: "Jarkko Sakkinen" <jarkko@kernel.org>
> An: "Lino Sanfilippo" <LinoSanfilippo@gmx.de>
> Cc: peterhuewe@gmx.de, jgg@ziepe.ca, linus.walleij@linaro.org, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org
> Betreff: Re: [PATCH v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler
>
> On Sun, Jun 20, 2021 at 04:34:44AM +0200, Lino Sanfilippo wrote:
> > Interrupt handling at least includes reading and writing the interrupt
> > status register within the interrupt routine. For accesses over SPI a mutex
> > is used in the concerning functions. Since this requires a sleepable
> > context request a threaded interrupt handler for this case.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>
> I'll test this after rc1 PR (I have one NUC which uses tpm_tis_spi).
>
> /Jarkko
>

any news from this patch? Did you already have the opportunity to test it?

Regards,
Lino