[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87ft4oos54.fsf@iwork.i-did-not-set--mail-host-address--so-tickle-me>
Date: Wed, 02 Dec 2020 22:43:51 +0200
From: Adrian Ratiu <adrian.ratiu@...labora.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, Peter Huewe <peterhuewe@....de>,
Jason Gunthorpe <jgg@...pe.ca>, linux-kernel@...r.kernel.org,
kernel@...labora.com, Duncan Laurie <dlaurie@...omium.org>,
Stephen Boyd <swboyd@...omium.org>,
Helen Koike <helen.koike@...labora.com>,
Ezequiel Garcia <ezequiel@...labora.com>
Subject: Re: [PATCH v4] char: tpm: add i2c driver for cr50
On Wed, 02 Dec 2020, Jarkko Sakkinen <jarkko@...nel.org> wrote:
> On Wed, Dec 02, 2020 at 12:58:05PM +0200, Adrian Ratiu wrote:
>> From: "dlaurie@...omium.org" <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 bytes 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. This driver is based on the existing
>> infineon I2C TPM driver, which most closely matches the cr50
>> i2c protocol behavior. Cc: Helen Koike
>> <helen.koike@...labora.com> Cc: Jarkko Sakkinen
>> <jarkko@...nel.org> Cc: Ezequiel Garcia
>> <ezequiel@...labora.com> 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>
>> Signed-off-by: Fabien Lahoudere
>> <fabien.lahoudere@...labora.com> Signed-off-by: Adrian Ratiu
>> <adrian.ratiu@...labora.com> --- Changes in v4:
>> - Replace force_release enum with defines (Jarkko)
>> Changes in v3:
>> - Misc small fixes (typos/renamings, comments, default
>> values) - Moved i2c_write memcpy before lock to minimize
>> critical section (Helen) - Dropped priv->locality because it
>> stored a constant value (Helen) - Many kdoc, function name
>> and style fixes in general (Jarkko) - Kept the force release
>> enum instead of defines or bool (Ezequiel)
>> Changes in v2:
>> - Various small fixes all over (reorder includes,
>> MAX_BUFSIZE, comments, etc) - Reworked return values of
>> i2c_wait_tpm_ready() to fix timeout mis-handling
>> so ret == 0 now means success, the wait period jiffies is
>> ignored because that number is meaningless and return a proper
>> timeout error in case jiffies == 0.
>> - Make i2c default to 1 message per transfer (requested by
>> Helen) - Move -EIO error reporting to transfer function to
>> cleanup transfer() itself
>> and its R/W callers
>> - Remove magic value hardcodings and introduce enum
>> force_release.
>> Applies on next-20201201, tested on Chromebook EVE. ---
>> drivers/char/tpm/Kconfig | 10 +
>> drivers/char/tpm/Makefile | 2 +
>> drivers/char/tpm/tpm_tis_i2c_cr50.c | 767
>> ++++++++++++++++++++++++++++ 3 files changed, 779
>> insertions(+) create mode 100644
>> drivers/char/tpm/tpm_tis_i2c_cr50.c
>> diff --git a/drivers/char/tpm/Kconfig
>> b/drivers/char/tpm/Kconfig index a18c314da211..4308f9ca7a43
>> 100644 --- a/drivers/char/tpm/Kconfig +++
>> b/drivers/char/tpm/Kconfig @@ -86,6 +86,16 @@ config
>> TCG_TIS_SYNQUACER
>> To compile this driver as a module, choose M here; the
>> module will be called tpm_tis_synquacer.
>> +config TCG_TIS_I2C_CR50 + tristate "TPM Interface
>> Specification 2.0 Interface (I2C - CR50)" + depends on I2C +
>> select TCG_CR50 + help + This is a driver for the Google
>> cr50 I2C TPM interface which is a + custom microcontroller
>> and requires a custom i2c protocol interface + to
>> handle the limitations of the hardware. To compile this driver
>> + as a module, choose M here; the module will be called
>> tcg_tis_i2c_cr50. +
>> config TCG_TIS_I2C_ATMEL tristate "TPM Interface Specification
>> 1.2 Interface (I2C - Atmel)" depends on I2C
>> diff --git a/drivers/char/tpm/Makefile
>> b/drivers/char/tpm/Makefile index 84db4fb3a9c9..66d39ea6bd10
>> 100644 --- a/drivers/char/tpm/Makefile +++
>> b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@
>> obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
>> tpm_tis_spi-y := tpm_tis_spi_main.o
>> tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
>> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o +
>> obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
>> obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
>> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
>> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode 100644
>> index 000000000000..a374853a3b4b --- /dev/null +++
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,767 @@ +//
>> SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2016 Google
>> Inc.
>
> Should be 2020.
>
>> + * + * Based on Linux Kernel TPM driver by + * Peter Huewe
>> <peter.huewe@...ineon.com> + * Copyright (C) 2011 Infineon
>> Technologies
>
> Not sure how this was derived.
>
Indeed I think we should just mention the original author and
driver like Infineon driver itself does. Thanks!
>> + * + * 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/interrupt.h> +#include
>> <linux/module.h> +#include <linux/pm.h> +#include
>> <linux/slab.h> +#include <linux/wait.h> + +#include
>> "tpm_tis_core.h" + +#define TPM_CR50_MAX_BUFSIZE 64
>> +#define TPM_CR50_TIMEOUT_SHORT_MS 2 /* Short timeout
>> during transactions */ +#define TPM_CR50_TIMEOUT_NOIRQ_MS 20
>> /* Timeout for TPM ready without IRQ */ +#define
>> TPM_CR50_I2C_DID_VID 0x00281ae0L /* Device and vendor
>> ID reg value */ +#define TPM_CR50_I2C_MAX_RETRIES 3 /*
>> Max retries due to I2C errors */ +#define
>> TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between
>> retries on I2C */ +#define TPM_CR50_I2C_RETRY_DELAY_HI 65
>> /* Max usecs between retries on I2C */ + +#define
>> TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) +#define
>> TPM_I2C_STS(l) (0x0001 | ((l) << 4)) +#define
>> TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4)) +#define
>> TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) + +#define
>> TPM_I2C_CR50_NO_FORCE 0 +#define TPM_I2C_CR50_FORCE 1
>
> No need for these.
>
>> + +/** + * struct tpm_i2c_cr50_priv_data - Driver private data.
>> + * @irq: Irq number used for this chip. + * If irq <=
>> 0, then a fixed timeout is used instead of waiting for irq. +
>> * @tpm_ready: Struct used by irq handler to signal R/W
>> readiness. + * @buf: Buffer used for i2c writes, with i2c
>> address prepended to content.
>
> Not properly aligned.
>
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>
>> + */ +struct tpm_i2c_cr50_priv_data { + int irq; +
>> struct completion tpm_ready; + u8
>> buf[TPM_CR50_MAX_BUFSIZE]; +}; + +/** + *
>> tpm_cr50_i2c_int_handler() - cr50 interrupt handler. + *
>> @dummy: Unused parameter. + * @dev_id: TPM chip information.
>
> This is alignment everywhere. Why the parameter is called
> "dev_id" anyway?
>
I think it got copied from all the other tpm drivers which use the
"dev_id" identifier in the irq handler. In the end it's a void
pointer in C, it could be anything. :)
I will improve the name while at it with the alignments, thanks!
>> + * + * The cr50 interrupt handler signals waiting threads that
>> the + * interrupt has been asserted. It does not do any
>> interrupt triggered + * processing but is instead used to avoid
>> fixed delays. + */ +static irqreturn_t
>> tpm_cr50_i2c_int_handler(int dummy, void *dev_id) +{ +
>> struct tpm_chip *chip = dev_id; + struct
>> tpm_i2c_cr50_priv_data *priv = dev_get_drvdata(&chip->dev); + +
>> complete(&priv->tpm_ready); + + return IRQ_HANDLED; +} +
>> +/** + * tpm_cr50_i2c_wait_tpm_ready() - Wait for tpm to signal
>> ready. + * @chip: TPM chip information. + * + * Wait for
>> completion interrupt if available, otherwise use a fixed + *
>> delay for the TPM to be ready. + * + * Return: 0 for success
>> or timeout error number. + */ +static int
>> tpm_cr50_i2c_wait_tpm_ready(struct tpm_chip *chip) +{ +
>> struct tpm_i2c_cr50_priv_data *priv =
>> dev_get_drvdata(&chip->dev); + + /* Use a safe fixed delay
>> if interrupt is not supported */ + if (priv->irq <= 0) { +
>> msleep(TPM_CR50_TIMEOUT_NOIRQ_MS); + return 0; + }
>> + + /* Wait for interrupt to indicate TPM is ready to respond
>> */ + if (!wait_for_completion_timeout(&priv->tpm_ready, +
>> msecs_to_jiffies(chip->timeout_a))) { +
>> dev_warn(&chip->dev, "Timeout waiting for TPM ready\n"); +
>> return -ETIMEDOUT; + } + + return 0; +} + +/** + *
>> tpm_cr50_i2c_enable_tpm_irq() - Enable TPM irq. + * @chip: TPM
>> chip information. + */ +static void
>> tpm_cr50_i2c_enable_tpm_irq(struct tpm_chip *chip) +{ +
>> struct tpm_i2c_cr50_priv_data *priv =
>> dev_get_drvdata(&chip->dev); + + if (priv->irq > 0) { +
>> reinit_completion(&priv->tpm_ready); +
>> enable_irq(priv->irq); + } +} + +/** + *
>> tpm_cr50_i2c_disable_tpm_irq() - Disable TPM irq. + * @chip:
>> TPM chip information. + */ +static void
>> tpm_cr50_i2c_disable_tpm_irq(struct tpm_chip *chip) +{ +
>> struct tpm_i2c_cr50_priv_data *priv =
>> dev_get_drvdata(&chip->dev); + + if (priv->irq > 0) +
>> disable_irq(priv->irq); +} + +/** + *
>> tpm_cr50_i2c_transfer_message() - Transfer a message over i2c.
>> + * @dev: Device information. + * @adapter: I2C adapter. + *
>> @msg:Mmessage to transfer.
>
> Alignment etc.
>
>> + * + * Call unlocked i2c transfer routine with the provided
>> parameters and + * retry in case of bus errors. + * + *
>> Return: 0 on success, otherwise negative errno. + */ +static
>> int tpm_cr50_i2c_transfer_message(struct device *dev, +
>> struct i2c_adapter *adapter, +
>> struct i2c_msg *msg) +{ + int rc; + unsigned int try;
>
> Opposite order would be more readable (reverse christmas tree).
>
>> + + for (try = 0; try < TPM_CR50_I2C_MAX_RETRIES; try++) { +
>> rc = __i2c_transfer(adapter, msg, 1); + if (rc ==
>> 1) + return 0; /* Successfully transferred the
>> message */ + if (try) +
>> dev_warn(dev, "i2c transfer failed (attempt %d/%d): %d\n", +
>> try + 1, TPM_CR50_I2C_MAX_RETRIES, rc); +
>> usleep_range(TPM_CR50_I2C_RETRY_DELAY_LO, +
>> TPM_CR50_I2C_RETRY_DELAY_HI);
>
> Can be probably put into one line without checkpatch.pl
> complaining.
>
> Giving up at this point.
Thanks for the feedback, will send another version with fixes
soon.
Adrian
>
> /Jarkko
Powered by blists - more mailing lists