Re: [PATCH] tpm_tis: Add missing start/stop_tpm_chip calls

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Lukasz Majczak <lma@semihalf.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
	Radoslaw Biernacki <rad@semihalf.com>,
	Marcin Wojtas <mw@semihalf.com>, Alex Levin <levinale@google.com>
Subject: Re: [PATCH] tpm_tis: Add missing start/stop_tpm_chip calls
Date: Sat, 30 Jan 2021 00:49:58 +0200	[thread overview]
Message-ID: <YBSRFsTNjadQMndD@kernel.org> (raw)
In-Reply-To: <20210125171846.GA31929@roeck-us.net>

On Mon, Jan 25, 2021 at 09:18:46AM -0800, Guenter Roeck wrote:
> Hi Lukasz,
> 
> On Sat, Jan 23, 2021 at 02:42:47AM +0100, Lukasz Majczak wrote:
> > There is a missing call to start_tpm_chip before the call to
> > the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current
> > approach maight work for tpm2, it fails for tpm1.x - in that case
> > call to tpm_get_timeouts() or tpm_tis_probe_irq_single() tries to
> > transmit TPM commands on a disabled chip what what doesn't succeed
> 
> s/what what/what/

s/maight/might/

Also, would be nice to have the capatalization of acronyms correct
and consistent. E.g. tpm1.x should be rather written as "TPM 1.x
chips".

It's also incorrect to state that something fails for TPM 1.x chips,
unless you can somehow make a sense that every single TPM 1.x at wild
fails, which probably is not true.

> > and in turn causes tpm_tis_core_init() to fail.
> > Tested on Samsung Chromebook Pro (Caroline).

Anyone can tell me what does Caroline mean anyway?

> > 
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> >  drivers/char/tpm/tpm_tis_core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 92c51c6cfd1b..ff0e5fe46a9d 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -1063,12 +1063,16 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> >  	init_waitqueue_head(&priv->read_queue);
> >  	init_waitqueue_head(&priv->int_queue);
> >  	if (irq != -1) {
> > +		rc = tpm_chip_start(chip);
> 
> Unless I am missing something, the underlying problem seems to be
> the calls to tpm1_getcap(). From other code calling this function,
> it looks like it may only require tpm_clk_enable() to work.

I don't claim to be bus expert but afaik CLKRUN is used with to power
on/off clock, when using LPC bus.

Also, TPM interface specification speaks about CLKRUN in the section
6.3 "TPM LPC Hardware Protocol".

> With that in mind, would it possibly be better to call tpm_clk_enable()
> and tpm_clk_disable() around the calls to tpm1_getcap(), ie in
> tpm1_get_timeouts() and in tpm_tis_gen_interrupt() ?
> 
> This would avoid the unnecessary calls to tpm_chip_start() and
> tpm_chip_stop() for tpm2 chips.

Not enough information about hardware and no klog. Cannot make much
conclusions with the information available.

> Thanks,
> Guenter

Off-topic: I noticed some dumb code in tpm_tis_core.c:

	if (chip->ops->clk_enable != NULL)
		chip->ops->clk_enable(chip, false);

These should be definitely changed as:

       tpm_tis_clkrun_enable(chip, false);

->clk_enable() should be only used in tpm-interface.c.

Not a bug, just really stupid code (and harder to trace).

/Jarkko