Re: Patch "tpm_tis: reserve chip for duration of tpm_tis_core_init" has been added to the 5.4-stable tree

From: Jerry Snitselaar <jsnitsel@redhat.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Christian Bundy <christianbundy@fraction.io>,
	Jason Gunthorpe <jgg@ziepe.ca>, Peter Huewe <peterhuewe@gmx.de>,
	Stefan Berger <stefanb@linux.vnet.ibm.com>,
	stable-commits@vger.kernel.org, linux-integrity@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: Patch "tpm_tis: reserve chip for duration of tpm_tis_core_init" has been added to the 5.4-stable tree
Date: Mon, 30 Dec 2019 18:02:56 -0700	[thread overview]
Message-ID: <20191231010256.kymv4shwmx5jcmey@cantor> (raw)
In-Reply-To: <2c4a80e0d30bf1dfe89c6e3469d1dbfb008275fa.camel@linux.intel.com>

On Tue Dec 31 19, Jarkko Sakkinen wrote:
>On Sun, 2019-12-29 at 23:41 -0800, Dan Williams wrote:
>> This looked like the wrong revert to me, and testing confirms that
>> this does not fix the problem.
>>
>> As I mentioned in the original report [1] the commit that bisect flagged was:
>>
>>     5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's
>>
>> That commit moved tpm_chip_start() before irq probing. Commit
>> 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init"
>> does not appear to change anything in that regard.
>>
>> Perhaps this hardware has always had broken interrupts and needs to be
>> quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0
>> workaround.
>>
>>
>> [1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/
>
>I think for short term, yes, it is better to revert the commits
>that make things more broken.
>
>for-linus-v5.5-rc5 branch contains three commits that exactly do
>this i.e. the reverts that Stefan sent and revert to Jerry's earlier
>commit.
>
>After that is out of the table it is easier to analyze how the code
>should be actually refactored. Like, I have no idea when I get
>local HW that can reproduce this and Jerry still seems to have the
>same issue. It'd be nice make the exactly right changes instead of
>reverts but situation is what it is.
>

The only other thought I had was moving the tpm_chip_start/stop
into tpm_tis_probe_irq_single around the tpm_tis_gen_interrupt call.
I don't know why doing the clkrun bit after setting the interrupt
register values would matter, but I'm not sure what else there is
that would be different than when that stuff was happening in
down in tpm_try_transmit. Without hardware to poke at it is hard
to get anywhere.

>Please check the branch and ACK/NAK if I can add tested-by's (and
>other tags).
>
>/Jarkko
>