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: [email protected]
Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
Signed-off-by: Lino Sanfilippo <[email protected]>
---
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
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: [email protected]
> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> Signed-off-by: Lino Sanfilippo <[email protected]>
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
>
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: [email protected]
>> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>
> I'll test this after rc1 PR (I have one NUC which uses tpm_tis_spi).
>
> /Jarkko
>
Sounds great, thank you!
Regards,
Lino
Hi Jarko,
> Gesendet: Mittwoch, 23. Juni 2021 um 15:34 Uhr
> Von: "Jarkko Sakkinen" <[email protected]>
> An: "Lino Sanfilippo" <[email protected]>
> Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> 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: [email protected]
> > Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
> > Signed-off-by: Lino Sanfilippo <[email protected]>
>
> 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