Re: [PATCH 1/2] tpm: Factor out common startup code

From: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
To: Jason Gunthorpe
	<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	andrew zamansky
	<andrew.zamansky-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org>,
	tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
	gcwilson-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org,
	azamansk-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org,
	Dan.Morav-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org,
	stimpy1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Subject: Re: [PATCH 1/2] tpm: Factor out common startup code
Date: Wed, 22 Jun 2016 15:22:28 +0300	[thread overview]
Message-ID: <20160622122227.GA27577@intel.com> (raw)
In-Reply-To: <20160620210757.GB11746-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

On Mon, Jun 20, 2016 at 03:07:57PM -0600, Jason Gunthorpe wrote:
> On Sun, Jun 19, 2016 at 02:39:24PM +0200, Jarkko Sakkinen wrote:
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Tested-by: Andrew Zamansky <andrew.zamansky-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org>
> > 
> > Couldn't tpm?_auto_startup() be static functions inside tpm-chip.c?
> 
> Why? tpm-chip is for chip functions and tpm-interface is for command
> issuing functions, the startup command sequencors seem appropriately
> placed...
> 
> These are still module private functions.

Hmm.. interesting point and I actually realized that this related to
work that I'm doing right now.

I'm working on a patches to move TPM 1.x protocol level code to
tpm1-cmd.c. We want to do so that one can conditionally compile out TPM
1.x when it is not needed. Another reason is that, I would not like to
put TPM 1.x sealing code to tpm-interface.c.

Given that I think it is cool to keep these in tpm-interface.c in this
in tpm-interface.c.

Summary: lets keep it in a way that these functions are in
tpm-interface.c for now.

> > > +enum TPM_OPS_FLAGS {
> > > +	TPM_OPS_PROBE_TPM2 = BIT(0),
> > 
> > I see two alternatives here:
> > 
> > 1. Make this work for tpm_tis.c if it is doable.
> > 2. Remove this flag and call tpm2_probe() inside tpm_i2c_nuvoton.c.
> > 
> > If this flag works only for a single driver, it does not bring any value.
> 
> We already have two drivers that auto probe, you don't think there
> will be more? The idea is to try and remove these low level entry
> points so drivers are simpler.
> 
> TIS doesn't use it because it needs to get things setup in advance for
> interrupt auto-probing, which is a very TIS unique thing.
> 
> Alternatively we can drop the auto-probe from nuvoton and force it to
> rely on compatible string matching to enter TPM2 mode.

The patch set used TPM2_OPS_PROBE_TPM2 only for nuvoton driver. The more
recent one used it incorrectly with tpm_tis.

> Jason

PS. Would you have time to look at https://lkml.org/lkml/2016/6/17/887
and give your feedback? :)

/Jarkko

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape