Message-ID: <5d49915f.1c69fb81.bd889.bc77@mx.google.com>
Date:   Mon, 05 Aug 2019 16:52:55 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Alexander Steffen <Alexander.Steffen@...ineon.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Peter Huewe <peterhuewe@....de>
Cc:     Duncan Laurie <dlaurie@...omium.org>, linux-kernel@...r.kernel.org,
        Jason Gunthorpe <jgg@...pe.ca>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-integrity@...r.kernel.org,
        Andrey Pronin <apronin@...omium.org>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH v2 6/6] tpm: Add driver for cr50 on I2C

Quoting Alexander Steffen (2019-07-17 08:19:19)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Duncan Laurie <dlaurie@...omium.org>
> > 
> > Add TPM 2.0 compatible I2C interface for chips with cr50 firmware.
> > 
> > The firmware running on the currently supported H1 MCU requires a
> > special driver to handle its specific protocol, and this makes it
> > unsuitable to use tpm_tis_core_* and instead it must implement the
> > underlying TPM protocol similar to the other I2C TPM drivers.
> > 
> > - All 4 byes of status register must be read/written at once.
> > - FIFO and burst count is limited to 63 and must be drained by AP.
> > - Provides an interrupt to indicate when read response data is ready
> > and when the TPM is finished processing write data.
> 
> And why does this prevent using the existing tpm_tis_core 
> infrastructure? Taking the status register as an example, you could just 
> teach read_bytes to look at the requested address, and if it lies 
> between 0x18 and 0x1b read the whole status register and then return 
> only the subset that has been requested originally.
> 
> Both approaches might not be pretty, but I'd prefer having shared code 
> with explicit code paths for the differences instead of having two 
> copies of mostly the same algorithm where a simple diff will print out a 
> lot more than just the crucial differences.

There are a few i2c tpm drivers in drivers/char/tpm/. I still haven't
looked at the details but maybe this will work out. I'm planning to drop
this patch from the series and revisit this after getting the SPI driver
merged.

> 
> > This driver is based on the existing infineon I2C TPM driver, which
> > most closely matches the cr50 i2c protocol behavior.  The driver is
> > intentionally kept very similar in structure and style to the
> > corresponding drivers in coreboot and depthcharge.
> > 
> > Signed-off-by: Duncan Laurie <dlaurie@...omium.org>
> > [swboyd@...omium.org: Depend on i2c even if it's a module, replace
> > boilier plate with SPDX tag, drop asm/byteorder.h include, simplify
> > return from probe]
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> > ---
> >   drivers/char/tpm/Kconfig    |  10 +
> >   drivers/char/tpm/Makefile   |   1 +
> >   drivers/char/tpm/cr50_i2c.c | 705 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 716 insertions(+)
> >   create mode 100644 drivers/char/tpm/cr50_i2c.c
> > 
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index b7028bfa6f87..57a8c3540265 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -119,6 +119,16 @@ config TCG_CR50
> >       ---help---
> >         Common routines shared by drivers for Cr50-based devices.
> >   
> > +config TCG_CR50_I2C
> > +     tristate "Cr50 I2C Interface"
> > +     depends on I2C
> > +     select TCG_CR50
> > +     ---help---
> > +       If you have a H1 secure module running Cr50 firmware on I2C bus,
> > +       say Yes and it will be accessible from within Linux. To compile
> > +       this driver as a module, choose M here; the module will be called
> > +       cr50_i2c.
> > +
> >   config TCG_CR50_SPI
> >       tristate "Cr50 SPI Interface"
> >       depends on SPI
> > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > index 4e89538c73c8..3ac3448c21fa 100644
> > --- a/drivers/char/tpm/Makefile
> > +++ b/drivers/char/tpm/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
> >   obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
> >   obj-$(CONFIG_TCG_CR50) += cr50.o
> >   obj-$(CONFIG_TCG_CR50_SPI) += cr50_spi.o
> > +obj-$(CONFIG_TCG_CR50_I2C) += cr50_i2c.o
> >   obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
> >   obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> >   obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> > diff --git a/drivers/char/tpm/cr50_i2c.c b/drivers/char/tpm/cr50_i2c.c
> > new file mode 100644
> > index 000000000000..25934c038b9b
> > --- /dev/null
> > +++ b/drivers/char/tpm/cr50_i2c.c
> > @@ -0,0 +1,705 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2016 Google Inc.
> > + *
> > + * Based on Linux Kernel TPM driver by
> > + * Peter Huewe <peter.huewe@...ineon.com>
> > + * Copyright (C) 2011 Infineon Technologies
> > + */
> > +
> > +/*
> > + * cr50 is a firmware for H1 secure modules that requires special
> > + * handling for the I2C interface.
> > + *
> > + * - Use an interrupt for transaction status instead of hardcoded delays
> > + * - Must use write+wait+read read protocol
> > + * - All 4 bytes of status register must be read/written at once
> > + * - Burst count max is 63 bytes, and burst count behaves
> > + *   slightly differently than other I2C TPMs
> > + * - When reading from FIFO the full burstcnt must be read
> > + *   instead of just reading header and determining the remainder
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/completion.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/wait.h>
> > +#include "cr50.h"
> > +#include "tpm.h"
> > +
> > +#define CR50_MAX_BUFSIZE     63
> > +#define CR50_TIMEOUT_SHORT_MS        2       /* Short timeout during transactions */
> > +#define CR50_TIMEOUT_NOIRQ_MS        20      /* Timeout for TPM ready without IRQ */
> > +#define CR50_I2C_DID_VID     0x00281ae0L
> > +#define CR50_I2C_MAX_RETRIES 3       /* Max retries due to I2C errors */
> > +#define CR50_I2C_RETRY_DELAY_LO      55      /* Min usecs between retries on I2C */
> > +#define CR50_I2C_RETRY_DELAY_HI      65      /* Max usecs between retries on I2C */
> > +
> > +static unsigned short rng_quality = 1022;
> > +
> > +module_param(rng_quality, ushort, 0644);
> > +MODULE_PARM_DESC(rng_quality,
> > +              "Estimation of true entropy, in bits per 1024 bits.");
> > +
> > +struct priv_data {
> > +     int irq;
> > +     int locality;
> > +     struct completion tpm_ready;
> > +     u8 buf[CR50_MAX_BUFSIZE + sizeof(u8)];
> > +};
> > +
> > +/*
> > + * The cr50 interrupt handler just signals waiting threads that the
> > + * interrupt was asserted.  It does not do any processing triggered
> > + * by interrupts but is instead used to avoid fixed delays.
> > + */
> > +static irqreturn_t cr50_i2c_int_handler(int dummy, void *dev_id)
> > +{
> > +     struct tpm_chip *chip = dev_id;
> > +     struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > +     complete(&priv->tpm_ready);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Wait for completion interrupt if available, otherwise use a fixed
> > + * delay for the TPM to be ready.
> > + *
> > + * Returns negative number for error, positive number for success.
> > + */
> > +static int cr50_i2c_wait_tpm_ready(struct tpm_chip *chip)
> > +{
> > +     struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +     long rc;
> > +
> > +     /* Use a safe fixed delay if interrupt is not supported */
> > +     if (priv->irq <= 0) {
> > +             msleep(CR50_TIMEOUT_NOIRQ_MS);
> > +             return 1;
> > +     }
> > +
> > +     /* Wait for interrupt to indicate TPM is ready to respond */
> > +     rc = wait_for_completion_timeout(&priv->tpm_ready,
> > +             msecs_to_jiffies(chip->timeout_a));
> > +
> > +     if (rc == 0)
> > +             dev_warn(&chip->dev, "Timeout waiting for TPM ready\n");
> > +
> > +     return rc;
> > +}
> > +
> > +static void cr50_i2c_enable_tpm_irq(struct tpm_chip *chip)
> > +{
> > +     struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > +     if (priv->irq > 0) {
> > +             reinit_completion(&priv->tpm_ready);
> > +             enable_irq(priv->irq);
> > +     }
> > +}
> > +
> > +static void cr50_i2c_disable_tpm_irq(struct tpm_chip *chip)
> > +{
> > +     struct priv_data *priv = dev_get_drvdata(&chip->dev);
> > +
> > +     if (priv->irq > 0)
> > +             disable_irq(priv->irq);
> > +}
> > +
> > +/*
> > + * cr50_i2c_transfer - transfer messages over i2c
> > + *
> > + * @adapter: i2c adapter
> > + * @msgs: array of messages to transfer
> > + * @num: number of messages in the array
> > + *
> > + * Call unlocked i2c transfer routine with the provided parameters and retry
> > + * in case of bus errors. Returns the number of transferred messages.
> > + */
> 
> Documentation is nice, why is it only present for some of the functions? 
> Also, the dev parameter is missing in the list of parameters above.

Ok.

> 
> > +static int cr50_i2c_transfer(struct device *dev, struct i2c_adapter *adapter,
> > +                          struct i2c_msg *msgs, int num)
> > +{
> > +     int rc, try;
> > +
> > +     for (try = 0; try < CR50_I2C_MAX_RETRIES; try++) {
> > +             rc = __i2c_transfer(adapter, msgs, num);
> > +             if (rc > 0)
> > +                     break;
> > +             if (try)
> > +                     dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n",
> > +                              try+1, CR50_I2C_MAX_RETRIES, rc);
> 
> Why does this not generate a message when the first attempt fails?
> 

Hmm looks like an off-by-one bug. 

> > +
> > +enum tis_access {
> > +     TPM_ACCESS_VALID = 0x80,
> > +     TPM_ACCESS_ACTIVE_LOCALITY = 0x20,
> > +     TPM_ACCESS_REQUEST_PENDING = 0x04,
> > +     TPM_ACCESS_REQUEST_USE = 0x02,
> > +};
> > +
> > +enum tis_status {
> > +     TPM_STS_VALID = 0x80,
> > +     TPM_STS_COMMAND_READY = 0x40,
> > +     TPM_STS_GO = 0x20,
> > +     TPM_STS_DATA_AVAIL = 0x10,
> > +     TPM_STS_DATA_EXPECT = 0x08,
> > +};
> > +
> > +enum tis_defaults {
> > +     TIS_SHORT_TIMEOUT = 750,        /* ms */
> > +     TIS_LONG_TIMEOUT = 2000,        /* 2 sec */
> > +};
> 
> This is already defined in tpm_tis_core.h. Do you really need to 
> redefine it here?

This whole chunk of code is copying from tpm_i2c_infineon.c and I'm not
sure why that driver redefines things either. If i include
tpm_tis_core.h then I need to undef the below defines so they can be
redefined in this file.

> 
> > +
> > +#define      TPM_ACCESS(l)                   (0x0000 | ((l) << 4))
> > +#define      TPM_STS(l)                      (0x0001 | ((l) << 4))
> > +#define      TPM_DATA_FIFO(l)                (0x0005 | ((l) << 4))
> > +#define      TPM_DID_VID(l)                  (0x0006 | ((l) << 4))
> > +

Powered by blists - more mailing lists