Message ID | 20210620023444.14684-1-LinoSanfilippo@gmx.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] tpm, tpm_tis_spi: Allow to sleep in the interrupt handler | expand |
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 >
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
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
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);
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(-) base-commit: 913ec3c22ef425d63dd0bc81fb008ce7f9bcb07b