Re: [PATCH v4] char: tpm: add i2c driver for cr50

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 02 Dec 2020, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote:
On Wed, Dec 02, 2020 at 12:58:05PM +0200, Adrian Ratiu wrote:
From: "dlaurie@xxxxxxxxxxxx" <dlaurie@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx> Cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx> Cc: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> Signed-off-by: Duncan Laurie <dlaurie@xxxxxxxxxxxx> [swboyd@xxxxxxxxxxxx: 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@xxxxxxxxxxxx> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx> Signed-off-by: Adrian Ratiu <adrian.ratiu@xxxxxxxxxxxxx> --- 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@xxxxxxxxxxxx> + * 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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux