* question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
@ 2019-11-12 3:36 Jerry Snitselaar
2019-11-12 13:28 ` Stefan Berger
2019-11-12 20:07 ` Jarkko Sakkinen
0 siblings, 2 replies; 10+ messages in thread
From: Jerry Snitselaar @ 2019-11-12 3:36 UTC (permalink / raw)
To: Stefan Berger, Jarkko Sakkinen, linux-integrity, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 3:36 question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init Jerry Snitselaar
@ 2019-11-12 13:28 ` Stefan Berger
2019-11-12 14:24 ` Jerry Snitselaar
2019-11-12 20:17 ` Jarkko Sakkinen
2019-11-12 20:07 ` Jarkko Sakkinen
1 sibling, 2 replies; 10+ messages in thread
From: Stefan Berger @ 2019-11-12 13:28 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 13:28 ` Stefan Berger
@ 2019-11-12 14:24 ` Jerry Snitselaar
2019-11-12 15:35 ` Stefan Berger
2019-11-12 20:17 ` Jarkko Sakkinen
1 sibling, 1 reply; 10+ messages in thread
From: Jerry Snitselaar @ 2019-11-12 14:24 UTC (permalink / raw)
To: Stefan Berger; +Cc: Jarkko Sakkinen, linux-integrity, linux-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 14:24 ` Jerry Snitselaar
@ 2019-11-12 15:35 ` Stefan Berger
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Berger @ 2019-11-12 15:35 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-integrity, linux-kernel
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...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 3:36 question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init Jerry Snitselaar
2019-11-12 13:28 ` Stefan Berger
@ 2019-11-12 20:07 ` Jarkko Sakkinen
2019-11-12 20:17 ` Jerry Snitselaar
1 sibling, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-11-12 20:07 UTC (permalink / raw)
To: Stefan Berger, linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 13:28 ` Stefan Berger
2019-11-12 14:24 ` Jerry Snitselaar
@ 2019-11-12 20:17 ` Jarkko Sakkinen
2019-11-15 19:14 ` Jerry Snitselaar
1 sibling, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-11-12 20:17 UTC (permalink / raw)
To: Stefan Berger; +Cc: linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 20:07 ` Jarkko Sakkinen
@ 2019-11-12 20:17 ` Jerry Snitselaar
2019-11-12 20:30 ` Stefan Berger
0 siblings, 1 reply; 10+ messages in thread
From: Jerry Snitselaar @ 2019-11-12 20:17 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: Stefan Berger, linux-integrity, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 20:17 ` Jerry Snitselaar
@ 2019-11-12 20:30 ` Stefan Berger
2019-11-14 16:48 ` Jarkko Sakkinen
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Berger @ 2019-11-12 20:30 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 20:30 ` Stefan Berger
@ 2019-11-14 16:48 ` Jarkko Sakkinen
0 siblings, 0 replies; 10+ messages in thread
From: Jarkko Sakkinen @ 2019-11-14 16:48 UTC (permalink / raw)
To: Stefan Berger; +Cc: linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init
2019-11-12 20:17 ` Jarkko Sakkinen
@ 2019-11-15 19:14 ` Jerry Snitselaar
0 siblings, 0 replies; 10+ messages in thread
From: Jerry Snitselaar @ 2019-11-15 19:14 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: Stefan Berger, linux-integrity, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-15 19:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12 3:36 question about setting TPM_CHIP_FLAG_IRQ in tpm_tis_core_init Jerry Snitselaar
2019-11-12 13:28 ` Stefan Berger
2019-11-12 14:24 ` Jerry Snitselaar
2019-11-12 15:35 ` Stefan Berger
2019-11-12 20:17 ` Jarkko Sakkinen
2019-11-15 19:14 ` Jerry Snitselaar
2019-11-12 20:07 ` Jarkko Sakkinen
2019-11-12 20:17 ` Jerry Snitselaar
2019-11-12 20:30 ` Stefan Berger
2019-11-14 16:48 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).