question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jerry Snitselaar]

Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts").
Doesn't tpm_tis_send set this flag, and setting it here in tpm_tis_core_init short circuits what
tpm_tis_send was doing before? There is a bug report of an interrupt storm from a tpm on a t490s laptop
with the Fedora 31 kernel (5.3), and I'm wondering if this change could cause that. Before they got
the warning about interrupts not working, and using polling instead.

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Stefan Berger]

On 11/11/19 10:36 PM, Jerry Snitselaar wrote:
> Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ 
> before probing for interrupts").
> Doesn't tpm_tis_send set this flag, and setting it here in 
> tpm_tis_core_init short circuits what
> tpm_tis_send was doing before? There is a bug report of an interrupt 
> storm from a tpm on a t490s laptop
> with the Fedora 31 kernel (5.3), and I'm wondering if this change 
> could cause that. Before they got
> the warning about interrupts not working, and using polling instead.
>
I set this flag for the TIS because it wasn't set anywhere else. 
tpm_tis_send() wouldn't set the flag but go via the path:

if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)

         return tpm_tis_send_main(chip, buf, len);

the only other line for the TIS to set the IRQ flag was in the same 
function further below, though that wouldn't be reached due to the above:

[...]

priv->irq = irq;

chip->flags |= TPM_CHIP_FLAG_IRQ;


    Stefan

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jerry Snitselaar]

On Tue Nov 12 19, Stefan Berger wrote:
>On 11/11/19 10:36 PM, Jerry Snitselaar wrote:
>>Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ 
>>before probing for interrupts").
>>Doesn't tpm_tis_send set this flag, and setting it here in 
>>tpm_tis_core_init short circuits what
>>tpm_tis_send was doing before? There is a bug report of an interrupt 
>>storm from a tpm on a t490s laptop
>>with the Fedora 31 kernel (5.3), and I'm wondering if this change 
>>could cause that. Before they got
>>the warning about interrupts not working, and using polling instead.
>>
>I set this flag for the TIS because it wasn't set anywhere else. 
>tpm_tis_send() wouldn't set the flag but go via the path:
>
>if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>
>        return tpm_tis_send_main(chip, buf, len);
>
>the only other line for the TIS to set the IRQ flag was in the same 
>function further below, though that wouldn't be reached due to the 
>above:
>
>[...]
>
>priv->irq = irq;
>
>chip->flags |= TPM_CHIP_FLAG_IRQ;
>
>
>   Stefan
>
>

Ugh, you're right I was reading that as ! around both the flag and priv->irq_tested.

Should the flag be cleared if tpm_tis_probe_irq_single fails prior to calling
tpm_tis_gen_interrupt?

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Stefan Berger]

On 11/12/19 9:24 AM, Jerry Snitselaar wrote:
> On Tue Nov 12 19, Stefan Berger wrote:
>> On 11/11/19 10:36 PM, Jerry Snitselaar wrote:
>>> Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ 
>>> before probing for interrupts").
>>> Doesn't tpm_tis_send set this flag, and setting it here in 
>>> tpm_tis_core_init short circuits what
>>> tpm_tis_send was doing before? There is a bug report of an interrupt 
>>> storm from a tpm on a t490s laptop
>>> with the Fedora 31 kernel (5.3), and I'm wondering if this change 
>>> could cause that. Before they got
>>> the warning about interrupts not working, and using polling instead.
>>>
>> I set this flag for the TIS because it wasn't set anywhere else. 
>> tpm_tis_send() wouldn't set the flag but go via the path:
>>
>> if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>>
>>         return tpm_tis_send_main(chip, buf, len);
>>
>> the only other line for the TIS to set the IRQ flag was in the same 
>> function further below, though that wouldn't be reached due to the 
>> above:
>>
>> [...]
>>
>> priv->irq = irq;
>>
>> chip->flags |= TPM_CHIP_FLAG_IRQ;
>>
>>
>>    Stefan
>>
>>
>
> Ugh, you're right I was reading that as ! around both the flag and 
> priv->irq_tested.
>
> Should the flag be cleared if tpm_tis_probe_irq_single fails prior to 
> calling
> tpm_tis_gen_interrupt?
>
The disable_interrupts() should be called to reset the flag if, while 
probing, the interrupt handler wasn't called. Maybe that t490s returns 
either via this path

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis_core.c#L631

or this one here

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis_core.c#L634

thinking the (shared) interrupt is not for it?! But this would mean 
TPM_INT_STATUS is broken...

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jarkko Sakkinen]

On Mon, Nov 11, 2019 at 08:36:37PM -0700, Jerry Snitselaar wrote:
> Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ
> before probing for interrupts").  Doesn't tpm_tis_send set this flag,
> and setting it here in tpm_tis_core_init short circuits what
> tpm_tis_send was doing before? There is a bug report of an interrupt
> storm from a tpm on a t490s laptop with the Fedora 31 kernel (5.3),
> and I'm wondering if this change could cause that. Before they got the
> warning about interrupts not working, and using polling instead.

Looks like it. Stefan?

/Jarkko

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jarkko Sakkinen]

On Tue, Nov 12, 2019 at 08:28:57AM -0500, Stefan Berger wrote:
> I set this flag for the TIS because it wasn't set anywhere else.
> tpm_tis_send() wouldn't set the flag but go via the path:
> 
> if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
> 
>         return tpm_tis_send_main(chip, buf, len);

Wondering why this isn't just "if (priv->irq_tested)"? Isn't that the
whole point. The tail is the test part e.g. should be executed when
IRQ testing is done.

/Jarkko

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jerry Snitselaar]

On Tue Nov 12 19, Jarkko Sakkinen wrote:
>On Mon, Nov 11, 2019 at 08:36:37PM -0700, Jerry Snitselaar wrote:
>> Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ
>> before probing for interrupts").  Doesn't tpm_tis_send set this flag,
>> and setting it here in tpm_tis_core_init short circuits what
>> tpm_tis_send was doing before? There is a bug report of an interrupt
>> storm from a tpm on a t490s laptop with the Fedora 31 kernel (5.3),
>> and I'm wondering if this change could cause that. Before they got the
>> warning about interrupts not working, and using polling instead.
>
>Looks like it. Stefan?
>
>/Jarkko
>

Stefan is right about the condition check at the beginning of tpm_tis_send.

	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
		return tpm_tis_send_main(chip, buf, len);

Before his change it would've gone straight to calling
tpm_tis_send_main instead of jumping down and doing the irq test, due
to the flag not being set. With his change it should now skip this
tpm_tis_send_main call when tpm_tis_gen_interrupt is called, and then
after that time through tpm_tis_send priv->irq_tested will be set, and
the flag should be set as to whether or not irqs were working.

I should hopefully have access to a t490s in a few days so I can look at it,
and try to figure out what is happening.

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Stefan Berger]

On 11/12/19 3:17 PM, Jerry Snitselaar wrote:
> On Tue Nov 12 19, Jarkko Sakkinen wrote:
>> On Mon, Nov 11, 2019 at 08:36:37PM -0700, Jerry Snitselaar wrote:
>>> Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ
>>> before probing for interrupts").  Doesn't tpm_tis_send set this flag,
>>> and setting it here in tpm_tis_core_init short circuits what
>>> tpm_tis_send was doing before? There is a bug report of an interrupt
>>> storm from a tpm on a t490s laptop with the Fedora 31 kernel (5.3),
>>> and I'm wondering if this change could cause that. Before they got the
>>> warning about interrupts not working, and using polling instead.
>>
>> Looks like it. Stefan?
>>
>> /Jarkko
>>
>
> Stefan is right about the condition check at the beginning of 
> tpm_tis_send.
>
>     if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>         return tpm_tis_send_main(chip, buf, len);
>
> Before his change it would've gone straight to calling
> tpm_tis_send_main instead of jumping down and doing the irq test, due
> to the flag not being set. With his change it should now skip this
> tpm_tis_send_main call when tpm_tis_gen_interrupt is called, and then
> after that time through tpm_tis_send priv->irq_tested will be set, and
> the flag should be set as to whether or not irqs were working.
>
> I should hopefully have access to a t490s in a few days so I can look 
> at it,
> and try to figure out what is happening.
>
I hope the t490s is an outlier. Give the patch I just posted a try.

     Stefan

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jarkko Sakkinen]

On Tue, Nov 12, 2019 at 03:30:51PM -0500, Stefan Berger wrote:
> On 11/12/19 3:17 PM, Jerry Snitselaar wrote:
> > On Tue Nov 12 19, Jarkko Sakkinen wrote:
> > > On Mon, Nov 11, 2019 at 08:36:37PM -0700, Jerry Snitselaar wrote:
> > > > Question about 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ
> > > > before probing for interrupts").  Doesn't tpm_tis_send set this flag,
> > > > and setting it here in tpm_tis_core_init short circuits what
> > > > tpm_tis_send was doing before? There is a bug report of an interrupt
> > > > storm from a tpm on a t490s laptop with the Fedora 31 kernel (5.3),
> > > > and I'm wondering if this change could cause that. Before they got the
> > > > warning about interrupts not working, and using polling instead.
> > > 
> > > Looks like it. Stefan?
> > > 
> > > /Jarkko
> > > 
> > 
> > Stefan is right about the condition check at the beginning of
> > tpm_tis_send.
> > 
> >     if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
> >         return tpm_tis_send_main(chip, buf, len);
> > 
> > Before his change it would've gone straight to calling
> > tpm_tis_send_main instead of jumping down and doing the irq test, due
> > to the flag not being set. With his change it should now skip this
> > tpm_tis_send_main call when tpm_tis_gen_interrupt is called, and then
> > after that time through tpm_tis_send priv->irq_tested will be set, and
> > the flag should be set as to whether or not irqs were working.
> > 
> > I should hopefully have access to a t490s in a few days so I can look at
> > it,
> > and try to figure out what is happening.
> > 
> I hope the t490s is an outlier. Give the patch I just posted a try.

First I must be first that it is the best way to fix the bug. Also,
it did not have fixes tag.

/Jarkko

Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init

[Jerry Snitselaar]

On Tue Nov 12 19, Jarkko Sakkinen wrote:
>On Tue, Nov 12, 2019 at 08:28:57AM -0500, Stefan Berger wrote:
>> I set this flag for the TIS because it wasn't set anywhere else.
>> tpm_tis_send() wouldn't set the flag but go via the path:
>>
>> if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>>
>>         return tpm_tis_send_main(chip, buf, len);
>
>Wondering why this isn't just "if (priv->irq_tested)"? Isn't that the
>whole point. The tail is the test part e.g. should be executed when
>IRQ testing is done.
>
>/Jarkko
>

I wonder if it would make sense to rename tpm_tis_send_main to tpm_tis_send,
move the irq testing bits from the current tpm_tis_send to tpm_tis_gen_interrupt,
and have tpm_tis_gen_interrupt build its own tpmbufs to send via tpm_tis_send
for the testing. Have all the irq testing bits are off on their own and separated out
from sending commands.