This patch series adds support for the the H1 secure microcontroller
running cr50 firmware found on various recent Chromebooks. This driver
is necessary to boot into a ChromeOS userspace environment. It
implements support for several functions, including TPM-like
functionality, and supports SPI and I2C interfaces.
The last time this was series sent looks to be [1]. I've looked over the
patches and review comments and tried to address any feedback that
Andrey didn't address (really minor things like newlines). The first
three patches add a couple pre-requisite core changes so that the
drivers can be merged. The last three patches add the SPI and i2c drivers
along with the DT binding.
[1] https://lkml.kernel.org/r/[email protected]
Changes from v1:
* Dropped symlink and sysfs patches
* Removed 'is_suspended' bits
* Added new patch to freeze khwrng thread
* Moved binding to google,cr50.txt and added Reviewed-by tag from Rob
Andrey Pronin (4):
tpm_tis_core: add optional max xfer size check
tpm_tis_spi: add max xfer size
dt-bindings: tpm: document properties for cr50
tpm: add driver for cr50 on SPI
Duncan Laurie (1):
tpm: Add driver for cr50 on I2C
Stephen Boyd (1):
hwrng: core: Freeze khwrng thread during suspend
.../bindings/security/tpm/google,cr50.txt | 19 +
drivers/char/hw_random/core.c | 5 +-
drivers/char/tpm/Kconfig | 26 +
drivers/char/tpm/Makefile | 3 +
drivers/char/tpm/cr50.c | 33 +
drivers/char/tpm/cr50.h | 15 +
drivers/char/tpm/cr50_i2c.c | 705 ++++++++++++++++++
drivers/char/tpm/cr50_spi.c | 450 +++++++++++
drivers/char/tpm/tpm_tis_core.c | 9 +-
drivers/char/tpm/tpm_tis_core.h | 1 +
drivers/char/tpm/tpm_tis_spi.c | 1 +
11 files changed, 1265 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt
create mode 100644 drivers/char/tpm/cr50.c
create mode 100644 drivers/char/tpm/cr50.h
create mode 100644 drivers/char/tpm/cr50_i2c.c
create mode 100644 drivers/char/tpm/cr50_spi.c
base-commit: 0ecfebd2b52404ae0c54a878c872bb93363ada36
--
Sent by a computer through tubes
From: Andrey Pronin <[email protected]>
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware.
Signed-off-by: Andrey Pronin <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../bindings/security/tpm/google,cr50.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/google,cr50.txt
diff --git a/Documentation/devicetree/bindings/security/tpm/google,cr50.txt b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
new file mode 100644
index 000000000000..401f4ba281b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/google,cr50.txt
@@ -0,0 +1,19 @@
+* H1 Secure Microcontroller with Cr50 Firmware on SPI Bus.
+
+H1 Secure Microcontroller running Cr50 firmware provides several
+functions, including TPM-like functionality. It communicates over
+SPI using the FIFO protocol described in the PTP Spec, section 6.
+
+Required properties:
+- compatible: Should be "google,cr50".
+- spi-max-frequency: Maximum SPI frequency.
+
+Example:
+
+&spi0 {
+ cr50@0 {
+ compatible = "google,cr50";
+ reg = <0>;
+ spi-max-frequency = <800000>;
+ };
+};
--
Sent by a computer through tubes
From: Duncan Laurie <[email protected]>
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.
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 <[email protected]>
[[email protected]: 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 <[email protected]>
---
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 <[email protected]>
+ * 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.
+ */
+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);
+ usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
+ }
+
+ return rc;
+}
+
+/*
+ * cr50_i2c_read() - read from TPM register
+ *
+ * @chip: TPM chip information
+ * @addr: register address to read from
+ * @buffer: provided by caller
+ * @len: number of bytes to read
+ *
+ * 1) send register address byte 'addr' to the TPM
+ * 2) wait for TPM to indicate it is ready
+ * 3) read 'len' bytes of TPM response into the provided 'buffer'
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t len)
+{
+ struct i2c_client *client = to_i2c_client(chip->dev.parent);
+ struct i2c_msg msg1 = {
+ .addr = client->addr,
+ .len = 1,
+ .buf = &addr
+ };
+ struct i2c_msg msg2 = {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = buffer
+ };
+ int rc;
+
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ /* Prepare for completion interrupt */
+ cr50_i2c_enable_tpm_irq(chip);
+
+ /* Send the register address byte to the TPM */
+ rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg1, 1);
+ if (rc <= 0)
+ goto out;
+
+ /* Wait for TPM to be ready with response data */
+ rc = cr50_i2c_wait_tpm_ready(chip);
+ if (rc < 0)
+ goto out;
+
+ /* Read response data from the TPM */
+ rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg2, 1);
+
+out:
+ cr50_i2c_disable_tpm_irq(chip);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ if (rc < 0)
+ return rc;
+ if (rc == 0)
+ return -EIO; /* No i2c segments transferred */
+
+ return 0;
+}
+
+/*
+ * cr50_i2c_write() - write to TPM register
+ *
+ * @chip: TPM chip information
+ * @addr: register address to write to
+ * @buffer: data to write
+ * @len: number of bytes to write
+ *
+ * 1) prepend the provided address to the provided data
+ * 2) send the address+data to the TPM
+ * 3) wait for TPM to indicate it is done writing
+ *
+ * Returns negative number for error, 0 for success.
+ */
+static int cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
+ size_t len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ struct i2c_client *client = to_i2c_client(chip->dev.parent);
+ struct i2c_msg msg1 = {
+ .addr = client->addr,
+ .len = len + 1,
+ .buf = priv->buf
+ };
+ int rc;
+
+ if (len > CR50_MAX_BUFSIZE)
+ return -EINVAL;
+
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ /* Prepend the 'register address' to the buffer */
+ priv->buf[0] = addr;
+ memcpy(priv->buf + 1, buffer, len);
+
+ /* Prepare for completion interrupt */
+ cr50_i2c_enable_tpm_irq(chip);
+
+ /* Send write request buffer with address */
+ rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg1, 1);
+ if (rc <= 0)
+ goto out;
+
+ /* Wait for TPM to be ready, ignore timeout */
+ cr50_i2c_wait_tpm_ready(chip);
+
+out:
+ cr50_i2c_disable_tpm_irq(chip);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+ if (rc < 0)
+ return rc;
+ if (rc == 0)
+ return -EIO; /* No i2c segments transferred */
+
+ return 0;
+}
+
+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 */
+};
+
+#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))
+
+static int check_locality(struct tpm_chip *chip, int loc)
+{
+ u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
+ u8 buf;
+ int rc;
+
+ rc = cr50_i2c_read(chip, TPM_ACCESS(loc), &buf, 1);
+ if (rc < 0)
+ return rc;
+
+ if ((buf & mask) == mask)
+ return loc;
+
+ return -EIO;
+}
+
+static void release_locality(struct tpm_chip *chip, int force)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
+ u8 addr = TPM_ACCESS(priv->locality);
+ u8 buf;
+
+ if (cr50_i2c_read(chip, addr, &buf, 1) < 0)
+ return;
+
+ if (force || (buf & mask) == mask) {
+ buf = TPM_ACCESS_ACTIVE_LOCALITY;
+ cr50_i2c_write(chip, addr, &buf, 1);
+ }
+
+ priv->locality = 0;
+}
+
+static int request_locality(struct tpm_chip *chip, int loc)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf = TPM_ACCESS_REQUEST_USE;
+ unsigned long stop;
+ int rc;
+
+ if (check_locality(chip, loc) == loc)
+ return loc;
+
+ rc = cr50_i2c_write(chip, TPM_ACCESS(loc), &buf, 1);
+ if (rc < 0)
+ return rc;
+
+ stop = jiffies + chip->timeout_a;
+ do {
+ if (check_locality(chip, loc) == loc) {
+ priv->locality = loc;
+ return loc;
+ }
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ } while (time_before(jiffies, stop));
+
+ return -ETIMEDOUT;
+}
+
+/* cr50 requires all 4 bytes of status register to be read */
+static u8 cr50_i2c_tis_status(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf[4];
+
+ if (cr50_i2c_read(chip, TPM_STS(priv->locality), buf, sizeof(buf)) < 0)
+ return 0;
+ return buf[0];
+}
+
+/* cr50 requires all 4 bytes of status register to be written */
+static void cr50_i2c_tis_ready(struct tpm_chip *chip)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ u8 buf[4] = { TPM_STS_COMMAND_READY };
+
+ cr50_i2c_write(chip, TPM_STS(priv->locality), buf, sizeof(buf));
+ msleep(CR50_TIMEOUT_SHORT_MS);
+}
+
+/*
+ * cr50 uses bytes 3:2 of status register for burst count and
+ * all 4 bytes must be read
+ */
+static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 mask,
+ size_t *burst, int *status)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ unsigned long stop;
+ u8 buf[4];
+
+ /* wait for burstcount */
+ stop = jiffies + chip->timeout_b;
+ do {
+ if (cr50_i2c_read(chip, TPM_STS(priv->locality),
+ (u8 *)&buf, sizeof(buf)) < 0) {
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ continue;
+ }
+
+ *status = *buf;
+ *burst = le16_to_cpup((__le16 *)(buf + 1));
+
+ if ((*status & mask) == mask &&
+ *burst > 0 && *burst <= CR50_MAX_BUFSIZE)
+ return 0;
+
+ msleep(CR50_TIMEOUT_SHORT_MS);
+ } while (time_before(jiffies, stop));
+
+ dev_err(&chip->dev, "Timeout reading burst and status\n");
+ return -ETIMEDOUT;
+}
+
+static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ int status, rc;
+ size_t burstcnt, cur, len, expected;
+ u8 addr = TPM_DATA_FIFO(priv->locality);
+ u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
+
+ if (buf_len < TPM_HEADER_SIZE)
+ return -EINVAL;
+
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
+ dev_err(&chip->dev,
+ "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
+ burstcnt, buf_len, TPM_HEADER_SIZE);
+ rc = -EIO;
+ goto out_err;
+ }
+
+ /* Read first chunk of burstcnt bytes */
+ rc = cr50_i2c_read(chip, addr, buf, burstcnt);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Read of first chunk failed\n");
+ goto out_err;
+ }
+
+ /* Determine expected data in the return buffer */
+ expected = be32_to_cpup((__be32 *)(buf + 2));
+ if (expected > buf_len) {
+ dev_err(&chip->dev, "Too much data in FIFO\n");
+ goto out_err;
+ }
+
+ /* Now read the rest of the data */
+ cur = burstcnt;
+ while (cur < expected) {
+ /* Read updated burst count and check status */
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ len = min_t(size_t, burstcnt, expected - cur);
+ rc = cr50_i2c_read(chip, addr, buf + cur, len);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Read failed\n");
+ goto out_err;
+ }
+
+ cur += len;
+ }
+
+ /* Ensure TPM is done reading data */
+ rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+ if (status & TPM_STS_DATA_AVAIL) {
+ dev_err(&chip->dev, "Data still available\n");
+ rc = -EIO;
+ goto out_err;
+ }
+
+ release_locality(chip, 0);
+ return cur;
+
+out_err:
+ /* Abort current transaction if still pending */
+ if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
+ cr50_i2c_tis_ready(chip);
+
+ release_locality(chip, 0);
+ return rc;
+}
+
+static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+ struct priv_data *priv = dev_get_drvdata(&chip->dev);
+ int rc, status;
+ size_t burstcnt, limit, sent = 0;
+ u8 tpm_go[4] = { TPM_STS_GO };
+ unsigned long stop;
+
+ rc = request_locality(chip, 0);
+ if (rc < 0)
+ return rc;
+
+ /* Wait until TPM is ready for a command */
+ stop = jiffies + chip->timeout_b;
+ while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
+ if (time_after(jiffies, stop)) {
+ rc = -ETIMEDOUT;
+ goto out_err;
+ }
+
+ cr50_i2c_tis_ready(chip);
+ }
+
+ while (len > 0) {
+ u8 mask = TPM_STS_VALID;
+
+ /* Wait for data if this is not the first chunk */
+ if (sent > 0)
+ mask |= TPM_STS_DATA_EXPECT;
+
+ /* Read burst count and check status */
+ rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+
+ /*
+ * Use burstcnt - 1 to account for the address byte
+ * that is inserted by cr50_i2c_write()
+ */
+ limit = min_t(size_t, burstcnt - 1, len);
+ rc = cr50_i2c_write(chip, TPM_DATA_FIFO(priv->locality),
+ &buf[sent], limit);
+ if (rc < 0) {
+ dev_err(&chip->dev, "Write failed\n");
+ goto out_err;
+ }
+
+ sent += limit;
+ len -= limit;
+ }
+
+ /* Ensure TPM is not expecting more data */
+ rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
+ if (rc < 0)
+ goto out_err;
+ if (status & TPM_STS_DATA_EXPECT) {
+ dev_err(&chip->dev, "Data still expected\n");
+ rc = -EIO;
+ goto out_err;
+ }
+
+ /* Start the TPM command */
+ rc = cr50_i2c_write(chip, TPM_STS(priv->locality), tpm_go,
+ sizeof(tpm_go));
+ if (rc < 0) {
+ dev_err(&chip->dev, "Start command failed\n");
+ goto out_err;
+ }
+ return 0;
+
+out_err:
+ /* Abort current transaction if still pending */
+ if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
+ cr50_i2c_tis_ready(chip);
+
+ release_locality(chip, 0);
+ return rc;
+}
+
+static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
+{
+ return (status == TPM_STS_COMMAND_READY);
+}
+
+static const struct tpm_class_ops cr50_i2c = {
+ .flags = TPM_OPS_AUTO_STARTUP,
+ .status = &cr50_i2c_tis_status,
+ .recv = &cr50_i2c_tis_recv,
+ .send = &cr50_i2c_tis_send,
+ .cancel = &cr50_i2c_tis_ready,
+ .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_canceled = &cr50_i2c_req_canceled,
+};
+
+static int cr50_i2c_init(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct tpm_chip *chip;
+ struct priv_data *priv;
+ u8 buf[4];
+ u32 vendor;
+ int rc;
+
+ chip = tpmm_chip_alloc(dev, &cr50_i2c);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* cr50 is a TPM 2.0 chip */
+ chip->flags |= TPM_CHIP_FLAG_TPM2;
+
+ /* Default timeouts */
+ chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+ chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+
+ dev_set_drvdata(&chip->dev, priv);
+ init_completion(&priv->tpm_ready);
+
+ if (client->irq > 0) {
+ rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ dev->driver->name, chip);
+ if (rc < 0) {
+ dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
+ return rc;
+ }
+
+ disable_irq(client->irq);
+ priv->irq = client->irq;
+ } else {
+ dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
+ CR50_TIMEOUT_NOIRQ_MS);
+ }
+
+ rc = request_locality(chip, 0);
+ if (rc < 0) {
+ dev_err(dev, "Could not request locality\n");
+ return rc;
+ }
+
+ /* Read four bytes from DID_VID register */
+ rc = cr50_i2c_read(chip, TPM_DID_VID(0), buf, sizeof(buf));
+ if (rc < 0) {
+ dev_err(dev, "Could not read vendor id\n");
+ release_locality(chip, 1);
+ return rc;
+ }
+
+ vendor = le32_to_cpup((__le32 *)buf);
+ if (vendor != CR50_I2C_DID_VID) {
+ dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
+ release_locality(chip, 1);
+ return -ENODEV;
+ }
+
+ dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
+ client->addr, client->irq, vendor >> 16);
+
+ chip->hwrng.quality = rng_quality;
+
+ return tpm_chip_register(chip);
+}
+
+static const struct i2c_device_id cr50_i2c_table[] = {
+ {"cr50_i2c", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cr50_i2c_acpi_id[] = {
+ { "GOOG0005", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_i2c_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
+#endif
+
+static int cr50_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return -ENODEV;
+
+ return cr50_i2c_init(client);
+}
+
+static int cr50_i2c_remove(struct i2c_client *client)
+{
+ struct tpm_chip *chip = i2c_get_clientdata(client);
+
+ tpm_chip_unregister(chip);
+ release_locality(chip, 1);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, cr50_suspend, cr50_resume);
+
+static struct i2c_driver cr50_i2c_driver = {
+ .id_table = cr50_i2c_table,
+ .probe = cr50_i2c_probe,
+ .remove = cr50_i2c_remove,
+ .driver = {
+ .name = "cr50_i2c",
+ .pm = &cr50_i2c_pm,
+ .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
+ .of_match_table = of_match_ptr(of_cr50_i2c_match),
+ },
+};
+
+module_i2c_driver(cr50_i2c_driver);
+
+MODULE_DESCRIPTION("cr50 TPM I2C Driver");
+MODULE_LICENSE("GPL");
--
Sent by a computer through tubes
From: Andrey Pronin <[email protected]>
Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
firmware. The firmware running on the currently supported H1
Secure Microcontroller requires a special driver to handle its
specifics:
- need to ensure a certain delay between spi transactions, or else
the chip may miss some part of the next transaction;
- if there is no spi activity for some time, it may go to sleep,
and needs to be waken up before sending further commands;
- access to vendor-specific registers.
Signed-off-by: Andrey Pronin <[email protected]>
[[email protected]: Replace boilerplate with SPDX tag, drop
suspended bit and remove ifdef checks in cr50.h, push tpm.h
include into cr50.c]
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/char/tpm/Kconfig | 16 ++
drivers/char/tpm/Makefile | 2 +
drivers/char/tpm/cr50.c | 33 +++
drivers/char/tpm/cr50.h | 15 ++
drivers/char/tpm/cr50_spi.c | 450 ++++++++++++++++++++++++++++++++++++
5 files changed, 516 insertions(+)
create mode 100644 drivers/char/tpm/cr50.c
create mode 100644 drivers/char/tpm/cr50.h
create mode 100644 drivers/char/tpm/cr50_spi.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 88a3c06fc153..b7028bfa6f87 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -114,6 +114,22 @@ config TCG_ATMEL
will be accessible from within Linux. To compile this driver
as a module, choose M here; the module will be called tpm_atmel.
+config TCG_CR50
+ bool
+ ---help---
+ Common routines shared by drivers for Cr50-based devices.
+
+config TCG_CR50_SPI
+ tristate "Cr50 SPI Interface"
+ depends on SPI
+ select TCG_TIS_CORE
+ select TCG_CR50
+ ---help---
+ If you have a H1 secure module running Cr50 firmware on SPI 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_spi.
+
config TCG_INFINEON
tristate "Infineon Technologies TPM Interface"
depends on PNP
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..4e89538c73c8 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
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_INFINEON) += tpm_infineon.o
obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
diff --git a/drivers/char/tpm/cr50.c b/drivers/char/tpm/cr50.c
new file mode 100644
index 000000000000..8c83ec25f8e1
--- /dev/null
+++ b/drivers/char/tpm/cr50.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2016 Google Inc.
+ */
+
+/*
+ * This file contains common code for devices with Cr50 firmware.
+ */
+
+#include <linux/export.h>
+#include <linux/suspend.h>
+#include "cr50.h"
+#include "tpm.h"
+
+#ifdef CONFIG_PM_SLEEP
+int cr50_resume(struct device *dev)
+{
+ if (pm_suspend_via_firmware())
+ return tpm_pm_resume(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL(cr50_resume);
+
+int cr50_suspend(struct device *dev)
+{
+ if (pm_suspend_via_firmware())
+ return tpm_pm_suspend(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL(cr50_suspend);
+#endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/char/tpm/cr50.h b/drivers/char/tpm/cr50.h
new file mode 100644
index 000000000000..1c635fae03e8
--- /dev/null
+++ b/drivers/char/tpm/cr50.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2016 Google Inc.
+ * This file contains common code for devices with Cr50 firmware.
+ */
+
+#ifndef __CR50_H__
+#define __CR50_H__
+
+struct device;
+
+int cr50_resume(struct device *dev);
+int cr50_suspend(struct device *dev);
+
+#endif /* __CR50_H__ */
diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
new file mode 100644
index 000000000000..3c1b472297ad
--- /dev/null
+++ b/drivers/char/tpm/cr50_spi.c
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This device driver implements a TCG PTP FIFO interface over SPI for chips
+ * with Cr50 firmware.
+ * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+#include "cr50.h"
+#include "tpm_tis_core.h"
+
+/*
+ * Cr50 timing constants:
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
+ * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep.
+ * - requires waiting for "ready" IRQ, if supported; or waiting for at least
+ * CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
+ * - waits for up to CR50_FLOW_CONTROL_MSEC for flow control 'ready' indication.
+ */
+#define CR50_SLEEP_DELAY_MSEC 1000
+#define CR50_WAKE_START_DELAY_MSEC 1
+#define CR50_NOIRQ_ACCESS_DELAY_MSEC 2
+#define CR50_READY_IRQ_TIMEOUT_MSEC TPM2_TIMEOUT_A
+#define CR50_FLOW_CONTROL_MSEC TPM2_TIMEOUT_A
+#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
+
+#define MAX_SPI_FRAMESIZE 64
+
+#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
+
+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 cr50_spi_phy {
+ struct tpm_tis_data priv;
+ struct spi_device *spi_device;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access_jiffies;
+ unsigned long wake_after_jiffies;
+
+ unsigned long access_delay_jiffies;
+ unsigned long sleep_delay_jiffies;
+ unsigned int wake_start_delay_msec;
+
+ struct completion tpm_ready;
+
+ unsigned int irq_confirmation_attempt;
+ bool irq_needs_confirmation;
+ bool irq_confirmed;
+
+ u8 tx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
+ u8 rx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
+};
+
+static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
+{
+ return container_of(data, struct cr50_spi_phy, priv);
+}
+
+/*
+ * 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_spi_irq_handler(int dummy, void *dev_id)
+{
+ struct cr50_spi_phy *phy = dev_id;
+
+ phy->irq_confirmed = true;
+ complete(&phy->tpm_ready);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
+static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ unsigned long allowed_access =
+ phy->last_access_jiffies + phy->access_delay_jiffies;
+ unsigned long time_now = jiffies;
+
+ if (time_in_range_open(time_now,
+ phy->last_access_jiffies, allowed_access)) {
+ unsigned long remaining =
+ wait_for_completion_timeout(&phy->tpm_ready,
+ allowed_access - time_now);
+ if (remaining == 0 && phy->irq_confirmed) {
+ dev_warn(&phy->spi_device->dev,
+ "Timeout waiting for TPM ready IRQ\n");
+ }
+ }
+ if (phy->irq_needs_confirmation) {
+ if (phy->irq_confirmed) {
+ phy->irq_needs_confirmation = false;
+ phy->access_delay_jiffies =
+ msecs_to_jiffies(CR50_READY_IRQ_TIMEOUT_MSEC);
+ dev_info(&phy->spi_device->dev,
+ "TPM ready IRQ confirmed on attempt %u\n",
+ phy->irq_confirmation_attempt);
+ } else if (++phy->irq_confirmation_attempt >
+ MAX_IRQ_CONFIRMATION_ATTEMPTS) {
+ phy->irq_needs_confirmation = false;
+ dev_warn(&phy->spi_device->dev,
+ "IRQ not confirmed - will use delays\n");
+ }
+ }
+}
+
+/*
+ * Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static bool cr50_needs_waking(struct cr50_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies,
+ phy->last_access_jiffies,
+ phy->wake_after_jiffies);
+}
+
+static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
+{
+ if (cr50_needs_waking(phy)) {
+ /* Assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* Wait for it to fully wake */
+ msleep(phy->wake_start_delay_msec);
+ }
+ /* Reset the time when we need to wake Cr50 again */
+ phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
+}
+
+/*
+ * Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
+{
+ unsigned long timeout_jiffies =
+ jiffies + msecs_to_jiffies(CR50_FLOW_CONTROL_MSEC);
+ struct spi_message m;
+ int ret;
+ struct spi_transfer spi_xfer = {
+ .rx_buf = phy->rx_buf,
+ .len = 1,
+ .cs_change = 1,
+ };
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+ if (time_after(jiffies, timeout_jiffies)) {
+ dev_warn(&phy->spi_device->dev,
+ "Timeout during flow control\n");
+ return -EBUSY;
+ }
+ } while (!(phy->rx_buf[0] & 0x01));
+ return 0;
+}
+
+static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, const u8 *tx, u8 *rx)
+{
+ struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
+ struct spi_message m;
+ struct spi_transfer spi_xfer = {
+ .tx_buf = phy->tx_buf,
+ .rx_buf = phy->rx_buf,
+ .len = 4,
+ .cs_change = 1,
+ };
+ int ret;
+
+ if (len > MAX_SPI_FRAMESIZE)
+ return -EINVAL;
+
+ /*
+ * Do this outside of spi_bus_lock in case cr50 is not the
+ * only device on that spi bus.
+ */
+ mutex_lock(&phy->time_track_mutex);
+ cr50_ensure_access_delay(phy);
+ cr50_wake_if_needed(phy);
+
+ phy->tx_buf[0] = (tx ? 0x00 : 0x80) | (len - 1);
+ phy->tx_buf[1] = 0xD4;
+ phy->tx_buf[2] = (addr >> 8) & 0xFF;
+ phy->tx_buf[3] = addr & 0xFF;
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+
+ spi_bus_lock(phy->spi_device->master);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ goto exit;
+
+ ret = cr50_spi_flow_control(phy);
+ if (ret < 0)
+ goto exit;
+
+ spi_xfer.cs_change = 0;
+ spi_xfer.len = len;
+ if (tx) {
+ memcpy(phy->tx_buf, tx, len);
+ spi_xfer.rx_buf = NULL;
+ } else {
+ spi_xfer.tx_buf = NULL;
+ }
+
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ reinit_completion(&phy->tpm_ready);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (rx)
+ memcpy(rx, phy->rx_buf, len);
+
+exit:
+ spi_bus_unlock(phy->spi_device->master);
+ phy->last_access_jiffies = jiffies;
+ mutex_unlock(&phy->time_track_mutex);
+
+ return ret;
+}
+
+static int cr50_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, NULL, result);
+}
+
+static int cr50_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, const u8 *value)
+{
+ return cr50_spi_xfer_bytes(data, addr, len, value, NULL);
+}
+
+static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+{
+ int rc;
+ __le16 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)&le_val);
+ if (!rc)
+ *result = le16_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+{
+ int rc;
+ __le32 le_val;
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u32), (u8 *)&le_val);
+ if (!rc)
+ *result = le32_to_cpu(le_val);
+ return rc;
+}
+
+static int cr50_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+{
+ __le32 le_val = cpu_to_le32(value);
+
+ return data->phy_ops->write_bytes(data, addr, sizeof(u32),
+ (u8 *)&le_val);
+}
+
+static void cr50_get_fw_version(struct tpm_tis_data *data, char *fw_ver)
+{
+ int i, len = 0;
+ char fw_ver_block[4];
+
+ /*
+ * Write anything to TPM_CR50_FW_VER to start from the beginning
+ * of the version string
+ */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
+ fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
+ fw_ver[len] = fw_ver_block[i];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = 0;
+}
+
+static const struct tpm_tis_phy_ops cr50_spi_phy_ops = {
+ .read_bytes = cr50_spi_read_bytes,
+ .write_bytes = cr50_spi_write_bytes,
+ .read16 = cr50_spi_read16,
+ .read32 = cr50_spi_read32,
+ .write32 = cr50_spi_write32,
+ .max_xfer_size = MAX_SPI_FRAMESIZE,
+};
+
+static int cr50_spi_probe(struct spi_device *dev)
+{
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+ struct cr50_spi_phy *phy;
+ int rc;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->spi_device = dev;
+
+ phy->access_delay_jiffies =
+ msecs_to_jiffies(CR50_NOIRQ_ACCESS_DELAY_MSEC);
+ phy->sleep_delay_jiffies = msecs_to_jiffies(CR50_SLEEP_DELAY_MSEC);
+ phy->wake_start_delay_msec = CR50_WAKE_START_DELAY_MSEC;
+
+ mutex_init(&phy->time_track_mutex);
+ phy->wake_after_jiffies = jiffies;
+ phy->last_access_jiffies = jiffies;
+
+ init_completion(&phy->tpm_ready);
+ if (dev->irq > 0) {
+ rc = devm_request_irq(&dev->dev, dev->irq, cr50_spi_irq_handler,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ "cr50_spi", phy);
+ if (rc < 0) {
+ if (rc == -EPROBE_DEFER)
+ return rc;
+ dev_warn(&dev->dev, "Requesting IRQ %d failed: %d\n",
+ dev->irq, rc);
+ /*
+ * This is not fatal, the driver will fall back to
+ * delays automatically, since tpm_ready will never
+ * be completed without a registered irq handler.
+ * So, just fall through.
+ */
+ } else {
+ /*
+ * IRQ requested, let's verify that it is actually
+ * triggered, before relying on it.
+ */
+ phy->irq_needs_confirmation = true;
+ }
+ } else {
+ dev_warn(&dev->dev,
+ "No IRQ - will use delays between transactions.\n");
+ }
+
+ phy->priv.rng_quality = rng_quality;
+
+ rc = tpm_tis_core_init(&dev->dev, &phy->priv, -1, &cr50_spi_phy_ops,
+ NULL);
+ if (rc < 0)
+ return rc;
+
+ cr50_get_fw_version(&phy->priv, fw_ver);
+ dev_info(&dev->dev, "Cr50 firmware version: %s\n", fw_ver);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cr50_spi_resume(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+ struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
+
+ /*
+ * Jiffies not increased during suspend, so we need to reset
+ * the time to wake Cr50 after resume.
+ */
+ phy->wake_after_jiffies = jiffies;
+
+ return cr50_resume(dev);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cr50_spi_pm, cr50_suspend, cr50_spi_resume);
+
+static int cr50_spi_remove(struct spi_device *dev)
+{
+ struct tpm_chip *chip = spi_get_drvdata(dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+ return 0;
+}
+
+static const struct spi_device_id cr50_spi_id[] = {
+ { "cr50", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, cr50_spi_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cr50_spi_match[] = {
+ { .compatible = "google,cr50", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
+#endif
+
+static struct spi_driver cr50_spi_driver = {
+ .driver = {
+ .name = "cr50_spi",
+ .pm = &cr50_spi_pm,
+ .of_match_table = of_match_ptr(of_cr50_spi_match),
+ },
+ .probe = cr50_spi_probe,
+ .remove = cr50_spi_remove,
+ .id_table = cr50_spi_id,
+};
+module_spi_driver(cr50_spi_driver);
+
+MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
+MODULE_LICENSE("GPL v2");
--
Sent by a computer through tubes
The hwrng_fill() function can run while devices are suspending and
resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
is suspended, the hwrng may hang the bus while attempting to add some
randomness. It's been observed on ChromeOS devices with suspend-to-idle
(s2idle) and an i2c based hwrng that this kthread may run and ask the
hwrng device for randomness before the i2c bus has been resumed.
Let's make this kthread freezable so that we don't try to touch the
hwrng during suspend/resume. This ensures that we can't cause the hwrng
backing driver to get into a bad state because the device is guaranteed
to be resumed before the hwrng kthread is thawed.
Cc: Andrey Pronin <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Duncan Laurie <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/char/hw_random/core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 95be7228f327..3b88af3149a7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -13,6 +13,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/err.h>
+#include <linux/freezer.h>
#include <linux/fs.h>
#include <linux/hw_random.h>
#include <linux/kernel.h>
@@ -421,7 +422,9 @@ static int hwrng_fillfn(void *unused)
{
long rc;
- while (!kthread_should_stop()) {
+ set_freezable();
+
+ while (!kthread_freezable_should_stop(NULL)) {
struct hwrng *rng;
rng = get_current_rng();
--
Sent by a computer through tubes
From: Andrey Pronin <[email protected]>
Reject burstcounts larger than 64 bytes reported by tpm.
SPI Hardware Protocol defined in section 6.4 of TCG PTP
Spec supports up to 64 bytes of data in a transaction.
Signed-off-by: Andrey Pronin <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/char/tpm/tpm_tis_spi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..27393275a3f0 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -190,6 +190,7 @@ static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
.read16 = tpm_tis_spi_read16,
.read32 = tpm_tis_spi_read32,
.write32 = tpm_tis_spi_write32,
+ .max_xfer_size = MAX_SPI_FRAMESIZE,
};
static int tpm_tis_spi_probe(struct spi_device *dev)
--
Sent by a computer through tubes
From: Andrey Pronin <[email protected]>
If tpm reports a bigger burstcnt than allowed by the physical protocol,
set burstcnt to the max allowed value.
In practice, seen in case of xfer issues (e.g. in spi interface case,
lost header causing flow control issues and wrong values returned on read
from TPM_STS). Without catching, causes the physical layer to reject xfer.
Signed-off-by: Andrey Pronin <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
[[email protected]: Drop extra parenthesis in if statement]
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 9 ++++++++-
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c3181ea9f271..5134b05487e5 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -268,8 +268,15 @@ static int get_burstcount(struct tpm_chip *chip)
return rc;
burstcnt = (value >> 8) & 0xFFFF;
- if (burstcnt)
+ if (burstcnt) {
+ if (priv->phy_ops->max_xfer_size &&
+ burstcnt > priv->phy_ops->max_xfer_size) {
+ dev_warn(&chip->dev,
+ "Bad burstcnt read: %d\n", burstcnt);
+ burstcnt = priv->phy_ops->max_xfer_size;
+ }
return burstcnt;
+ }
usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX);
} while (time_before(jiffies, stop));
return -EBUSY;
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 7337819f5d7b..248e8ac8fd02 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -106,6 +106,7 @@ struct tpm_tis_phy_ops {
int (*read16)(struct tpm_tis_data *data, u32 addr, u16 *result);
int (*read32)(struct tpm_tis_data *data, u32 addr, u32 *result);
int (*write32)(struct tpm_tis_data *data, u32 addr, u32 src);
+ u16 max_xfer_size;
};
static inline int tpm_tis_read_bytes(struct tpm_tis_data *data, u32 addr,
--
Sent by a computer through tubes
On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> The hwrng_fill() function can run while devices are suspending and
> resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> is suspended, the hwrng may hang the bus while attempting to add some
> randomness. It's been observed on ChromeOS devices with suspend-to-idle
> (s2idle) and an i2c based hwrng that this kthread may run and ask the
> hwrng device for randomness before the i2c bus has been resumed.
>
> Let's make this kthread freezable so that we don't try to touch the
> hwrng during suspend/resume. This ensures that we can't cause the hwrng
> backing driver to get into a bad state because the device is guaranteed
> to be resumed before the hwrng kthread is thawed.
>
> Cc: Andrey Pronin <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Duncan Laurie <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/char/hw_random/core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Do you want this to go through the crypto tree? If so you need
to cc the linux-crypto mailing list.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 17.07.2019 00:45, Stephen Boyd wrote:
> From: Andrey Pronin <[email protected]>
>
> Reject burstcounts larger than 64 bytes reported by tpm.
This is not the correct thing to do here. To quote the specification:
"burstCount is defined as the number of bytes that can be written to or
read from the data FIFO by the software without incurring a wait state."
(https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf
Page 84)
If the FIFO contains 1k of data, it is completely valid for the TPM to
report that as its burstCount, there is no need to arbitrarily limit it.
Also, burstCount is a property of the high-level TIS protocol, that
should not really care whether the low-level transfers are done via LPC
or SPI (or I2C). Since tpm_tis_spi can only transfer 64 bytes at a time,
it is its job to split larger transfers (which it does perfectly fine).
This also has the advantage that burstCount needs only to be read once,
and then we can do 16 SPI transfers in a row to read that 1k of data.
With your change, it will read 64 bytes, then read burstCount again,
before reading the next 64 bytes and so on. This unnecessarily limits
performance.
Maybe you can describe the problem you're trying to solve in more
detail, so that a better solution can be found, since this is clearly
something not intended by the spec.
Alexander
On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> The hwrng_fill() function can run while devices are suspending and
> resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> is suspended, the hwrng may hang the bus while attempting to add some
> randomness. It's been observed on ChromeOS devices with suspend-to-idle
> (s2idle) and an i2c based hwrng that this kthread may run and ask the
> hwrng device for randomness before the i2c bus has been resumed.
You mean the TPM here right?
Should we be more careful in the TPM code to check if the TPM is
suspended before trying to use it, rather than muck up callers?
Jason
On 17.07.2019 00:45, Stephen Boyd wrote:
> From: Andrey Pronin <[email protected]>
>
> Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
> firmware. The firmware running on the currently supported H1
> Secure Microcontroller requires a special driver to handle its
> specifics:
> - need to ensure a certain delay between spi transactions, or else
> the chip may miss some part of the next transaction;
> - if there is no spi activity for some time, it may go to sleep,
> and needs to be waken up before sending further commands;
> - access to vendor-specific registers.
>
> Signed-off-by: Andrey Pronin <[email protected]>
> [[email protected]: Replace boilerplate with SPDX tag, drop
> suspended bit and remove ifdef checks in cr50.h, push tpm.h
> include into cr50.c]
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/char/tpm/Kconfig | 16 ++
> drivers/char/tpm/Makefile | 2 +
> drivers/char/tpm/cr50.c | 33 +++
> drivers/char/tpm/cr50.h | 15 ++
> drivers/char/tpm/cr50_spi.c | 450 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 516 insertions(+)
> create mode 100644 drivers/char/tpm/cr50.c
> create mode 100644 drivers/char/tpm/cr50.h
> create mode 100644 drivers/char/tpm/cr50_spi.c
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 88a3c06fc153..b7028bfa6f87 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -114,6 +114,22 @@ config TCG_ATMEL
> will be accessible from within Linux. To compile this driver
> as a module, choose M here; the module will be called tpm_atmel.
>
> +config TCG_CR50
> + bool
> + ---help---
> + Common routines shared by drivers for Cr50-based devices.
> +
Is it a common pattern to add config options that are not useful on
their own? When would I ever enable TCG_CR50 without also enabling
TCG_CR50_SPI? Why can't you just use TCG_CR50_SPI for everything?
> +config TCG_CR50_SPI
> + tristate "Cr50 SPI Interface"
> + depends on SPI
> + select TCG_TIS_CORE
> + select TCG_CR50
> + ---help---
> + If you have a H1 secure module running Cr50 firmware on SPI 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_spi.
> +
> config TCG_INFINEON
> tristate "Infineon Technologies TPM Interface"
> depends on PNP
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a01c4cab902a..4e89538c73c8 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
> 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_INFINEON) += tpm_infineon.o
> obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> diff --git a/drivers/char/tpm/cr50.c b/drivers/char/tpm/cr50.c
> new file mode 100644
> index 000000000000..8c83ec25f8e1
> --- /dev/null
> +++ b/drivers/char/tpm/cr50.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2016 Google Inc.
> + */
> +
> +/*
> + * This file contains common code for devices with Cr50 firmware.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/suspend.h>
> +#include "cr50.h"
> +#include "tpm.h"
> +
> +#ifdef CONFIG_PM_SLEEP
> +int cr50_resume(struct device *dev)
> +{
> + if (pm_suspend_via_firmware())
> + return tpm_pm_resume(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(cr50_resume);
> +
> +int cr50_suspend(struct device *dev)
> +{
> + if (pm_suspend_via_firmware())
> + return tpm_pm_suspend(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(cr50_suspend);
> +#endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/char/tpm/cr50.h b/drivers/char/tpm/cr50.h
> new file mode 100644
> index 000000000000..1c635fae03e8
> --- /dev/null
> +++ b/drivers/char/tpm/cr50.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2016 Google Inc.
> + * This file contains common code for devices with Cr50 firmware.
> + */
> +
> +#ifndef __CR50_H__
> +#define __CR50_H__
> +
> +struct device;
> +
> +int cr50_resume(struct device *dev);
> +int cr50_suspend(struct device *dev);
> +
> +#endif /* __CR50_H__ */
> diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
> new file mode 100644
> index 000000000000..3c1b472297ad
> --- /dev/null
> +++ b/drivers/char/tpm/cr50_spi.c
> @@ -0,0 +1,450 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016 Google, Inc
> + *
> + * This device driver implements a TCG PTP FIFO interface over SPI for chips
> + * with Cr50 firmware.
> + * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm.h>
> +#include <linux/spi/spi.h>
> +#include <linux/wait.h>
> +#include "cr50.h"
> +#include "tpm_tis_core.h"
> +
> +/*
> + * Cr50 timing constants:
> + * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
> + * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep.
> + * - requires waiting for "ready" IRQ, if supported; or waiting for at least
> + * CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
> + * - waits for up to CR50_FLOW_CONTROL_MSEC for flow control 'ready' indication.
> + */
> +#define CR50_SLEEP_DELAY_MSEC 1000
> +#define CR50_WAKE_START_DELAY_MSEC 1
> +#define CR50_NOIRQ_ACCESS_DELAY_MSEC 2
> +#define CR50_READY_IRQ_TIMEOUT_MSEC TPM2_TIMEOUT_A
> +#define CR50_FLOW_CONTROL_MSEC TPM2_TIMEOUT_A
> +#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
> +
> +#define MAX_SPI_FRAMESIZE 64
> +
> +#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
> +#define TPM_CR50_MAX_FW_VER_LEN 64
> +
> +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.");
What is the purpose of this parameter? None of the other TPM drivers
have it.
> +
> +struct cr50_spi_phy {
> + struct tpm_tis_data priv;
> + struct spi_device *spi_device;
> +
> + struct mutex time_track_mutex;
> + unsigned long last_access_jiffies;
> + unsigned long wake_after_jiffies;
> +
> + unsigned long access_delay_jiffies;
> + unsigned long sleep_delay_jiffies;
> + unsigned int wake_start_delay_msec;
> +
> + struct completion tpm_ready;
> +
> + unsigned int irq_confirmation_attempt;
> + bool irq_needs_confirmation;
> + bool irq_confirmed;
> +
> + u8 tx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> + u8 rx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> +};
> +
> +static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
> +{
> + return container_of(data, struct cr50_spi_phy, priv);
> +}
> +
> +/*
> + * 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_spi_irq_handler(int dummy, void *dev_id)
> +{
> + struct cr50_spi_phy *phy = dev_id;
> +
> + phy->irq_confirmed = true;
> + complete(&phy->tpm_ready);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * Cr50 needs to have at least some delay between consecutive
> + * transactions. Make sure we wait.
> + */
> +static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
> +{
> + /*
> + * Note: There is a small chance, if Cr50 is not accessed in a few days,
> + * that time_in_range will not provide the correct result after the wrap
> + * around for jiffies. In this case, we'll have an unneeded short delay,
> + * which is fine.
> + */
> + unsigned long allowed_access =
> + phy->last_access_jiffies + phy->access_delay_jiffies;
> + unsigned long time_now = jiffies;
> +
> + if (time_in_range_open(time_now,
> + phy->last_access_jiffies, allowed_access)) {
> + unsigned long remaining =
> + wait_for_completion_timeout(&phy->tpm_ready,
> + allowed_access - time_now);
> + if (remaining == 0 && phy->irq_confirmed) {
> + dev_warn(&phy->spi_device->dev,
> + "Timeout waiting for TPM ready IRQ\n");
> + }
> + }
> + if (phy->irq_needs_confirmation) {
> + if (phy->irq_confirmed) {
> + phy->irq_needs_confirmation = false;
> + phy->access_delay_jiffies =
> + msecs_to_jiffies(CR50_READY_IRQ_TIMEOUT_MSEC);
> + dev_info(&phy->spi_device->dev,
> + "TPM ready IRQ confirmed on attempt %u\n",
> + phy->irq_confirmation_attempt);
> + } else if (++phy->irq_confirmation_attempt >
> + MAX_IRQ_CONFIRMATION_ATTEMPTS) {
> + phy->irq_needs_confirmation = false;
> + dev_warn(&phy->spi_device->dev,
> + "IRQ not confirmed - will use delays\n");
> + }
> + }
> +}
> +
> +/*
> + * Cr50 might go to sleep if there is no SPI activity for some time and
> + * miss the first few bits/bytes on the bus. In such case, wake it up
> + * by asserting CS and give it time to start up.
> + */
> +static bool cr50_needs_waking(struct cr50_spi_phy *phy)
> +{
> + /*
> + * Note: There is a small chance, if Cr50 is not accessed in a few days,
> + * that time_in_range will not provide the correct result after the wrap
> + * around for jiffies. In this case, we'll probably timeout or read
> + * incorrect value from TPM_STS and just retry the operation.
> + */
> + return !time_in_range_open(jiffies,
> + phy->last_access_jiffies,
> + phy->wake_after_jiffies);
> +}
> +
> +static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
> +{
> + if (cr50_needs_waking(phy)) {
> + /* Assert CS, wait 1 msec, deassert CS */
> + struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
> +
> + spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
> + /* Wait for it to fully wake */
> + msleep(phy->wake_start_delay_msec);
wake_start_delay_msec is always 1, isn't it? (Why is that a variable at
all? I see only one place that ever sets it.) Then msleep is not the
best function to use, since it will usually sleep much longer. Use
usleep_range instead. See Documentation/timers/timers-howto.txt.
> + }
> + /* Reset the time when we need to wake Cr50 again */
> + phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
> +}
> +
> +/*
> + * Flow control: clock the bus and wait for cr50 to set LSB before
> + * sending/receiving data. TCG PTP spec allows it to happen during
> + * the last byte of header, but cr50 never does that in practice,
> + * and earlier versions had a bug when it was set too early, so don't
> + * check for it during header transfer.
> + */
> +static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
> +{
> + unsigned long timeout_jiffies =
> + jiffies + msecs_to_jiffies(CR50_FLOW_CONTROL_MSEC);
> + struct spi_message m;
> + int ret;
> + struct spi_transfer spi_xfer = {
> + .rx_buf = phy->rx_buf,
> + .len = 1,
> + .cs_change = 1,
> + };
> +
> + do {
> + spi_message_init(&m);
> + spi_message_add_tail(&spi_xfer, &m);
> + ret = spi_sync_locked(phy->spi_device, &m);
> + if (ret < 0)
> + return ret;
> + if (time_after(jiffies, timeout_jiffies)) {
> + dev_warn(&phy->spi_device->dev,
> + "Timeout during flow control\n");
> + return -EBUSY;
> + }
> + } while (!(phy->rx_buf[0] & 0x01));
> + return 0;
> +}
> +
> +static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
> + u16 len, const u8 *tx, u8 *rx)
> +{
> + struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
> + struct spi_message m;
> + struct spi_transfer spi_xfer = {
> + .tx_buf = phy->tx_buf,
> + .rx_buf = phy->rx_buf,
> + .len = 4,
> + .cs_change = 1,
> + };
> + int ret;
> +
> + if (len > MAX_SPI_FRAMESIZE)
> + return -EINVAL;
> +
> + /*
> + * Do this outside of spi_bus_lock in case cr50 is not the
> + * only device on that spi bus.
> + */
> + mutex_lock(&phy->time_track_mutex);
> + cr50_ensure_access_delay(phy);
> + cr50_wake_if_needed(phy);
> +
> + phy->tx_buf[0] = (tx ? 0x00 : 0x80) | (len - 1);
> + phy->tx_buf[1] = 0xD4;
> + phy->tx_buf[2] = (addr >> 8) & 0xFF;
> + phy->tx_buf[3] = addr & 0xFF;
> +
> + spi_message_init(&m);
> + spi_message_add_tail(&spi_xfer, &m);
> +
> + spi_bus_lock(phy->spi_device->master);
> + ret = spi_sync_locked(phy->spi_device, &m);
> + if (ret < 0)
> + goto exit;
> +
> + ret = cr50_spi_flow_control(phy);
> + if (ret < 0)
> + goto exit;
> +
> + spi_xfer.cs_change = 0;
> + spi_xfer.len = len;
> + if (tx) {
> + memcpy(phy->tx_buf, tx, len);
> + spi_xfer.rx_buf = NULL;
> + } else {
> + spi_xfer.tx_buf = NULL;
> + }
> +
> + spi_message_init(&m);
> + spi_message_add_tail(&spi_xfer, &m);
> + reinit_completion(&phy->tpm_ready);
> + ret = spi_sync_locked(phy->spi_device, &m);
> + if (rx)
> + memcpy(rx, phy->rx_buf, len);
> +
> +exit:
> + spi_bus_unlock(phy->spi_device->master);
> + phy->last_access_jiffies = jiffies;
> + mutex_unlock(&phy->time_track_mutex);
> +
> + return ret;
> +}
> +
> +static int cr50_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
> + u16 len, u8 *result)
> +{
> + return cr50_spi_xfer_bytes(data, addr, len, NULL, result);
> +}
> +
> +static int cr50_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
> + u16 len, const u8 *value)
> +{
> + return cr50_spi_xfer_bytes(data, addr, len, value, NULL);
> +}
> +
> +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> +{
> + int rc;
> + __le16 le_val;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)&le_val);
> + if (!rc)
> + *result = le16_to_cpu(le_val);
> + return rc;
> +}
> +
> +static int cr50_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
> +{
> + int rc;
> + __le32 le_val;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u32), (u8 *)&le_val);
> + if (!rc)
> + *result = le32_to_cpu(le_val);
> + return rc;
> +}
> +
> +static int cr50_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
> +{
> + __le32 le_val = cpu_to_le32(value);
> +
> + return data->phy_ops->write_bytes(data, addr, sizeof(u32),
> + (u8 *)&le_val);
> +}
> +
> +static void cr50_get_fw_version(struct tpm_tis_data *data, char *fw_ver)
> +{
> + int i, len = 0;
> + char fw_ver_block[4];
> +
> + /*
> + * Write anything to TPM_CR50_FW_VER to start from the beginning
> + * of the version string
> + */
> + tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
> +
> + /* Read the string, 4 bytes at a time, until we get '\0' */
> + do {
> + tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
> + fw_ver_block);
> + for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
> + fw_ver[len] = fw_ver_block[i];
> + } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
> + fw_ver[len] = 0;
> +}
> +
> +static const struct tpm_tis_phy_ops cr50_spi_phy_ops = {
> + .read_bytes = cr50_spi_read_bytes,
> + .write_bytes = cr50_spi_write_bytes,
> + .read16 = cr50_spi_read16,
> + .read32 = cr50_spi_read32,
> + .write32 = cr50_spi_write32,
> + .max_xfer_size = MAX_SPI_FRAMESIZE,
> +};
> +
> +static int cr50_spi_probe(struct spi_device *dev)
> +{
> + char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
> + struct cr50_spi_phy *phy;
> + int rc;
> +
> + phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
> + if (!phy)
> + return -ENOMEM;
> +
> + phy->spi_device = dev;
> +
> + phy->access_delay_jiffies =
> + msecs_to_jiffies(CR50_NOIRQ_ACCESS_DELAY_MSEC);
> + phy->sleep_delay_jiffies = msecs_to_jiffies(CR50_SLEEP_DELAY_MSEC);
> + phy->wake_start_delay_msec = CR50_WAKE_START_DELAY_MSEC;
> +
> + mutex_init(&phy->time_track_mutex);
> + phy->wake_after_jiffies = jiffies;
> + phy->last_access_jiffies = jiffies;
> +
> + init_completion(&phy->tpm_ready);
> + if (dev->irq > 0) {
> + rc = devm_request_irq(&dev->dev, dev->irq, cr50_spi_irq_handler,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + "cr50_spi", phy);
> + if (rc < 0) {
> + if (rc == -EPROBE_DEFER)
> + return rc;
> + dev_warn(&dev->dev, "Requesting IRQ %d failed: %d\n",
> + dev->irq, rc);
> + /*
> + * This is not fatal, the driver will fall back to
> + * delays automatically, since tpm_ready will never
> + * be completed without a registered irq handler.
> + * So, just fall through.
> + */
> + } else {
> + /*
> + * IRQ requested, let's verify that it is actually
> + * triggered, before relying on it.
> + */
> + phy->irq_needs_confirmation = true;
> + }
> + } else {
> + dev_warn(&dev->dev,
> + "No IRQ - will use delays between transactions.\n");
> + }
> +
> + phy->priv.rng_quality = rng_quality;
> +
> + rc = tpm_tis_core_init(&dev->dev, &phy->priv, -1, &cr50_spi_phy_ops,
> + NULL);
> + if (rc < 0)
> + return rc;
> +
> + cr50_get_fw_version(&phy->priv, fw_ver);
> + dev_info(&dev->dev, "Cr50 firmware version: %s\n", fw_ver);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cr50_spi_resume(struct device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
> + struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
> +
> + /*
> + * Jiffies not increased during suspend, so we need to reset
> + * the time to wake Cr50 after resume.
> + */
> + phy->wake_after_jiffies = jiffies;
> +
> + return cr50_resume(dev);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cr50_spi_pm, cr50_suspend, cr50_spi_resume);
> +
> +static int cr50_spi_remove(struct spi_device *dev)
> +{
> + struct tpm_chip *chip = spi_get_drvdata(dev);
> +
> + tpm_chip_unregister(chip);
> + tpm_tis_remove(chip);
> + return 0;
> +}
> +
> +static const struct spi_device_id cr50_spi_id[] = {
> + { "cr50", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, cr50_spi_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cr50_spi_match[] = {
> + { .compatible = "google,cr50", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_cr50_spi_match);
> +#endif
> +
> +static struct spi_driver cr50_spi_driver = {
> + .driver = {
> + .name = "cr50_spi",
> + .pm = &cr50_spi_pm,
> + .of_match_table = of_match_ptr(of_cr50_spi_match),
> + },
> + .probe = cr50_spi_probe,
> + .remove = cr50_spi_remove,
> + .id_table = cr50_spi_id,
> +};
> +module_spi_driver(cr50_spi_driver);
> +
> +MODULE_DESCRIPTION("Cr50 TCG PTP FIFO SPI driver");
> +MODULE_LICENSE("GPL v2");
This copies a lot of code from tpm_tis_spi, but then slightly changes
some things, without really explaining why. For example, struct
cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
single iobuf, that is allocated via devm_kmalloc instead of being part
of the struct. Maybe the difference matters, maybe not, who knows?
Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
tpm_tis_spi, so that it can intercept the calls, execute the additional
actions (like waking up the device), but then let tpm_tis_spi do the
common work?
Alexander
On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Andrey Pronin <[email protected]>
> >
> > Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
> > firmware. The firmware running on the currently supported H1
> > Secure Microcontroller requires a special driver to handle its
> > specifics:
> > - need to ensure a certain delay between spi transactions, or else
> > the chip may miss some part of the next transaction;
> > - if there is no spi activity for some time, it may go to sleep,
> > and needs to be waken up before sending further commands;
> > - access to vendor-specific registers.
> >
> > Signed-off-by: Andrey Pronin <[email protected]>
> > [[email protected]: Replace boilerplate with SPDX tag, drop
> > suspended bit and remove ifdef checks in cr50.h, push tpm.h
> > include into cr50.c]
> > Signed-off-by: Stephen Boyd <[email protected]>
> > drivers/char/tpm/Kconfig | 16 ++
> > drivers/char/tpm/Makefile | 2 +
> > drivers/char/tpm/cr50.c | 33 +++
> > drivers/char/tpm/cr50.h | 15 ++
> > drivers/char/tpm/cr50_spi.c | 450 ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 516 insertions(+)
> > create mode 100644 drivers/char/tpm/cr50.c
> > create mode 100644 drivers/char/tpm/cr50.h
> > create mode 100644 drivers/char/tpm/cr50_spi.c
> >
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index 88a3c06fc153..b7028bfa6f87 100644
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -114,6 +114,22 @@ config TCG_ATMEL
> > will be accessible from within Linux. To compile this driver
> > as a module, choose M here; the module will be called tpm_atmel.
> > +config TCG_CR50
> > + bool
> > + ---help---
> > + Common routines shared by drivers for Cr50-based devices.
> > +
>
> Is it a common pattern to add config options that are not useful on their
> own? When would I ever enable TCG_CR50 without also enabling TCG_CR50_SPI?
> Why can't you just use TCG_CR50_SPI for everything?
This is an internal kconfig symbol, it isn't seen by the user, which
is a pretty normal pattern.
But I don't think the help should be included (since it cannot be
seen), and I'm pretty sure it should be a tristate
But overall, it might be better to just double link the little helper:
obj-$(CONFIG_TCG_CR50_SPI) += cr50.o cr50_spi.o
obj-$(CONFIG_TCG_CR50_I2C) += cr50.o cr50_i2c.o
As we don't actually ever expect to load both modules into the same
system
Jason
On 17.07.2019 00:45, Stephen Boyd wrote:
> From: Duncan Laurie <[email protected]>
>
> 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.
> 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 <[email protected]>
> [[email protected]: 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 <[email protected]>
> ---
> 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 <[email protected]>
> + * 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.
> +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?
> + usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI);
> + }
> +
> + return rc;
> +}
> +
> +/*
> + * cr50_i2c_read() - read from TPM register
> + *
> + * @chip: TPM chip information
> + * @addr: register address to read from
> + * @buffer: provided by caller
> + * @len: number of bytes to read
> + *
> + * 1) send register address byte 'addr' to the TPM
> + * 2) wait for TPM to indicate it is ready
> + * 3) read 'len' bytes of TPM response into the provided 'buffer'
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t len)
> +{
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
> + .addr = client->addr,
> + .len = 1,
> + .buf = &addr
> + };
> + struct i2c_msg msg2 = {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = buffer
> + };
> + int rc;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send the register address byte to the TPM */
> + rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg1, 1);
> + if (rc <= 0)
> + goto out;
> +
> + /* Wait for TPM to be ready with response data */
> + rc = cr50_i2c_wait_tpm_ready(chip);
> + if (rc < 0)
> + goto out;
> +
> + /* Read response data from the TPM */
> + rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg2, 1);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> + if (rc == 0)
> + return -EIO; /* No i2c segments transferred */
> +
> + return 0;
> +}
> +
> +/*
> + * cr50_i2c_write() - write to TPM register
> + *
> + * @chip: TPM chip information
> + * @addr: register address to write to
> + * @buffer: data to write
> + * @len: number of bytes to write
> + *
> + * 1) prepend the provided address to the provided data
> + * 2) send the address+data to the TPM
> + * 3) wait for TPM to indicate it is done writing
> + *
> + * Returns negative number for error, 0 for success.
> + */
> +static int cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
> + size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + struct i2c_client *client = to_i2c_client(chip->dev.parent);
> + struct i2c_msg msg1 = {
> + .addr = client->addr,
> + .len = len + 1,
> + .buf = priv->buf
> + };
> + int rc;
> +
> + if (len > CR50_MAX_BUFSIZE)
> + return -EINVAL;
> +
> + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + /* Prepend the 'register address' to the buffer */
> + priv->buf[0] = addr;
> + memcpy(priv->buf + 1, buffer, len);
> +
> + /* Prepare for completion interrupt */
> + cr50_i2c_enable_tpm_irq(chip);
> +
> + /* Send write request buffer with address */
> + rc = cr50_i2c_transfer(&chip->dev, client->adapter, &msg1, 1);
> + if (rc <= 0)
> + goto out;
> +
> + /* Wait for TPM to be ready, ignore timeout */
> + cr50_i2c_wait_tpm_ready(chip);
> +
> +out:
> + cr50_i2c_disable_tpm_irq(chip);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> + if (rc < 0)
> + return rc;
> + if (rc == 0)
> + return -EIO; /* No i2c segments transferred */
> +
> + return 0;
> +}
> +
> +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?
> +
> +#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))
> +
> +static int check_locality(struct tpm_chip *chip, int loc)
> +{
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
> + u8 buf;
> + int rc;
> +
> + rc = cr50_i2c_read(chip, TPM_ACCESS(loc), &buf, 1);
> + if (rc < 0)
> + return rc;
> +
> + if ((buf & mask) == mask)
> + return loc;
> +
> + return -EIO;
> +}
> +
> +static void release_locality(struct tpm_chip *chip, int force)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
> + u8 addr = TPM_ACCESS(priv->locality);
> + u8 buf;
> +
> + if (cr50_i2c_read(chip, addr, &buf, 1) < 0)
> + return;
> +
> + if (force || (buf & mask) == mask) {
> + buf = TPM_ACCESS_ACTIVE_LOCALITY;
> + cr50_i2c_write(chip, addr, &buf, 1);
> + }
> +
> + priv->locality = 0;
> +}
> +
> +static int request_locality(struct tpm_chip *chip, int loc)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf = TPM_ACCESS_REQUEST_USE;
> + unsigned long stop;
> + int rc;
> +
> + if (check_locality(chip, loc) == loc)
> + return loc;
> +
> + rc = cr50_i2c_write(chip, TPM_ACCESS(loc), &buf, 1);
> + if (rc < 0)
> + return rc;
> +
> + stop = jiffies + chip->timeout_a;
> + do {
> + if (check_locality(chip, loc) == loc) {
> + priv->locality = loc;
> + return loc;
> + }
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + return -ETIMEDOUT;
> +}
> +
> +/* cr50 requires all 4 bytes of status register to be read */
> +static u8 cr50_i2c_tis_status(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4];
> +
> + if (cr50_i2c_read(chip, TPM_STS(priv->locality), buf, sizeof(buf)) < 0)
> + return 0;
> + return buf[0];
> +}
> +
> +/* cr50 requires all 4 bytes of status register to be written */
> +static void cr50_i2c_tis_ready(struct tpm_chip *chip)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + u8 buf[4] = { TPM_STS_COMMAND_READY };
> +
> + cr50_i2c_write(chip, TPM_STS(priv->locality), buf, sizeof(buf));
> + msleep(CR50_TIMEOUT_SHORT_MS);
> +}
> +
> +/*
> + * cr50 uses bytes 3:2 of status register for burst count and
> + * all 4 bytes must be read
> + */
> +static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 mask,
> + size_t *burst, int *status)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + unsigned long stop;
> + u8 buf[4];
> +
> + /* wait for burstcount */
> + stop = jiffies + chip->timeout_b;
> + do {
> + if (cr50_i2c_read(chip, TPM_STS(priv->locality),
> + (u8 *)&buf, sizeof(buf)) < 0) {
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + continue;
> + }
> +
> + *status = *buf;
> + *burst = le16_to_cpup((__le16 *)(buf + 1));
> +
> + if ((*status & mask) == mask &&
> + *burst > 0 && *burst <= CR50_MAX_BUFSIZE)
> + return 0;
> +
> + msleep(CR50_TIMEOUT_SHORT_MS);
> + } while (time_before(jiffies, stop));
> +
> + dev_err(&chip->dev, "Timeout reading burst and status\n");
> + return -ETIMEDOUT;
> +}
> +
> +static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int status, rc;
> + size_t burstcnt, cur, len, expected;
> + u8 addr = TPM_DATA_FIFO(priv->locality);
> + u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
> +
> + if (buf_len < TPM_HEADER_SIZE)
> + return -EINVAL;
> +
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
> + dev_err(&chip->dev,
> + "Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
> + burstcnt, buf_len, TPM_HEADER_SIZE);
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Read first chunk of burstcnt bytes */
> + rc = cr50_i2c_read(chip, addr, buf, burstcnt);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read of first chunk failed\n");
> + goto out_err;
> + }
> +
> + /* Determine expected data in the return buffer */
> + expected = be32_to_cpup((__be32 *)(buf + 2));
> + if (expected > buf_len) {
> + dev_err(&chip->dev, "Too much data in FIFO\n");
> + goto out_err;
> + }
> +
> + /* Now read the rest of the data */
> + cur = burstcnt;
> + while (cur < expected) {
> + /* Read updated burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + len = min_t(size_t, burstcnt, expected - cur);
> + rc = cr50_i2c_read(chip, addr, buf + cur, len);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Read failed\n");
> + goto out_err;
> + }
> +
> + cur += len;
> + }
> +
> + /* Ensure TPM is done reading data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_AVAIL) {
> + dev_err(&chip->dev, "Data still available\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + release_locality(chip, 0);
> + return cur;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + release_locality(chip, 0);
> + return rc;
> +}
> +
> +static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> + struct priv_data *priv = dev_get_drvdata(&chip->dev);
> + int rc, status;
> + size_t burstcnt, limit, sent = 0;
> + u8 tpm_go[4] = { TPM_STS_GO };
> + unsigned long stop;
> +
> + rc = request_locality(chip, 0);
> + if (rc < 0)
> + return rc;
> +
> + /* Wait until TPM is ready for a command */
> + stop = jiffies + chip->timeout_b;
> + while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
> + if (time_after(jiffies, stop)) {
> + rc = -ETIMEDOUT;
> + goto out_err;
> + }
> +
> + cr50_i2c_tis_ready(chip);
> + }
> +
> + while (len > 0) {
> + u8 mask = TPM_STS_VALID;
> +
> + /* Wait for data if this is not the first chunk */
> + if (sent > 0)
> + mask |= TPM_STS_DATA_EXPECT;
> +
> + /* Read burst count and check status */
> + rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> +
> + /*
> + * Use burstcnt - 1 to account for the address byte
> + * that is inserted by cr50_i2c_write()
> + */
> + limit = min_t(size_t, burstcnt - 1, len);
> + rc = cr50_i2c_write(chip, TPM_DATA_FIFO(priv->locality),
> + &buf[sent], limit);
> + if (rc < 0) {
> + dev_err(&chip->dev, "Write failed\n");
> + goto out_err;
> + }
> +
> + sent += limit;
> + len -= limit;
> + }
> +
> + /* Ensure TPM is not expecting more data */
> + rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
> + if (rc < 0)
> + goto out_err;
> + if (status & TPM_STS_DATA_EXPECT) {
> + dev_err(&chip->dev, "Data still expected\n");
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* Start the TPM command */
> + rc = cr50_i2c_write(chip, TPM_STS(priv->locality), tpm_go,
> + sizeof(tpm_go));
> + if (rc < 0) {
> + dev_err(&chip->dev, "Start command failed\n");
> + goto out_err;
> + }
> + return 0;
> +
> +out_err:
> + /* Abort current transaction if still pending */
> + if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
> + cr50_i2c_tis_ready(chip);
> +
> + release_locality(chip, 0);
> + return rc;
> +}
> +
> +static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> + return (status == TPM_STS_COMMAND_READY);
> +}
> +
> +static const struct tpm_class_ops cr50_i2c = {
> + .flags = TPM_OPS_AUTO_STARTUP,
> + .status = &cr50_i2c_tis_status,
> + .recv = &cr50_i2c_tis_recv,
> + .send = &cr50_i2c_tis_send,
> + .cancel = &cr50_i2c_tis_ready,
> + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_canceled = &cr50_i2c_req_canceled,
> +};
> +
> +static int cr50_i2c_init(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct tpm_chip *chip;
> + struct priv_data *priv;
> + u8 buf[4];
> + u32 vendor;
> + int rc;
> +
> + chip = tpmm_chip_alloc(dev, &cr50_i2c);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* cr50 is a TPM 2.0 chip */
> + chip->flags |= TPM_CHIP_FLAG_TPM2;
> +
> + /* Default timeouts */
> + chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> + chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +
> + dev_set_drvdata(&chip->dev, priv);
> + init_completion(&priv->tpm_ready);
> +
> + if (client->irq > 0) {
> + rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + dev->driver->name, chip);
> + if (rc < 0) {
> + dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
> + return rc;
> + }
> +
> + disable_irq(client->irq);
> + priv->irq = client->irq;
> + } else {
> + dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
> + CR50_TIMEOUT_NOIRQ_MS);
> + }
> +
> + rc = request_locality(chip, 0);
> + if (rc < 0) {
> + dev_err(dev, "Could not request locality\n");
> + return rc;
> + }
> +
> + /* Read four bytes from DID_VID register */
> + rc = cr50_i2c_read(chip, TPM_DID_VID(0), buf, sizeof(buf));
> + if (rc < 0) {
> + dev_err(dev, "Could not read vendor id\n");
> + release_locality(chip, 1);
> + return rc;
> + }
> +
> + vendor = le32_to_cpup((__le32 *)buf);
> + if (vendor != CR50_I2C_DID_VID) {
> + dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
> + release_locality(chip, 1);
> + return -ENODEV;
> + }
> +
> + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
> + client->addr, client->irq, vendor >> 16);
> +
> + chip->hwrng.quality = rng_quality;
> +
> + return tpm_chip_register(chip);
> +}
> +
> +static const struct i2c_device_id cr50_i2c_table[] = {
> + {"cr50_i2c", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cr50_i2c_acpi_id[] = {
> + { "GOOG0005", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cr50_i2c_match[] = {
> + { .compatible = "google,cr50", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
> +#endif
> +
> +static int cr50_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -ENODEV;
> +
> + return cr50_i2c_init(client);
> +}
> +
> +static int cr50_i2c_remove(struct i2c_client *client)
> +{
> + struct tpm_chip *chip = i2c_get_clientdata(client);
> +
> + tpm_chip_unregister(chip);
> + release_locality(chip, 1);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, cr50_suspend, cr50_resume);
> +
> +static struct i2c_driver cr50_i2c_driver = {
> + .id_table = cr50_i2c_table,
> + .probe = cr50_i2c_probe,
> + .remove = cr50_i2c_remove,
> + .driver = {
> + .name = "cr50_i2c",
> + .pm = &cr50_i2c_pm,
> + .acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
> + .of_match_table = of_match_ptr(of_cr50_i2c_match),
> + },
> +};
> +
> +module_i2c_driver(cr50_i2c_driver);
> +
> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
> +MODULE_LICENSE("GPL")
Quoting Herbert Xu (2019-07-16 18:43:59)
> On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> > The hwrng_fill() function can run while devices are suspending and
> > resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> > is suspended, the hwrng may hang the bus while attempting to add some
> > randomness. It's been observed on ChromeOS devices with suspend-to-idle
> > (s2idle) and an i2c based hwrng that this kthread may run and ask the
> > hwrng device for randomness before the i2c bus has been resumed.
> >
> > Let's make this kthread freezable so that we don't try to touch the
> > hwrng during suspend/resume. This ensures that we can't cause the hwrng
> > backing driver to get into a bad state because the device is guaranteed
> > to be resumed before the hwrng kthread is thawed.
> >
> > Cc: Andrey Pronin <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: Duncan Laurie <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > drivers/char/hw_random/core.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
>
> Do you want this to go through the crypto tree? If so you need
> to cc the linux-crypto mailing list.
>
Sure. I'll resend just this one Cced to the linux-crypto list and to
you.
Quoting Jason Gunthorpe (2019-07-17 04:39:56)
> On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> > The hwrng_fill() function can run while devices are suspending and
> > resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> > is suspended, the hwrng may hang the bus while attempting to add some
> > randomness. It's been observed on ChromeOS devices with suspend-to-idle
> > (s2idle) and an i2c based hwrng that this kthread may run and ask the
> > hwrng device for randomness before the i2c bus has been resumed.
>
> You mean the TPM here right?
In my case yes, but in general it isn't the TPM.
>
> Should we be more careful in the TPM code to check if the TPM is
> suspended before trying to use it, rather than muck up callers?
>
Given that it's not just a TPM issue I don't see how checking in the TPM
is going to fix this problem. It's better to not try to get random bytes
from the hwrng when the device could be suspended.
Quoting Jason Gunthorpe (2019-07-17 05:25:58)
> On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index 88a3c06fc153..b7028bfa6f87 100644
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -114,6 +114,22 @@ config TCG_ATMEL
> > > will be accessible from within Linux. To compile this driver
> > > as a module, choose M here; the module will be called tpm_atmel.
> > > +config TCG_CR50
> > > + bool
> > > + ---help---
> > > + Common routines shared by drivers for Cr50-based devices.
> > > +
> >
> > Is it a common pattern to add config options that are not useful on their
> > own? When would I ever enable TCG_CR50 without also enabling TCG_CR50_SPI?
> > Why can't you just use TCG_CR50_SPI for everything?
>
> This is an internal kconfig symbol, it isn't seen by the user, which
> is a pretty normal pattern.
>
> But I don't think the help should be included (since it cannot be
> seen), and I'm pretty sure it should be a tristate
Good point. I'll fix it.
>
> But overall, it might be better to just double link the little helper:
>
> obj-$(CONFIG_TCG_CR50_SPI) += cr50.o cr50_spi.o
> obj-$(CONFIG_TCG_CR50_I2C) += cr50.o cr50_i2c.o
>
> As we don't actually ever expect to load both modules into the same
> system
>
Sometimes we have both drivers built-in. To maintain the tiny space
savings I'd prefer to just leave this as helpless and tristate.
On Wed, Jul 17, 2019 at 09:42:32AM -0700, Stephen Boyd wrote:
> Quoting Jason Gunthorpe (2019-07-17 04:39:56)
> > On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> > > The hwrng_fill() function can run while devices are suspending and
> > > resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> > > is suspended, the hwrng may hang the bus while attempting to add some
> > > randomness. It's been observed on ChromeOS devices with suspend-to-idle
> > > (s2idle) and an i2c based hwrng that this kthread may run and ask the
> > > hwrng device for randomness before the i2c bus has been resumed.
> >
> > You mean the TPM here right?
>
> In my case yes, but in general it isn't the TPM.
>
> >
> > Should we be more careful in the TPM code to check if the TPM is
> > suspended before trying to use it, rather than muck up callers?
> >
>
> Given that it's not just a TPM issue I don't see how checking in the TPM
> is going to fix this problem. It's better to not try to get random bytes
> from the hwrng when the device could be suspended.
I think the same comment would apply to all the other suspend capable
hwrngs...
It just seems weird to do this, what about all the other tpm API
users? Do they have a racy problem with suspend too?
Jason
On Wed, Jul 17, 2019 at 09:49:42AM -0700, Stephen Boyd wrote:
> Quoting Jason Gunthorpe (2019-07-17 05:25:58)
> > On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> > > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > index 88a3c06fc153..b7028bfa6f87 100644
> > > > +++ b/drivers/char/tpm/Kconfig
> > > > @@ -114,6 +114,22 @@ config TCG_ATMEL
> > > > will be accessible from within Linux. To compile this driver
> > > > as a module, choose M here; the module will be called tpm_atmel.
> > > > +config TCG_CR50
> > > > + bool
> > > > + ---help---
> > > > + Common routines shared by drivers for Cr50-based devices.
> > > > +
> > >
> > > Is it a common pattern to add config options that are not useful on their
> > > own? When would I ever enable TCG_CR50 without also enabling TCG_CR50_SPI?
> > > Why can't you just use TCG_CR50_SPI for everything?
> >
> > This is an internal kconfig symbol, it isn't seen by the user, which
> > is a pretty normal pattern.
> >
> > But I don't think the help should be included (since it cannot be
> > seen), and I'm pretty sure it should be a tristate
>
> Good point. I'll fix it.
>
> >
> > But overall, it might be better to just double link the little helper:
> >
> > obj-$(CONFIG_TCG_CR50_SPI) += cr50.o cr50_spi.o
> > obj-$(CONFIG_TCG_CR50_I2C) += cr50.o cr50_i2c.o
> >
> > As we don't actually ever expect to load both modules into the same
> > system
> >
>
> Sometimes we have both drivers built-in. To maintain the tiny space
> savings I'd prefer to just leave this as helpless and tristate.
If it is builtin you only get one copy of cr50.o anyhow. The only
differences is for modules, then the two modules will both be a bit
bigger instead of a 3rd module being created
Jason
Quoting Jason Gunthorpe (2019-07-17 09:50:11)
> On Wed, Jul 17, 2019 at 09:42:32AM -0700, Stephen Boyd wrote:
> > Quoting Jason Gunthorpe (2019-07-17 04:39:56)
> > > On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> > > > The hwrng_fill() function can run while devices are suspending and
> > > > resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> > > > is suspended, the hwrng may hang the bus while attempting to add some
> > > > randomness. It's been observed on ChromeOS devices with suspend-to-idle
> > > > (s2idle) and an i2c based hwrng that this kthread may run and ask the
> > > > hwrng device for randomness before the i2c bus has been resumed.
> > >
> > > You mean the TPM here right?
> >
> > In my case yes, but in general it isn't the TPM.
> >
> > >
> > > Should we be more careful in the TPM code to check if the TPM is
> > > suspended before trying to use it, rather than muck up callers?
> > >
> >
> > Given that it's not just a TPM issue I don't see how checking in the TPM
> > is going to fix this problem. It's better to not try to get random bytes
> > from the hwrng when the device could be suspended.
>
> I think the same comment would apply to all the other suspend capable
> hwrngs...
Yes. That's exactly my point. A hwrng that's suspended will fail here
and it's better to just not try until it's guaranteed to have resumed.
>
> It just seems weird to do this, what about all the other tpm API
> users? Do they have a racy problem with suspend too?
I haven't looked at them. Are they being called from suspend/resume
paths? I don't think anything for the userspace API can be a problem
because those tasks are all frozen. The only problem would be some
kernel internal API that TPM API exposes. I did a quick grep and I see
things like IMA or the trusted keys APIs that might need a closer look.
Either way, trying to hold off a TPM operation from the TPM API when
we're suspended isn't really possible. If something like IMA needs to
get TPM data from deep suspend path and it fails because the device is
suspended, all we can do is return an error from TPM APIs and hope the
caller can recover. The fix is probably going to be to change the code
to not call into the TPM API until the hardware has resumed by avoiding
doing anything with the TPM until resume is over. So we're at best able
to make same sort of band-aid here in the TPM API where all we can do is
say -EAGAIN but we can't tell the caller when to try again.
Quoting Jason Gunthorpe (2019-07-17 09:56:28)
> On Wed, Jul 17, 2019 at 09:49:42AM -0700, Stephen Boyd wrote:
> > Quoting Jason Gunthorpe (2019-07-17 05:25:58)
> > > On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> > > > On 17.07.2019 00:45, Stephen Boyd wrote:
> > >
> > > But overall, it might be better to just double link the little helper:
> > >
> > > obj-$(CONFIG_TCG_CR50_SPI) += cr50.o cr50_spi.o
> > > obj-$(CONFIG_TCG_CR50_I2C) += cr50.o cr50_i2c.o
> > >
> > > As we don't actually ever expect to load both modules into the same
> > > system
> > >
> >
> > Sometimes we have both drivers built-in. To maintain the tiny space
> > savings I'd prefer to just leave this as helpless and tristate.
>
> If it is builtin you only get one copy of cr50.o anyhow. The only
> differences is for modules, then the two modules will both be a bit
> bigger instead of a 3rd module being created
>
Yes. The space savings comes from having the extra module 'cr50.ko' that
holds almost nothing at all when the two drivers are modules.
On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> Quoting Jason Gunthorpe (2019-07-17 09:56:28)
> > On Wed, Jul 17, 2019 at 09:49:42AM -0700, Stephen Boyd wrote:
> > > Quoting Jason Gunthorpe (2019-07-17 05:25:58)
> > > > On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> > > > > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > >
> > > > But overall, it might be better to just double link the little helper:
> > > >
> > > > obj-$(CONFIG_TCG_CR50_SPI) += cr50.o cr50_spi.o
> > > > obj-$(CONFIG_TCG_CR50_I2C) += cr50.o cr50_i2c.o
> > > >
> > > > As we don't actually ever expect to load both modules into the same
> > > > system
> > > >
> > >
> > > Sometimes we have both drivers built-in. To maintain the tiny space
> > > savings I'd prefer to just leave this as helpless and tristate.
> >
> > If it is builtin you only get one copy of cr50.o anyhow. The only
> > differences is for modules, then the two modules will both be a bit
> > bigger instead of a 3rd module being created
> >
>
> Yes. The space savings comes from having the extra module 'cr50.ko' that
> holds almost nothing at all when the two drivers are modules.
I'm not sure it is an actual savings, there is alot of minimum
overhead and alignment to have a module in the first place.
Jason
Quoting Jason Gunthorpe (2019-07-17 10:12:16)
> On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> >
> > Yes. The space savings comes from having the extra module 'cr50.ko' that
> > holds almost nothing at all when the two drivers are modules.
>
> I'm not sure it is an actual savings, there is alot of minimum
> overhead and alignment to have a module in the first place.
>
Yeah. I'm pretty sure that's why it's a bool and not a tristate for this
symbol. A module has overhead that is not necessary for these little
helpers.
On Wed, Jul 17, 2019 at 10:22:20AM -0700, Stephen Boyd wrote:
> Quoting Jason Gunthorpe (2019-07-17 10:12:16)
> > On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> > >
> > > Yes. The space savings comes from having the extra module 'cr50.ko' that
> > > holds almost nothing at all when the two drivers are modules.
> >
> > I'm not sure it is an actual savings, there is alot of minimum
> > overhead and alignment to have a module in the first place.
> >
>
> Yeah. I'm pretty sure that's why it's a bool and not a tristate for this
> symbol. A module has overhead that is not necessary for these little
> helpers.
Linking driver stuff like that to the kernel is pretty hacky, IMHO
Jason
Quoting Jason Gunthorpe (2019-07-17 10:25:44)
> On Wed, Jul 17, 2019 at 10:22:20AM -0700, Stephen Boyd wrote:
> > Quoting Jason Gunthorpe (2019-07-17 10:12:16)
> > > On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> > > >
> > > > Yes. The space savings comes from having the extra module 'cr50.ko' that
> > > > holds almost nothing at all when the two drivers are modules.
> > >
> > > I'm not sure it is an actual savings, there is alot of minimum
> > > overhead and alignment to have a module in the first place.
> > >
> >
> > Yeah. I'm pretty sure that's why it's a bool and not a tristate for this
> > symbol. A module has overhead that is not necessary for these little
> > helpers.
>
> Linking driver stuff like that to the kernel is pretty hacky, IMHO
>
So combine lines?
obj-$(CONFIG_...) += cr50.o cr50_spi.o
Sounds great.
Quoting Alexander Steffen (2019-07-17 01:07:11)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Andrey Pronin <[email protected]>
> >
> > Reject burstcounts larger than 64 bytes reported by tpm.
>
> This is not the correct thing to do here. To quote the specification:
>
> "burstCount is defined as the number of bytes that can be written to or
> read from the data FIFO by the software without incurring a wait state."
> (https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf
> Page 84)
Thanks for pointing this out. I think we were using this SPI driver for
cr50 but then we wrote our own version of this driver with the
differences required to make cr50 work properly. I suspect we can drop
this patch, but we've been carrying it forward for a while now, so I'll
have to check with Andrey and others to make sure it's safe to remove.
>
> If the FIFO contains 1k of data, it is completely valid for the TPM to
> report that as its burstCount, there is no need to arbitrarily limit it.
>
> Also, burstCount is a property of the high-level TIS protocol, that
> should not really care whether the low-level transfers are done via LPC
> or SPI (or I2C). Since tpm_tis_spi can only transfer 64 bytes at a time,
> it is its job to split larger transfers (which it does perfectly fine).
> This also has the advantage that burstCount needs only to be read once,
> and then we can do 16 SPI transfers in a row to read that 1k of data.
> With your change, it will read 64 bytes, then read burstCount again,
> before reading the next 64 bytes and so on. This unnecessarily limits
> performance.
>
> Maybe you can describe the problem you're trying to solve in more
> detail, so that a better solution can be found, since this is clearly
> something not intended by the spec.
Right. The burst count we read from cr50 is never going to be larger
than max_xfer_size that we specify in the cr50 driver here, so this is
probably all useless and we can even drop the patch before this one that
adds support for this burst count capping feature.
On Wed, Jul 17, 2019 at 11:21 AM Stephen Boyd <[email protected]> wrote:
>
> Quoting Jason Gunthorpe (2019-07-17 10:25:44)
> > On Wed, Jul 17, 2019 at 10:22:20AM -0700, Stephen Boyd wrote:
> > > Quoting Jason Gunthorpe (2019-07-17 10:12:16)
> > > > On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> > > > >
> > > > > Yes. The space savings comes from having the extra module 'cr50.ko' that
> > > > > holds almost nothing at all when the two drivers are modules.
> > > >
> > > > I'm not sure it is an actual savings, there is alot of minimum
> > > > overhead and alignment to have a module in the first place.
> > > >
> > >
> > > Yeah. I'm pretty sure that's why it's a bool and not a tristate for this
> > > symbol. A module has overhead that is not necessary for these little
> > > helpers.
> >
> > Linking driver stuff like that to the kernel is pretty hacky, IMHO
> >
>
> So combine lines?
>
> obj-$(CONFIG_...) += cr50.o cr50_spi.o
>
> Sounds great.
>
Please keep in mind that cr50.c exports symbols. If cr50.o is added to
two modules, those symbols will subsequently available from both
modules. To avoid that, you might want to consider removing the
EXPORT_SYMBOL() declarations from cr50.c.
I don't know what happens if those two modules are both built into the
kernel (as happens, for example, with allyesconfig). Does the linker
try to load cr50.o twice, resulting in duplicate symbols ?
Guenter
On Wed, Jul 17, 2019 at 11:30:42AM -0700, Guenter Roeck wrote:
> On Wed, Jul 17, 2019 at 11:21 AM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Jason Gunthorpe (2019-07-17 10:25:44)
> > > On Wed, Jul 17, 2019 at 10:22:20AM -0700, Stephen Boyd wrote:
> > > > Quoting Jason Gunthorpe (2019-07-17 10:12:16)
> > > > > On Wed, Jul 17, 2019 at 10:05:52AM -0700, Stephen Boyd wrote:
> > > > > >
> > > > > > Yes. The space savings comes from having the extra module 'cr50.ko' that
> > > > > > holds almost nothing at all when the two drivers are modules.
> > > > >
> > > > > I'm not sure it is an actual savings, there is alot of minimum
> > > > > overhead and alignment to have a module in the first place.
> > > > >
> > > >
> > > > Yeah. I'm pretty sure that's why it's a bool and not a tristate for this
> > > > symbol. A module has overhead that is not necessary for these little
> > > > helpers.
> > >
> > > Linking driver stuff like that to the kernel is pretty hacky, IMHO
> > >
> >
> > So combine lines?
> >
> > obj-$(CONFIG_...) += cr50.o cr50_spi.o
> >
> > Sounds great.
> >
>
> Please keep in mind that cr50.c exports symbols. If cr50.o is added to
> two modules, those symbols will subsequently available from both
> modules. To avoid that, you might want to consider removing the
> EXPORT_SYMBOL() declarations from cr50.c.
Yep
> I don't know what happens if those two modules are both built into the
> kernel (as happens, for example, with allyesconfig). Does the linker
> try to load cr50.o twice, resulting in duplicate symbols ?
Hum. Looks like it uses --whole-archive here and would probably
break? Maybe not, hns recently sent a patch doing this, but maybe they
never tested it too.
Jason
Quoting Alexander Steffen (2019-07-17 05:00:06)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Andrey Pronin <[email protected]>
> >
> > Add TPM2.0 PTP FIFO compatible SPI interface for chips with Cr50
> > firmware. The firmware running on the currently supported H1
> > Secure Microcontroller requires a special driver to handle its
> > specifics:
> > - need to ensure a certain delay between spi transactions, or else
> > the chip may miss some part of the next transaction;
> > - if there is no spi activity for some time, it may go to sleep,
> > and needs to be waken up before sending further commands;
> > - access to vendor-specific registers.
> >
> > Signed-off-by: Andrey Pronin <[email protected]>
> > [[email protected]: Replace boilerplate with SPDX tag, drop
> > suspended bit and remove ifdef checks in cr50.h, push tpm.h
> > include into cr50.c]
> > Signed-off-by: Stephen Boyd <[email protected]>
> > diff --git a/drivers/char/tpm/cr50_spi.c b/drivers/char/tpm/cr50_spi.c
> > new file mode 100644
> > index 000000000000..3c1b472297ad
> > --- /dev/null
> > +++ b/drivers/char/tpm/cr50_spi.c
> > @@ -0,0 +1,450 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2016 Google, Inc
> > + *
> > + * This device driver implements a TCG PTP FIFO interface over SPI for chips
> > + * with Cr50 firmware.
> > + * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pm.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/wait.h>
> > +#include "cr50.h"
> > +#include "tpm_tis_core.h"
> > +
> > +/*
> > + * Cr50 timing constants:
> > + * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
> > + * - needs up to CR50_WAKE_START_DELAY_MSEC to wake after sleep.
> > + * - requires waiting for "ready" IRQ, if supported; or waiting for at least
> > + * CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
> > + * - waits for up to CR50_FLOW_CONTROL_MSEC for flow control 'ready' indication.
> > + */
> > +#define CR50_SLEEP_DELAY_MSEC 1000
> > +#define CR50_WAKE_START_DELAY_MSEC 1
> > +#define CR50_NOIRQ_ACCESS_DELAY_MSEC 2
> > +#define CR50_READY_IRQ_TIMEOUT_MSEC TPM2_TIMEOUT_A
> > +#define CR50_FLOW_CONTROL_MSEC TPM2_TIMEOUT_A
> > +#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
> > +
> > +#define MAX_SPI_FRAMESIZE 64
> > +
> > +#define TPM_CR50_FW_VER(l) (0x0F90 | ((l) << 12))
> > +#define TPM_CR50_MAX_FW_VER_LEN 64
> > +
> > +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.");
>
> What is the purpose of this parameter? None of the other TPM drivers
> have it.
I think the idea is to let users override the quality if they decide
that they don't want to use the default value specified in the driver.
>
> > +
> > +struct cr50_spi_phy {
> > + struct tpm_tis_data priv;
> > + struct spi_device *spi_device;
> > +
> > + struct mutex time_track_mutex;
> > + unsigned long last_access_jiffies;
> > + unsigned long wake_after_jiffies;
> > +
> > + unsigned long access_delay_jiffies;
> > + unsigned long sleep_delay_jiffies;
> > + unsigned int wake_start_delay_msec;
> > +
> > + struct completion tpm_ready;
> > +
> > + unsigned int irq_confirmation_attempt;
> > + bool irq_needs_confirmation;
> > + bool irq_confirmed;
> > +
> > + u8 tx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> > + u8 rx_buf[MAX_SPI_FRAMESIZE] ____cacheline_aligned;
> > +};
> > +
> > +static struct cr50_spi_phy *to_cr50_spi_phy(struct tpm_tis_data *data)
> > +{
> > + return container_of(data, struct cr50_spi_phy, priv);
> > +}
> > +
> > +/*
> > + * 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_spi_irq_handler(int dummy, void *dev_id)
> > +{
> > + struct cr50_spi_phy *phy = dev_id;
> > +
> > + phy->irq_confirmed = true;
> > + complete(&phy->tpm_ready);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Cr50 needs to have at least some delay between consecutive
> > + * transactions. Make sure we wait.
> > + */
> > +static void cr50_ensure_access_delay(struct cr50_spi_phy *phy)
> > +{
> > + /*
> > + * Note: There is a small chance, if Cr50 is not accessed in a few days,
> > + * that time_in_range will not provide the correct result after the wrap
> > + * around for jiffies. In this case, we'll have an unneeded short delay,
> > + * which is fine.
> > + */
> > + unsigned long allowed_access =
> > + phy->last_access_jiffies + phy->access_delay_jiffies;
> > + unsigned long time_now = jiffies;
> > +
> > + if (time_in_range_open(time_now,
> > + phy->last_access_jiffies, allowed_access)) {
> > + unsigned long remaining =
> > + wait_for_completion_timeout(&phy->tpm_ready,
> > + allowed_access - time_now);
> > + if (remaining == 0 && phy->irq_confirmed) {
> > + dev_warn(&phy->spi_device->dev,
> > + "Timeout waiting for TPM ready IRQ\n");
> > + }
> > + }
> > + if (phy->irq_needs_confirmation) {
> > + if (phy->irq_confirmed) {
> > + phy->irq_needs_confirmation = false;
> > + phy->access_delay_jiffies =
> > + msecs_to_jiffies(CR50_READY_IRQ_TIMEOUT_MSEC);
> > + dev_info(&phy->spi_device->dev,
> > + "TPM ready IRQ confirmed on attempt %u\n",
> > + phy->irq_confirmation_attempt);
> > + } else if (++phy->irq_confirmation_attempt >
> > + MAX_IRQ_CONFIRMATION_ATTEMPTS) {
> > + phy->irq_needs_confirmation = false;
> > + dev_warn(&phy->spi_device->dev,
> > + "IRQ not confirmed - will use delays\n");
> > + }
> > + }
> > +}
> > +
> > +/*
> > + * Cr50 might go to sleep if there is no SPI activity for some time and
> > + * miss the first few bits/bytes on the bus. In such case, wake it up
> > + * by asserting CS and give it time to start up.
> > + */
> > +static bool cr50_needs_waking(struct cr50_spi_phy *phy)
> > +{
> > + /*
> > + * Note: There is a small chance, if Cr50 is not accessed in a few days,
> > + * that time_in_range will not provide the correct result after the wrap
> > + * around for jiffies. In this case, we'll probably timeout or read
> > + * incorrect value from TPM_STS and just retry the operation.
> > + */
> > + return !time_in_range_open(jiffies,
> > + phy->last_access_jiffies,
> > + phy->wake_after_jiffies);
> > +}
> > +
> > +static void cr50_wake_if_needed(struct cr50_spi_phy *phy)
> > +{
> > + if (cr50_needs_waking(phy)) {
> > + /* Assert CS, wait 1 msec, deassert CS */
> > + struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
> > +
> > + spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
> > + /* Wait for it to fully wake */
> > + msleep(phy->wake_start_delay_msec);
>
> wake_start_delay_msec is always 1, isn't it? (Why is that a variable at
> all? I see only one place that ever sets it.) Then msleep is not the
> best function to use, since it will usually sleep much longer. Use
> usleep_range instead. See Documentation/timers/timers-howto.txt.
Thanks. Will fix to be 1ms to 2ms range.
>
> > + }
> > + /* Reset the time when we need to wake Cr50 again */
> > + phy->wake_after_jiffies = jiffies + phy->sleep_delay_jiffies;
> > +}
> > +
> > +/*
> > + * Flow control: clock the bus and wait for cr50 to set LSB before
> > + * sending/receiving data. TCG PTP spec allows it to happen during
> > + * the last byte of header, but cr50 never does that in practice,
> > + * and earlier versions had a bug when it was set too early, so don't
> > + * check for it during header transfer.
> > + */
> > +static int cr50_spi_flow_control(struct cr50_spi_phy *phy)
> > +{
> > + unsigned long timeout_jiffies =
> > + jiffies + msecs_to_jiffies(CR50_FLOW_CONTROL_MSEC);
> > + struct spi_message m;
> > + int ret;
> > + struct spi_transfer spi_xfer = {
> > + .rx_buf = phy->rx_buf,
> > + .len = 1,
> > + .cs_change = 1,
> > + };
> > +
> > + do {
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + return ret;
> > + if (time_after(jiffies, timeout_jiffies)) {
> > + dev_warn(&phy->spi_device->dev,
> > + "Timeout during flow control\n");
> > + return -EBUSY;
> > + }
> > + } while (!(phy->rx_buf[0] & 0x01));
> > + return 0;
> > +}
> > +
> > +static int cr50_spi_xfer_bytes(struct tpm_tis_data *data, u32 addr,
> > + u16 len, const u8 *tx, u8 *rx)
> > +{
> > + struct cr50_spi_phy *phy = to_cr50_spi_phy(data);
> > + struct spi_message m;
> > + struct spi_transfer spi_xfer = {
> > + .tx_buf = phy->tx_buf,
> > + .rx_buf = phy->rx_buf,
> > + .len = 4,
> > + .cs_change = 1,
> > + };
> > + int ret;
> > +
> > + if (len > MAX_SPI_FRAMESIZE)
> > + return -EINVAL;
> > +
> > + /*
> > + * Do this outside of spi_bus_lock in case cr50 is not the
> > + * only device on that spi bus.
> > + */
> > + mutex_lock(&phy->time_track_mutex);
> > + cr50_ensure_access_delay(phy);
> > + cr50_wake_if_needed(phy);
> > +
> > + phy->tx_buf[0] = (tx ? 0x00 : 0x80) | (len - 1);
> > + phy->tx_buf[1] = 0xD4;
> > + phy->tx_buf[2] = (addr >> 8) & 0xFF;
> > + phy->tx_buf[3] = addr & 0xFF;
> > +
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > +
> > + spi_bus_lock(phy->spi_device->master);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + ret = cr50_spi_flow_control(phy);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + spi_xfer.cs_change = 0;
> > + spi_xfer.len = len;
> > + if (tx) {
> > + memcpy(phy->tx_buf, tx, len);
> > + spi_xfer.rx_buf = NULL;
> > + } else {
> > + spi_xfer.tx_buf = NULL;
> > + }
> > +
> > + spi_message_init(&m);
> > + spi_message_add_tail(&spi_xfer, &m);
> > + reinit_completion(&phy->tpm_ready);
> > + ret = spi_sync_locked(phy->spi_device, &m);
> > + if (rx)
> > + memcpy(rx, phy->rx_buf, len);
> > +
> > +exit:
> > + spi_bus_unlock(phy->spi_device->master);
> > + phy->last_access_jiffies = jiffies;
> > + mutex_unlock(&phy->time_track_mutex);
> > +
> > + return ret;
> > +}
>
> This copies a lot of code from tpm_tis_spi, but then slightly changes
> some things, without really explaining why.
The commit text has some explanations. Here's the copy/paste from above:
> > - need to ensure a certain delay between spi transactions, or else
> > the chip may miss some part of the next transaction;
> > - if there is no spi activity for some time, it may go to sleep,
> > and needs to be waken up before sending further commands;
> > - access to vendor-specific registers.
Do you want me to describe something further?
> For example, struct
> cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
> single iobuf, that is allocated via devm_kmalloc instead of being part
> of the struct. Maybe the difference matters, maybe not, who knows?
Ok. Are you asking if this is a full-duplex SPI device?
>
> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> tpm_tis_spi, so that it can intercept the calls, execute the additional
> actions (like waking up the device), but then let tpm_tis_spi do the
> common work?
>
I suppose the read{16,32} and write32 functions could be reused. I'm not
sure how great it will be if we combine these two drivers, but I can
give it a try today and see how it looks.
Quoting Stephen Boyd (2019-07-17 12:57:34)
> Quoting Alexander Steffen (2019-07-17 05:00:06)
> >
> > Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> > tpm_tis_spi, so that it can intercept the calls, execute the additional
> > actions (like waking up the device), but then let tpm_tis_spi do the
> > common work?
> >
>
> I suppose the read{16,32} and write32 functions could be reused. I'm not
> sure how great it will be if we combine these two drivers, but I can
> give it a try today and see how it looks.
>
Here's the patch. I haven't tested it besides compile testing.
----8<----
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 19513e622053..12f4026c3620 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -34,14 +34,54 @@
#include <linux/of_irq.h>
#include <linux/of_gpio.h>
#include <linux/tpm.h>
+#include "cr50.h"
#include "tpm.h"
#include "tpm_tis_core.h"
#define MAX_SPI_FRAMESIZE 64
+/*
+ * Cr50 timing constants:
+ * - can go to sleep not earlier than after CR50_SLEEP_DELAY_MSEC.
+ * - needs up to CR50_WAKE_START_DELAY_USEC to wake after sleep.
+ * - requires waiting for "ready" IRQ, if supported; or waiting for at least
+ * CR50_NOIRQ_ACCESS_DELAY_MSEC between transactions, if IRQ is not supported.
+ * - waits for up to CR50_FLOW_CONTROL for flow control 'ready' indication.
+ */
+#define CR50_SLEEP_DELAY_MSEC 1000
+#define CR50_WAKE_START_DELAY_USEC 1000
+#define CR50_NOIRQ_ACCESS_DELAY msecs_to_jiffies(2)
+#define CR50_READY_IRQ_TIMEOUT msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define CR50_FLOW_CONTROL msecs_to_jiffies(TPM2_TIMEOUT_A)
+#define MAX_IRQ_CONFIRMATION_ATTEMPTS 3
+
+#define TPM_CR50_FW_VER(l) (0x0f90 | ((l) << 12))
+#define TPM_CR50_MAX_FW_VER_LEN 64
+#define TIS_IS_CR50 1
+
+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 tpm_tis_spi_phy {
struct tpm_tis_data priv;
struct spi_device *spi_device;
+
+ struct mutex time_track_mutex;
+ unsigned long last_access;
+ unsigned long wake_after;
+
+ unsigned long access_delay;
+
+ struct completion ready;
+
+ unsigned int irq_confirmation_attempt;
+ bool irq_needs_confirmation;
+ bool irq_confirmed;
+ bool is_cr50;
+
u8 *iobuf;
};
@@ -50,6 +90,127 @@ static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_spi_phy, priv);
}
+/*
+ * 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_spi_irq_handler(int dummy, void *dev_id)
+{
+ struct tpm_tis_spi_phy *phy = dev_id;
+
+ phy->irq_confirmed = true;
+ complete(&phy->ready);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * Cr50 needs to have at least some delay between consecutive
+ * transactions. Make sure we wait.
+ */
+static void cr50_ensure_access_delay(struct tpm_tis_spi_phy *phy)
+{
+ unsigned long allowed_access = phy->last_access + phy->access_delay;
+ unsigned long time_now = jiffies;
+ struct device *dev = &phy->spi_device->dev;
+
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll have an unneeded short delay,
+ * which is fine.
+ */
+ if (time_in_range_open(time_now, phy->last_access, allowed_access)) {
+ unsigned long remaining, timeout = allowed_access - time_now;
+
+ remaining = wait_for_completion_timeout(&phy->ready, timeout);
+ if (!remaining && phy->irq_confirmed)
+ dev_warn(dev, "Timeout waiting for TPM ready IRQ\n");
+ }
+
+ if (phy->irq_needs_confirmation) {
+ unsigned int attempt = ++phy->irq_confirmation_attempt;
+
+ if (phy->irq_confirmed) {
+ phy->irq_needs_confirmation = false;
+ phy->access_delay = CR50_READY_IRQ_TIMEOUT;
+ dev_info(dev, "TPM ready IRQ confirmed on attempt %u\n",
+ attempt);
+ } else if (attempt > MAX_IRQ_CONFIRMATION_ATTEMPTS) {
+ phy->irq_needs_confirmation = false;
+ dev_warn(dev, "IRQ not confirmed - will use delays\n");
+ }
+ }
+}
+
+/*
+ * Cr50 might go to sleep if there is no SPI activity for some time and
+ * miss the first few bits/bytes on the bus. In such case, wake it up
+ * by asserting CS and give it time to start up.
+ */
+static bool cr50_needs_waking(struct tpm_tis_spi_phy *phy)
+{
+ /*
+ * Note: There is a small chance, if Cr50 is not accessed in a few days,
+ * that time_in_range will not provide the correct result after the wrap
+ * around for jiffies. In this case, we'll probably timeout or read
+ * incorrect value from TPM_STS and just retry the operation.
+ */
+ return !time_in_range_open(jiffies, phy->last_access, phy->wake_after);
+}
+
+static void cr50_wake_if_needed(struct tpm_tis_spi_phy *phy)
+{
+ if (cr50_needs_waking(phy)) {
+ /* Assert CS, wait 1 msec, deassert CS */
+ struct spi_transfer spi_cs_wake = { .delay_usecs = 1000 };
+
+ spi_sync_transfer(phy->spi_device, &spi_cs_wake, 1);
+ /* Wait for it to fully wake */
+ usleep_range(CR50_WAKE_START_DELAY_USEC,
+ CR50_WAKE_START_DELAY_USEC * 2);
+ }
+ /* Reset the time when we need to wake Cr50 again */
+ phy->wake_after = jiffies + msecs_to_jiffies(CR50_SLEEP_DELAY_MSEC);
+
+}
+
+/*
+ * Flow control: clock the bus and wait for cr50 to set LSB before
+ * sending/receiving data. TCG PTP spec allows it to happen during
+ * the last byte of header, but cr50 never does that in practice,
+ * and earlier versions had a bug when it was set too early, so don't
+ * check for it during header transfer.
+ */
+static int cr50_spi_flow_control(struct tpm_tis_spi_phy *phy)
+{
+ struct device *dev = &phy->spi_device->dev;
+ unsigned long timeout = jiffies + CR50_FLOW_CONTROL;
+ struct spi_message m;
+ int ret;
+ struct spi_transfer spi_xfer = {
+ .rx_buf = phy->iobuf,
+ .len = 1,
+ .cs_change = 1,
+ };
+
+ do {
+ spi_message_init(&m);
+ spi_message_add_tail(&spi_xfer, &m);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ return ret;
+
+ if (time_after(jiffies, timeout)) {
+ dev_warn(dev, "Timeout during flow control\n");
+ return -EBUSY;
+ }
+ } while (!(phy->iobuf[0] & 0x01));
+
+ return 0;
+}
+
static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
u8 *in, const u8 *out)
{
@@ -60,6 +221,12 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
struct spi_transfer spi_xfer;
u8 transfer_len;
+ mutex_lock(&phy->time_track_mutex);
+ if (phy->is_cr50) {
+ cr50_ensure_access_delay(phy);
+ cr50_wake_if_needed(phy);
+ }
+
spi_bus_lock(phy->spi_device->master);
while (len) {
@@ -82,7 +249,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
if (ret < 0)
goto exit;
- if ((phy->iobuf[3] & 0x01) == 0) {
+ if (phy->is_cr50) {
+ ret = cr50_spi_flow_control(phy);
+ if (ret < 0)
+ goto exit;
+ } else if ((phy->iobuf[3] & 0x01) == 0) {
// handle SPI wait states
phy->iobuf[0] = 0;
@@ -117,6 +288,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
spi_message_init(&m);
spi_message_add_tail(&spi_xfer, &m);
+ reinit_completion(&phy->ready);
ret = spi_sync_locked(phy->spi_device, &m);
if (ret < 0)
goto exit;
@@ -131,6 +303,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
exit:
spi_bus_unlock(phy->spi_device->master);
+ phy->last_access = jiffies;
+ mutex_lock(&phy->time_track_mutex);
return ret;
}
@@ -192,10 +366,37 @@ static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
.write32 = tpm_tis_spi_write32,
};
+static void cr50_print_fw_version(struct tpm_tis_spi_phy *phy)
+{
+ int i, len = 0;
+ char fw_ver[TPM_CR50_MAX_FW_VER_LEN + 1];
+ char fw_ver_block[4];
+ struct tpm_tis_data *data = &phy->priv;
+
+ /*
+ * Write anything to TPM_CR50_FW_VER to start from the beginning
+ * of the version string
+ */
+ tpm_tis_write8(data, TPM_CR50_FW_VER(data->locality), 0);
+
+ /* Read the string, 4 bytes at a time, until we get '\0' */
+ do {
+ tpm_tis_read_bytes(data, TPM_CR50_FW_VER(data->locality), 4,
+ fw_ver_block);
+ for (i = 0; i < 4 && fw_ver_block[i]; ++len, ++i)
+ fw_ver[len] = fw_ver_block[i];
+ } while (i == 4 && len < TPM_CR50_MAX_FW_VER_LEN);
+ fw_ver[len] = '\0';
+
+ dev_info(&phy->spi_device->dev, "Cr50 firmware version: %s\n", fw_ver);
+}
+
static int tpm_tis_spi_probe(struct spi_device *dev)
{
struct tpm_tis_spi_phy *phy;
- int irq;
+ int ret, irq = -1;
+ struct device_node *np = dev->dev.of_node;
+ const struct spi_device_id *spi_dev_id = spi_get_device_id(dev);
phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_spi_phy),
GFP_KERNEL);
@@ -208,17 +409,94 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
if (!phy->iobuf)
return -ENOMEM;
- /* If the SPI device has an IRQ then use that */
- if (dev->irq > 0)
+ phy->is_cr50 = of_device_is_compatible(np, "google,cr50") ||
+ (spi_dev_id && spi_dev_id->driver_data == TIS_IS_CR50);
+
+ if (phy->is_cr50) {
+ phy->access_delay = CR50_NOIRQ_ACCESS_DELAY;
+
+ mutex_init(&phy->time_track_mutex);
+ phy->wake_after = jiffies;
+ phy->last_access = jiffies;
+
+ init_completion(&phy->ready);
+ if (dev->irq > 0) {
+ ret = devm_request_irq(&dev->dev, dev->irq, cr50_spi_irq_handler,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ "cr50_spi", phy);
+ if (ret < 0) {
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ dev_warn(&dev->dev, "Requesting IRQ %d failed: %d\n",
+ dev->irq, ret);
+ /*
+ * This is not fatal, the driver will fall back to
+ * delays automatically, since ready will never
+ * be completed without a registered irq handler.
+ * So, just fall through.
+ */
+ } else {
+ /*
+ * IRQ requested, let's verify that it is actually
+ * triggered, before relying on it.
+ */
+ phy->irq_needs_confirmation = true;
+ }
+ } else {
+ dev_warn(&dev->dev,
+ "No IRQ - will use delays between transactions.\n");
+ }
+
+ phy->priv.rng_quality = rng_quality;
+ } else if (dev->irq > 0) {
+ /* If the SPI device has an IRQ then use that */
irq = dev->irq;
- else
- irq = -1;
+ }
- return tpm_tis_core_init(&dev->dev, &phy->priv, irq, &tpm_spi_phy_ops,
+ ret = tpm_tis_core_init(&dev->dev, &phy->priv, irq, &tpm_spi_phy_ops,
NULL);
+
+ if (!ret && phy->is_cr50)
+ cr50_print_fw_version(phy);
+
+ return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int tpm_tis_spi_pm_suspend(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+
+ if (phy->is_cr50)
+ return cr50_suspend(dev);
+
+ return tpm_pm_suspend(dev);
+}
+
+static int tpm_tis_spi_pm_resume(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+
+ if (phy->is_cr50) {
+ /*
+ * Jiffies not increased during suspend, so we need to reset
+ * the time to wake Cr50 after resume.
+ */
+ phy->wake_after = jiffies;
+
+ return cr50_resume(dev);
+ }
+
+ return tpm_tis_resume(dev);
}
+#endif
-static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
+static SIMPLE_DEV_PM_OPS(tpm_tis_pm,
+ tpm_tis_spi_pm_suspend, tpm_tis_spi_pm_resume);
static int tpm_tis_spi_remove(struct spi_device *dev)
{
@@ -230,12 +508,14 @@ static int tpm_tis_spi_remove(struct spi_device *dev)
}
static const struct spi_device_id tpm_tis_spi_id[] = {
+ {"cr50", TIS_IS_CR50},
{"tpm_tis_spi", 0},
{}
};
MODULE_DEVICE_TABLE(spi, tpm_tis_spi_id);
static const struct of_device_id of_tis_spi_match[] = {
+ { .compatible = "google,cr50", },
{ .compatible = "st,st33htpm-spi", },
{ .compatible = "infineon,slb9670", },
{ .compatible = "tcg,tpm_tis-spi", },
Quoting Stephen Boyd (2019-07-17 14:38:36)
> @@ -131,6 +303,8 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>
> exit:
> spi_bus_unlock(phy->spi_device->master);
> + phy->last_access = jiffies;
> + mutex_lock(&phy->time_track_mutex);
This should be mutex_unlock().
On 17.07.2019 21:57, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-17 05:00:06)
>> On 17.07.2019 00:45, Stephen Boyd wrote:
>>> From: Andrey Pronin <[email protected]>
>>>
>>> +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.");
>>
>> What is the purpose of this parameter? None of the other TPM drivers
>> have it.
>
> I think the idea is to let users override the quality if they decide
> that they don't want to use the default value specified in the driver.
But isn't this something that applies to all TPMs, not only cr50? So
shouldn't this parameter be added to one of the global modules (tpm?
tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis,
tpm_tis_spi, ...) need this parameter to provide a consistent interface
for the user?
>> This copies a lot of code from tpm_tis_spi, but then slightly changes
>> some things, without really explaining why.
>
> The commit text has some explanations. Here's the copy/paste from above:
>
>>> - need to ensure a certain delay between spi transactions, or else
>>> the chip may miss some part of the next transaction;
>>> - if there is no spi activity for some time, it may go to sleep,
>>> and needs to be waken up before sending further commands;
>>> - access to vendor-specific registers.
>
> Do you want me to describe something further?
>
>> For example, struct
>> cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
>> single iobuf, that is allocated via devm_kmalloc instead of being part
>> of the struct. Maybe the difference matters, maybe not, who knows?
>
> Ok. Are you asking if this is a full-duplex SPI device?
No, this was meant as an example for the previous question. As far as I
understood it, cr50 is basically compliant to the spec implemented by
tpm_tis_spi, but needs special handling in some cases. Therefore, I'd
expect a driver for cr50 to look exactly like tpm_tis_spi except for the
special bits here and there. The way buffers are allocated within the
driver is probably not something that should differ because of the TPM chip.
Alexander
On 17.07.2019 23:38, Stephen Boyd wrote:
> Quoting Stephen Boyd (2019-07-17 12:57:34)
>> Quoting Alexander Steffen (2019-07-17 05:00:06)
>>>
>>> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
>>> tpm_tis_spi, so that it can intercept the calls, execute the additional
>>> actions (like waking up the device), but then let tpm_tis_spi do the
>>> common work?
>>>
>>
>> I suppose the read{16,32} and write32 functions could be reused. I'm not
>> sure how great it will be if we combine these two drivers, but I can
>> give it a try today and see how it looks.
>>
>
> Here's the patch. I haven't tested it besides compile testing.
Thanks for providing this. Makes it much easier to see what the actual
differences between the devices are.
Do we have a general policy on how to support devices that are very
similar but need special handling in some places? Not duplicating the
whole driver just to change a few things definitely seems like an
improvement (and has already been done in the past, as with
TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
tpm_tis_spi.c? Or is there some way to keep a clearer separation,
especially when (in the future) we have multiple devices that all have
their own set of deviations from the spec?
Alexander
Quoting Alexander Steffen (2019-07-18 09:47:14)
> On 17.07.2019 21:57, Stephen Boyd wrote:
> >
> > I think the idea is to let users override the quality if they decide
> > that they don't want to use the default value specified in the driver.
>
> But isn't this something that applies to all TPMs, not only cr50? So
> shouldn't this parameter be added to one of the global modules (tpm?
> tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis,
> tpm_tis_spi, ...) need this parameter to provide a consistent interface
> for the user?
Looking at commit 7a64c5597aa4 ("tpm: Allow tpm_tis drivers to set hwrng
quality.") I think all low-level drivers need to set the hwrng quality
somehow. I'm not sure how tpm_tis_spi will do that in general, but at
least for cr50 we have derived this quality number.
I can move this module parameter to tpm_tis_core.c, but then it will be
a global hwrng quality override for whatever tpm is registered through
tpm_tis_core instead of per-tpm driver. This is sort of a problem right
now too if we have two tpm_tis_spi devices. I can drop this parameter if
you want.
>
> >
> > Do you want me to describe something further?
> >
> >> For example, struct
> >> cr50_spi_phy contains both tx_buf and rx_buf, whereas tpm_tis_spi uses a
> >> single iobuf, that is allocated via devm_kmalloc instead of being part
> >> of the struct. Maybe the difference matters, maybe not, who knows?
> >
> > Ok. Are you asking if this is a full-duplex SPI device?
>
> No, this was meant as an example for the previous question. As far as I
> understood it, cr50 is basically compliant to the spec implemented by
> tpm_tis_spi, but needs special handling in some cases. Therefore, I'd
> expect a driver for cr50 to look exactly like tpm_tis_spi except for the
> special bits here and there. The way buffers are allocated within the
> driver is probably not something that should differ because of the TPM chip.
>
Ok.
Quoting Alexander Steffen (2019-07-18 09:47:22)
> On 17.07.2019 23:38, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2019-07-17 12:57:34)
> >> Quoting Alexander Steffen (2019-07-17 05:00:06)
> >>>
> >>> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> >>> tpm_tis_spi, so that it can intercept the calls, execute the additional
> >>> actions (like waking up the device), but then let tpm_tis_spi do the
> >>> common work?
> >>>
> >>
> >> I suppose the read{16,32} and write32 functions could be reused. I'm not
> >> sure how great it will be if we combine these two drivers, but I can
> >> give it a try today and see how it looks.
> >>
> >
> > Here's the patch. I haven't tested it besides compile testing.
The code seems to work but I haven't done any extensive testing besides
making sure that the TPM responds to pcr reads and some commands like
reading random numbers.
>
> Thanks for providing this. Makes it much easier to see what the actual
> differences between the devices are.
>
> Do we have a general policy on how to support devices that are very
> similar but need special handling in some places? Not duplicating the
> whole driver just to change a few things definitely seems like an
> improvement (and has already been done in the past, as with
> TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
> tpm_tis_spi.c? Or is there some way to keep a clearer separation,
> especially when (in the future) we have multiple devices that all have
> their own set of deviations from the spec?
>
If you have any ideas on how to do it please let me know. At this point,
I'd prefer if the maintainers could provide direction on what they want.
On 18.07.2019 20:07, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-18 09:47:14)
>> On 17.07.2019 21:57, Stephen Boyd wrote:
>>>
>>> I think the idea is to let users override the quality if they decide
>>> that they don't want to use the default value specified in the driver.
>>
>> But isn't this something that applies to all TPMs, not only cr50? So
>> shouldn't this parameter be added to one of the global modules (tpm?
>> tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis,
>> tpm_tis_spi, ...) need this parameter to provide a consistent interface
>> for the user?
>
> Looking at commit 7a64c5597aa4 ("tpm: Allow tpm_tis drivers to set hwrng
> quality.") I think all low-level drivers need to set the hwrng quality
> somehow. I'm not sure how tpm_tis_spi will do that in general, but at
> least for cr50 we have derived this quality number.
>
> I can move this module parameter to tpm_tis_core.c, but then it will be
> a global hwrng quality override for whatever tpm is registered through
> tpm_tis_core instead of per-tpm driver. This is sort of a problem right
> now too if we have two tpm_tis_spi devices. I can drop this parameter if
> you want.
Since it does not seem like a critical feature, maybe just drop it for
now. Then we can figure out a way to do it properly and add it later.
Alexander
On 18.07.2019 20:11, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-18 09:47:22)
>> On 17.07.2019 23:38, Stephen Boyd wrote:
>>> Quoting Stephen Boyd (2019-07-17 12:57:34)
>>>> Quoting Alexander Steffen (2019-07-17 05:00:06)
>>>>>
>>>>> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
>>>>> tpm_tis_spi, so that it can intercept the calls, execute the additional
>>>>> actions (like waking up the device), but then let tpm_tis_spi do the
>>>>> common work?
>>>>>
>>>>
>>>> I suppose the read{16,32} and write32 functions could be reused. I'm not
>>>> sure how great it will be if we combine these two drivers, but I can
>>>> give it a try today and see how it looks.
>>>>
>>>
>>> Here's the patch. I haven't tested it besides compile testing.
>
> The code seems to work but I haven't done any extensive testing besides
> making sure that the TPM responds to pcr reads and some commands like
> reading random numbers.
>
>>
>> Thanks for providing this. Makes it much easier to see what the actual
>> differences between the devices are.
>>
>> Do we have a general policy on how to support devices that are very
>> similar but need special handling in some places? Not duplicating the
>> whole driver just to change a few things definitely seems like an
>> improvement (and has already been done in the past, as with
>> TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
>> tpm_tis_spi.c? Or is there some way to keep a clearer separation,
>> especially when (in the future) we have multiple devices that all have
>> their own set of deviations from the spec?
>>
>
> If you have any ideas on how to do it please let me know. At this point,
> I'd prefer if the maintainers could provide direction on what they want.
Sure, I'd expect Jarkko will say something once he's back from vacation.
Alexander
Quoting Alexander Steffen (2019-07-19 00:53:00)
> On 18.07.2019 20:11, Stephen Boyd wrote:
> > Quoting Alexander Steffen (2019-07-18 09:47:22)
> >> On 17.07.2019 23:38, Stephen Boyd wrote:
> >>> Quoting Stephen Boyd (2019-07-17 12:57:34)
> >>>> Quoting Alexander Steffen (2019-07-17 05:00:06)
> >>>>>
> >>>>> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> >>>>> tpm_tis_spi, so that it can intercept the calls, execute the additional
> >>>>> actions (like waking up the device), but then let tpm_tis_spi do the
> >>>>> common work?
> >>>>>
> >>>>
> >>>> I suppose the read{16,32} and write32 functions could be reused. I'm not
> >>>> sure how great it will be if we combine these two drivers, but I can
> >>>> give it a try today and see how it looks.
> >>>>
> >>>
> >>> Here's the patch. I haven't tested it besides compile testing.
> >
> > The code seems to work but I haven't done any extensive testing besides
> > making sure that the TPM responds to pcr reads and some commands like
> > reading random numbers.
> >
> >>
> >> Thanks for providing this. Makes it much easier to see what the actual
> >> differences between the devices are.
> >>
> >> Do we have a general policy on how to support devices that are very
> >> similar but need special handling in some places? Not duplicating the
> >> whole driver just to change a few things definitely seems like an
> >> improvement (and has already been done in the past, as with
> >> TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
> >> tpm_tis_spi.c? Or is there some way to keep a clearer separation,
> >> especially when (in the future) we have multiple devices that all have
> >> their own set of deviations from the spec?
> >>
> >
> > If you have any ideas on how to do it please let me know. At this point,
> > I'd prefer if the maintainers could provide direction on what they want.
>
> Sure, I'd expect Jarkko will say something once he's back from vacation.
>
Should I just resend this patch series? I haven't attempted to make the
i2c driver changes, but at least the SPI driver changes seem good enough
to resend.
On Thu, 2019-08-01 at 09:02 -0700, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-19 00:53:00)
> > On 18.07.2019 20:11, Stephen Boyd wrote:
> > > Quoting Alexander Steffen (2019-07-18 09:47:22)
> > > > On 17.07.2019 23:38, Stephen Boyd wrote:
> > > > > Quoting Stephen Boyd (2019-07-17 12:57:34)
> > > > > > Quoting Alexander Steffen (2019-07-17 05:00:06)
> > > > > > > Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> > > > > > > tpm_tis_spi, so that it can intercept the calls, execute the additional
> > > > > > > actions (like waking up the device), but then let tpm_tis_spi do the
> > > > > > > common work?
> > > > > > >
> > > > > >
> > > > > > I suppose the read{16,32} and write32 functions could be reused. I'm not
> > > > > > sure how great it will be if we combine these two drivers, but I can
> > > > > > give it a try today and see how it looks.
> > > > > >
> > > > >
> > > > > Here's the patch. I haven't tested it besides compile testing.
> > >
> > > The code seems to work but I haven't done any extensive testing besides
> > > making sure that the TPM responds to pcr reads and some commands like
> > > reading random numbers.
> > >
> > > > Thanks for providing this. Makes it much easier to see what the actual
> > > > differences between the devices are.
> > > >
> > > > Do we have a general policy on how to support devices that are very
> > > > similar but need special handling in some places? Not duplicating the
> > > > whole driver just to change a few things definitely seems like an
> > > > improvement (and has already been done in the past, as with
> > > > TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
> > > > tpm_tis_spi.c? Or is there some way to keep a clearer separation,
> > > > especially when (in the future) we have multiple devices that all have
> > > > their own set of deviations from the spec?
> > > >
> > >
> > > If you have any ideas on how to do it please let me know. At this point,
> > > I'd prefer if the maintainers could provide direction on what they want.
> >
> > Sure, I'd expect Jarkko will say something once he's back from vacation.
> >
>
> Should I just resend this patch series? I haven't attempted to make the
> i2c driver changes, but at least the SPI driver changes seem good enough
> to resend.
Hi, I'm back. If there are already like obvious changes, please send an
update and I'll take a look at that.
/Jarkko
Quoting Jarkko Sakkinen (2019-08-02 08:21:06)
> On Thu, 2019-08-01 at 09:02 -0700, Stephen Boyd wrote:
> > Quoting Alexander Steffen (2019-07-19 00:53:00)
> > > On 18.07.2019 20:11, Stephen Boyd wrote:
> > > > Quoting Alexander Steffen (2019-07-18 09:47:22)
> > > > > On 17.07.2019 23:38, Stephen Boyd wrote:
> > > > > > Quoting Stephen Boyd (2019-07-17 12:57:34)
> > > > > > > Quoting Alexander Steffen (2019-07-17 05:00:06)
> > > > > > > > Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> > > > > > > > tpm_tis_spi, so that it can intercept the calls, execute the additional
> > > > > > > > actions (like waking up the device), but then let tpm_tis_spi do the
> > > > > > > > common work?
> > > > > > > >
> > > > > > >
> > > > > > > I suppose the read{16,32} and write32 functions could be reused. I'm not
> > > > > > > sure how great it will be if we combine these two drivers, but I can
> > > > > > > give it a try today and see how it looks.
> > > > > > >
> > > > > >
> > > > > > Here's the patch. I haven't tested it besides compile testing.
> > > >
> > > > The code seems to work but I haven't done any extensive testing besides
> > > > making sure that the TPM responds to pcr reads and some commands like
> > > > reading random numbers.
> > > >
> > > > > Thanks for providing this. Makes it much easier to see what the actual
> > > > > differences between the devices are.
> > > > >
> > > > > Do we have a general policy on how to support devices that are very
> > > > > similar but need special handling in some places? Not duplicating the
> > > > > whole driver just to change a few things definitely seems like an
> > > > > improvement (and has already been done in the past, as with
> > > > > TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
> > > > > tpm_tis_spi.c? Or is there some way to keep a clearer separation,
> > > > > especially when (in the future) we have multiple devices that all have
> > > > > their own set of deviations from the spec?
> > > > >
> > > >
> > > > If you have any ideas on how to do it please let me know. At this point,
> > > > I'd prefer if the maintainers could provide direction on what they want.
> > >
> > > Sure, I'd expect Jarkko will say something once he's back from vacation.
> > >
> >
> > Should I just resend this patch series? I haven't attempted to make the
> > i2c driver changes, but at least the SPI driver changes seem good enough
> > to resend.
>
> Hi, I'm back. If there are already like obvious changes, please send an
> update and I'll take a look at that.
>
Ok. Will do today. Thanks.
On Thu, Aug 01, 2019 at 09:02:02AM -0700, Stephen Boyd wrote:
> Quoting Alexander Steffen (2019-07-19 00:53:00)
> > On 18.07.2019 20:11, Stephen Boyd wrote:
> > > Quoting Alexander Steffen (2019-07-18 09:47:22)
> > >> On 17.07.2019 23:38, Stephen Boyd wrote:
> > >>> Quoting Stephen Boyd (2019-07-17 12:57:34)
> > >>>> Quoting Alexander Steffen (2019-07-17 05:00:06)
> > >>>>>
> > >>>>> Can't the code be shared more explicitly, e.g. by cr50_spi wrapping
> > >>>>> tpm_tis_spi, so that it can intercept the calls, execute the additional
> > >>>>> actions (like waking up the device), but then let tpm_tis_spi do the
> > >>>>> common work?
> > >>>>>
> > >>>>
> > >>>> I suppose the read{16,32} and write32 functions could be reused. I'm not
> > >>>> sure how great it will be if we combine these two drivers, but I can
> > >>>> give it a try today and see how it looks.
> > >>>>
> > >>>
> > >>> Here's the patch. I haven't tested it besides compile testing.
> > >
> > > The code seems to work but I haven't done any extensive testing besides
> > > making sure that the TPM responds to pcr reads and some commands like
> > > reading random numbers.
> > >
> > >>
> > >> Thanks for providing this. Makes it much easier to see what the actual
> > >> differences between the devices are.
> > >>
> > >> Do we have a general policy on how to support devices that are very
> > >> similar but need special handling in some places? Not duplicating the
> > >> whole driver just to change a few things definitely seems like an
> > >> improvement (and has already been done in the past, as with
> > >> TPM_TIS_ITPM_WORKAROUND). But should all the code just be added to
> > >> tpm_tis_spi.c? Or is there some way to keep a clearer separation,
> > >> especially when (in the future) we have multiple devices that all have
> > >> their own set of deviations from the spec?
> > >>
> > >
> > > If you have any ideas on how to do it please let me know. At this point,
> > > I'd prefer if the maintainers could provide direction on what they want.
> >
> > Sure, I'd expect Jarkko will say something once he's back from vacation.
> >
>
> Should I just resend this patch series? I haven't attempted to make the
> i2c driver changes, but at least the SPI driver changes seem good enough
> to resend.
Go ahead.
/Jarkko
On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> The hwrng_fill() function can run while devices are suspending and
> resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> is suspended, the hwrng may hang the bus while attempting to add some
> randomness. It's been observed on ChromeOS devices with suspend-to-idle
> (s2idle) and an i2c based hwrng that this kthread may run and ask the
> hwrng device for randomness before the i2c bus has been resumed.
>
> Let's make this kthread freezable so that we don't try to touch the
> hwrng during suspend/resume. This ensures that we can't cause the hwrng
> backing driver to get into a bad state because the device is guaranteed
> to be resumed before the hwrng kthread is thawed.
>
> Cc: Andrey Pronin <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Duncan Laurie <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
This does not need a fixes tag?
/Jarkko
On Wed, Jul 17, 2019 at 02:00:06PM +0200, Alexander Steffen wrote:
> Is it a common pattern to add config options that are not useful on their
> own? When would I ever enable TCG_CR50 without also enabling TCG_CR50_SPI?
> Why can't you just use TCG_CR50_SPI for everything?
Agreed. If the kernel only has the spi driver, we only want option for
that.
/Jarkko
On Tue, Jul 16, 2019 at 03:45:18PM -0700, Stephen Boyd wrote:
> From: Duncan Laurie <[email protected]>
>
> 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.
>
> 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 <[email protected]>
> [[email protected]: 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 <[email protected]>
Apologies. I missed this when I stated my comment about SPI.
/Jarkko
On Thu, Jul 18, 2019 at 06:47:14PM +0200, Alexander Steffen wrote:
> On 17.07.2019 21:57, Stephen Boyd wrote:
> > Quoting Alexander Steffen (2019-07-17 05:00:06)
> > > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > > From: Andrey Pronin <[email protected]>
> > > >
> > > > +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.");
> > >
> > > What is the purpose of this parameter? None of the other TPM drivers
> > > have it.
> >
> > I think the idea is to let users override the quality if they decide
> > that they don't want to use the default value specified in the driver.
>
> But isn't this something that applies to all TPMs, not only cr50? So
> shouldn't this parameter be added to one of the global modules (tpm?
> tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis, tpm_tis_spi,
> ...) need this parameter to provide a consistent interface for the user?
This definitely something that is out of context of the patch set and
thus must be removed from the patch set.
/Jarkko
Quoting Jarkko Sakkinen (2019-08-02 13:43:18)
> On Thu, Jul 18, 2019 at 06:47:14PM +0200, Alexander Steffen wrote:
> > On 17.07.2019 21:57, Stephen Boyd wrote:
> > > Quoting Alexander Steffen (2019-07-17 05:00:06)
> > > > On 17.07.2019 00:45, Stephen Boyd wrote:
> > > > > From: Andrey Pronin <[email protected]>
> > > > >
> > > > > +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.");
> > > >
> > > > What is the purpose of this parameter? None of the other TPM drivers
> > > > have it.
> > >
> > > I think the idea is to let users override the quality if they decide
> > > that they don't want to use the default value specified in the driver.
> >
> > But isn't this something that applies to all TPMs, not only cr50? So
> > shouldn't this parameter be added to one of the global modules (tpm?
> > tpm_tis_core?) instead? Or do all low-level drivers (tpm_tis, tpm_tis_spi,
> > ...) need this parameter to provide a consistent interface for the user?
>
> This definitely something that is out of context of the patch set and
> thus must be removed from the patch set.
Ok. I've dropped this part of the patch.
Quoting Stephen Boyd (2019-07-17 10:03:22)
> Quoting Jason Gunthorpe (2019-07-17 09:50:11)
> > On Wed, Jul 17, 2019 at 09:42:32AM -0700, Stephen Boyd wrote:
>
> Yes. That's exactly my point. A hwrng that's suspended will fail here
> and it's better to just not try until it's guaranteed to have resumed.
>
> >
> > It just seems weird to do this, what about all the other tpm API
> > users? Do they have a racy problem with suspend too?
>
> I haven't looked at them. Are they being called from suspend/resume
> paths? I don't think anything for the userspace API can be a problem
> because those tasks are all frozen. The only problem would be some
> kernel internal API that TPM API exposes. I did a quick grep and I see
> things like IMA or the trusted keys APIs that might need a closer look.
>
> Either way, trying to hold off a TPM operation from the TPM API when
> we're suspended isn't really possible. If something like IMA needs to
> get TPM data from deep suspend path and it fails because the device is
> suspended, all we can do is return an error from TPM APIs and hope the
> caller can recover. The fix is probably going to be to change the code
> to not call into the TPM API until the hardware has resumed by avoiding
> doing anything with the TPM until resume is over. So we're at best able
> to make same sort of band-aid here in the TPM API where all we can do is
> say -EAGAIN but we can't tell the caller when to try again.
>
Andrey talked to me a little about this today. Andrey would prefer we
don't just let the TPM go into a wonky state if it's used during
suspend/resume so that it can stay resilient to errors. Sounds OK to me,
but my point still stands that we need to fix the callers.
I'll resurrect the IS_SUSPENDED flag and make it set generically by the
tpm_pm_suspend() and tpm_pm_resume() functions and then spit out a big
WARN_ON() and return an error value like -EAGAIN if the TPM functions
are called when the TPM is suspended. I hope we don't hit the warning
message, but if we do then at least we can track it down rather quickly
and figure out how to fix the caller instead of just silently returning
-EAGAIN and hoping for that to be visible to the user.
This patch will still be required to avoid the WARN message, so I'll
resend with the Cc to crypto list so it can be picked up.
Quoting Jarkko Sakkinen (2019-08-02 13:22:09)
> On Tue, Jul 16, 2019 at 03:45:13PM -0700, Stephen Boyd wrote:
> > The hwrng_fill() function can run while devices are suspending and
> > resuming. If the hwrng is behind a bus such as i2c or SPI and that bus
> > is suspended, the hwrng may hang the bus while attempting to add some
> > randomness. It's been observed on ChromeOS devices with suspend-to-idle
> > (s2idle) and an i2c based hwrng that this kthread may run and ask the
> > hwrng device for randomness before the i2c bus has been resumed.
> >
> > Let's make this kthread freezable so that we don't try to touch the
> > hwrng during suspend/resume. This ensures that we can't cause the hwrng
> > backing driver to get into a bad state because the device is guaranteed
> > to be resumed before the hwrng kthread is thawed.
> >
> > Cc: Andrey Pronin <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: Duncan Laurie <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
>
> This does not need a fixes tag?
>
I'll add Fixes: be4000bc4644 ("hwrng: create filler thread")
Quoting Alexander Steffen (2019-07-17 08:19:19)
> On 17.07.2019 00:45, Stephen Boyd wrote:
> > From: Duncan Laurie <[email protected]>
> >
> > 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 <[email protected]>
> > [[email protected]: 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + * 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))
> > +
On 03.08.2019 00:50, Stephen Boyd wrote:
> Quoting Stephen Boyd (2019-07-17 10:03:22)
>> Quoting Jason Gunthorpe (2019-07-17 09:50:11)
>>> On Wed, Jul 17, 2019 at 09:42:32AM -0700, Stephen Boyd wrote:
>>
>> Yes. That's exactly my point. A hwrng that's suspended will fail here
>> and it's better to just not try until it's guaranteed to have resumed.
>>
>>>
>>> It just seems weird to do this, what about all the other tpm API
>>> users? Do they have a racy problem with suspend too?
>>
>> I haven't looked at them. Are they being called from suspend/resume
>> paths? I don't think anything for the userspace API can be a problem
>> because those tasks are all frozen. The only problem would be some
>> kernel internal API that TPM API exposes. I did a quick grep and I see
>> things like IMA or the trusted keys APIs that might need a closer look.
>>
>> Either way, trying to hold off a TPM operation from the TPM API when
>> we're suspended isn't really possible. If something like IMA needs to
>> get TPM data from deep suspend path and it fails because the device is
>> suspended, all we can do is return an error from TPM APIs and hope the
>> caller can recover. The fix is probably going to be to change the code
>> to not call into the TPM API until the hardware has resumed by avoiding
>> doing anything with the TPM until resume is over. So we're at best able
>> to make same sort of band-aid here in the TPM API where all we can do is
>> say -EAGAIN but we can't tell the caller when to try again.
>>
>
> Andrey talked to me a little about this today. Andrey would prefer we
> don't just let the TPM go into a wonky state if it's used during
> suspend/resume so that it can stay resilient to errors. Sounds OK to me,
> but my point still stands that we need to fix the callers.
>
> I'll resurrect the IS_SUSPENDED flag and make it set generically by the
> tpm_pm_suspend() and tpm_pm_resume() functions and then spit out a big
> WARN_ON() and return an error value like -EAGAIN if the TPM functions
> are called when the TPM is suspended. I hope we don't hit the warning
> message, but if we do then at least we can track it down rather quickly
> and figure out how to fix the caller instead of just silently returning
> -EAGAIN and hoping for that to be visible to the user.
There is another use case I see for this functionality: There are ways
for user space to upgrade the TPM's firmware via /dev/tpm0 (using e.g.
TPM2_FieldUpgradeStart/TPM2_FieldUpgradeData). While upgrading, the
normal TPM functionality might not be available (commands return
TPM_RC_UPGRADE or other error codes). Even after the upgrade is
finished, the TPM might continue to refuse command execution (e.g. with
TPM_RC_REBOOT).
I'm not sure whether all in-kernel users are prepared to deal correctly
with those error codes. But even if they are, it might be better to
block them from sending commands in the first place, to not interfere
with the upgrade process.
What would you think about a way for a user space upgrade tool to also
set this flag, to make the TPM unavailable for everything but the
upgrade process?
Alexander
On Fri, Aug 16, 2019 at 05:56:12PM +0200, Alexander Steffen wrote:
> > Andrey talked to me a little about this today. Andrey would prefer we
> > don't just let the TPM go into a wonky state if it's used during
> > suspend/resume so that it can stay resilient to errors. Sounds OK to me,
> > but my point still stands that we need to fix the callers.
> >
> > I'll resurrect the IS_SUSPENDED flag and make it set generically by the
> > tpm_pm_suspend() and tpm_pm_resume() functions and then spit out a big
> > WARN_ON() and return an error value like -EAGAIN if the TPM functions
> > are called when the TPM is suspended. I hope we don't hit the warning
> > message, but if we do then at least we can track it down rather quickly
> > and figure out how to fix the caller instead of just silently returning
> > -EAGAIN and hoping for that to be visible to the user.
>
> There is another use case I see for this functionality: There are ways for
> user space to upgrade the TPM's firmware via /dev/tpm0 (using e.g.
> TPM2_FieldUpgradeStart/TPM2_FieldUpgradeData). While upgrading, the normal
> TPM functionality might not be available (commands return TPM_RC_UPGRADE or
> other error codes). Even after the upgrade is finished, the TPM might
> continue to refuse command execution (e.g. with TPM_RC_REBOOT).
>
> I'm not sure whether all in-kernel users are prepared to deal correctly with
> those error codes. But even if they are, it might be better to block them
> from sending commands in the first place, to not interfere with the upgrade
> process.
>
> What would you think about a way for a user space upgrade tool to also set
> this flag, to make the TPM unavailable for everything but the upgrade
> process?
>
> Alexander
NOTE: Just commenting the FW use case.
I don't like it because it contains variable amount of reserved time
for a hardware resource.
Right now a user thread gets a lease of one TPM command for /dev/tpm0
and that is how I would like to keep it.
/Jarkko