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); }
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.
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
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
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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Hans de Goede -
Jarkko Sakkinen