[v2] tpm_tis: reserve chip for duration of tpm_tis_core_init

Commit Message

Jerry Snitselaar Dec. 11, 2019, 11:54 p.m. UTC

Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
issuing commands to the tpm during initialization, just reserve the
chip after wait_startup, and release it when we are ready to call
tpm_chip_register.

Cc: Christian Bundy <[e-mail]>
Cc: Dan Williams <[e-mail]>
Cc: Peter Huewe <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Jason Gunthorpe <[e-mail]>
Cc: Stefan Berger <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
Signed-off-by: Jerry Snitselaar <[e-mail]>
---
v2: Fix integrity list address and add 5b359c7c4372 to fixes

 drivers/char/tpm/tpm_tis_core.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Dan Williams Dec. 12, 2019, 2:15 a.m. UTC | #1

On Wed, Dec 11, 2019 at 3:56 PM Jerry Snitselaar <[e-mail]> wrote:
>
> Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> issuing commands to the tpm during initialization, just reserve the
> chip after wait_startup, and release it when we are ready to call
> tpm_chip_register.
>
> Cc: Christian Bundy <[e-mail]>
> Cc: Dan Williams <[e-mail]>
> Cc: Peter Huewe <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Jason Gunthorpe <[e-mail]>
> Cc: Stefan Berger <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

Ugh, sorry, I guess this jinxed it. This patch does not address the
IRQ storm on the platform I reported earlier.

Dan Williams Dec. 12, 2019, 2:18 a.m. UTC | #2

On Wed, Dec 11, 2019 at 6:15 PM Dan Williams <[e-mail]> wrote:
>
> On Wed, Dec 11, 2019 at 3:56 PM Jerry Snitselaar <[e-mail]> wrote:
> >
> > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > issuing commands to the tpm during initialization, just reserve the
> > chip after wait_startup, and release it when we are ready to call
> > tpm_chip_register.
> >
> > Cc: Christian Bundy <[e-mail]>
> > Cc: Dan Williams <[e-mail]>
> > Cc: Peter Huewe <[e-mail]>
> > Cc: Jarkko Sakkinen <[e-mail]>
> > Cc: Jason Gunthorpe <[e-mail]>
> > Cc: Stefan Berger <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>
> Ugh, sorry, I guess this jinxed it. This patch does not address the
> IRQ storm on the platform I reported earlier.

Are the reverts making their way upstream?

Jarkko Sakkinen Dec. 17, 2019, 12:58 a.m. UTC | #3

On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> issuing commands to the tpm during initialization, just reserve the
> chip after wait_startup, and release it when we are ready to call
> tpm_chip_register.
> 
> Cc: Christian Bundy <[e-mail]>
> Cc: Dan Williams <[e-mail]>
> Cc: Peter Huewe <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Jason Gunthorpe <[e-mail]>
> Cc: Stefan Berger <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> Signed-off-by: Jerry Snitselaar <[e-mail]>

I pushed to my master with minor tweaks and added my tags.

Please check before I put it to linux-next.

/Jarkko

Dan Williams Dec. 17, 2019, 1:18 a.m. UTC | #4

On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > issuing commands to the tpm during initialization, just reserve the
> > chip after wait_startup, and release it when we are ready to call
> > tpm_chip_register.
> >
> > Cc: Christian Bundy <[e-mail]>
> > Cc: Dan Williams <[e-mail]>
> > Cc: Peter Huewe <[e-mail]>
> > Cc: Jarkko Sakkinen <[e-mail]>
> > Cc: Jason Gunthorpe <[e-mail]>
> > Cc: Stefan Berger <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > Signed-off-by: Jerry Snitselaar <[e-mail]>
>
> I pushed to my master with minor tweaks and added my tags.
>
> Please check before I put it to linux-next.

I don't see it yet here:

http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master

However, I wanted to make sure you captured that this does *not* fix
the interrupt issue. I.e. make sure you remove the "Fixes:
5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
tag.

With that said, are you going to include the revert of:

1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

...in your -rc3 pull?

Jarkko Sakkinen Dec. 17, 2019, 1:19 a.m. UTC | #5

On Tue, 2019-12-17 at 02:58 +0200, Jarkko Sakkinen wrote:
> On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > issuing commands to the tpm during initialization, just reserve the
> > chip after wait_startup, and release it when we are ready to call
> > tpm_chip_register.
> > 
> > Cc: Christian Bundy <[e-mail]>
> > Cc: Dan Williams <[e-mail]>
> > Cc: Peter Huewe <[e-mail]>
> > Cc: Jarkko Sakkinen <[e-mail]>
> > Cc: Jason Gunthorpe <[e-mail]>
> > Cc: Stefan Berger <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > Signed-off-by: Jerry Snitselaar <[e-mail]>
> 
> I pushed to my master with minor tweaks and added my tags.
> 
> Please check before I put it to linux-next.

Oops. Forgot to push please check now

git://git.infradead.org/users/jjs/linux-tpmdd.git master

/Jarkko

Jarkko Sakkinen Dec. 17, 2019, 1:25 a.m. UTC | #6

On Wed, 2019-12-11 at 18:18 -0800, Dan Williams wrote:
> On Wed, Dec 11, 2019 at 6:15 PM Dan Williams <[e-mail]> wrote:
> > On Wed, Dec 11, 2019 at 3:56 PM Jerry Snitselaar <[e-mail]> wrote:
> > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > > issuing commands to the tpm during initialization, just reserve the
> > > chip after wait_startup, and release it when we are ready to call
> > > tpm_chip_register.
> > > 
> > > Cc: Christian Bundy <[e-mail]>
> > > Cc: Dan Williams <[e-mail]>
> > > Cc: Peter Huewe <[e-mail]>
> > > Cc: Jarkko Sakkinen <[e-mail]>
> > > Cc: Jason Gunthorpe <[e-mail]>
> > > Cc: Stefan Berger <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > 
> > Ugh, sorry, I guess this jinxed it. This patch does not address the
> > IRQ storm on the platform I reported earlier.
> 
> Are the reverts making their way upstream?

Not yet.

Cannot randomly apply patches without answer to why. Given that
some changes are already landed changes it would be better to
create a patch based on reverts (in the sense of code change)
and commit message what is going on.

/Jarkko

Jerry Snitselaar Dec. 17, 2019, 2 a.m. UTC | #7

On Mon Dec 16 19, Dan Williams wrote:
>On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
><[e-mail]> wrote:
>>
>> On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
>> > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
>> > issuing commands to the tpm during initialization, just reserve the
>> > chip after wait_startup, and release it when we are ready to call
>> > tpm_chip_register.
>> >
>> > Cc: Christian Bundy <[e-mail]>
>> > Cc: Dan Williams <[e-mail]>
>> > Cc: Peter Huewe <[e-mail]>
>> > Cc: Jarkko Sakkinen <[e-mail]>
>> > Cc: Jason Gunthorpe <[e-mail]>
>> > Cc: Stefan Berger <[e-mail]>
>> > Cc: [e-mail]
>> > Cc: [e-mail]
>> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>> > Signed-off-by: Jerry Snitselaar <[e-mail]>
>>
>> I pushed to my master with minor tweaks and added my tags.
>>
>> Please check before I put it to linux-next.
>
>I don't see it yet here:
>
>http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
>
>However, I wanted to make sure you captured that this does *not* fix
>the interrupt issue. I.e. make sure you remove the "Fixes:
>5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
>tag.
>
>With that said, are you going to include the revert of:
>
>1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts

Dan, with the above reverted do you still get the screaming interrupt?

>5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
>
>...in your -rc3 pull?
>

Dan Williams Dec. 17, 2019, 2:14 a.m. UTC | #8

On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
>
> On Mon Dec 16 19, Dan Williams wrote:
> >On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
> ><[e-mail]> wrote:
> >>
> >> On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> >> > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> >> > issuing commands to the tpm during initialization, just reserve the
> >> > chip after wait_startup, and release it when we are ready to call
> >> > tpm_chip_register.
> >> >
> >> > Cc: Christian Bundy <[e-mail]>
> >> > Cc: Dan Williams <[e-mail]>
> >> > Cc: Peter Huewe <[e-mail]>
> >> > Cc: Jarkko Sakkinen <[e-mail]>
> >> > Cc: Jason Gunthorpe <[e-mail]>
> >> > Cc: Stefan Berger <[e-mail]>
> >> > Cc: [e-mail]
> >> > Cc: [e-mail]
> >> > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> >> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> >> > Signed-off-by: Jerry Snitselaar <[e-mail]>
> >>
> >> I pushed to my master with minor tweaks and added my tags.
> >>
> >> Please check before I put it to linux-next.
> >
> >I don't see it yet here:
> >
> >http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
> >
> >However, I wanted to make sure you captured that this does *not* fix
> >the interrupt issue. I.e. make sure you remove the "Fixes:
> >5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
> >tag.
> >
> >With that said, are you going to include the revert of:
> >
> >1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>
> Dan, with the above reverted do you still get the screaming interrupt?

Yes, the screaming interrupt goes away, although it is replaced by
these messages when the driver starts:

[    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
[    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
[    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
polling instead

If the choice is "error message + polled-mode" vs "pinning a cpu with
interrupts" I'd accept the former, but wanted Jarkko with his
maintainer hat to weigh in.

Is there a simple sanity check I can run to see if the TPM is still
operational in this state?

Jarkko Sakkinen Dec. 17, 2019, 12:05 p.m. UTC | #9

On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
> On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
> > On Mon Dec 16 19, Dan Williams wrote:
> > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
> > > <[e-mail]> wrote:
> > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > > > > issuing commands to the tpm during initialization, just reserve the
> > > > > chip after wait_startup, and release it when we are ready to call
> > > > > tpm_chip_register.
> > > > > 
> > > > > Cc: Christian Bundy <[e-mail]>
> > > > > Cc: Dan Williams <[e-mail]>
> > > > > Cc: Peter Huewe <[e-mail]>
> > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > Cc: Jason Gunthorpe <[e-mail]>
> > > > > Cc: Stefan Berger <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
> > > > 
> > > > I pushed to my master with minor tweaks and added my tags.
> > > > 
> > > > Please check before I put it to linux-next.
> > > 
> > > I don't see it yet here:
> > > 
> > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
> > > 
> > > However, I wanted to make sure you captured that this does *not* fix
> > > the interrupt issue. I.e. make sure you remove the "Fixes:
> > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
> > > tag.
> > > 
> > > With that said, are you going to include the revert of:
> > > 
> > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > 
> > Dan, with the above reverted do you still get the screaming interrupt?
> 
> Yes, the screaming interrupt goes away, although it is replaced by
> these messages when the driver starts:
> 
> [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
> [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
> [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
> polling instead
> 
> If the choice is "error message + polled-mode" vs "pinning a cpu with
> interrupts" I'd accept the former, but wanted Jarkko with his
> maintainer hat to weigh in.
> 
> Is there a simple sanity check I can run to see if the TPM is still
> operational in this state?

What about T490S?

/Jarkko

Jerry Snitselaar Dec. 17, 2019, 5:18 p.m. UTC | #10

On Tue Dec 17 19, Jarkko Sakkinen wrote:
>On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
>> On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
>> > On Mon Dec 16 19, Dan Williams wrote:
>> > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
>> > > <[e-mail]> wrote:
>> > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
>> > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
>> > > > > issuing commands to the tpm during initialization, just reserve the
>> > > > > chip after wait_startup, and release it when we are ready to call
>> > > > > tpm_chip_register.
>> > > > >
>> > > > > Cc: Christian Bundy <[e-mail]>
>> > > > > Cc: Dan Williams <[e-mail]>
>> > > > > Cc: Peter Huewe <[e-mail]>
>> > > > > Cc: Jarkko Sakkinen <[e-mail]>
>> > > > > Cc: Jason Gunthorpe <[e-mail]>
>> > > > > Cc: Stefan Berger <[e-mail]>
>> > > > > Cc: [e-mail]
>> > > > > Cc: [e-mail]
>> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>> > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
>> > > >
>> > > > I pushed to my master with minor tweaks and added my tags.
>> > > >
>> > > > Please check before I put it to linux-next.
>> > >
>> > > I don't see it yet here:
>> > >
>> > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
>> > >
>> > > However, I wanted to make sure you captured that this does *not* fix
>> > > the interrupt issue. I.e. make sure you remove the "Fixes:
>> > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
>> > > tag.
>> > >
>> > > With that said, are you going to include the revert of:
>> > >
>> > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>> >
>> > Dan, with the above reverted do you still get the screaming interrupt?
>>
>> Yes, the screaming interrupt goes away, although it is replaced by
>> these messages when the driver starts:
>>
>> [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
>> [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
>> [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
>> polling instead
>>
>> If the choice is "error message + polled-mode" vs "pinning a cpu with
>> interrupts" I'd accept the former, but wanted Jarkko with his
>> maintainer hat to weigh in.
>>
>> Is there a simple sanity check I can run to see if the TPM is still
>> operational in this state?
>
>What about T490S?
>
>/Jarkko
>

Hi Jarkko, I'm waiting to hear back from the t490s user, but I imagine
it still has the problem as well.

Christian, were you able to try this patch and verify it still
resolves the issue you were having with the kernel failing to get the
timeouts and durations from the tpm?

Thanks,
Jerry

Jerry Snitselaar Dec. 17, 2019, 5:26 p.m. UTC | #11

On Tue Dec 17 19, Jerry Snitselaar wrote:
>On Tue Dec 17 19, Jarkko Sakkinen wrote:
>>On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
>>>On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
>>>> On Mon Dec 16 19, Dan Williams wrote:
>>>> > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
>>>> > <[e-mail]> wrote:
>>>> > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
>>>> > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
>>>> > > > issuing commands to the tpm during initialization, just reserve the
>>>> > > > chip after wait_startup, and release it when we are ready to call
>>>> > > > tpm_chip_register.
>>>> > > >
>>>> > > > Cc: Christian Bundy <[e-mail]>
>>>> > > > Cc: Dan Williams <[e-mail]>
>>>> > > > Cc: Peter Huewe <[e-mail]>
>>>> > > > Cc: Jarkko Sakkinen <[e-mail]>
>>>> > > > Cc: Jason Gunthorpe <[e-mail]>
>>>> > > > Cc: Stefan Berger <[e-mail]>
>>>> > > > Cc: [e-mail]
>>>> > > > Cc: [e-mail]
>>>> > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>>>> > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>>>> > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
>>>> > >
>>>> > > I pushed to my master with minor tweaks and added my tags.
>>>> > >
>>>> > > Please check before I put it to linux-next.
>>>> >
>>>> > I don't see it yet here:
>>>> >
>>>> > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
>>>> >
>>>> > However, I wanted to make sure you captured that this does *not* fix
>>>> > the interrupt issue. I.e. make sure you remove the "Fixes:
>>>> > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
>>>> > tag.
>>>> >
>>>> > With that said, are you going to include the revert of:
>>>> >
>>>> > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>>>>
>>>> Dan, with the above reverted do you still get the screaming interrupt?
>>>
>>>Yes, the screaming interrupt goes away, although it is replaced by
>>>these messages when the driver starts:
>>>
>>>[    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
>>>[    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
>>>[    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
>>>polling instead
>>>
>>>If the choice is "error message + polled-mode" vs "pinning a cpu with
>>>interrupts" I'd accept the former, but wanted Jarkko with his
>>>maintainer hat to weigh in.
>>>
>>>Is there a simple sanity check I can run to see if the TPM is still
>>>operational in this state?
>>
>>What about T490S?
>>
>>/Jarkko
>>
>
>Hi Jarkko, I'm waiting to hear back from the t490s user, but I imagine
>it still has the problem as well.
>

It appears he is out of the office until January, so I doubt I will hear
anything back until then.

>Christian, were you able to try this patch and verify it still
>resolves the issue you were having with the kernel failing to get the
>timeouts and durations from the tpm?
>
>Thanks,
>Jerry

Jerry Snitselaar Dec. 17, 2019, 8:29 p.m. UTC | #12

On Tue Dec 17 19, Jarkko Sakkinen wrote:
>On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
>> On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
>> > On Mon Dec 16 19, Dan Williams wrote:
>> > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
>> > > <[e-mail]> wrote:
>> > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
>> > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
>> > > > > issuing commands to the tpm during initialization, just reserve the
>> > > > > chip after wait_startup, and release it when we are ready to call
>> > > > > tpm_chip_register.
>> > > > >
>> > > > > Cc: Christian Bundy <[e-mail]>
>> > > > > Cc: Dan Williams <[e-mail]>
>> > > > > Cc: Peter Huewe <[e-mail]>
>> > > > > Cc: Jarkko Sakkinen <[e-mail]>
>> > > > > Cc: Jason Gunthorpe <[e-mail]>
>> > > > > Cc: Stefan Berger <[e-mail]>
>> > > > > Cc: [e-mail]
>> > > > > Cc: [e-mail]
>> > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>> > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
>> > > >
>> > > > I pushed to my master with minor tweaks and added my tags.
>> > > >
>> > > > Please check before I put it to linux-next.
>> > >
>> > > I don't see it yet here:
>> > >
>> > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
>> > >
>> > > However, I wanted to make sure you captured that this does *not* fix
>> > > the interrupt issue. I.e. make sure you remove the "Fixes:
>> > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
>> > > tag.
>> > >
>> > > With that said, are you going to include the revert of:
>> > >
>> > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>> >
>> > Dan, with the above reverted do you still get the screaming interrupt?
>>
>> Yes, the screaming interrupt goes away, although it is replaced by
>> these messages when the driver starts:
>>
>> [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
>> [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
>> [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
>> polling instead
>>
>> If the choice is "error message + polled-mode" vs "pinning a cpu with
>> interrupts" I'd accept the former, but wanted Jarkko with his
>> maintainer hat to weigh in.
>>
>> Is there a simple sanity check I can run to see if the TPM is still
>> operational in this state?
>
>What about T490S?
>
>/Jarkko
>

I just heard back from the t490s user, and as expected the problem still exists
there with this patch.

Regards,
Jerry

Jarkko Sakkinen Dec. 18, 2019, 11:06 p.m. UTC | #13

On Tue, 2019-12-17 at 10:18 -0700, Jerry Snitselaar wrote:
> On Tue Dec 17 19, Jarkko Sakkinen wrote:
> > On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
> > > On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
> > > > On Mon Dec 16 19, Dan Williams wrote:
> > > > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
> > > > > <[e-mail]> wrote:
> > > > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> > > > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > > > > > > issuing commands to the tpm during initialization, just reserve the
> > > > > > > chip after wait_startup, and release it when we are ready to call
> > > > > > > tpm_chip_register.
> > > > > > > 
> > > > > > > Cc: Christian Bundy <[e-mail]>
> > > > > > > Cc: Dan Williams <[e-mail]>
> > > > > > > Cc: Peter Huewe <[e-mail]>
> > > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > > Cc: Jason Gunthorpe <[e-mail]>
> > > > > > > Cc: Stefan Berger <[e-mail]>
> > > > > > > Cc: [e-mail]
> > > > > > > Cc: [e-mail]
> > > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > > > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
> > > > > > 
> > > > > > I pushed to my master with minor tweaks and added my tags.
> > > > > > 
> > > > > > Please check before I put it to linux-next.
> > > > > 
> > > > > I don't see it yet here:
> > > > > 
> > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
> > > > > 
> > > > > However, I wanted to make sure you captured that this does *not* fix
> > > > > the interrupt issue. I.e. make sure you remove the "Fixes:
> > > > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
> > > > > tag.
> > > > > 
> > > > > With that said, are you going to include the revert of:
> > > > > 
> > > > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > > > 
> > > > Dan, with the above reverted do you still get the screaming interrupt?
> > > 
> > > Yes, the screaming interrupt goes away, although it is replaced by
> > > these messages when the driver starts:
> > > 
> > > [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
> > > [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
> > > [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
> > > polling instead
> > > 
> > > If the choice is "error message + polled-mode" vs "pinning a cpu with
> > > interrupts" I'd accept the former, but wanted Jarkko with his
> > > maintainer hat to weigh in.
> > > 
> > > Is there a simple sanity check I can run to see if the TPM is still
> > > operational in this state?
> > 
> > What about T490S?
> > 
> > /Jarkko
> > 
> 
> Hi Jarkko, I'm waiting to hear back from the t490s user, but I imagine
> it still has the problem as well.
> 
> Christian, were you able to try this patch and verify it still
> resolves the issue you were having with the kernel failing to get the
> timeouts and durations from the tpm?

Including those reverts would be a bogus change at this point.

The fix that I already applied obviously fixes an issue even if
it does not fix all the issues.

/Jarkko

Jarkko Sakkinen Dec. 18, 2019, 11:09 p.m. UTC | #14

On Tue, 2019-12-17 at 10:26 -0700, Jerry Snitselaar wrote:
> It appears he is out of the office until January, so I doubt I will hear
> anything back until then.

Unless we have access to a platform to sanely validate a fix
before that, we will have to wait until January then.

/Jarkko

Jarkko Sakkinen Dec. 18, 2019, 11:12 p.m. UTC | #15

On Tue, 2019-12-17 at 13:29 -0700, Jerry Snitselaar wrote:
> I just heard back from the t490s user, and as expected the problem still exists
> there with this patch.

OK. Now we have to reason what still is behaving incorrectly
and create a proper fix. Is he available to test a subsequent
fix?

Since the holidays are coming it might anyway take early Jan
before we can test anything but we can still try to find the
root cause by inspection.

/Jarkko

Dan Williams Dec. 18, 2019, 11:31 p.m. UTC | #16

On Wed, Dec 18, 2019 at 3:07 PM Jarkko Sakkinen
<[e-mail]> wrote:
>
> On Tue, 2019-12-17 at 10:18 -0700, Jerry Snitselaar wrote:
> > On Tue Dec 17 19, Jarkko Sakkinen wrote:
> > > On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
> > > > On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
> > > > > On Mon Dec 16 19, Dan Williams wrote:
> > > > > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
> > > > > > <[e-mail]> wrote:
> > > > > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
> > > > > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
> > > > > > > > issuing commands to the tpm during initialization, just reserve the
> > > > > > > > chip after wait_startup, and release it when we are ready to call
> > > > > > > > tpm_chip_register.
> > > > > > > >
> > > > > > > > Cc: Christian Bundy <[e-mail]>
> > > > > > > > Cc: Dan Williams <[e-mail]>
> > > > > > > > Cc: Peter Huewe <[e-mail]>
> > > > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > > > Cc: Jason Gunthorpe <[e-mail]>
> > > > > > > > Cc: Stefan Berger <[e-mail]>
> > > > > > > > Cc: [e-mail]
> > > > > > > > Cc: [e-mail]
> > > > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> > > > > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
> > > > > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
> > > > > > >
> > > > > > > I pushed to my master with minor tweaks and added my tags.
> > > > > > >
> > > > > > > Please check before I put it to linux-next.
> > > > > >
> > > > > > I don't see it yet here:
> > > > > >
> > > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
> > > > > >
> > > > > > However, I wanted to make sure you captured that this does *not* fix
> > > > > > the interrupt issue. I.e. make sure you remove the "Fixes:
> > > > > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
> > > > > > tag.
> > > > > >
> > > > > > With that said, are you going to include the revert of:
> > > > > >
> > > > > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > > > >
> > > > > Dan, with the above reverted do you still get the screaming interrupt?
> > > >
> > > > Yes, the screaming interrupt goes away, although it is replaced by
> > > > these messages when the driver starts:
> > > >
> > > > [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
> > > > [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
> > > > [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
> > > > polling instead
> > > >
> > > > If the choice is "error message + polled-mode" vs "pinning a cpu with
> > > > interrupts" I'd accept the former, but wanted Jarkko with his
> > > > maintainer hat to weigh in.
> > > >
> > > > Is there a simple sanity check I can run to see if the TPM is still
> > > > operational in this state?
> > >
> > > What about T490S?
> > >
> > > /Jarkko
> > >
> >
> > Hi Jarkko, I'm waiting to hear back from the t490s user, but I imagine
> > it still has the problem as well.
> >
> > Christian, were you able to try this patch and verify it still
> > resolves the issue you were having with the kernel failing to get the
> > timeouts and durations from the tpm?
>
> Including those reverts would be a bogus change at this point.

I'm failing to see how you arrived at that conclusion.

> The fix that I already applied obviously fixes an issue even if
> it does not fix all the issues.

These patches take a usable system and make it unusable:

1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

...they need to be reverted, or the regression needs to be fixed, but
asserting that you fixed something else unrelated does not help.

Jerry Snitselaar Dec. 19, 2019, 10:07 a.m. UTC | #17

On Wed Dec 18 19, Dan Williams wrote:
>On Wed, Dec 18, 2019 at 3:07 PM Jarkko Sakkinen
><[e-mail]> wrote:
>>
>> On Tue, 2019-12-17 at 10:18 -0700, Jerry Snitselaar wrote:
>> > On Tue Dec 17 19, Jarkko Sakkinen wrote:
>> > > On Mon, 2019-12-16 at 18:14 -0800, Dan Williams wrote:
>> > > > On Mon, Dec 16, 2019 at 6:00 PM Jerry Snitselaar <[e-mail]> wrote:
>> > > > > On Mon Dec 16 19, Dan Williams wrote:
>> > > > > > On Mon, Dec 16, 2019 at 4:59 PM Jarkko Sakkinen
>> > > > > > <[e-mail]> wrote:
>> > > > > > > On Wed, 2019-12-11 at 16:54 -0700, Jerry Snitselaar wrote:
>> > > > > > > > Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when
>> > > > > > > > issuing commands to the tpm during initialization, just reserve the
>> > > > > > > > chip after wait_startup, and release it when we are ready to call
>> > > > > > > > tpm_chip_register.
>> > > > > > > >
>> > > > > > > > Cc: Christian Bundy <[e-mail]>
>> > > > > > > > Cc: Dan Williams <[e-mail]>
>> > > > > > > > Cc: Peter Huewe <[e-mail]>
>> > > > > > > > Cc: Jarkko Sakkinen <[e-mail]>
>> > > > > > > > Cc: Jason Gunthorpe <[e-mail]>
>> > > > > > > > Cc: Stefan Berger <[e-mail]>
>> > > > > > > > Cc: [e-mail]
>> > > > > > > > Cc: [e-mail]
>> > > > > > > > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
>> > > > > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")
>> > > > > > > > Signed-off-by: Jerry Snitselaar <[e-mail]>
>> > > > > > >
>> > > > > > > I pushed to my master with minor tweaks and added my tags.
>> > > > > > >
>> > > > > > > Please check before I put it to linux-next.
>> > > > > >
>> > > > > > I don't see it yet here:
>> > > > > >
>> > > > > > http://git.infradead.org/users/jjs/linux-tpmdd.git/shortlog/refs/heads/master
>> > > > > >
>> > > > > > However, I wanted to make sure you captured that this does *not* fix
>> > > > > > the interrupt issue. I.e. make sure you remove the "Fixes:
>> > > > > > 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")"
>> > > > > > tag.
>> > > > > >
>> > > > > > With that said, are you going to include the revert of:
>> > > > > >
>> > > > > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>> > > > >
>> > > > > Dan, with the above reverted do you still get the screaming interrupt?
>> > > >
>> > > > Yes, the screaming interrupt goes away, although it is replaced by
>> > > > these messages when the driver starts:
>> > > >
>> > > > [    3.725131] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
>> > > > [    3.725358] tpm tpm0: tpm_try_transmit: send(): error -5
>> > > > [    3.725359] tpm tpm0: [Firmware Bug]: TPM interrupt not working,
>> > > > polling instead
>> > > >
>> > > > If the choice is "error message + polled-mode" vs "pinning a cpu with
>> > > > interrupts" I'd accept the former, but wanted Jarkko with his
>> > > > maintainer hat to weigh in.
>> > > >
>> > > > Is there a simple sanity check I can run to see if the TPM is still
>> > > > operational in this state?
>> > >
>> > > What about T490S?
>> > >
>> > > /Jarkko
>> > >
>> >
>> > Hi Jarkko, I'm waiting to hear back from the t490s user, but I imagine
>> > it still has the problem as well.
>> >
>> > Christian, were you able to try this patch and verify it still
>> > resolves the issue you were having with the kernel failing to get the
>> > timeouts and durations from the tpm?
>>
>> Including those reverts would be a bogus change at this point.
>
>I'm failing to see how you arrived at that conclusion.
>
>> The fix that I already applied obviously fixes an issue even if
>> it does not fix all the issues.
>
>These patches take a usable system and make it unusable:
>
>1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
>5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
>
>...they need to be reverted, or the regression needs to be fixed, but
>asserting that you fixed something else unrelated does not help.
>

Reverting 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before
probing for interrupts") would at least allow people impacted by this
to boot their systems without disabling the tpm, or blacklisting the
module while we figure this out. From what I can tell the tpm_tis code
was operating in that state since 570a36097f30 ("tpm: drop 'irq' from
struct tpm_vendor_specific") until Stefan's patch.

Regards,
Jerry

Jarkko Sakkinen Dec. 27, 2019, 5:09 a.m. UTC | #18

On Thu, 2019-12-19 at 03:07 -0700, Jerry Snitselaar wrote:
> > These patches take a usable system and make it unusable:
> > 
> > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > 5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
> > 
> > ...they need to be reverted, or the regression needs to be fixed, but
> > asserting that you fixed something else unrelated does not help.
> > 
> 
> Reverting 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before
> probing for interrupts") would at least allow people impacted by this
> to boot their systems without disabling the tpm, or blacklisting the
> module while we figure this out. From what I can tell the tpm_tis code
> was operating in that state since 570a36097f30 ("tpm: drop 'irq' from
> struct tpm_vendor_specific") until Stefan's patch.

I'll formalize a fix based on the reverts.

Sorry for the holiday latency.

/Jarkko

Jarkko Sakkinen Dec. 27, 2019, 5:42 a.m. UTC | #19

On Fri, 2019-12-27 at 07:09 +0200, Jarkko Sakkinen wrote:
> On Thu, 2019-12-19 at 03:07 -0700, Jerry Snitselaar wrote:
> > > These patches take a usable system and make it unusable:
> > > 
> > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > > 5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
> > > 
> > > ...they need to be reverted, or the regression needs to be fixed, but
> > > asserting that you fixed something else unrelated does not help.
> > > 
> > 
> > Reverting 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before
> > probing for interrupts") would at least allow people impacted by this
> > to boot their systems without disabling the tpm, or blacklisting the
> > module while we figure this out. From what I can tell the tpm_tis code
> > was operating in that state since 570a36097f30 ("tpm: drop 'irq' from
> > struct tpm_vendor_specific") until Stefan's patch.
> 
> I'll formalize a fix based on the reverts.
> 
> Sorry for the holiday latency.

OK, have a branch now for the PR:

for-linus-v5.5-rc4

Note: now contains the first revert but I'll add another patch if required.

/Jarkko

Jarkko Sakkinen Dec. 27, 2019, 6:03 a.m. UTC | #20

On Fri, 2019-12-27 at 07:42 +0200, Jarkko Sakkinen wrote:
> On Fri, 2019-12-27 at 07:09 +0200, Jarkko Sakkinen wrote:
> > On Thu, 2019-12-19 at 03:07 -0700, Jerry Snitselaar wrote:
> > > > These patches take a usable system and make it unusable:
> > > > 
> > > > 1ea32c83c699 tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
> > > > 5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
> > > > 
> > > > ...they need to be reverted, or the regression needs to be fixed, but
> > > > asserting that you fixed something else unrelated does not help.
> > > > 
> > > 
> > > Reverting 1ea32c83c699 ("tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before
> > > probing for interrupts") would at least allow people impacted by this
> > > to boot their systems without disabling the tpm, or blacklisting the
> > > module while we figure this out. From what I can tell the tpm_tis code
> > > was operating in that state since 570a36097f30 ("tpm: drop 'irq' from
> > > struct tpm_vendor_specific") until Stefan's patch.
> > 
> > I'll formalize a fix based on the reverts.
> > 
> > Sorry for the holiday latency.
> 
> OK, have a branch now for the PR:
> 
> for-linus-v5.5-rc4
> 
> Note: now contains the first revert but I'll add another patch if required.

Jerry, can you check this and send me revert to your earlier fix *if*
required but first test with just this fix applied.

Thanks.

/Jarkko

Patch

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8af2cee1a762..308756d278b3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -978,13 +978,13 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 	if (wait_startup(chip, 0) != 0) {
 		rc = -ENODEV;
-		goto out_err;
+		goto out_start;
 	}
 
 	/* Take control of the TPM's interrupt hardware and shut it off */
 	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
 	if (rc < 0)
-		goto out_err;
+		goto out_start;
 
 	intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
 		   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
@@ -993,9 +993,8 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 
 	rc = tpm_chip_start(chip);
 	if (rc)
-		goto out_err;
+		goto out_start;
 	rc = tpm2_probe(chip);
-	tpm_chip_stop(chip);
 	if (rc)
 		goto out_err;
 
@@ -1059,7 +1058,6 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 			goto out_err;
 		}
 
-		tpm_chip_start(chip);
 		chip->flags |= TPM_CHIP_FLAG_IRQ;
 		if (irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
@@ -1070,18 +1068,17 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		} else {
 			tpm_tis_probe_irq(chip, intmask);
 		}
-		tpm_chip_stop(chip);
 	}
+	tpm_chip_stop(chip);
 
 	rc = tpm_chip_register(chip);
 	if (rc)
-		goto out_err;
-
-	if (chip->ops->clk_enable != NULL)
-		chip->ops->clk_enable(chip, false);
+		goto out_start;
 
 	return 0;
 out_err:
+	tpm_chip_stop(chip);
+out_start:
 	if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
 		chip->ops->clk_enable(chip, false);