[PATCH 0/4] tpm_tis: fix interrupts (again)

[PATCH 0/4] tpm_tis: fix interrupts (again)

[James Bottomley]

The current state of the TIS TPM is that interrupts have been globally
disabled by various changes.  The problems we got reported the last
time they were enabled was interrupt storms.  With my own TIS TPM,
I've found that this is caused because my TPM doesn't do legacy
cycles, The TIS spec (chapter 6.1 "Locality Usage Per Register")
requires any TIS TPM without legacy cycles not to act on any write to
an interrupt register unless the locality is enabled.  This means if
an interrupt fires after we relinquish the locality, the TPM_EOI in
the interrupt routine is ineffective meaning the same interrupt
triggers over and over again.  This problem also means we can have
trouble setting up interrupts on TIS TPMs because the current init
code does the setup before the locality is claimed for the first time.

James Bottomley (4):
  tpm_tis: Clean up locality release
  tpm_tis: Fix interrupts for TIS TPMs without legacy cycles
  tpm_tis: fix IRQ probing
  Revert "tpm: Revert "tpm_tis_core: Turn on the TPM before probing
    IRQ's""

 drivers/char/tpm/tpm_tis_core.c | 182 ++++++++++++++++++++------------
 1 file changed, 115 insertions(+), 67 deletions(-)

-- 
2.28.0

[PATCH 1/4] tpm_tis: Clean up locality release

[James Bottomley]

The current release locality code seems to be based on the
misunderstanding that the TPM interrupts when a locality is released:
it doesn't, only when the locality is acquired.

Furthermore, there seems to be no point in waiting for the locality to
be released.  All it does is penalize the last TPM user.  However, if
there's no next TPM user, this is a pointless wait and if there is a
next TPM user, they'll pay the penalty waiting for the new locality
(or possibly not if it's the same as the old locality).

Fix the code by making release_locality as simple write to release
with no waiting for completion.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm_tis_core.c | 47 +--------------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 92c51c6cfd1b..a9fa40714c64 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -134,58 +134,13 @@ static bool check_locality(struct tpm_chip *chip, int l)
 	return false;
 }
 
-static bool locality_inactive(struct tpm_chip *chip, int l)
-{
-	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	int rc;
-	u8 access;
-
-	rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
-	if (rc < 0)
-		return false;
-
-	if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
-	    == TPM_ACCESS_VALID)
-		return true;
-
-	return false;
-}
-
 static int release_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	unsigned long stop, timeout;
-	long rc;
 
 	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
 
-	stop = jiffies + chip->timeout_a;
-
-	if (chip->flags & TPM_CHIP_FLAG_IRQ) {
-again:
-		timeout = stop - jiffies;
-		if ((long)timeout <= 0)
-			return -1;
-
-		rc = wait_event_interruptible_timeout(priv->int_queue,
-						      (locality_inactive(chip, l)),
-						      timeout);
-
-		if (rc > 0)
-			return 0;
-
-		if (rc == -ERESTARTSYS && freezing(current)) {
-			clear_thread_flag(TIF_SIGPENDING);
-			goto again;
-		}
-	} else {
-		do {
-			if (locality_inactive(chip, l))
-				return 0;
-			tpm_msleep(TPM_TIMEOUT);
-		} while (time_before(jiffies, stop));
-	}
-	return -1;
+	return 0;
 }
 
 static int request_locality(struct tpm_chip *chip, int l)
-- 
2.28.0

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Jarkko Sakkinen]

On Tue, Sep 29, 2020 at 03:32:13PM -0700, James Bottomley wrote:
> The current release locality code seems to be based on the
> misunderstanding that the TPM interrupts when a locality is released:
> it doesn't, only when the locality is acquired.
> 
> Furthermore, there seems to be no point in waiting for the locality to
> be released.  All it does is penalize the last TPM user.  However, if
> there's no next TPM user, this is a pointless wait and if there is a
> next TPM user, they'll pay the penalty waiting for the new locality
> (or possibly not if it's the same as the old locality).
> 
> Fix the code by making release_locality as simple write to release
> with no waiting for completion.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Adding Jerry for feedback.

Probably should have (if accepted).

Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality")

> ---
>  drivers/char/tpm/tpm_tis_core.c | 47 +--------------------------------
>  1 file changed, 1 insertion(+), 46 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 92c51c6cfd1b..a9fa40714c64 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -134,58 +134,13 @@ static bool check_locality(struct tpm_chip *chip, int l)
>  	return false;
>  }
>  
> -static bool locality_inactive(struct tpm_chip *chip, int l)
> -{
> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int rc;
> -	u8 access;
> -
> -	rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
> -	if (rc < 0)
> -		return false;
> -
> -	if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
> -	    == TPM_ACCESS_VALID)
> -		return true;
> -
> -	return false;
> -}
> -
>  static int release_locality(struct tpm_chip *chip, int l)

Should be void.

>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	unsigned long stop, timeout;
> -	long rc;
>  
>  	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>  
> -	stop = jiffies + chip->timeout_a;
> -
> -	if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> -again:
> -		timeout = stop - jiffies;
> -		if ((long)timeout <= 0)
> -			return -1;
> -
> -		rc = wait_event_interruptible_timeout(priv->int_queue,
> -						      (locality_inactive(chip, l)),
> -						      timeout);
> -
> -		if (rc > 0)
> -			return 0;
> -
> -		if (rc == -ERESTARTSYS && freezing(current)) {
> -			clear_thread_flag(TIF_SIGPENDING);
> -			goto again;
> -		}
> -	} else {
> -		do {
> -			if (locality_inactive(chip, l))
> -				return 0;
> -			tpm_msleep(TPM_TIMEOUT);
> -		} while (time_before(jiffies, stop));
> -	}
> -	return -1;
> +	return 0;
>  }
>  
>  static int request_locality(struct tpm_chip *chip, int l)
> -- 
> 2.28.0
> 

/Jarkko

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Jarkko Sakkinen]

On Wed, Sep 30, 2020 at 05:26:10AM +0300, Jarkko Sakkinen wrote:
> Adding Jerry for feedback.

Ugh, sorry, Jerry was already in the CC list.

/Jarkko

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Jerry Snitselaar]

James Bottomley @ 2020-09-29 15:32 MST:

> The current release locality code seems to be based on the
> misunderstanding that the TPM interrupts when a locality is released:
> it doesn't, only when the locality is acquired.
>
> Furthermore, there seems to be no point in waiting for the locality to
> be released.  All it does is penalize the last TPM user.  However, if
> there's no next TPM user, this is a pointless wait and if there is a
> next TPM user, they'll pay the penalty waiting for the new locality
> (or possibly not if it's the same as the old locality).
>
> Fix the code by making release_locality as simple write to release
> with no waiting for completion.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 47 +--------------------------------
>  1 file changed, 1 insertion(+), 46 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 92c51c6cfd1b..a9fa40714c64 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -134,58 +134,13 @@ static bool check_locality(struct tpm_chip *chip, int l)
>  	return false;
>  }
>  
> -static bool locality_inactive(struct tpm_chip *chip, int l)
> -{
> -	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int rc;
> -	u8 access;
> -
> -	rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access);
> -	if (rc < 0)
> -		return false;
> -
> -	if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
> -	    == TPM_ACCESS_VALID)
> -		return true;
> -
> -	return false;
> -}
> -
>  static int release_locality(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	unsigned long stop, timeout;
> -	long rc;
>  
>  	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>  
> -	stop = jiffies + chip->timeout_a;
> -
> -	if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> -again:
> -		timeout = stop - jiffies;
> -		if ((long)timeout <= 0)
> -			return -1;
> -
> -		rc = wait_event_interruptible_timeout(priv->int_queue,
> -						      (locality_inactive(chip, l)),
> -						      timeout);
> -
> -		if (rc > 0)
> -			return 0;
> -
> -		if (rc == -ERESTARTSYS && freezing(current)) {
> -			clear_thread_flag(TIF_SIGPENDING);
> -			goto again;
> -		}
> -	} else {
> -		do {
> -			if (locality_inactive(chip, l))
> -				return 0;
> -			tpm_msleep(TPM_TIMEOUT);
> -		} while (time_before(jiffies, stop));
> -	}
> -	return -1;
> +	return 0;
>  }
>  
>  static int request_locality(struct tpm_chip *chip, int l)

My recollection is that this was added because there were some chips
that took so long to release locality that a subsequent request_locality
call was seeing the locality as already active, moving on, and then the
locality was getting released out from under the user.

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[James Bottomley]

On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
> James Bottomley @ 2020-09-29 15:32 MST:
> 
> > The current release locality code seems to be based on the
> > misunderstanding that the TPM interrupts when a locality is
> > released: it doesn't, only when the locality is acquired.
> > 
> > Furthermore, there seems to be no point in waiting for the locality
> > to be released.  All it does is penalize the last TPM
> > user.  However, if there's no next TPM user, this is a pointless
> > wait and if there is
> > a
> > next TPM user, they'll pay the penalty waiting for the new locality
> > (or possibly not if it's the same as the old locality).
> > 
> > Fix the code by making release_locality as simple write to release
> > with no waiting for completion.
[...]
> My recollection is that this was added because there were some chips
> that took so long to release locality that a subsequent
> request_locality call was seeing the locality as already active,
> moving on, and then the locality was getting released out from under
> the user.

Well, I could simply dump the interrupt code, which can never work and
we could always poll.

However, there also appears to be a bug in our locality requesting
code.  We write the request and wait for the grant, but a grant should
be signalled by not only the ACCESS_ACTIVE_LOCALITY being 1 but also
the ACCESS_REQUEST_USE going to 0.  As you say, if we're slow to
relinquish, ACCESS_ACTIVE_LOCALITY could already be 1 and we'd think we
were granted, but ACCESS_REQUEST_USE should stay 1 until the TPM
actually grants the next request.

If I code up a fix is there any chance you still have access to a
problem TPM?  Mine all seem to grant and release localities fairly
instantaneously.

James

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Jerry Snitselaar]

James Bottomley @ 2020-09-30 16:03 MST:

> On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
>> James Bottomley @ 2020-09-29 15:32 MST:
>> 
>> > The current release locality code seems to be based on the
>> > misunderstanding that the TPM interrupts when a locality is
>> > released: it doesn't, only when the locality is acquired.
>> > 
>> > Furthermore, there seems to be no point in waiting for the locality
>> > to be released.  All it does is penalize the last TPM
>> > user.  However, if there's no next TPM user, this is a pointless
>> > wait and if there is
>> > a
>> > next TPM user, they'll pay the penalty waiting for the new locality
>> > (or possibly not if it's the same as the old locality).
>> > 
>> > Fix the code by making release_locality as simple write to release
>> > with no waiting for completion.
> [...]
>> My recollection is that this was added because there were some chips
>> that took so long to release locality that a subsequent
>> request_locality call was seeing the locality as already active,
>> moving on, and then the locality was getting released out from under
>> the user.
>
> Well, I could simply dump the interrupt code, which can never work and
> we could always poll.
>
> However, there also appears to be a bug in our locality requesting
> code.  We write the request and wait for the grant, but a grant should
> be signalled by not only the ACCESS_ACTIVE_LOCALITY being 1 but also
> the ACCESS_REQUEST_USE going to 0.  As you say, if we're slow to
> relinquish, ACCESS_ACTIVE_LOCALITY could already be 1 and we'd think we
> were granted, but ACCESS_REQUEST_USE should stay 1 until the TPM
> actually grants the next request.
>
> If I code up a fix is there any chance you still have access to a
> problem TPM?  Mine all seem to grant and release localities fairly
> instantaneously.
>
> James

Sorry, I seemed to make a mess of it. I don't have access to a system where it
occurred, but cc'ing Laurent since he reported the problem and might
still have access to the system.

I'd say fix up the check for locality request to look at
ACCESS_REQUEST_USE, and go with this patch to clean up locality release.
Hopefully Laurent still has access and can test. I do have a laptop now
where I should be able to test the other bits in your patchset since
this is one of the models that hit interrupt storm problem when Stefan's
2 patches were originally applied. Lenovo applied a fix to their bios,
but this should still have the older one version that has the issue. I'm
on PTO this week, but I will try to spend some time in the next couple
days reproducing and then trying your patches.

Regards,
Jerry

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[James Bottomley]

On Wed, 2020-09-30 at 17:01 -0700, Jerry Snitselaar wrote:
> James Bottomley @ 2020-09-30 16:03 MST:
> 
> > On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
> > > James Bottomley @ 2020-09-29 15:32 MST:
> > > 
> > > > The current release locality code seems to be based on the
> > > > misunderstanding that the TPM interrupts when a locality is
> > > > released: it doesn't, only when the locality is acquired.
> > > > 
> > > > Furthermore, there seems to be no point in waiting for the
> > > > locality to be released.  All it does is penalize the last TPM
> > > > user.  However, if there's no next TPM user, this is a
> > > > pointless wait and if there is a next TPM user, they'll pay the
> > > > penalty waiting for the new locality (or possibly not if it's
> > > > the same as the old locality).
> > > > 
> > > > Fix the code by making release_locality as simple write to
> > > > release with no waiting for completion.
> > [...]
> > > My recollection is that this was added because there were some
> > > chips that took so long to release locality that a subsequent
> > > request_locality call was seeing the locality as already active,
> > > moving on, and then the locality was getting released out from
> > > under the user.
> > 
> > Well, I could simply dump the interrupt code, which can never work
> > and we could always poll.
> > 
> > However, there also appears to be a bug in our locality requesting
> > code.  We write the request and wait for the grant, but a grant
> > should be signalled by not only the ACCESS_ACTIVE_LOCALITY being 1
> > but also the ACCESS_REQUEST_USE going to 0.  As you say, if we're
> > slow to relinquish, ACCESS_ACTIVE_LOCALITY could already be 1 and
> > we'd think we were granted, but ACCESS_REQUEST_USE should stay 1
> > until the TPM actually grants the next request.
> > 
> > If I code up a fix is there any chance you still have access to a
> > problem TPM?  Mine all seem to grant and release localities fairly
> > instantaneously.
> > 
> > James
> 
> Sorry, I seemed to make a mess of it. I don't have access to a system
> where it occurred, but cc'ing Laurent since he reported the problem
> and might still have access to the system.
> 
> I'd say fix up the check for locality request to look at
> ACCESS_REQUEST_USE, and go with this patch to clean up locality
> release. Hopefully Laurent still has access and can test. I do have a
> laptop now where I should be able to test the other bits in your
> patchset since this is one of the models that hit interrupt storm
> problem when Stefan's 2 patches were originally applied. Lenovo
> applied a fix to their bios, but this should still have the older one
> version that has the issue. I'm on PTO this week, but I will try to
> spend some time in the next couple days reproducing and then trying
> your patches.

Thanks.  I think the patch to fix to request access is very simple ...
it's just to check the request bit has gone to zero, so I've attached
it below.  It seems to work fine for me.

James

---

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 0a86cf392466..5e56e8c67791 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -168,7 +168,8 @@ static bool check_locality(struct tpm_chip *chip, int l)
 	if (rc < 0)
 		return false;
 
-	if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
+	if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID
+		       | TPM_ACCESS_REQUEST_USE)) ==
 	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
 		priv->locality = l;
 		return true;

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Laurent Bigonville]

Le 1/10/20 à 17:58, James Bottomley a écrit :
Hello,
> On Wed, 2020-09-30 at 17:01 -0700, Jerry Snitselaar wrote:
>> James Bottomley @ 2020-09-30 16:03 MST:
>>
>>> On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
>>>> James Bottomley @ 2020-09-29 15:32 MST:
>>>>
>>>>> The current release locality code seems to be based on the
>>>>> misunderstanding that the TPM interrupts when a locality is
>>>>> released: it doesn't, only when the locality is acquired.
>>>>>
>>>>> Furthermore, there seems to be no point in waiting for the
>>>>> locality to be released.  All it does is penalize the last TPM
>>>>> user.  However, if there's no next TPM user, this is a
>>>>> pointless wait and if there is a next TPM user, they'll pay the
>>>>> penalty waiting for the new locality (or possibly not if it's
>>>>> the same as the old locality).
>>>>>
>>>>> Fix the code by making release_locality as simple write to
>>>>> release with no waiting for completion.
>>> [...]
>>>> My recollection is that this was added because there were some
>>>> chips that took so long to release locality that a subsequent
>>>> request_locality call was seeing the locality as already active,
>>>> moving on, and then the locality was getting released out from
>>>> under the user.
>>> Well, I could simply dump the interrupt code, which can never work
>>> and we could always poll.
>>>
>>> However, there also appears to be a bug in our locality requesting
>>> code.  We write the request and wait for the grant, but a grant
>>> should be signalled by not only the ACCESS_ACTIVE_LOCALITY being 1
>>> but also the ACCESS_REQUEST_USE going to 0.  As you say, if we're
>>> slow to relinquish, ACCESS_ACTIVE_LOCALITY could already be 1 and
>>> we'd think we were granted, but ACCESS_REQUEST_USE should stay 1
>>> until the TPM actually grants the next request.
>>>
>>> If I code up a fix is there any chance you still have access to a
>>> problem TPM?  Mine all seem to grant and release localities fairly
>>> instantaneously.
>>>
>>> James
>> Sorry, I seemed to make a mess of it. I don't have access to a system
>> where it occurred, but cc'ing Laurent since he reported the problem
>> and might still have access to the system.
>>
>> I'd say fix up the check for locality request to look at
>> ACCESS_REQUEST_USE, and go with this patch to clean up locality
>> release. Hopefully Laurent still has access and can test. I do have a
>> laptop now where I should be able to test the other bits in your
>> patchset since this is one of the models that hit interrupt storm
>> problem when Stefan's 2 patches were originally applied. Lenovo
>> applied a fix to their bios, but this should still have the older one
>> version that has the issue. I'm on PTO this week, but I will try to
>> spend some time in the next couple days reproducing and then trying
>> your patches.
> Thanks.  I think the patch to fix to request access is very simple ...
> it's just to check the request bit has gone to zero, so I've attached
> it below.  It seems to work fine for me.
>
Sorry for the (really) late answer. I still do have access to the same 
system. Do you still need something from me?

But I do have two issues with the tpm chip on that system (probably not 
related to the discussion you were having here) so I'm not sure I will 
be able to easily test that everything is working:

1) The machine is in dualboot with windows 10 and for some reasons, 
every time I'm rebooting between linux and windows the chip is locking 
itself. AFAICS, when rebooting windows multiple time it's not happening. 
And the grace period is around 36h...

2) I just updated to 5.10 today (debian updated the kernel in unstable) 
and I get a WARNING when the tpm_tis module is being loaded:

kernel: ------------[ cut here ]------------
kernel: TPM returned invalid status
kernel: WARNING: CPU: 3 PID: 443 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0x86/0xa0 [tpm_tis_core]
kernel: Modules linked in: tpm_tis(+) tpm_tis_core tpm asus_atk0110 rng_core evdev loop(+) firewire_sbp2 msr parport_pc sunrpc ppdev lp parport fuse configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c crc32c_generic raid1 raid0 >
kernel: CPU: 3 PID: 443 Comm: systemd-udevd Tainted: G          I       5.10.0-1-amd64 #1 Debian 5.10.4-1
kernel: Hardware name: System manufacturer System Product Name/P6T DELUXE V2, BIOS 0406    04/24/2009
kernel: RIP: 0010:tpm_tis_status+0x86/0xa0 [tpm_tis_core]
kernel: Code: 00 75 30 48 83 c4 18 c3 31 c0 80 3d e3 48 00 00 00 75 e0 48 c7 c7 4c 83 18 c1 88 44 24 07 c6 05 cf 48 00 00 01 e8 9a 57 ce fc <0f> 0b 0f b6 44 24 07 eb c0 e8 bc ca d1 fc 66 66 2e 0f 1f 84 00 00
kernel: RSP: 0018:ffffb98b0076faa0 EFLAGS: 00010286
kernel: RAX: 0000000000000000 RBX: ffff8dc1892a1000 RCX: ffff8dc52dad8a08
kernel: RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff8dc52dad8a00
kernel: RBP: 00000000ffff5d8a R08: 0000000000000000 R09: ffffb98b0076f8c0
kernel: R10: ffffb98b0076f8b8 R11: ffffffffbe8cb268 R12: 0000000000000016
kernel: R13: ffff8dc183bec000 R14: 0000000000001000 R15: ffffb98b0076fada
kernel: FS:  00007f6916f658c0(0000) GS:ffff8dc52dac0000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 00007f6915e1ee38 CR3: 00000001205ee000 CR4: 00000000000006e0
kernel: Call Trace:
kernel:  tpm_transmit+0x15f/0x3d0 [tpm]
kernel:  tpm_transmit_cmd+0x25/0x90 [tpm]
kernel:  tpm2_probe+0xe2/0x140 [tpm]
kernel:  tpm_tis_core_init+0x1d5/0x2b0 [tpm_tis_core]
kernel:  ? tpm_tis_init.part.0+0x130/0x130 [tpm_tis]
kernel:  tpm_tis_pnp_init+0xe1/0x110 [tpm_tis]
kernel:  pnp_device_probe+0xaf/0x140
kernel:  really_probe+0x205/0x460
kernel:  driver_probe_device+0xe1/0x150
kernel:  device_driver_attach+0xa1/0xb0
kernel:  __driver_attach+0x8a/0x150
kernel:  ? device_driver_attach+0xb0/0xb0
kernel:  ? device_driver_attach+0xb0/0xb0
kernel:  bus_for_each_dev+0x78/0xc0
kernel:  bus_add_driver+0x12b/0x1e0
kernel:  driver_register+0x8b/0xe0
kernel:  ? 0xffffffffc1193000
kernel:  init_tis+0xa0/0x1000 [tpm_tis]
kernel:  do_one_initcall+0x44/0x1d0
kernel:  ? do_init_module+0x23/0x250
kernel:  ? kmem_cache_alloc_trace+0xf5/0x200
kernel:  do_init_module+0x5c/0x250
kernel:  __do_sys_finit_module+0xb1/0x110
kernel:  do_syscall_64+0x33/0x80
kernel:  entry_SYSCALL_64_after_hwframe+0x44/0xa9
kernel: RIP: 0033:0x7f691741f959
kernel: Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 07 55 0c 00 f7 d8 64 89 01 48
kernel: RSP: 002b:00007ffe0c62a958 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
kernel: RAX: ffffffffffffffda RBX: 00005630aad85910 RCX: 00007f691741f959
kernel: RDX: 0000000000000000 RSI: 00007f69175aae4d RDI: 0000000000000012
kernel: RBP: 0000000000020000 R08: 0000000000000000 R09: 00005630aad71d88
kernel: R10: 0000000000000012 R11: 0000000000000246 R12: 00007f69175aae4d
kernel: R13: 0000000000000000 R14: 00005630aad864f0 R15: 00005630aad85910
kernel: ---[ end trace 3dd14c12be7cbb7c ]---
kernel: tpm_tis 00:06: 1.2 TPM (device-id 0x6871, rev-id 1)

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[Jarkko Sakkinen]

On Wed, Sep 30, 2020 at 04:03:25PM -0700, James Bottomley wrote:
> On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
> > James Bottomley @ 2020-09-29 15:32 MST:
> > 
> > > The current release locality code seems to be based on the
> > > misunderstanding that the TPM interrupts when a locality is
> > > released: it doesn't, only when the locality is acquired.
> > > 
> > > Furthermore, there seems to be no point in waiting for the locality
> > > to be released.  All it does is penalize the last TPM
> > > user.  However, if there's no next TPM user, this is a pointless
> > > wait and if there is
> > > a
> > > next TPM user, they'll pay the penalty waiting for the new locality
> > > (or possibly not if it's the same as the old locality).
> > > 
> > > Fix the code by making release_locality as simple write to release
> > > with no waiting for completion.
> [...]
> > My recollection is that this was added because there were some chips
> > that took so long to release locality that a subsequent
> > request_locality call was seeing the locality as already active,
> > moving on, and then the locality was getting released out from under
> > the user.
> 
> Well, I could simply dump the interrupt code, which can never work and
> we could always poll.

Side-topic: What is the benefit of using int's in a TPM driver anyway? I
have never had any interest to dive into this with tpm_crb because I
don't have the answer.

*Perhaps* in some smallest form factor battery run devices you could get
some gain in run-time power saving but usually in such situations you
use something similar to TEE to do a measured boot.

/Jarkko

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[James Bottomley]

On Thu, 2020-10-01 at 05:01 +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 30, 2020 at 04:03:25PM -0700, James Bottomley wrote:
> > On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
> > > James Bottomley @ 2020-09-29 15:32 MST:
> > > 
> > > > The current release locality code seems to be based on the
> > > > misunderstanding that the TPM interrupts when a locality is
> > > > released: it doesn't, only when the locality is acquired.
> > > > 
> > > > Furthermore, there seems to be no point in waiting for the
> > > > locality to be released.  All it does is penalize the last TPM
> > > > user.  However, if there's no next TPM user, this is a
> > > > pointless wait and if there is a next TPM user, they'll pay the
> > > > penalty waiting for the new locality (or possibly not if it's
> > > > the same as the old locality).
> > > > 
> > > > Fix the code by making release_locality as simple write to
> > > > release with no waiting for completion.
> > [...]
> > > My recollection is that this was added because there were some
> > > chips that took so long to release locality that a subsequent
> > > request_locality call was seeing the locality as already active,
> > > moving on, and then the locality was getting released out from
> > > under the user.
> > 
> > Well, I could simply dump the interrupt code, which can never work
> > and we could always poll.
> 
> Side-topic: What is the benefit of using int's in a TPM driver
> anyway? I have never had any interest to dive into this with tpm_crb
> because I don't have the answer.

polling for events that don't immediately happen is a huge waste of
time.  That's why interrupts were invented in the first place.  If you
poll too fast, you consume wakeups which are really expensive to idle
time and if you poll too slowly you wait too long and your throughput
really tanks.  For stuff like disk and network transfers interrupts are
basically essential.  For less high volume stuff, like the TPM, we can
get away with polling, but it's hugely suboptimal if you have a large
number of events to get through ... like updating the IMA log.

> *Perhaps* in some smallest form factor battery run devices you could
> get some gain in run-time power saving but usually in such situations
> you use something similar to TEE to do a measured boot.

It's not about power saving, it's about doing stuff at the right time.

James

Re: [PATCH 1/4] tpm_tis: Clean up locality release

[James Bottomley]

On Wed, 2020-09-30 at 21:49 -0700, James Bottomley wrote:
> On Thu, 2020-10-01 at 05:01 +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 30, 2020 at 04:03:25PM -0700, James Bottomley wrote:
> > > On Wed, 2020-09-30 at 14:19 -0700, Jerry Snitselaar wrote:
> > > > James Bottomley @ 2020-09-29 15:32 MST:
> > > > 
> > > > > The current release locality code seems to be based on the
> > > > > misunderstanding that the TPM interrupts when a locality is
> > > > > released: it doesn't, only when the locality is acquired.
> > > > > 
> > > > > Furthermore, there seems to be no point in waiting for the
> > > > > locality to be released.  All it does is penalize the last
> > > > > TPM user.  However, if there's no next TPM user, this is a
> > > > > pointless wait and if there is a next TPM user, they'll pay
> > > > > the penalty waiting for the new locality (or possibly not if
> > > > > it's the same as the old locality).
> > > > > 
> > > > > Fix the code by making release_locality as simple write to
> > > > > release with no waiting for completion.
> > > [...]
> > > > My recollection is that this was added because there were some
> > > > chips that took so long to release locality that a subsequent
> > > > request_locality call was seeing the locality as already
> > > > active, moving on, and then the locality was getting released
> > > > out from under the user.
> > > 
> > > Well, I could simply dump the interrupt code, which can never
> > > work and we could always poll.
> > 
> > Side-topic: What is the benefit of using int's in a TPM driver
> > anyway? I have never had any interest to dive into this with
> > tpm_crb because I don't have the answer.
> 
> polling for events that don't immediately happen is a huge waste of
> time.  That's why interrupts were invented in the first place.  If
> you poll too fast, you consume wakeups which are really expensive to
> idle time and if you poll too slowly you wait too long and your
> throughput really tanks.  For stuff like disk and network transfers
> interrupts are basically essential.  For less high volume stuff, like
> the TPM, we can get away with polling, but it's hugely suboptimal if
> you have a large number of events to get through ... like updating
> the IMA log.

I suppose I should also add that for annoying TPMs that crash if you
poll too often, like the Atmel and my Nuvoton, using interrupts would
be a huge facilitator because you only touch the status register when
you know something has changed and the TPM is expecting you to check.  
Not that this will actually help me: my ACPI tables imply my TPM has no
interrupt line, unfortunately.

James

[PATCH 2/4] tpm_tis: Fix interrupts for TIS TPMs without legacy cycles

[James Bottomley]

If a TIS TPM doesn't have legacy cycles, any write to the interrupt
registers is ignored unless a locality is active.  This means even to
set up the interrupt vectors a locality must first be activated.  Fix
this by activating the 0 locality in the interrupt probe setup.

Since the TPM_EOI signalling also requires an active locality, the
interrupt routine cannot end an interrupt if the locality is released.
This can lead to a situation where the TPM goes command ready after
locality release and since the interrupt cannot be ended it refires
continuously.  Fix this by disabling all interrupts except locality
change when a locality is released (this only fires when a locality
becomes active, meaning the TPM_EOI should work).

Finally, since we now disable all status based interrupts in the
locality release, they must be re-enabled before waiting to check the
condition, so add interrupt enabling to the status wait.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------
 1 file changed, 101 insertions(+), 24 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index a9fa40714c64..02cc384fdaea 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -29,6 +29,46 @@
 
 static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
 
+static void enable_interrupt(struct tpm_chip *chip, u8 mask)
+{
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	u32 intmask;
+
+	/* Take control of the TPM's interrupt hardware and shut it off */
+	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
+
+	intmask |= mask | TPM_GLOBAL_INT_ENABLE;
+
+	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
+}
+
+static void disable_interrupt(struct tpm_chip *chip, u8 mask)
+{
+	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	u32 intmask;
+
+	/* Take control of the TPM's interrupt hardware and shut it off */
+	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
+
+	intmask &= ~mask;
+
+	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
+}
+
+static void enable_stat_interrupt(struct tpm_chip *chip, u8 stat)
+{
+	u32 mask = 0;
+
+	if (stat & TPM_STS_COMMAND_READY)
+		mask |= TPM_INTF_CMD_READY_INT;
+	if (stat & TPM_STS_VALID)
+		mask |= TPM_INTF_STS_VALID_INT;
+	if (stat & TPM_STS_DATA_AVAIL)
+		mask |= TPM_INTF_DATA_AVAIL_INT;
+
+	enable_interrupt(chip, mask);
+}
+
 static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
 					bool check_cancel, bool *canceled)
 {
@@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
 		timeout = stop - jiffies;
 		if ((long)timeout <= 0)
 			return -ETIME;
+		enable_stat_interrupt(chip, mask);
 		rc = wait_event_interruptible_timeout(*queue,
 			wait_for_tpm_stat_cond(chip, mask, check_cancel,
 					       &canceled),
 			timeout);
 		if (rc > 0) {
+			if (rc == 1)
+				dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n");
 			if (canceled)
 				return -ECANCELED;
 			return 0;
@@ -137,6 +180,28 @@ static bool check_locality(struct tpm_chip *chip, int l)
 static int release_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
+	u32 int_status;
+	int rc;
+
+	/*
+	 * Note that once we relinquish the locality, all writes to
+	 * the interrupt registers become ineffective meaning we can't
+	 * do a TPM_EOI.  This means we must disable every interrupt
+	 * except the locality change one to avoid interrupt
+	 * storms.
+	 */
+	disable_interrupt(chip, TPM_INTF_CMD_READY_INT
+			  | TPM_INTF_STS_VALID_INT
+			  | TPM_INTF_DATA_AVAIL_INT);
+
+	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
+	if (rc < 0)
+		return rc;
+
+	/* Clear all pending */
+	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
+	if (rc < 0)
+		return rc;
 
 	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
 
@@ -163,12 +228,17 @@ static int request_locality(struct tpm_chip *chip, int l)
 		timeout = stop - jiffies;
 		if ((long)timeout <= 0)
 			return -1;
+
 		rc = wait_event_interruptible_timeout(priv->int_queue,
 						      (check_locality
 						       (chip, l)),
 						      timeout);
-		if (rc > 0)
+		if (rc > 1)
+			return l;
+		if (rc == 1) {
+			dev_info(&chip->dev, "Lost Interrupt waiting for locality\n");
 			return l;
+		}
 		if (rc == -ERESTARTSYS && freezing(current)) {
 			clear_thread_flag(TIF_SIGPENDING);
 			goto again;
@@ -464,6 +534,10 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	irq = priv->irq;
 	priv->irq = 0;
 	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+	enable_interrupt(chip, TPM_INTF_CMD_READY_INT
+			 | TPM_INTF_LOCALITY_CHANGE_INT
+			 | TPM_INTF_DATA_AVAIL_INT
+			 | TPM_INTF_STS_VALID_INT);
 	rc = tpm_tis_send_main(chip, buf, len);
 	priv->irq = irq;
 	chip->flags |= TPM_CHIP_FLAG_IRQ;
@@ -718,7 +792,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
  * irq is seen then leave the chip setup for IRQ operation, otherwise reverse
  * everything and leave in polling mode. Returns 0 on success.
  */
-static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
+static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
 				    int flags, int irq)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
@@ -752,9 +826,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	if (rc < 0)
 		return rc;
 
-	/* Turn on */
-	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
-			     intmask | TPM_GLOBAL_INT_ENABLE);
+	/*
+	 * Turn on.  The locality change interrupt is the only one
+	 * always enabled
+	 */
+	enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT);
 	if (rc < 0)
 		return rc;
 
@@ -786,7 +862,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
  * do not have ACPI/etc. We typically expect the interrupt to be declared if
  * present.
  */
-static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
+static void tpm_tis_probe_irq(struct tpm_chip *chip)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 	u8 original_int_vec;
@@ -800,11 +876,9 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
 	if (!original_int_vec) {
 		if (IS_ENABLED(CONFIG_X86))
 			for (i = 3; i <= 15; i++)
-				if (!tpm_tis_probe_irq_single(chip, intmask, 0,
-							      i))
+				if (!tpm_tis_probe_irq_single(chip, 0, i))
 					return;
-	} else if (!tpm_tis_probe_irq_single(chip, intmask, 0,
-					     original_int_vec))
+	} else if (!tpm_tis_probe_irq_single(chip, 0, original_int_vec))
 		return;
 }
 
@@ -1029,8 +1103,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		}
 
 		if (irq) {
-			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
-						 irq);
+			tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq);
 			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
 				dev_err(&chip->dev, FW_BUG
 					"TPM interrupt not working, polling instead\n");
@@ -1038,7 +1111,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 				disable_interrupts(chip);
 			}
 		} else {
-			tpm_tis_probe_irq(chip, intmask);
+			tpm_tis_probe_irq(chip);
 		}
 	}
 
@@ -1064,12 +1137,23 @@ EXPORT_SYMBOL_GPL(tpm_tis_core_init);
 static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	u32 intmask;
 	int rc;
 
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, true);
 
+	/*
+	 * must have the locality before we can enable interrupts, so
+	 * poll for the locality being ready
+	 */
+	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+	if (request_locality(chip, 0) != 0) {
+		dev_err(&chip->dev, "Failed to enable interrupts after suspend\n");
+		goto out;
+	}
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
+
+
 	/* reenable interrupts that device may have lost or
 	 * BIOS/firmware may have disabled
 	 */
@@ -1077,17 +1161,10 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 	if (rc < 0)
 		goto out;
 
-	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
-	if (rc < 0)
-		goto out;
-
-	intmask |= TPM_INTF_CMD_READY_INT
-	    | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
-	    | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
+	enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT);
 
-	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
-
-out:
+ out:
+	release_locality(chip, 0);
 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, false);
 
-- 
2.28.0

Re: [PATCH 2/4] tpm_tis: Fix interrupts for TIS TPMs without legacy cycles

[Jarkko Sakkinen]

On Tue, Sep 29, 2020 at 03:32:14PM -0700, James Bottomley wrote:
> If a TIS TPM doesn't have legacy cycles, any write to the interrupt
> registers is ignored unless a locality is active.  This means even to
> set up the interrupt vectors a locality must first be activated.  Fix
> this by activating the 0 locality in the interrupt probe setup.
> 
> Since the TPM_EOI signalling also requires an active locality, the
> interrupt routine cannot end an interrupt if the locality is released.
> This can lead to a situation where the TPM goes command ready after
> locality release and since the interrupt cannot be ended it refires
> continuously.  Fix this by disabling all interrupts except locality
> change when a locality is released (this only fires when a locality
> becomes active, meaning the TPM_EOI should work).
> 
> Finally, since we now disable all status based interrupts in the
> locality release, they must be re-enabled before waiting to check the
> condition, so add interrupt enabling to the status wait.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------
>  1 file changed, 101 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index a9fa40714c64..02cc384fdaea 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -29,6 +29,46 @@
>  
>  static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>  
> +static void enable_interrupt(struct tpm_chip *chip, u8 mask)

Even if this has not been followed before I'd prefeer that for new
functions or when modifying the signature of function they'd be
prefixed with tpm_tis_.

It is is just more practical e.g. for grepping stuff.

I'm fine with full 'interrupt' would but I also think that just
'int' would be perfectly fine. Not something I'm too opionated
about.

> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 intmask;
> +
> +	/* Take control of the TPM's interrupt hardware and shut it off */
> +	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +
> +	intmask |= mask | TPM_GLOBAL_INT_ENABLE;
> +
> +	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +}
> +
> +static void disable_interrupt(struct tpm_chip *chip, u8 mask)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 intmask;
> +
> +	/* Take control of the TPM's interrupt hardware and shut it off */
> +	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +
> +	intmask &= ~mask;
> +
> +	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +}
> +
> +static void enable_stat_interrupt(struct tpm_chip *chip, u8 stat)

I'd use plural (interrupts or ints).

> +{
> +	u32 mask = 0;
> +
> +	if (stat & TPM_STS_COMMAND_READY)
> +		mask |= TPM_INTF_CMD_READY_INT;
> +	if (stat & TPM_STS_VALID)
> +		mask |= TPM_INTF_STS_VALID_INT;
> +	if (stat & TPM_STS_DATA_AVAIL)
> +		mask |= TPM_INTF_DATA_AVAIL_INT;
> +
> +	enable_interrupt(chip, mask);
> +}
> +
>  static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>  					bool check_cancel, bool *canceled)
>  {
> @@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
>  		timeout = stop - jiffies;
>  		if ((long)timeout <= 0)
>  			return -ETIME;
> +		enable_stat_interrupt(chip, mask);
>  		rc = wait_event_interruptible_timeout(*queue,
>  			wait_for_tpm_stat_cond(chip, mask, check_cancel,
>  					       &canceled),
>  			timeout);
>  		if (rc > 0) {
> +			if (rc == 1)
> +				dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n");
>  			if (canceled)
>  				return -ECANCELED;
>  			return 0;
> @@ -137,6 +180,28 @@ static bool check_locality(struct tpm_chip *chip, int l)
>  static int release_locality(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 int_status;
> +	int rc;
> +
> +	/*
> +	 * Note that once we relinquish the locality, all writes to
> +	 * the interrupt registers become ineffective meaning we can't
> +	 * do a TPM_EOI.  This means we must disable every interrupt
> +	 * except the locality change one to avoid interrupt
> +	 * storms.
> +	 */
> +	disable_interrupt(chip, TPM_INTF_CMD_READY_INT
> +			  | TPM_INTF_STS_VALID_INT
> +			  | TPM_INTF_DATA_AVAIL_INT);
> +
> +	rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* Clear all pending */
> +	rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status);
> +	if (rc < 0)
> +		return rc;
>  
>  	tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY);
>  
> @@ -163,12 +228,17 @@ static int request_locality(struct tpm_chip *chip, int l)
>  		timeout = stop - jiffies;
>  		if ((long)timeout <= 0)
>  			return -1;
> +
>  		rc = wait_event_interruptible_timeout(priv->int_queue,
>  						      (check_locality
>  						       (chip, l)),
>  						      timeout);
> -		if (rc > 0)
> +		if (rc > 1)
> +			return l;
> +		if (rc == 1) {
> +			dev_info(&chip->dev, "Lost Interrupt waiting for locality\n");
>  			return l;
> +		}
>  		if (rc == -ERESTARTSYS && freezing(current)) {
>  			clear_thread_flag(TIF_SIGPENDING);
>  			goto again;
> @@ -464,6 +534,10 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	irq = priv->irq;
>  	priv->irq = 0;
>  	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +	enable_interrupt(chip, TPM_INTF_CMD_READY_INT
> +			 | TPM_INTF_LOCALITY_CHANGE_INT
> +			 | TPM_INTF_DATA_AVAIL_INT
> +			 | TPM_INTF_STS_VALID_INT);
>  	rc = tpm_tis_send_main(chip, buf, len);
>  	priv->irq = irq;
>  	chip->flags |= TPM_CHIP_FLAG_IRQ;
> @@ -718,7 +792,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>   * irq is seen then leave the chip setup for IRQ operation, otherwise reverse
>   * everything and leave in polling mode. Returns 0 on success.
>   */
> -static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> +static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
>  				    int flags, int irq)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> @@ -752,9 +826,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>  	if (rc < 0)
>  		return rc;
>  
> -	/* Turn on */
> -	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> -			     intmask | TPM_GLOBAL_INT_ENABLE);
> +	/*
> +	 * Turn on.  The locality change interrupt is the only one
> +	 * always enabled
> +	 */
> +	enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -786,7 +862,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>   * do not have ACPI/etc. We typically expect the interrupt to be declared if
>   * present.
>   */
> -static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
> +static void tpm_tis_probe_irq(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  	u8 original_int_vec;
> @@ -800,11 +876,9 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask)
>  	if (!original_int_vec) {
>  		if (IS_ENABLED(CONFIG_X86))
>  			for (i = 3; i <= 15; i++)
> -				if (!tpm_tis_probe_irq_single(chip, intmask, 0,
> -							      i))
> +				if (!tpm_tis_probe_irq_single(chip, 0, i))
>  					return;
> -	} else if (!tpm_tis_probe_irq_single(chip, intmask, 0,
> -					     original_int_vec))
> +	} else if (!tpm_tis_probe_irq_single(chip, 0, original_int_vec))
>  		return;
>  }
>  
> @@ -1029,8 +1103,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		}
>  
>  		if (irq) {
> -			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
> -						 irq);
> +			tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq);
>  			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
>  				dev_err(&chip->dev, FW_BUG
>  					"TPM interrupt not working, polling instead\n");
> @@ -1038,7 +1111,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  				disable_interrupts(chip);
>  			}
>  		} else {
> -			tpm_tis_probe_irq(chip, intmask);
> +			tpm_tis_probe_irq(chip);
>  		}
>  	}
>  
> @@ -1064,12 +1137,23 @@ EXPORT_SYMBOL_GPL(tpm_tis_core_init);
>  static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	u32 intmask;
>  	int rc;
>  
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, true);
>  
> +	/*
> +	 * must have the locality before we can enable interrupts, so
> +	 * poll for the locality being ready
> +	 */
> +	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +	if (request_locality(chip, 0) != 0) {
> +		dev_err(&chip->dev, "Failed to enable interrupts after suspend\n");
> +		goto out;
> +	}
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
> +
> +
>  	/* reenable interrupts that device may have lost or
>  	 * BIOS/firmware may have disabled
>  	 */
> @@ -1077,17 +1161,10 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  	if (rc < 0)
>  		goto out;
>  
> -	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -	if (rc < 0)
> -		goto out;
> -
> -	intmask |= TPM_INTF_CMD_READY_INT
> -	    | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
> -	    | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
> +	enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT);
>  
> -	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> -
> -out:
> + out:
> +	release_locality(chip, 0);
>  	if (chip->ops->clk_enable != NULL)
>  		chip->ops->clk_enable(chip, false);
>  
> -- 
> 2.28.0
> 

Agree with the change minus the cosmetic stuff that I mentioned.

/Jarko

[PATCH 3/4] tpm_tis: fix IRQ probing

[James Bottomley]

Unless the TPM_CHIP_FLAG_IRQ is set somewhere, an initial probe of the
IRQ never gets done.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm_tis_core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 02cc384fdaea..b8ab26077cb1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -808,6 +808,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
 	}
 	priv->irq = irq;
 
+	/*
+	 * note writes to the interrupt registers are only effective
+	 * when the TPM is in the active locality, so we have to
+	 * request the locality here to get the interrupt set up.
+	 * This request has no corresponding release, because the
+	 * locality will be relinquished at the end of the tpm command
+	 * that probes the interrupts
+	 */
+	if (request_locality(chip, 0) != 0) {
+		dev_err(&chip->dev, "failed to gain locality for irq probe\n");
+		return -EBUSY;
+	}
+
 	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
 			   &original_int_vec);
 	if (rc < 0)
@@ -835,6 +848,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
 		return rc;
 
 	priv->irq_tested = false;
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
-- 
2.28.0

Re: [PATCH 3/4] tpm_tis: fix IRQ probing

[Jarkko Sakkinen]

On Tue, Sep 29, 2020 at 03:32:15PM -0700, James Bottomley wrote:
> Unless the TPM_CHIP_FLAG_IRQ is set somewhere, an initial probe of the
> IRQ never gets done.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Please add a clear statement what the patch does in imperative form.

> ---
>  drivers/char/tpm/tpm_tis_core.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 02cc384fdaea..b8ab26077cb1 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -808,6 +808,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
>  	}
>  	priv->irq = irq;
>  
> +	/*
> +	 * note writes to the interrupt registers are only effective
> +	 * when the TPM is in the active locality, so we have to
> +	 * request the locality here to get the interrupt set up.
> +	 * This request has no corresponding release, because the
> +	 * locality will be relinquished at the end of the tpm command
> +	 * that probes the interrupts
> +	 */
> +	if (request_locality(chip, 0) != 0) {
> +		dev_err(&chip->dev, "failed to gain locality for irq probe\n");
> +		return -EBUSY;
> +	}
> +
>  	rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>  			   &original_int_vec);
>  	if (rc < 0)
> @@ -835,6 +848,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip,
>  		return rc;
>  
>  	priv->irq_tested = false;
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
>  
>  	/* Generate an interrupt by having the core call through to
>  	 * tpm_tis_send
> -- 
> 2.28.0
> 

Agree with the code change tho.

/Jarkko

[PATCH 4/4] Revert "tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's""

[James Bottomley]

Revert the patch aa4a63dd9816 which stops interrupt probing from
working, now that it should be safe to allow interrupt probing on all
systems without incurring interrupt storms.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm_tis_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index b8ab26077cb1..0a86cf392466 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1116,6 +1116,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 			goto out_err;
 		}
 
+		tpm_chip_start(chip);
 		if (irq) {
 			tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq);
 			if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
@@ -1127,6 +1128,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		} else {
 			tpm_tis_probe_irq(chip);
 		}
+		tpm_chip_stop(chip);
 	}
 
 	rc = tpm_chip_register(chip);
-- 
2.28.0

Re: [PATCH 4/4] Revert "tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's""

[Jarkko Sakkinen]

On Tue, Sep 29, 2020 at 03:32:16PM -0700, James Bottomley wrote:
> Revert the patch aa4a63dd9816 which stops interrupt probing from
> working, now that it should be safe to allow interrupt probing on all
> systems without incurring interrupt storms.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

I will give this the review tag once the 1-3 have it.

/Jarkko