* [PATCH] tpm/tpm_tis: Free IRQ if probing fails
@ 2020-04-12 17:04 Jarkko Sakkinen
2020-04-13 10:04 ` Hans de Goede
0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-04-12 17:04 UTC (permalink / raw)
To: linux-integrity
Cc: Jarkko Sakkinen, stable, Hans de Goede, Peter Huewe,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list
Call devm_free_irq() if we have to revert to polling in order not to
unnecessarily reserve the IRQ for the life-cycle of the driver.
Cc: stable@vger.kernel.org # 4.5.x
Reported-by: Hans de Goede <hdegoede@redhat.com>
Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/tpm_tis_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 27c6ca031e23..ae6868e7b696 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1062,9 +1062,12 @@ 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);
- if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
+ if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");
+ devm_free_irq(chip->dev.parent, priv->irq,
+ chip);
+ }
} else {
tpm_tis_probe_irq(chip, intmask);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-12 17:04 [PATCH] tpm/tpm_tis: Free IRQ if probing fails Jarkko Sakkinen @ 2020-04-13 10:04 ` Hans de Goede 2020-04-13 18:07 ` Jarkko Sakkinen 0 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2020-04-13 10:04 UTC (permalink / raw) To: Jarkko Sakkinen, linux-integrity Cc: stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list Hi Jarkko, On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: > Call devm_free_irq() if we have to revert to polling in order not to > unnecessarily reserve the IRQ for the life-cycle of the driver. > > Cc: stable@vger.kernel.org # 4.5.x > Reported-by: Hans de Goede <hdegoede@redhat.com> > Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm_tis_core.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 27c6ca031e23..ae6868e7b696 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -1062,9 +1062,12 @@ 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); > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > dev_err(&chip->dev, FW_BUG > "TPM interrupt not working, polling instead\n"); > + devm_free_irq(chip->dev.parent, priv->irq, > + chip); > + } My initial plan was actually to do something similar, but if the probe code is actually ever fixed to work as intended again then this will lead to a double free as then the IRQ-test path of tpm_tis_send() will have called disable_interrupts() which already calls devm_free_irq(). You could check for chip->irq != 0 here to avoid that. But it all is rather messy, which is why I went with the "#if 0" approach in my patch. Also we will currently ALWAYS hit the "TPM interrupt not working, polling instead" error because the TPM_CHIP_FLAG_IRQ never gets set. So if we are going to do an interim fix (and we should) we should really also silence that error. Regards, Hans p.s. I'm currently in contact with Lenovo trying to figure out what is going on here with the always firing IRQ on the X1 8th gen, I guess the fix for that might also help with the T490 issue. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-13 10:04 ` Hans de Goede @ 2020-04-13 18:07 ` Jarkko Sakkinen 2020-04-13 18:11 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Jarkko Sakkinen @ 2020-04-13 18:07 UTC (permalink / raw) To: Hans de Goede Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: > Hi Jarkko, > > On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: > > Call devm_free_irq() if we have to revert to polling in order not to > > unnecessarily reserve the IRQ for the life-cycle of the driver. > > > > Cc: stable@vger.kernel.org # 4.5.x > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > drivers/char/tpm/tpm_tis_core.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > index 27c6ca031e23..ae6868e7b696 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -1062,9 +1062,12 @@ 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); > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > dev_err(&chip->dev, FW_BUG > > "TPM interrupt not working, polling instead\n"); > > + devm_free_irq(chip->dev.parent, priv->irq, > > + chip); > > + } > > My initial plan was actually to do something similar, but if the probe code > is actually ever fixed to work as intended again then this will lead to a > double free as then the IRQ-test path of tpm_tis_send() will have called > disable_interrupts() which already calls devm_free_irq(). > > You could check for chip->irq != 0 here to avoid that. > > But it all is rather messy, which is why I went with the "#if 0" approach > in my patch. I think it is right way to fix it. It is a bug independent of the issue we are experiencing. However, what you are suggesting should be done in addition. Do you have a patch in place or do you want me to refine mine? /Jarkko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-13 18:07 ` Jarkko Sakkinen @ 2020-04-13 18:11 ` Hans de Goede 2020-04-14 7:13 ` Jarkko Sakkinen 0 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2020-04-13 18:11 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list Hi, On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: > On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: >> Hi Jarkko, >> >> On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: >>> Call devm_free_irq() if we have to revert to polling in order not to >>> unnecessarily reserve the IRQ for the life-cycle of the driver. >>> >>> Cc: stable@vger.kernel.org # 4.5.x >>> Reported-by: Hans de Goede <hdegoede@redhat.com> >>> Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") >>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>> --- >>> drivers/char/tpm/tpm_tis_core.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>> index 27c6ca031e23..ae6868e7b696 100644 >>> --- a/drivers/char/tpm/tpm_tis_core.c >>> +++ b/drivers/char/tpm/tpm_tis_core.c >>> @@ -1062,9 +1062,12 @@ 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); >>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >>> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >>> dev_err(&chip->dev, FW_BUG >>> "TPM interrupt not working, polling instead\n"); >>> + devm_free_irq(chip->dev.parent, priv->irq, >>> + chip); >>> + } >> >> My initial plan was actually to do something similar, but if the probe code >> is actually ever fixed to work as intended again then this will lead to a >> double free as then the IRQ-test path of tpm_tis_send() will have called >> disable_interrupts() which already calls devm_free_irq(). >> >> You could check for chip->irq != 0 here to avoid that. >> >> But it all is rather messy, which is why I went with the "#if 0" approach >> in my patch. > > I think it is right way to fix it. It is a bug independent of the issue > we are experiencing. > > However, what you are suggesting should be done in addition. Do you have > a patch in place or do you want me to refine mine? I do not have a patch ready for this, if you can refine yours that would be great. Regards, Hans ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-13 18:11 ` Hans de Goede @ 2020-04-14 7:13 ` Jarkko Sakkinen 2020-04-14 8:26 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Jarkko Sakkinen @ 2020-04-14 7:13 UTC (permalink / raw) To: Hans de Goede Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list On Mon, Apr 13, 2020 at 08:11:15PM +0200, Hans de Goede wrote: > Hi, > > On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: > > On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: > > > Hi Jarkko, > > > > > > On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: > > > > Call devm_free_irq() if we have to revert to polling in order not to > > > > unnecessarily reserve the IRQ for the life-cycle of the driver. > > > > > > > > Cc: stable@vger.kernel.org # 4.5.x > > > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > > > Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > --- > > > > drivers/char/tpm/tpm_tis_core.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > > > index 27c6ca031e23..ae6868e7b696 100644 > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > @@ -1062,9 +1062,12 @@ 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); > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > dev_err(&chip->dev, FW_BUG > > > > "TPM interrupt not working, polling instead\n"); > > > > + devm_free_irq(chip->dev.parent, priv->irq, > > > > + chip); > > > > + } > > > > > > My initial plan was actually to do something similar, but if the probe code > > > is actually ever fixed to work as intended again then this will lead to a > > > double free as then the IRQ-test path of tpm_tis_send() will have called > > > disable_interrupts() which already calls devm_free_irq(). > > > > > > You could check for chip->irq != 0 here to avoid that. > > > > > > But it all is rather messy, which is why I went with the "#if 0" approach > > > in my patch. > > > > I think it is right way to fix it. It is a bug independent of the issue > > we are experiencing. > > > > However, what you are suggesting should be done in addition. Do you have > > a patch in place or do you want me to refine mine? > > I do not have a patch ready for this, if you can refine yours that would > be great. Thanks! Just wanted to confirm. /Jarkko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-14 7:13 ` Jarkko Sakkinen @ 2020-04-14 8:26 ` Hans de Goede 2020-04-14 16:04 ` Jarkko Sakkinen 0 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2020-04-14 8:26 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list Hi, On 4/14/20 9:13 AM, Jarkko Sakkinen wrote: > On Mon, Apr 13, 2020 at 08:11:15PM +0200, Hans de Goede wrote: >> Hi, >> >> On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: >>> On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: >>>> Hi Jarkko, >>>> >>>> On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: >>>>> Call devm_free_irq() if we have to revert to polling in order not to >>>>> unnecessarily reserve the IRQ for the life-cycle of the driver. >>>>> >>>>> Cc: stable@vger.kernel.org # 4.5.x >>>>> Reported-by: Hans de Goede <hdegoede@redhat.com> >>>>> Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") >>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>>>> --- >>>>> drivers/char/tpm/tpm_tis_core.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>>>> index 27c6ca031e23..ae6868e7b696 100644 >>>>> --- a/drivers/char/tpm/tpm_tis_core.c >>>>> +++ b/drivers/char/tpm/tpm_tis_core.c >>>>> @@ -1062,9 +1062,12 @@ 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); >>>>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >>>>> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >>>>> dev_err(&chip->dev, FW_BUG >>>>> "TPM interrupt not working, polling instead\n"); >>>>> + devm_free_irq(chip->dev.parent, priv->irq, >>>>> + chip); >>>>> + } >>>> >>>> My initial plan was actually to do something similar, but if the probe code >>>> is actually ever fixed to work as intended again then this will lead to a >>>> double free as then the IRQ-test path of tpm_tis_send() will have called >>>> disable_interrupts() which already calls devm_free_irq(). >>>> >>>> You could check for chip->irq != 0 here to avoid that. Erm in case you haven't figured it out yet this should be priv->irq != 0, sorry. >>>> >>>> But it all is rather messy, which is why I went with the "#if 0" approach >>>> in my patch. >>> >>> I think it is right way to fix it. It is a bug independent of the issue >>> we are experiencing. >>> >>> However, what you are suggesting should be done in addition. Do you have >>> a patch in place or do you want me to refine mine? >> >> I do not have a patch ready for this, if you can refine yours that would >> be great. > > Thanks! Just wanted to confirm. And thank you for working on a (temporary?) fix for this. Regards, Hans ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-14 8:26 ` Hans de Goede @ 2020-04-14 16:04 ` Jarkko Sakkinen 2020-04-14 16:45 ` Jarkko Sakkinen 0 siblings, 1 reply; 10+ messages in thread From: Jarkko Sakkinen @ 2020-04-14 16:04 UTC (permalink / raw) To: Hans de Goede Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list On Tue, Apr 14, 2020 at 10:26:32AM +0200, Hans de Goede wrote: > Hi, > > On 4/14/20 9:13 AM, Jarkko Sakkinen wrote: > > On Mon, Apr 13, 2020 at 08:11:15PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: > > > > On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: > > > > > Hi Jarkko, > > > > > > > > > > On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: > > > > > > Call devm_free_irq() if we have to revert to polling in order not to > > > > > > unnecessarily reserve the IRQ for the life-cycle of the driver. > > > > > > > > > > > > Cc: stable@vger.kernel.org # 4.5.x > > > > > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > > > > > Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > --- > > > > > > drivers/char/tpm/tpm_tis_core.c | 5 ++++- > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > > > > > index 27c6ca031e23..ae6868e7b696 100644 > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > > > @@ -1062,9 +1062,12 @@ 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); > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > dev_err(&chip->dev, FW_BUG > > > > > > "TPM interrupt not working, polling instead\n"); > > > > > > + devm_free_irq(chip->dev.parent, priv->irq, > > > > > > + chip); > > > > > > + } > > > > > > > > > > My initial plan was actually to do something similar, but if the probe code > > > > > is actually ever fixed to work as intended again then this will lead to a > > > > > double free as then the IRQ-test path of tpm_tis_send() will have called > > > > > disable_interrupts() which already calls devm_free_irq(). > > > > > > > > > > You could check for chip->irq != 0 here to avoid that. > > Erm in case you haven't figured it out yet this should be priv->irq != 0, sorry. Yup. > > > > > > > > > > But it all is rather messy, which is why I went with the "#if 0" approach > > > > > in my patch. > > > > > > > > I think it is right way to fix it. It is a bug independent of the issue > > > > we are experiencing. > > > > > > > > However, what you are suggesting should be done in addition. Do you have > > > > a patch in place or do you want me to refine mine? > > > > > > I do not have a patch ready for this, if you can refine yours that would > > > be great. > > > > Thanks! Just wanted to confirm. > > And thank you for working on a (temporary?) fix for this. As far as I see it, it is orthogonal fix that needs to be backported to stable kernels. This bug predates the issue we're seeing now. /Jarkko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-14 16:04 ` Jarkko Sakkinen @ 2020-04-14 16:45 ` Jarkko Sakkinen 2020-04-14 17:15 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Jarkko Sakkinen @ 2020-04-14 16:45 UTC (permalink / raw) To: Hans de Goede Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list On Tue, Apr 14, 2020 at 07:04:07PM +0300, Jarkko Sakkinen wrote: > On Tue, Apr 14, 2020 at 10:26:32AM +0200, Hans de Goede wrote: > > Hi, > > > > On 4/14/20 9:13 AM, Jarkko Sakkinen wrote: > > > On Mon, Apr 13, 2020 at 08:11:15PM +0200, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: > > > > > On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: > > > > > > Hi Jarkko, > > > > > > > > > > > > On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: > > > > > > > Call devm_free_irq() if we have to revert to polling in order not to > > > > > > > unnecessarily reserve the IRQ for the life-cycle of the driver. > > > > > > > > > > > > > > Cc: stable@vger.kernel.org # 4.5.x > > > > > > > Reported-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > > --- > > > > > > > drivers/char/tpm/tpm_tis_core.c | 5 ++++- > > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > > > > > > index 27c6ca031e23..ae6868e7b696 100644 > > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > > > > @@ -1062,9 +1062,12 @@ 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); > > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > > dev_err(&chip->dev, FW_BUG > > > > > > > "TPM interrupt not working, polling instead\n"); > > > > > > > + devm_free_irq(chip->dev.parent, priv->irq, > > > > > > > + chip); > > > > > > > + } > > > > > > > > > > > > My initial plan was actually to do something similar, but if the probe code > > > > > > is actually ever fixed to work as intended again then this will lead to a > > > > > > double free as then the IRQ-test path of tpm_tis_send() will have called > > > > > > disable_interrupts() which already calls devm_free_irq(). > > > > > > > > > > > > You could check for chip->irq != 0 here to avoid that. > > > > Erm in case you haven't figured it out yet this should be priv->irq != 0, sorry. > > Yup. > > > > > > > > > > > > > But it all is rather messy, which is why I went with the "#if 0" approach > > > > > > in my patch. > > > > > > > > > > I think it is right way to fix it. It is a bug independent of the issue > > > > > we are experiencing. > > > > > > > > > > However, what you are suggesting should be done in addition. Do you have > > > > > a patch in place or do you want me to refine mine? > > > > > > > > I do not have a patch ready for this, if you can refine yours that would > > > > be great. > > > > > > Thanks! Just wanted to confirm. > > > > And thank you for working on a (temporary?) fix for this. > > As far as I see it, it is orthogonal fix that needs to be backported > to stable kernels. This bug predates the issue we're seeing now. Hey, I came to other thoughts on "how". Would probably make sense to always call disable_interrupts() aka no sense to add two separate code paths. What do you think? /Jarkko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-14 16:45 ` Jarkko Sakkinen @ 2020-04-14 17:15 ` Hans de Goede 2020-04-16 13:13 ` Jarkko Sakkinen 0 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2020-04-14 17:15 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list Hi, On 4/14/20 6:45 PM, Jarkko Sakkinen wrote: > On Tue, Apr 14, 2020 at 07:04:07PM +0300, Jarkko Sakkinen wrote: >> On Tue, Apr 14, 2020 at 10:26:32AM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 4/14/20 9:13 AM, Jarkko Sakkinen wrote: >>>> On Mon, Apr 13, 2020 at 08:11:15PM +0200, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 4/13/20 8:07 PM, Jarkko Sakkinen wrote: >>>>>> On Mon, Apr 13, 2020 at 12:04:25PM +0200, Hans de Goede wrote: >>>>>>> Hi Jarkko, >>>>>>> >>>>>>> On 4/12/20 7:04 PM, Jarkko Sakkinen wrote: >>>>>>>> Call devm_free_irq() if we have to revert to polling in order not to >>>>>>>> unnecessarily reserve the IRQ for the life-cycle of the driver. >>>>>>>> >>>>>>>> Cc: stable@vger.kernel.org # 4.5.x >>>>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup") >>>>>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>>>>>>> --- >>>>>>>> drivers/char/tpm/tpm_tis_core.c | 5 ++++- >>>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>>>>>>> index 27c6ca031e23..ae6868e7b696 100644 >>>>>>>> --- a/drivers/char/tpm/tpm_tis_core.c >>>>>>>> +++ b/drivers/char/tpm/tpm_tis_core.c >>>>>>>> @@ -1062,9 +1062,12 @@ 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); >>>>>>>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >>>>>>>> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >>>>>>>> dev_err(&chip->dev, FW_BUG >>>>>>>> "TPM interrupt not working, polling instead\n"); >>>>>>>> + devm_free_irq(chip->dev.parent, priv->irq, >>>>>>>> + chip); >>>>>>>> + } >>>>>>> >>>>>>> My initial plan was actually to do something similar, but if the probe code >>>>>>> is actually ever fixed to work as intended again then this will lead to a >>>>>>> double free as then the IRQ-test path of tpm_tis_send() will have called >>>>>>> disable_interrupts() which already calls devm_free_irq(). >>>>>>> >>>>>>> You could check for chip->irq != 0 here to avoid that. >>> >>> Erm in case you haven't figured it out yet this should be priv->irq != 0, sorry. >> >> Yup. >> >>>>>>> >>>>>>> But it all is rather messy, which is why I went with the "#if 0" approach >>>>>>> in my patch. >>>>>> >>>>>> I think it is right way to fix it. It is a bug independent of the issue >>>>>> we are experiencing. >>>>>> >>>>>> However, what you are suggesting should be done in addition. Do you have >>>>>> a patch in place or do you want me to refine mine? >>>>> >>>>> I do not have a patch ready for this, if you can refine yours that would >>>>> be great. >>>> >>>> Thanks! Just wanted to confirm. >>> >>> And thank you for working on a (temporary?) fix for this. >> >> As far as I see it, it is orthogonal fix that needs to be backported >> to stable kernels. This bug predates the issue we're seeing now. > > Hey, I came to other thoughts on "how". Would probably make sense > to always call disable_interrupts() aka no sense to add two separate > code paths. What do you think? Sounds good, I guess it would be best to combine that with a: if (priv->irq == 0) return; At the top of disable_interrupts() and then unconditionally call disable_interrupts() where your v1 of this patch calls devm_free_irq(). That would be a reasonable clean solution I think. Regards, Hans ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails 2020-04-14 17:15 ` Hans de Goede @ 2020-04-16 13:13 ` Jarkko Sakkinen 0 siblings, 0 replies; 10+ messages in thread From: Jarkko Sakkinen @ 2020-04-16 13:13 UTC (permalink / raw) To: Hans de Goede Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, open list On Tue, Apr 14, 2020 at 07:15:08PM +0200, Hans de Goede wrote: > Sounds good, I guess it would be best to combine that with a: > > if (priv->irq == 0) > return; > > At the top of disable_interrupts() and then unconditionally > call disable_interrupts() where your v1 of this patch > calls devm_free_irq(). That would be a reasonable clean > solution I think. Great, this was my plan (just wanted to double check). /Jarkko ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-16 13:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-12 17:04 [PATCH] tpm/tpm_tis: Free IRQ if probing fails Jarkko Sakkinen 2020-04-13 10:04 ` Hans de Goede 2020-04-13 18:07 ` Jarkko Sakkinen 2020-04-13 18:11 ` Hans de Goede 2020-04-14 7:13 ` Jarkko Sakkinen 2020-04-14 8:26 ` Hans de Goede 2020-04-14 16:04 ` Jarkko Sakkinen 2020-04-14 16:45 ` Jarkko Sakkinen 2020-04-14 17:15 ` Hans de Goede 2020-04-16 13:13 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.