Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Alexander Steffen <Alexander.Steffen@infineon.com>
Cc: Nayna Jain <nayna@linux.vnet.ibm.com>,
	kgold@linux.vnet.ibm.com, linux-integrity@vger.kernel.org,
	Mimi Zohar <zohar@linux.vnet.ibm.com>
Subject: Re: [RFC][PATCH 0/9] tpm: fix driver so that burstcount can be safely ignored
Date: Mon, 8 Jan 2018 12:50:52 +0200	[thread overview]
Message-ID: <20180108105052.ljiupenk7y6zwodu@linux.intel.com> (raw)
In-Reply-To: <33a00339-767e-2a55-6aa2-36e24a4af90b@infineon.com>

On Fri, Jan 05, 2018 at 08:38:11AM +0100, Alexander Steffen wrote:
> On 05.01.2018 07:46, Nayna Jain wrote:
> > On 01/04/2018 06:41 PM, Alexander.Steffen@infineon.com wrote:
> > > > On Fri, Dec 15, 2017 at 02:04:39PM +0200, Jarkko Sakkinen wrote:
> > > > > On Fri, Dec 08, 2017 at 07:46:49PM +0100, Alexander Steffen wrote:
> > > > > > Disclaimer: This is RFC because unfortunately I do not
> > > > > > have the time to
> > > > bring it up to the usual standards. But from lying around on my
> > > > hard disk it
> > > > definitely won't improve, so I've decided to publish it now, to
> > > > get at least
> > > > some feedback or maybe for somebody else to pick it up. Since the work
> > > > began some time ago, it is probably not rebased against the
> > > > current state
> > > > (but hopefully still applies).
> > > > > > This is a collection of all the fixes I made during the
> > > > > > investigation of the
> > > > problems with the "ignore burstcount" change. These are mainly
> > > > fixes for the
> > > > wait state handling in tpm_tis_spi, that probably was not really
> > > > tested so far,
> > > > since the previous implementation avoided wait states as far as
> > > > possible. It
> > > > also includes changes to tpm_tis_core, so that it follows the
> > > > specification
> > > > more closely. Finally, I've included a rebased version of the
> > > > original "ignore
> > > > burstcount" patch. Together, those patches pass all my tests and
> > > > also each
> > > > intermediate step is fine (so that bisecting still works).
> > > > > > Alexander Steffen (8):
> > > > > >    tpm_tis_core: clean up whitespace
> > > > > >    tpm_tis_core: access single TIS registers before doing complex
> > > > > >      transfers
> > > > > >    tpm_tis_core: correctly wait for flags to become zero
> > > > > >    tpm_tis_core: send all data in single operation
> > > > > >    tpm_tis_core: use XDATA_FIFO for transfers if available
> > > > > >    tpm_tis_spi: fix sending wrong data during wait state handling
> > > > > >    tpm_tis_spi: release CS line when wait state handling fails
> > > > > >    tpm_tis_spi: add delay between wait state retries
> > > > > > 
> > > > > > Nayna Jain (1):
> > > > > >    tpm: ignore burstcount to improve tpm_tis send() performance
> > > > > > 
> > > > > >   drivers/char/tpm/tpm_tis_core.c | 131 ++++++++++++++++++++---------
> > > > -----------
> > > > > >   drivers/char/tpm/tpm_tis_core.h |  26 ++++----
> > > > > >   drivers/char/tpm/tpm_tis_spi.c  |  21 +++++--
> > > > > >   3 files changed, 95 insertions(+), 83 deletions(-)
> > > > > > 
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > > Might not have time to review this before holidays but will do if
> > > > > I can.
> > > > Unfortunately this and your patches for broken TPMs came so late that
> > > > they have no chance to make into 4.16. Thus, I will postpone their
> > > > review after the pull request and take care of the high priority stuff
> > > > for that release.
> > > As long the "ignore burstcount" patch does not make it into 4.16
> > > either, that's fine. Then nothing new gets broken, it just does not
> > > get any faster.
> > 
> > Alex, if I am right, among following 4 patches,
> > 
> >    Patch 1 - tpm: move wait_for_tpm_stat() to respective driver files
> >    Patch 2 - tpm: ignore burstcount to improve tpm_tis send() performance
> >    Patch 3 - tpm: reduce tpm polling delay in tpm_tis_core
> >    Patch 4 - tpm: use tpm_msleep() value as max delay
> > 
> > it is only the burstcount patch (Patch 2) which caused the issue.
> > With that, Jarkko, Alex, can we just exclude this and have other three
> > patches pulled for 4.16 ?
> 
> Those other three patches do not cause any issues in my tests, so I'm fine
> with that.

Yes.

/Jarkko