[PATCH] tpm_tis_core: Disable broken IRQ handling code

[PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
the TPM_CHIP_FLAG_IRQ ever.

So the whole IRQ probing code is not useful, worse we rely on the
IRQ-test path of tpm_tis_send() to call disable_interrupts() if
interrupts do not work, but that path never gets entered because we
never set the TPM_CHIP_FLAG_IRQ.

So the remaining IRQ probe code calls request_irq() and never calls
free_irq() even when the interrupt is not working.

On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
to use and never free creates an interrupt storm followed by
an "irq XX: nobody cared" oops.

Since it is non-functional at the moment anyways, lets just completely
disable the IRQ code in tpm_tis_core for now.

Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note I'm working with Lenovo to try and get to the bottom of this.
---
 drivers/char/tpm/tpm_tis_core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 27c6ca031e23..647a4a4ccd0c 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -697,6 +697,7 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
 	}
 }
 
+#if 0 /* See the comment in tpm_tis_core_init */
 static irqreturn_t tis_int_handler(int dummy, void *dev_id)
 {
 	struct tpm_chip *chip = dev_id;
@@ -838,6 +839,7 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
 					     original_int_vec))
 		return;
 }
+#endif
 
 void tpm_tis_remove(struct tpm_chip *chip)
 {
@@ -1048,6 +1050,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&priv->read_queue);
 	init_waitqueue_head(&priv->int_queue);
+/*
+ * Interrupt support is broken ATM, we never set TPM_CHIP_FLAG_IRQ.
+ * The below code still registers an interrupt handler even though we never
+ * wait for the wait_queues it signals. On some systems the interrupt we try
+ * to use creates an interrupt storm followed by an "irq XX: nobody cared"
+ * oops. So disable this code for now.
+ */
+#if 0
 	if (irq != -1) {
 		/* Before doing irq testing issue a command to the TPM in polling mode
 		 * to make sure it works. May as well use that command to set the
@@ -1069,6 +1079,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 			tpm_tis_probe_irq(chip, intmask);
 		}
 	}
+#endif
 
 	rc = tpm_chip_register(chip);
 	if (rc)
-- 
2.26.0

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jason Gunthorpe]

On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> the TPM_CHIP_FLAG_IRQ ever.

This all used to work..

> So the whole IRQ probing code is not useful, worse we rely on the
> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> interrupts do not work, but that path never gets entered because we
> never set the TPM_CHIP_FLAG_IRQ.

The IRQ probing code should be deleted.
 
> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> to use and never free creates an interrupt storm followed by
> an "irq XX: nobody cared" oops.

Is this related to probing?

Jason

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Fri, Apr 10, 2020 at 01:38:26PM -0300, Jason Gunthorpe wrote:
> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > the TPM_CHIP_FLAG_IRQ ever.
> 
> This all used to work..

Yes, up until T490 issues.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Fri, Apr 10, 2020 at 01:38:26PM -0300, Jason Gunthorpe wrote:
> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > the TPM_CHIP_FLAG_IRQ ever.
> 
> This all used to work..
> 
> > So the whole IRQ probing code is not useful, worse we rely on the
> > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > interrupts do not work, but that path never gets entered because we
> > never set the TPM_CHIP_FLAG_IRQ.
> 
> The IRQ probing code should be deleted.
>  
> > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > to use and never free creates an interrupt storm followed by
> > an "irq XX: nobody cared" oops.
> 
> Is this related to probing?

Found it:

https://bugzilla.kernel.org/show_bug.cgi?id=203561

Has existed for a quite a while.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jerry Snitselaar]

On Fri Apr 10 20, Jason Gunthorpe wrote:
>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>> the TPM_CHIP_FLAG_IRQ ever.
>
>This all used to work..

IIRC this goes all the way back to 570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
when the flag was added. There was never anything initially setting it in the tpm_tis code.
There were checks added, but the only place it got set was where it did the interrupt test in
tpm_tis_send, and it can only get down to that code if the flag is set. Stefan's patch was enabling
the flag, but with the flag enabled some systems were seeing interrupt storms. I believe it has
been seen with both the t490 and an internal system that Dan Williams was working on at Intel.
Without access to hw seeing the problem the decision was to revert Stefan's patches while
we try to figure out the issues.

>
>> So the whole IRQ probing code is not useful, worse we rely on the
>> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>> interrupts do not work, but that path never gets entered because we
>> never set the TPM_CHIP_FLAG_IRQ.
>
>The IRQ probing code should be deleted.
>
>> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>> to use and never free creates an interrupt storm followed by
>> an "irq XX: nobody cared" oops.
>
>Is this related to probing?
>
>Jason
>

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jan Lübbe]

On Tue, 2020-04-14 at 10:44 -0700, Jerry Snitselaar wrote:
> On Fri Apr 10 20, Jason Gunthorpe wrote:
> 
> > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > the TPM_CHIP_FLAG_IRQ ever.
> > This all used to work..
> 
> 
> IIRC this goes all the way back to 570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
> when the flag was added. There was never anything initially setting it in the tpm_tis code.
> There were checks added, but the only place it got set was where it did the interrupt test in
> tpm_tis_send, and it can only get down to that code if the flag is set. Stefan's patch was enabling
> the flag, but with the flag enabled some systems were seeing interrupt storms. I believe it has
> been seen with both the t490 and an internal system that Dan Williams was working on at Intel.
> Without access to hw seeing the problem the decision was to revert Stefan's patches while
> we try to figure out the issues.

I stumbled across this when debugging a sporadic "tpm tpm0:
tpm_try_transmit: send(): error -62 (-ETIME)" we see on a i.MX6 board
with a TPM2 connected via SPI. It seems that comes from down in
wait_for_tpm_stat() when the TPM doesn't become ready quickly enough.

As this board actually has the IRQ connected, but is falling back to
polling, I took another look and what's wrong with the IRQ support. I
don't have it working yet, but have found some things I'd like to share
(and maybe get some fresh ideas).

I used Stefan Berger's "tpm_tis_core: Turn on the TPM before probing
IRQ's" and "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for
interrupts" to get it into the IRQ verification code in tpm_tis_send().

Then I changed the IRQ to threaded with IRQF_ONESHOT, as the handler is
calling into the SPI stack (which can sleep). A related problem exists
in wait_for_tpm_stat(), which uses the potentially sleeping
wait_for_tpm_stat_cond() as the condition check in
wait_event_interruptible_timeout(). This leads to a:
[   41.799086] do not call blocking ops when !TASK_RUNNING; state=1 set at [<f02406bc>] prepare_to_wait_event+0x84/0x1a4
seems to lead to working transfers, but nevertheless needs to be
changed to be compatible with sleeping busses.

So I think this probably only worked on systems which don't use SPI.

What still confuses me, is that after a few successful transfers I keep
getting IRQs with TPM_INTF_CMD_READY_INT set, although the handler
clear that bit. If i read the TIS spec correctly, it should only be
asserted by a 0->1 transition of
TPM_INT_STATUS_x.commandReadyIntOccured flag, which shouldn't happend
so often.

Regards,
Jan

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Tue, Apr 14, 2020 at 10:44:33AM -0700, Jerry Snitselaar wrote:
> On Fri Apr 10 20, Jason Gunthorpe wrote:
> > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > the TPM_CHIP_FLAG_IRQ ever.
> > 
> > This all used to work..
> 
> IIRC this goes all the way back to 570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
> when the flag was added. There was never anything initially setting it in the tpm_tis code.
> There were checks added, but the only place it got set was where it did the interrupt test in
> tpm_tis_send, and it can only get down to that code if the flag is set. Stefan's patch was enabling
> the flag, but with the flag enabled some systems were seeing interrupt storms. I believe it has
> been seen with both the t490 and an internal system that Dan Williams was working on at Intel.
> Without access to hw seeing the problem the decision was to revert Stefan's patches while
> we try to figure out the issues.

Thanks Jerry.

Yup, kinda hard to debug irq issues without local access
to the whole platform. They can be nasty issues to debug
in the 1st place.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> the TPM_CHIP_FLAG_IRQ ever.
> 
> So the whole IRQ probing code is not useful, worse we rely on the
> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> interrupts do not work, but that path never gets entered because we
> never set the TPM_CHIP_FLAG_IRQ.
> 
> So the remaining IRQ probe code calls request_irq() and never calls
> free_irq() even when the interrupt is not working.
> 
> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> to use and never free creates an interrupt storm followed by
> an "irq XX: nobody cared" oops.
> 
> Since it is non-functional at the moment anyways, lets just completely
> disable the IRQ code in tpm_tis_core for now.
> 
> Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note I'm working with Lenovo to try and get to the bottom of this.
> ---

OK if I recall correctly the reason for reverting was that the fixes
Stefan was sending were broken and no access to hardware were the
issues would be visible. The reason for not doing anything til this
day is that we don't have T490 available.

The lack of devm_free_irq() is an by itself, so please instead send
a fix that adds that to the code instead of "#if 0" crap.

Thank you.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Sat, Apr 11, 2020 at 12:07:02AM +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > the TPM_CHIP_FLAG_IRQ ever.
> > 
> > So the whole IRQ probing code is not useful, worse we rely on the
> > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > interrupts do not work, but that path never gets entered because we
> > never set the TPM_CHIP_FLAG_IRQ.
> > 
> > So the remaining IRQ probe code calls request_irq() and never calls
> > free_irq() even when the interrupt is not working.
> > 
> > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > to use and never free creates an interrupt storm followed by
> > an "irq XX: nobody cared" oops.
> > 
> > Since it is non-functional at the moment anyways, lets just completely
> > disable the IRQ code in tpm_tis_core for now.
> > 
> > Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Note I'm working with Lenovo to try and get to the bottom of this.
> > ---
> 
> OK if I recall correctly the reason for reverting was that the fixes
> Stefan was sending were broken and no access to hardware were the
> issues would be visible. The reason for not doing anything til this
> day is that we don't have T490 available.
> 
> The lack of devm_free_irq() is an by itself, so please instead send
> a fix that adds that to the code instead of "#if 0" crap.

If we were to go with your proposal, then you'd have to do "if 0" to
bunch of other places. Otherwise, the change would be incomplete. Thus,
it is now more sane just to free that IRQ.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Sat, Apr 11, 2020 at 12:09:36AM +0300, Jarkko Sakkinen wrote:
> On Sat, Apr 11, 2020 at 12:07:02AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > the TPM_CHIP_FLAG_IRQ ever.
> > > 
> > > So the whole IRQ probing code is not useful, worse we rely on the
> > > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > > interrupts do not work, but that path never gets entered because we
> > > never set the TPM_CHIP_FLAG_IRQ.
> > > 
> > > So the remaining IRQ probe code calls request_irq() and never calls
> > > free_irq() even when the interrupt is not working.
> > > 
> > > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > > to use and never free creates an interrupt storm followed by
> > > an "irq XX: nobody cared" oops.
> > > 
> > > Since it is non-functional at the moment anyways, lets just completely
> > > disable the IRQ code in tpm_tis_core for now.
> > > 
> > > Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Note I'm working with Lenovo to try and get to the bottom of this.
> > > ---
> > 
> > OK if I recall correctly the reason for reverting was that the fixes
> > Stefan was sending were broken and no access to hardware were the
> > issues would be visible. The reason for not doing anything til this
> > day is that we don't have T490 available.
> > 
> > The lack of devm_free_irq() is an by itself, so please instead send
> > a fix that adds that to the code instead of "#if 0" crap.
> 
> If we were to go with your proposal, then you'd have to do "if 0" to
> bunch of other places. Otherwise, the change would be incomplete. Thus,
> it is now more sane just to free that IRQ.

Also, already the commit that has the fixes tag disables IRQ handling
and that was for the reason that some hardware that we (basically me
and Jerry) did not have access to. That's why there is no proper fix.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Hi All,

On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>> the TPM_CHIP_FLAG_IRQ ever.
>>
>> So the whole IRQ probing code is not useful, worse we rely on the
>> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>> interrupts do not work, but that path never gets entered because we
>> never set the TPM_CHIP_FLAG_IRQ.
>>
>> So the remaining IRQ probe code calls request_irq() and never calls
>> free_irq() even when the interrupt is not working.
>>
>> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>> to use and never free creates an interrupt storm followed by
>> an "irq XX: nobody cared" oops.
>>
>> Since it is non-functional at the moment anyways, lets just completely
>> disable the IRQ code in tpm_tis_core for now.
>>
>> Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note I'm working with Lenovo to try and get to the bottom of this.
>> ---
> 
> OK if I recall correctly the reason for reverting was that the fixes
> Stefan was sending were broken and no access to hardware were the
> issues would be visible. The reason for not doing anything til this
> day is that we don't have T490 available.

So as promised I have been in contact with Lenovo about this.

Specifically I have been in contact with Lenovo about seeing an
IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
8th gen (X1C8), because of the now public plan that Lenovo will
offer ordering this model with Fedora pre-installed:
https://lwn.net/Articles/818595/

On the X1C8 the problem has been diagnosed to be a misconfigured
GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
TPM chip with its IRQ connected to a GPIO on the SoC which is
configured in Direct IRQ mode, so that it directly asserts
IRQs on one of the APIC IRQs.  The problem is that due to the
misconfiguration as soon as the IRQ is enabled it fires
continuously.

For the X1C8 this should be fixed in the BIOS of the first
batch which gets shipped out to customers so there we should
not have to worry about this.

It is likely (but not yet confirmed) that the issue on the T490
is the same, although on my test X1C8 device I got an IRQ storm,
followed by the kernel disabling the IRQ, not a non booting system.
I guess this might be due to kernel configuration differences.

Assuming that the issue on the T490 is the same, we might see a
BIOS update fixing this, but given that non-booting is
'not good ("tm")' even if there will be a BIOS fix we should
still do something at the kernel level to also work with the
older unfixed BIOS which is already out there.

I've been thinking about this and I'm afraid that the only thing
what we can do is add a DMI product-name (product-version for Lenovo)
string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
and set tpm_info.irq = -1 for devices on that list.

My plan is to prepare a RFC patch of such a blacklist, while we
wait for confirmation that the root cause on the T490 is the same
as on the X1C8, but before I work on that I'm wondering if
people agree that that is the best approach, or if there are
other suggestions for dealing with this ?

Regards,

Hans

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Hi All,

On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>> the TPM_CHIP_FLAG_IRQ ever.
>>
>> So the whole IRQ probing code is not useful, worse we rely on the
>> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>> interrupts do not work, but that path never gets entered because we
>> never set the TPM_CHIP_FLAG_IRQ.
>>
>> So the remaining IRQ probe code calls request_irq() and never calls
>> free_irq() even when the interrupt is not working.
>>
>> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>> to use and never free creates an interrupt storm followed by
>> an "irq XX: nobody cared" oops.
>>
>> Since it is non-functional at the moment anyways, lets just completely
>> disable the IRQ code in tpm_tis_core for now.
>>
>> Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note I'm working with Lenovo to try and get to the bottom of this.
>> ---
> 
> OK if I recall correctly the reason for reverting was that the fixes
> Stefan was sending were broken and no access to hardware were the
> issues would be visible. The reason for not doing anything til this
> day is that we don't have T490 available.

So as promised I have been in contact with Lenovo about this.

Specifically I have been in contact with Lenovo about seeing an
IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
8th gen (X1C8), because of the now public plan that Lenovo will
offer ordering this model with Fedora pre-installed:
https://lwn.net/Articles/818595/

On the X1C8 the problem has been diagnosed to be a misconfigured
GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
TPM chip with its IRQ connected to a GPIO on the SoC which is
configured in Direct IRQ mode, so that it directly asserts
IRQs on one of the APIC IRQs.  The problem is that due to the
misconfiguration as soon as the IRQ is enabled it fires
continuously.

For the X1C8 this should be fixed in the BIOS of the first
batch which gets shipped out to customers so there we should
not have to worry about this.

It is likely (but not yet confirmed) that the issue on the T490
is the same, although on my test X1C8 device I got an IRQ storm,
followed by the kernel disabling the IRQ, not a non booting system.
I guess this might be due to kernel configuration differences.

Assuming that the issue on the T490 is the same, we might see a
BIOS update fixing this, but given that non-booting is
'not good ("tm")' even if there will be a BIOS fix we should
still do something at the kernel level to also work with the
older unfixed BIOS which is already out there.

I've been thinking about this and I'm afraid that the only thing
what we can do is add a DMI product-name (product-version for Lenovo)
string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
and set tpm_info.irq = -1 for devices on that list.

My plan is to prepare a RFC patch of such a blacklist, while we
wait for confirmation that the root cause on the T490 is the same
as on the X1C8, but before I work on that I'm wondering if
people agree that that is the best approach, or if there are
other suggestions for dealing with this ?

Regards,

Hans

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jerry Snitselaar]

On Thu May 07 20, Hans de Goede wrote:
>Hi All,
>
>On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
>>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>>>Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>>>TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>>>the TPM_CHIP_FLAG_IRQ ever.
>>>
>>>So the whole IRQ probing code is not useful, worse we rely on the
>>>IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>>>interrupts do not work, but that path never gets entered because we
>>>never set the TPM_CHIP_FLAG_IRQ.
>>>
>>>So the remaining IRQ probe code calls request_irq() and never calls
>>>free_irq() even when the interrupt is not working.
>>>
>>>On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>>>to use and never free creates an interrupt storm followed by
>>>an "irq XX: nobody cared" oops.
>>>
>>>Since it is non-functional at the moment anyways, lets just completely
>>>disable the IRQ code in tpm_tis_core for now.
>>>
>>>Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>---
>>>Note I'm working with Lenovo to try and get to the bottom of this.
>>>---
>>
>>OK if I recall correctly the reason for reverting was that the fixes
>>Stefan was sending were broken and no access to hardware were the
>>issues would be visible. The reason for not doing anything til this
>>day is that we don't have T490 available.
>
>So as promised I have been in contact with Lenovo about this.
>
>Specifically I have been in contact with Lenovo about seeing an
>IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
>8th gen (X1C8), because of the now public plan that Lenovo will
>offer ordering this model with Fedora pre-installed:
>https://lwn.net/Articles/818595/
>
>On the X1C8 the problem has been diagnosed to be a misconfigured
>GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
>TPM chip with its IRQ connected to a GPIO on the SoC which is
>configured in Direct IRQ mode, so that it directly asserts
>IRQs on one of the APIC IRQs.  The problem is that due to the
>misconfiguration as soon as the IRQ is enabled it fires
>continuously.
>
>For the X1C8 this should be fixed in the BIOS of the first
>batch which gets shipped out to customers so there we should
>not have to worry about this.
>
>It is likely (but not yet confirmed) that the issue on the T490
>is the same, although on my test X1C8 device I got an IRQ storm,
>followed by the kernel disabling the IRQ, not a non booting system.
>I guess this might be due to kernel configuration differences.
>
>Assuming that the issue on the T490 is the same, we might see a
>BIOS update fixing this, but given that non-booting is
>'not good ("tm")' even if there will be a BIOS fix we should
>still do something at the kernel level to also work with the
>older unfixed BIOS which is already out there.
>
>I've been thinking about this and I'm afraid that the only thing
>what we can do is add a DMI product-name (product-version for Lenovo)
>string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
>and set tpm_info.irq = -1 for devices on that list.
>
>My plan is to prepare a RFC patch of such a blacklist, while we
>wait for confirmation that the root cause on the T490 is the same
>as on the X1C8, but before I work on that I'm wondering if
>people agree that that is the best approach, or if there are
>other suggestions for dealing with this ?
>
>Regards,
>
>Hans
>

Dan,

Could this be the cause of the problem on the system you were
seeing the issue with, or was that using PTT?

Regards,
Jerry

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Hi,

On 5/7/20 4:17 PM, Jerry Snitselaar wrote:
> On Thu May 07 20, Hans de Goede wrote:
>> Hi All,
>>
>> On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
>>> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>>>> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>>>> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>>>> the TPM_CHIP_FLAG_IRQ ever.
>>>>
>>>> So the whole IRQ probing code is not useful, worse we rely on the
>>>> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>>>> interrupts do not work, but that path never gets entered because we
>>>> never set the TPM_CHIP_FLAG_IRQ.
>>>>
>>>> So the remaining IRQ probe code calls request_irq() and never calls
>>>> free_irq() even when the interrupt is not working.
>>>>
>>>> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>>>> to use and never free creates an interrupt storm followed by
>>>> an "irq XX: nobody cared" oops.
>>>>
>>>> Since it is non-functional at the moment anyways, lets just completely
>>>> disable the IRQ code in tpm_tis_core for now.
>>>>
>>>> Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note I'm working with Lenovo to try and get to the bottom of this.
>>>> ---
>>>
>>> OK if I recall correctly the reason for reverting was that the fixes
>>> Stefan was sending were broken and no access to hardware were the
>>> issues would be visible. The reason for not doing anything til this
>>> day is that we don't have T490 available.
>>
>> So as promised I have been in contact with Lenovo about this.
>>
>> Specifically I have been in contact with Lenovo about seeing an
>> IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
>> 8th gen (X1C8), because of the now public plan that Lenovo will
>> offer ordering this model with Fedora pre-installed:
>> https://lwn.net/Articles/818595/
>>
>> On the X1C8 the problem has been diagnosed to be a misconfigured
>> GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
>> TPM chip with its IRQ connected to a GPIO on the SoC which is
>> configured in Direct IRQ mode, so that it directly asserts
>> IRQs on one of the APIC IRQs.  The problem is that due to the
>> misconfiguration as soon as the IRQ is enabled it fires
>> continuously.
>>
>> For the X1C8 this should be fixed in the BIOS of the first
>> batch which gets shipped out to customers so there we should
>> not have to worry about this.
>>
>> It is likely (but not yet confirmed) that the issue on the T490
>> is the same, although on my test X1C8 device I got an IRQ storm,
>> followed by the kernel disabling the IRQ, not a non booting system.
>> I guess this might be due to kernel configuration differences.
>>
>> Assuming that the issue on the T490 is the same, we might see a
>> BIOS update fixing this, but given that non-booting is
>> 'not good ("tm")' even if there will be a BIOS fix we should
>> still do something at the kernel level to also work with the
>> older unfixed BIOS which is already out there.
>>
>> I've been thinking about this and I'm afraid that the only thing
>> what we can do is add a DMI product-name (product-version for Lenovo)
>> string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
>> and set tpm_info.irq = -1 for devices on that list.
>>
>> My plan is to prepare a RFC patch of such a blacklist, while we
>> wait for confirmation that the root cause on the T490 is the same
>> as on the X1C8, but before I work on that I'm wondering if
>> people agree that that is the best approach, or if there are
>> other suggestions for dealing with this ?
>>
>> Regards,
>>
>> Hans
>>
> 
> Dan,
> 
> Could this be the cause of the problem on the system you were
> seeing the issue with, or was that using PTT?

The system I was seeing the issue on was the X1C8, so yes this
is (was with updated BIOS) the cause of the issue I was seeing.

Regards,

Hans

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Dan Williams]

On Thu, May 7, 2020 at 7:17 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
>
> On Thu May 07 20, Hans de Goede wrote:
> >Hi All,
> >
> >On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> >>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> >>>Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> >>>TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> >>>the TPM_CHIP_FLAG_IRQ ever.
> >>>
> >>>So the whole IRQ probing code is not useful, worse we rely on the
> >>>IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> >>>interrupts do not work, but that path never gets entered because we
> >>>never set the TPM_CHIP_FLAG_IRQ.
> >>>
> >>>So the remaining IRQ probe code calls request_irq() and never calls
> >>>free_irq() even when the interrupt is not working.
> >>>
> >>>On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> >>>to use and never free creates an interrupt storm followed by
> >>>an "irq XX: nobody cared" oops.
> >>>
> >>>Since it is non-functional at the moment anyways, lets just completely
> >>>disable the IRQ code in tpm_tis_core for now.
> >>>
> >>>Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> >>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>---
> >>>Note I'm working with Lenovo to try and get to the bottom of this.
> >>>---
> >>
> >>OK if I recall correctly the reason for reverting was that the fixes
> >>Stefan was sending were broken and no access to hardware were the
> >>issues would be visible. The reason for not doing anything til this
> >>day is that we don't have T490 available.
> >
> >So as promised I have been in contact with Lenovo about this.
> >
> >Specifically I have been in contact with Lenovo about seeing an
> >IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
> >8th gen (X1C8), because of the now public plan that Lenovo will
> >offer ordering this model with Fedora pre-installed:
> >https://lwn.net/Articles/818595/
> >
> >On the X1C8 the problem has been diagnosed to be a misconfigured
> >GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
> >TPM chip with its IRQ connected to a GPIO on the SoC which is
> >configured in Direct IRQ mode, so that it directly asserts
> >IRQs on one of the APIC IRQs.  The problem is that due to the
> >misconfiguration as soon as the IRQ is enabled it fires
> >continuously.
> >
> >For the X1C8 this should be fixed in the BIOS of the first
> >batch which gets shipped out to customers so there we should
> >not have to worry about this.
> >
> >It is likely (but not yet confirmed) that the issue on the T490
> >is the same, although on my test X1C8 device I got an IRQ storm,
> >followed by the kernel disabling the IRQ, not a non booting system.
> >I guess this might be due to kernel configuration differences.
> >
> >Assuming that the issue on the T490 is the same, we might see a
> >BIOS update fixing this, but given that non-booting is
> >'not good ("tm")' even if there will be a BIOS fix we should
> >still do something at the kernel level to also work with the
> >older unfixed BIOS which is already out there.
> >
> >I've been thinking about this and I'm afraid that the only thing
> >what we can do is add a DMI product-name (product-version for Lenovo)
> >string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
> >and set tpm_info.irq = -1 for devices on that list.
> >
> >My plan is to prepare a RFC patch of such a blacklist, while we
> >wait for confirmation that the root cause on the T490 is the same
> >as on the X1C8, but before I work on that I'm wondering if
> >people agree that that is the best approach, or if there are
> >other suggestions for dealing with this ?
> >
> >Regards,
> >
> >Hans
> >
>
> Dan,
>
> Could this be the cause of the problem on the system you were
> seeing the issue with, or was that using PTT?

It sounds similar, I'm just not immediately aware of where I can find
out how the GPIOs are routed on that development board. I'll poke
around.

What's PTT?

My concern with a blacklist is that the existing tpm_tis module
parameter to disable interrupts, IIRC, did not help mitigate this
problem. So I would think that if there is a blackilst it should at
least be amenable by module parameter for new platforms, or that
specifying "interrupts=0" to tpm_tis.ko behaves identically to the
device being placed on the list.

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jerry Snitselaar]

On Thu May 07 20, Dan Williams wrote:
>On Thu, May 7, 2020 at 7:17 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
>>
>> On Thu May 07 20, Hans de Goede wrote:
>> >Hi All,
>> >
>> >On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
>> >>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>> >>>Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>> >>>TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>> >>>the TPM_CHIP_FLAG_IRQ ever.
>> >>>
>> >>>So the whole IRQ probing code is not useful, worse we rely on the
>> >>>IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>> >>>interrupts do not work, but that path never gets entered because we
>> >>>never set the TPM_CHIP_FLAG_IRQ.
>> >>>
>> >>>So the remaining IRQ probe code calls request_irq() and never calls
>> >>>free_irq() even when the interrupt is not working.
>> >>>
>> >>>On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>> >>>to use and never free creates an interrupt storm followed by
>> >>>an "irq XX: nobody cared" oops.
>> >>>
>> >>>Since it is non-functional at the moment anyways, lets just completely
>> >>>disable the IRQ code in tpm_tis_core for now.
>> >>>
>> >>>Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>> >>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> >>>---
>> >>>Note I'm working with Lenovo to try and get to the bottom of this.
>> >>>---
>> >>
>> >>OK if I recall correctly the reason for reverting was that the fixes
>> >>Stefan was sending were broken and no access to hardware were the
>> >>issues would be visible. The reason for not doing anything til this
>> >>day is that we don't have T490 available.
>> >
>> >So as promised I have been in contact with Lenovo about this.
>> >
>> >Specifically I have been in contact with Lenovo about seeing an
>> >IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
>> >8th gen (X1C8), because of the now public plan that Lenovo will
>> >offer ordering this model with Fedora pre-installed:
>> >https://lwn.net/Articles/818595/
>> >
>> >On the X1C8 the problem has been diagnosed to be a misconfigured
>> >GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
>> >TPM chip with its IRQ connected to a GPIO on the SoC which is
>> >configured in Direct IRQ mode, so that it directly asserts
>> >IRQs on one of the APIC IRQs.  The problem is that due to the
>> >misconfiguration as soon as the IRQ is enabled it fires
>> >continuously.
>> >
>> >For the X1C8 this should be fixed in the BIOS of the first
>> >batch which gets shipped out to customers so there we should
>> >not have to worry about this.
>> >
>> >It is likely (but not yet confirmed) that the issue on the T490
>> >is the same, although on my test X1C8 device I got an IRQ storm,
>> >followed by the kernel disabling the IRQ, not a non booting system.
>> >I guess this might be due to kernel configuration differences.
>> >
>> >Assuming that the issue on the T490 is the same, we might see a
>> >BIOS update fixing this, but given that non-booting is
>> >'not good ("tm")' even if there will be a BIOS fix we should
>> >still do something at the kernel level to also work with the
>> >older unfixed BIOS which is already out there.
>> >
>> >I've been thinking about this and I'm afraid that the only thing
>> >what we can do is add a DMI product-name (product-version for Lenovo)
>> >string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
>> >and set tpm_info.irq = -1 for devices on that list.
>> >
>> >My plan is to prepare a RFC patch of such a blacklist, while we
>> >wait for confirmation that the root cause on the T490 is the same
>> >as on the X1C8, but before I work on that I'm wondering if
>> >people agree that that is the best approach, or if there are
>> >other suggestions for dealing with this ?
>> >
>> >Regards,
>> >
>> >Hans
>> >
>>
>> Dan,
>>
>> Could this be the cause of the problem on the system you were
>> seeing the issue with, or was that using PTT?
>
>It sounds similar, I'm just not immediately aware of where I can find
>out how the GPIOs are routed on that development board. I'll poke
>around.
>
>What's PTT?
>

PTT is Intel's firmware based TPM.

>My concern with a blacklist is that the existing tpm_tis module
>parameter to disable interrupts, IIRC, did not help mitigate this
>problem. So I would think that if there is a blackilst it should at
>least be amenable by module parameter for new platforms, or that
>specifying "interrupts=0" to tpm_tis.ko behaves identically to the
>device being placed on the list.
>

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Hi,

On 5/7/20 11:51 PM, Dan Williams wrote:
<snip>

> My concern with a blacklist is that the existing tpm_tis module
> parameter to disable interrupts, IIRC, did not help mitigate this
> problem. So I would think that if there is a blackilst it should at
> least be amenable by module parameter for new platforms, or that
> specifying "interrupts=0" to tpm_tis.ko behaves identically to the
> device being placed on the list.

Are you sure that disabling the interrupt through the kernel commandline
did not help on the T490? The reverted patches all have to do with
the irq-probing / irq-handling and if tpm_tis_core_init gets passed
an irq of -1 then all of that should be skipped?

Regards,

Hans

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Thu, May 07, 2020 at 02:51:43PM -0700, Dan Williams wrote:
> On Thu, May 7, 2020 at 7:17 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
> >
> > On Thu May 07 20, Hans de Goede wrote:
> > >Hi All,
> > >
> > >On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> > >>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > >>>Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > >>>TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > >>>the TPM_CHIP_FLAG_IRQ ever.
> > >>>
> > >>>So the whole IRQ probing code is not useful, worse we rely on the
> > >>>IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > >>>interrupts do not work, but that path never gets entered because we
> > >>>never set the TPM_CHIP_FLAG_IRQ.
> > >>>
> > >>>So the remaining IRQ probe code calls request_irq() and never calls
> > >>>free_irq() even when the interrupt is not working.
> > >>>
> > >>>On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > >>>to use and never free creates an interrupt storm followed by
> > >>>an "irq XX: nobody cared" oops.
> > >>>
> > >>>Since it is non-functional at the moment anyways, lets just completely
> > >>>disable the IRQ code in tpm_tis_core for now.
> > >>>
> > >>>Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > >>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >>>---
> > >>>Note I'm working with Lenovo to try and get to the bottom of this.
> > >>>---
> > >>
> > >>OK if I recall correctly the reason for reverting was that the fixes
> > >>Stefan was sending were broken and no access to hardware were the
> > >>issues would be visible. The reason for not doing anything til this
> > >>day is that we don't have T490 available.
> > >
> > >So as promised I have been in contact with Lenovo about this.
> > >
> > >Specifically I have been in contact with Lenovo about seeing an
> > >IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
> > >8th gen (X1C8), because of the now public plan that Lenovo will
> > >offer ordering this model with Fedora pre-installed:
> > >https://lwn.net/Articles/818595/
> > >
> > >On the X1C8 the problem has been diagnosed to be a misconfigured
> > >GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
> > >TPM chip with its IRQ connected to a GPIO on the SoC which is
> > >configured in Direct IRQ mode, so that it directly asserts
> > >IRQs on one of the APIC IRQs.  The problem is that due to the
> > >misconfiguration as soon as the IRQ is enabled it fires
> > >continuously.
> > >
> > >For the X1C8 this should be fixed in the BIOS of the first
> > >batch which gets shipped out to customers so there we should
> > >not have to worry about this.
> > >
> > >It is likely (but not yet confirmed) that the issue on the T490
> > >is the same, although on my test X1C8 device I got an IRQ storm,
> > >followed by the kernel disabling the IRQ, not a non booting system.
> > >I guess this might be due to kernel configuration differences.
> > >
> > >Assuming that the issue on the T490 is the same, we might see a
> > >BIOS update fixing this, but given that non-booting is
> > >'not good ("tm")' even if there will be a BIOS fix we should
> > >still do something at the kernel level to also work with the
> > >older unfixed BIOS which is already out there.
> > >
> > >I've been thinking about this and I'm afraid that the only thing
> > >what we can do is add a DMI product-name (product-version for Lenovo)
> > >string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
> > >and set tpm_info.irq = -1 for devices on that list.
> > >
> > >My plan is to prepare a RFC patch of such a blacklist, while we
> > >wait for confirmation that the root cause on the T490 is the same
> > >as on the X1C8, but before I work on that I'm wondering if
> > >people agree that that is the best approach, or if there are
> > >other suggestions for dealing with this ?
> > >
> > >Regards,
> > >
> > >Hans
> > >
> >
> > Dan,
> >
> > Could this be the cause of the problem on the system you were
> > seeing the issue with, or was that using PTT?
> 
> It sounds similar, I'm just not immediately aware of where I can find
> out how the GPIOs are routed on that development board. I'll poke
> around.
> 
> What's PTT?

Intel fTPM.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jerry Snitselaar]

On Thu May 07 20, Hans de Goede wrote:
>Hi All,
>
>On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
>>On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>>>Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>>>TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>>>the TPM_CHIP_FLAG_IRQ ever.
>>>
>>>So the whole IRQ probing code is not useful, worse we rely on the
>>>IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>>>interrupts do not work, but that path never gets entered because we
>>>never set the TPM_CHIP_FLAG_IRQ.
>>>
>>>So the remaining IRQ probe code calls request_irq() and never calls
>>>free_irq() even when the interrupt is not working.
>>>
>>>On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>>>to use and never free creates an interrupt storm followed by
>>>an "irq XX: nobody cared" oops.
>>>
>>>Since it is non-functional at the moment anyways, lets just completely
>>>disable the IRQ code in tpm_tis_core for now.
>>>
>>>Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>---
>>>Note I'm working with Lenovo to try and get to the bottom of this.
>>>---
>>
>>OK if I recall correctly the reason for reverting was that the fixes
>>Stefan was sending were broken and no access to hardware were the
>>issues would be visible. The reason for not doing anything til this
>>day is that we don't have T490 available.
>
>So as promised I have been in contact with Lenovo about this.
>
>Specifically I have been in contact with Lenovo about seeing an
>IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
>8th gen (X1C8), because of the now public plan that Lenovo will
>offer ordering this model with Fedora pre-installed:
>https://lwn.net/Articles/818595/
>
>On the X1C8 the problem has been diagnosed to be a misconfigured
>GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
>TPM chip with its IRQ connected to a GPIO on the SoC which is
>configured in Direct IRQ mode, so that it directly asserts
>IRQs on one of the APIC IRQs.  The problem is that due to the
>misconfiguration as soon as the IRQ is enabled it fires
>continuously.
>
>For the X1C8 this should be fixed in the BIOS of the first
>batch which gets shipped out to customers so there we should
>not have to worry about this.
>
>It is likely (but not yet confirmed) that the issue on the T490
>is the same, although on my test X1C8 device I got an IRQ storm,
>followed by the kernel disabling the IRQ, not a non booting system.
>I guess this might be due to kernel configuration differences.
>
>Assuming that the issue on the T490 is the same, we might see a
>BIOS update fixing this, but given that non-booting is
>'not good ("tm")' even if there will be a BIOS fix we should
>still do something at the kernel level to also work with the
>older unfixed BIOS which is already out there.
>
>I've been thinking about this and I'm afraid that the only thing
>what we can do is add a DMI product-name (product-version for Lenovo)
>string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
>and set tpm_info.irq = -1 for devices on that list.
>
>My plan is to prepare a RFC patch of such a blacklist, while we
>wait for confirmation that the root cause on the T490 is the same
>as on the X1C8, but before I work on that I'm wondering if
>people agree that that is the best approach, or if there are
>other suggestions for dealing with this ?
>
>Regards,
>
>Hans
>

Jarrko,

Any thoughts about how we should move forward on dealing with this?
I've got a report about the original problem Stefan was dealing with,
where the tpm isn't powered on when it tries to send a command to
generate an interrupt. The tpm is functioning so it isn't urgent,
but it would be good to get this cleaned up so users aren't getting
transmit errors and firmware bug messages. Hans did you make any
progress on the blacklist patch?

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Thu, Jun 04, 2020 at 02:17:12AM -0700, Jerry Snitselaar wrote:
> On Thu May 07 20, Hans de Goede wrote:
> > Hi All,
> > 
> > On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> > > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > > the TPM_CHIP_FLAG_IRQ ever.
> > > > 
> > > > So the whole IRQ probing code is not useful, worse we rely on the
> > > > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > > > interrupts do not work, but that path never gets entered because we
> > > > never set the TPM_CHIP_FLAG_IRQ.
> > > > 
> > > > So the remaining IRQ probe code calls request_irq() and never calls
> > > > free_irq() even when the interrupt is not working.
> > > > 
> > > > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > > > to use and never free creates an interrupt storm followed by
> > > > an "irq XX: nobody cared" oops.
> > > > 
> > > > Since it is non-functional at the moment anyways, lets just completely
> > > > disable the IRQ code in tpm_tis_core for now.
> > > > 
> > > > Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > > Note I'm working with Lenovo to try and get to the bottom of this.
> > > > ---
> > > 
> > > OK if I recall correctly the reason for reverting was that the fixes
> > > Stefan was sending were broken and no access to hardware were the
> > > issues would be visible. The reason for not doing anything til this
> > > day is that we don't have T490 available.
> > 
> > So as promised I have been in contact with Lenovo about this.
> > 
> > Specifically I have been in contact with Lenovo about seeing an
> > IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
> > 8th gen (X1C8), because of the now public plan that Lenovo will
> > offer ordering this model with Fedora pre-installed:
> > https://lwn.net/Articles/818595/
> > 
> > On the X1C8 the problem has been diagnosed to be a misconfigured
> > GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
> > TPM chip with its IRQ connected to a GPIO on the SoC which is
> > configured in Direct IRQ mode, so that it directly asserts
> > IRQs on one of the APIC IRQs.  The problem is that due to the
> > misconfiguration as soon as the IRQ is enabled it fires
> > continuously.
> > 
> > For the X1C8 this should be fixed in the BIOS of the first
> > batch which gets shipped out to customers so there we should
> > not have to worry about this.
> > 
> > It is likely (but not yet confirmed) that the issue on the T490
> > is the same, although on my test X1C8 device I got an IRQ storm,
> > followed by the kernel disabling the IRQ, not a non booting system.
> > I guess this might be due to kernel configuration differences.
> > 
> > Assuming that the issue on the T490 is the same, we might see a
> > BIOS update fixing this, but given that non-booting is
> > 'not good ("tm")' even if there will be a BIOS fix we should
> > still do something at the kernel level to also work with the
> > older unfixed BIOS which is already out there.
> > 
> > I've been thinking about this and I'm afraid that the only thing
> > what we can do is add a DMI product-name (product-version for Lenovo)
> > string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
> > and set tpm_info.irq = -1 for devices on that list.
> > 
> > My plan is to prepare a RFC patch of such a blacklist, while we
> > wait for confirmation that the root cause on the T490 is the same
> > as on the X1C8, but before I work on that I'm wondering if
> > people agree that that is the best approach, or if there are
> > other suggestions for dealing with this ?
> > 
> > Regards,
> > 
> > Hans
> > 
> 
> Jarrko,
> 
> Any thoughts about how we should move forward on dealing with this?
> I've got a report about the original problem Stefan was dealing with,
> where the tpm isn't powered on when it tries to send a command to
> generate an interrupt. The tpm is functioning so it isn't urgent,
> but it would be good to get this cleaned up so users aren't getting
> transmit errors and firmware bug messages. Hans did you make any
> progress on the blacklist patch?

We need the blacklist. Up until that warnings is the best we can do.

/Jarkko

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Hans de Goede]

Hi,

On 6/4/20 11:17 AM, Jerry Snitselaar wrote:
> On Thu May 07 20, Hans de Goede wrote:
>> Hi All,
>>
>> On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
>>> On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
>>>> Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
>>>> TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
>>>> the TPM_CHIP_FLAG_IRQ ever.
>>>>
>>>> So the whole IRQ probing code is not useful, worse we rely on the
>>>> IRQ-test path of tpm_tis_send() to call disable_interrupts() if
>>>> interrupts do not work, but that path never gets entered because we
>>>> never set the TPM_CHIP_FLAG_IRQ.
>>>>
>>>> So the remaining IRQ probe code calls request_irq() and never calls
>>>> free_irq() even when the interrupt is not working.
>>>>
>>>> On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
>>>> to use and never free creates an interrupt storm followed by
>>>> an "irq XX: nobody cared" oops.
>>>>
>>>> Since it is non-functional at the moment anyways, lets just completely
>>>> disable the IRQ code in tpm_tis_core for now.
>>>>
>>>> Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note I'm working with Lenovo to try and get to the bottom of this.
>>>> ---
>>>
>>> OK if I recall correctly the reason for reverting was that the fixes
>>> Stefan was sending were broken and no access to hardware were the
>>> issues would be visible. The reason for not doing anything til this
>>> day is that we don't have T490 available.
>>
>> So as promised I have been in contact with Lenovo about this.
>>
>> Specifically I have been in contact with Lenovo about seeing an
>> IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
>> 8th gen (X1C8), because of the now public plan that Lenovo will
>> offer ordering this model with Fedora pre-installed:
>> https://lwn.net/Articles/818595/
>>
>> On the X1C8 the problem has been diagnosed to be a misconfigured
>> GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
>> TPM chip with its IRQ connected to a GPIO on the SoC which is
>> configured in Direct IRQ mode, so that it directly asserts
>> IRQs on one of the APIC IRQs.  The problem is that due to the
>> misconfiguration as soon as the IRQ is enabled it fires
>> continuously.
>>
>> For the X1C8 this should be fixed in the BIOS of the first
>> batch which gets shipped out to customers so there we should
>> not have to worry about this.
>>
>> It is likely (but not yet confirmed) that the issue on the T490
>> is the same, although on my test X1C8 device I got an IRQ storm,
>> followed by the kernel disabling the IRQ, not a non booting system.
>> I guess this might be due to kernel configuration differences.
>>
>> Assuming that the issue on the T490 is the same, we might see a
>> BIOS update fixing this, but given that non-booting is
>> 'not good ("tm")' even if there will be a BIOS fix we should
>> still do something at the kernel level to also work with the
>> older unfixed BIOS which is already out there.
>>
>> I've been thinking about this and I'm afraid that the only thing
>> what we can do is add a DMI product-name (product-version for Lenovo)
>> string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
>> and set tpm_info.irq = -1 for devices on that list.
>>
>> My plan is to prepare a RFC patch of such a blacklist, while we
>> wait for confirmation that the root cause on the T490 is the same
>> as on the X1C8, but before I work on that I'm wondering if
>> people agree that that is the best approach, or if there are
>> other suggestions for dealing with this ?
>>
>> Regards,
>>
>> Hans
>>
> 
> Jarrko,
> 
> Any thoughts about how we should move forward on dealing with this?
> I've got a report about the original problem Stefan was dealing with,
> where the tpm isn't powered on when it tries to send a command to
> generate an interrupt. The tpm is functioning so it isn't urgent,
> but it would be good to get this cleaned up so users aren't getting
> transmit errors and firmware bug messages. Hans did you make any
> progress on the blacklist patch?

Not really. I wanted to confirm on the X1C8 which I have on loan
from Lenovo that it indeed was the GPIO pin misconfiguration which
was the issue. But even though the Lenovo BIOS team claimed that they
have fixed the root cause now, the workaround of not listing any
IRQ for the TPM is also still in place in the latest BIOS I have.

So I cannot test that the IRQ storm is gone with the newer BIOS :|

Still the GPIO pin misconfiguration likely was the issue of the
storm I was seeing on the X1C8 and also the T490s likely has the
same issue. I should actually receive a T490s loaner soon-ish,
because of some unrelated (Builtin privacy screen) work I'm doing.

Once I have received the T490s then I can test things on the T490s:
1. Revert the reverts so that in theory we get working TPM IRQ
    support in the kernel again
2. Check that the T490s does not like this
3. Write and test a blacklist patch

I'll ping my colleague who has arranged the loaner to ask what
the status is on it.

Regards,

Hans

Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

[Jarkko Sakkinen]

On Tue, Jun 16, 2020 at 10:24:21AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 6/4/20 11:17 AM, Jerry Snitselaar wrote:
> > On Thu May 07 20, Hans de Goede wrote:
> > > Hi All,
> > > 
> > > On 4/10/20 11:06 PM, Jarkko Sakkinen wrote:
> > > > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > > > the TPM_CHIP_FLAG_IRQ ever.
> > > > > 
> > > > > So the whole IRQ probing code is not useful, worse we rely on the
> > > > > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > > > > interrupts do not work, but that path never gets entered because we
> > > > > never set the TPM_CHIP_FLAG_IRQ.
> > > > > 
> > > > > So the remaining IRQ probe code calls request_irq() and never calls
> > > > > free_irq() even when the interrupt is not working.
> > > > > 
> > > > > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > > > > to use and never free creates an interrupt storm followed by
> > > > > an "irq XX: nobody cared" oops.
> > > > > 
> > > > > Since it is non-functional at the moment anyways, lets just completely
> > > > > disable the IRQ code in tpm_tis_core for now.
> > > > > 
> > > > > Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > ---
> > > > > Note I'm working with Lenovo to try and get to the bottom of this.
> > > > > ---
> > > > 
> > > > OK if I recall correctly the reason for reverting was that the fixes
> > > > Stefan was sending were broken and no access to hardware were the
> > > > issues would be visible. The reason for not doing anything til this
> > > > day is that we don't have T490 available.
> > > 
> > > So as promised I have been in contact with Lenovo about this.
> > > 
> > > Specifically I have been in contact with Lenovo about seeing an
> > > IRQ storm when the tpm_tis code tries to use the IRQ on a X1 carbon
> > > 8th gen (X1C8), because of the now public plan that Lenovo will
> > > offer ordering this model with Fedora pre-installed:
> > > https://lwn.net/Articles/818595/
> > > 
> > > On the X1C8 the problem has been diagnosed to be a misconfigured
> > > GPIO pin on the CPU (the SoC). The X1C8 uses an SPI connected
> > > TPM chip with its IRQ connected to a GPIO on the SoC which is
> > > configured in Direct IRQ mode, so that it directly asserts
> > > IRQs on one of the APIC IRQs.  The problem is that due to the
> > > misconfiguration as soon as the IRQ is enabled it fires
> > > continuously.
> > > 
> > > For the X1C8 this should be fixed in the BIOS of the first
> > > batch which gets shipped out to customers so there we should
> > > not have to worry about this.
> > > 
> > > It is likely (but not yet confirmed) that the issue on the T490
> > > is the same, although on my test X1C8 device I got an IRQ storm,
> > > followed by the kernel disabling the IRQ, not a non booting system.
> > > I guess this might be due to kernel configuration differences.
> > > 
> > > Assuming that the issue on the T490 is the same, we might see a
> > > BIOS update fixing this, but given that non-booting is
> > > 'not good ("tm")' even if there will be a BIOS fix we should
> > > still do something at the kernel level to also work with the
> > > older unfixed BIOS which is already out there.
> > > 
> > > I've been thinking about this and I'm afraid that the only thing
> > > what we can do is add a DMI product-name (product-version for Lenovo)
> > > string based blacklist for IRQ usage to drivers/char/tpm/tpm_tis.c
> > > and set tpm_info.irq = -1 for devices on that list.
> > > 
> > > My plan is to prepare a RFC patch of such a blacklist, while we
> > > wait for confirmation that the root cause on the T490 is the same
> > > as on the X1C8, but before I work on that I'm wondering if
> > > people agree that that is the best approach, or if there are
> > > other suggestions for dealing with this ?
> > > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > 
> > Jarrko,
> > 
> > Any thoughts about how we should move forward on dealing with this?
> > I've got a report about the original problem Stefan was dealing with,
> > where the tpm isn't powered on when it tries to send a command to
> > generate an interrupt. The tpm is functioning so it isn't urgent,
> > but it would be good to get this cleaned up so users aren't getting
> > transmit errors and firmware bug messages. Hans did you make any
> > progress on the blacklist patch?
> 
> Not really. I wanted to confirm on the X1C8 which I have on loan
> from Lenovo that it indeed was the GPIO pin misconfiguration which
> was the issue. But even though the Lenovo BIOS team claimed that they
> have fixed the root cause now, the workaround of not listing any
> IRQ for the TPM is also still in place in the latest BIOS I have.
> 
> So I cannot test that the IRQ storm is gone with the newer BIOS :|
> 
> Still the GPIO pin misconfiguration likely was the issue of the
> storm I was seeing on the X1C8 and also the T490s likely has the
> same issue. I should actually receive a T490s loaner soon-ish,
> because of some unrelated (Builtin privacy screen) work I'm doing.
> 
> Once I have received the T490s then I can test things on the T490s:
> 1. Revert the reverts so that in theory we get working TPM IRQ
>    support in the kernel again
> 2. Check that the T490s does not like this
> 3. Write and test a blacklist patch
> 
> I'll ping my colleague who has arranged the loaner to ask what
> the status is on it.

Thanks alot. Does not make sense to rush with this now that the
tree is not completely broken.

/Jarkko