[PATCH] tpm/tpm_tis: Free IRQ if probing fails

[PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Hans de Goede]

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.

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Hans de Goede]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Hans de Goede]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Hans de Goede]

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

Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

[Jarkko Sakkinen]

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