This patch adds a driver to support Infineon's SLB 9635 TT 1.2 Soft I2C
TPMs which follow the TGC TIS 1.2 TPM specification[1] and Infineon's I2C
Protocol Stack Specification 0.20.
The I2C Protocol Stack Specification is a simple adaption of the LPC TIS
Protocol to the I2C Bus.
The I2C TPMs can be used when LPC Bus is not available (i.e. non x86
architectures like ARM).
The driver is based on the tpm_tis.c driver by Leendert van Dorn and Kyleen Hall
and has quite similar functionality.
Tested on Nvidia ARM Tegra2 Development Platform and Beagleboard (ARM OMAP)
Tested with the Trousers[2] TSS API Testsuite v 0.3 [3]
Compile-tested on x86 (32/64-bit)
Updates since version 2.1.2:
- added sysfs entries for duration and timeouts
- updated to new tpm_do_selftest
Updates since version 2.1.0:
- improved error handling
- implemented workarounds needed by the tpm
- fixed typos
References:
[1] http://www.trustedcomputinggroup.org/resources/pc_client_work_group_pc_client_specific_tpm_interface_specification_tis_version_12/
[2] http://trousers.sourceforge.net/
[3] http://sourceforge.net/projects/trousers/files/TSS%20API%20test%20suite/0.3/
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/char/tpm/Kconfig | 11 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm_tis_i2c.c | 716 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 728 insertions(+), 0 deletions(-)
create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index a048199..e594d48 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -33,6 +33,17 @@ config TCG_TIS
from within Linux. To compile this driver as a module, choose
M here; the module will be called tpm_tis.
+config TCG_TIS_I2C
+ tristate "TPM Interface Specification 1.2 Interface (I2C)"
+ depends on I2C
+ ---help---
+ If you have a TPM security chip that is compliant with the
+ TCG TIS 1.2 TPM specification and Infineon's I2C Protocol Stack
+ Specification 0.20 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 tpm_tis_i2c.
+
config TCG_NSC
tristate "National Semiconductor TPM Interface"
depends on X86
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index ea3a1e0..b6e051a 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -6,6 +6,7 @@ ifdef CONFIG_ACPI
obj-$(CONFIG_TCG_TPM) += tpm_bios.o
endif
obj-$(CONFIG_TCG_TIS) += tpm_tis.o
+obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
new file mode 100644
index 0000000..8975abf
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,716 @@
+/*
+ * Copyright (C) 2012 Infineon Technologies
+ *
+ * Authors:
+ * Peter Huewe <[email protected]>
+ *
+ * Device driver for TCG/TCPA TPM (trusted platform module).
+ * Specifications at http://www.trustedcomputinggroup.org
+ *
+ * This device driver implements the TPM interface as defined in
+ * the TCG TPM Interface Spec version 1.2, revision 1.0 and the
+ * Infineon I2C Protocol Stack Specification v0.20.
+ *
+ * It is based on the original tpm_tis device driver from Leendert van
+ * Dorn and Kyleen Hall.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ *
+ */
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/wait.h>
+#include "tpm.h"
+
+/* max. buffer size supported by our tpm */
+#define TPM_BUFSIZE 1260
+
+/* max. number of iterations after i2c NAK */
+#define MAX_COUNT 3
+
+#define SLEEP_DURATION_LOW 55
+#define SLEEP_DURATION_HI 65
+
+/* max. number of iterations after i2c NAK for 'long' commands
+ * we need this especially for sending TPM_READY, since the cleanup after the
+ * transtion to the ready state may take some time, but it is unpredictable
+ * how long it will take.
+ */
+#define MAX_COUNT_LONG 50
+
+#define SLEEP_DURATION_LONG_LOW 200
+#define SLEEP_DURATION_LONG_HI 220
+
+/* expected value for DIDVID register */
+#define TPM_TIS_I2C_DID_VID 0x000b15d1L
+
+/* Structure to store I2C TPM specific stuff */
+struct tpm_inf_dev {
+ struct i2c_client *client;
+ u8 buf[TPM_BUFSIZE+sizeof(u8)]; /* max. buffer size + addr */
+ struct tpm_chip *chip;
+};
+
+static struct tpm_inf_dev tpm_dev;
+
+
+/*
+ * iic_tpm_read() - read from TPM register
+ * @addr: register address to read from
+ * @buffer: provided by caller
+ * @len: number of bytes to read
+ *
+ * Read len bytes from TPM register and put them into
+ * buffer (little-endian format, i.e. first byte is put into buffer[0]).
+ *
+ * NOTE: TPM is big-endian for multi-byte values. Multi-byte
+ * values have to be swapped.
+ *
+ * Return -EIO on error, 0 on success.
+ */
+static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
+{
+
+ struct i2c_msg msg1 = { tpm_dev.client->addr, 0, 1, &addr };
+ struct i2c_msg msg2 = { tpm_dev.client->addr, I2C_M_RD, len, buffer };
+
+ int rc;
+ int count;
+
+ for (count = 0; count < MAX_COUNT; count++) {
+ rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
+ if (rc > 0)
+ break; /* break here to skip sleep */
+
+ usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+ }
+
+ if (rc <= 0)
+ return -EIO;
+
+ /* After the TPM has successfully received the register address it needs
+ * some time, thus we're sleeping here again, before retrieving the data
+ */
+ for (count = 0; count < MAX_COUNT; count++) {
+ usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+ rc = i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
+ if (rc > 0)
+ break;
+
+ }
+
+ if (rc <= 0)
+ return -EIO;
+
+ return 0;
+}
+
+static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
+ unsigned int sleep_low,
+ unsigned int sleep_hi,
+ u8 max_count)
+{
+ int rc = -1;
+ int count;
+
+ struct i2c_msg msg1 = { tpm_dev.client->addr, 0, len + 1, tpm_dev.buf };
+
+ tpm_dev.buf[0] = addr;
+ memcpy(&(tpm_dev.buf[1]), buffer, len);
+
+ for (count = 0; count < max_count; count++) {
+ rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
+ if (rc > 0)
+ break;
+
+ usleep_range(sleep_low, sleep_hi);
+ }
+
+ if (rc <= 0)
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * iic_tpm_write() - write to TPM register
+ * @addr: register address to write to
+ * @buffer: containing data to be written
+ * @len: number of bytes to write
+ *
+ * Write len bytes from provided buffer to TPM register (little
+ * endian format, i.e. buffer[0] is written as first byte).
+ *
+ * NOTE: TPM is big-endian for multi-byte values. Multi-byte
+ * values have to be swapped.
+ *
+ * NOTE: use this function instead of the iic_tpm_write_generic function.
+ *
+ * Return -EIO on error, 0 on success
+ */
+static int iic_tpm_write(u8 addr, u8 *buffer, size_t len)
+{
+ return iic_tpm_write_generic(addr, buffer, len, SLEEP_DURATION_LOW,
+ SLEEP_DURATION_HI, MAX_COUNT);
+}
+
+/*
+ * This function is needed especially for the cleanup situation after
+ * sending TPM_READY
+ * */
+static int iic_tpm_write_long(u8 addr, u8 *buffer, size_t len)
+{
+ return iic_tpm_write_generic(addr, buffer, len, SLEEP_DURATION_LONG_LOW,
+ SLEEP_DURATION_LONG_HI, MAX_COUNT_LONG);
+}
+
+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 buf;
+ int rc;
+
+ rc = iic_tpm_read(TPM_ACCESS(loc), &buf, 1);
+ if (rc < 0)
+ return rc;
+
+ if ((buf & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
+ (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
+ chip->vendor.locality = loc;
+ return loc;
+ }
+
+ return -1;
+}
+
+static void release_locality(struct tpm_chip *chip, int loc, int force)
+{
+ u8 buf;
+ if (iic_tpm_read(TPM_ACCESS(loc), &buf, 1) < 0)
+ return;
+
+ if (force || (buf & (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID)) ==
+ (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID)) {
+ buf = TPM_ACCESS_ACTIVE_LOCALITY;
+ iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
+ }
+}
+
+static int request_locality(struct tpm_chip *chip, int loc)
+{
+ unsigned long stop;
+ u8 buf = TPM_ACCESS_REQUEST_USE;
+
+ if (check_locality(chip, loc) >= 0)
+ return loc;
+
+ iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
+
+ /* wait for burstcount */
+ stop = jiffies + chip->vendor.timeout_a;
+ do {
+ if (check_locality(chip, loc) >= 0)
+ return loc;
+ msleep(TPM_TIMEOUT);
+ } while (time_before(jiffies, stop));
+
+ return -1;
+}
+
+static u8 tpm_tis_i2c_status(struct tpm_chip *chip)
+{
+ /* NOTE: since i2c read may fail, return 0 in this case --> time-out */
+ u8 buf;
+ if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
+ return 0;
+ else
+ return buf;
+}
+
+static void tpm_tis_i2c_ready(struct tpm_chip *chip)
+{
+ /* this causes the current command to be aborted */
+ u8 buf = TPM_STS_COMMAND_READY;
+ iic_tpm_write_long(TPM_STS(chip->vendor.locality), &buf, 1);
+}
+
+static ssize_t get_burstcount(struct tpm_chip *chip)
+{
+ unsigned long stop;
+ ssize_t burstcnt;
+ u8 buf[3];
+
+ /* wait for burstcount */
+ /* which timeout value, spec has 2 answers (c & d) */
+ stop = jiffies + chip->vendor.timeout_d;
+ do {
+ /* Note: STS is little endian */
+ if (iic_tpm_read(TPM_STS(chip->vendor.locality) + 1, buf, 3) < 0)
+ burstcnt = 0;
+ else
+ burstcnt = (buf[2] << 16) + (buf[1] << 8) + buf[0];
+
+ if (burstcnt)
+ return burstcnt;
+
+ msleep(TPM_TIMEOUT);
+ } while (time_before(jiffies, stop));
+ return -EBUSY;
+}
+
+static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+ int *status)
+{
+ unsigned long stop;
+
+ /* check current status */
+ *status = tpm_tis_i2c_status(chip);
+ if ((*status & mask) == mask)
+ return 0;
+
+ stop = jiffies + timeout;
+ do {
+ msleep(TPM_TIMEOUT);
+ *status = tpm_tis_i2c_status(chip);
+ if ((*status & mask) == mask)
+ return 0;
+
+ } while (time_before(jiffies, stop));
+
+ return -ETIME;
+}
+
+static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+ size_t size = 0;
+ ssize_t burstcnt;
+ int rc;
+
+ while (size < count) {
+ burstcnt = get_burstcount(chip);
+
+ /* burstcnt < 0 = tpm is busy */
+ if (burstcnt < 0)
+ return burstcnt;
+
+ /* limit received data to max. left */
+ if (burstcnt > (count-size))
+ burstcnt = count-size;
+
+ rc = iic_tpm_read(TPM_DATA_FIFO(chip->vendor.locality),
+ &(buf[size]),
+ burstcnt);
+ if (rc == 0)
+ size += burstcnt;
+
+ }
+ return size;
+}
+
+static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+ int size = 0;
+ int expected, status;
+
+ if (count < TPM_HEADER_SIZE) {
+ size = -EIO;
+ goto out;
+ }
+
+ /* read first 10 bytes, including tag, paramsize, and result */
+ size = recv_data(chip, buf, TPM_HEADER_SIZE);
+ if (size < TPM_HEADER_SIZE) {
+ dev_err(chip->dev, "Unable to read header\n");
+ goto out;
+ }
+
+ expected = be32_to_cpu(*(__be32 *) (buf + 2));
+ if ((size_t)expected > count) {
+ size = -EIO;
+ goto out;
+ }
+
+ size += recv_data(chip, &buf[TPM_HEADER_SIZE],
+ expected - TPM_HEADER_SIZE);
+ if (size < expected) {
+ dev_err(chip->dev, "Unable to read remainder of result\n");
+ size = -ETIME;
+ goto out;
+ }
+
+ wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c, &status);
+ if (status & TPM_STS_DATA_AVAIL) { /* retry? */
+ dev_err(chip->dev, "Error left over data\n");
+ size = -EIO;
+ goto out;
+ }
+
+out:
+ tpm_tis_i2c_ready(chip);
+ /* The TPM needs some time to clean up here,
+ * so we sleep rather than keeping the bus busy
+ */
+ usleep_range(2400, 2600);
+ release_locality(chip, chip->vendor.locality, 0);
+ return size;
+}
+
+static int tpm_tis_i2c_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+ int rc, status;
+ ssize_t burstcnt;
+ size_t count = 0;
+ u8 sts = TPM_STS_GO;
+
+ if (len > TPM_BUFSIZE)
+ return -E2BIG; /* command is too long for our tpm, sorry */
+
+ if (request_locality(chip, 0) < 0)
+ return -EBUSY;
+
+ status = tpm_tis_i2c_status(chip);
+ if ((status & TPM_STS_COMMAND_READY) == 0) {
+ tpm_tis_i2c_ready(chip);
+ if (wait_for_stat
+ (chip, TPM_STS_COMMAND_READY,
+ chip->vendor.timeout_b, &status) < 0) {
+ rc = -ETIME;
+ goto out_err;
+ }
+ }
+
+ while (count < len - 1) {
+ burstcnt = get_burstcount(chip);
+ dev_dbg(chip->dev,
+ "send(): count=%zd, len=%zd, burstcount=%zd (plain)\n",
+ count, len, burstcnt);
+
+ /* burstcnt < 0 = tpm is busy */
+ if (burstcnt < 0)
+ return burstcnt;
+
+ if (burstcnt > (len-1-count))
+ burstcnt = len-1-count;
+
+ dev_dbg(chip->dev, "send(): burstcount=%zd\n", burstcnt);
+
+ rc = iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality),
+ &(buf[count]), burstcnt);
+ if (rc == 0)
+ count += burstcnt;
+
+ wait_for_stat(chip, TPM_STS_VALID,
+ chip->vendor.timeout_c, &status);
+
+ if ((status & TPM_STS_DATA_EXPECT) == 0) {
+ rc = -EIO;
+ goto out_err;
+ }
+
+ }
+
+ dev_dbg(chip->dev, "send(): last byte @ count=%zd\n", count);
+
+ /* write last byte */
+ iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality), &(buf[count]), 1);
+ wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c, &status);
+ if ((status & TPM_STS_DATA_EXPECT) != 0) {
+ rc = -EIO;
+ goto out_err;
+ }
+
+ /* go and do it */
+ iic_tpm_write(TPM_STS(chip->vendor.locality), &sts, 1);
+
+ return len;
+out_err:
+ tpm_tis_i2c_ready(chip);
+ /* The TPM needs some time to clean up here,
+ * so we sleep rather than keeping the bus busy
+ */
+ usleep_range(2400, 2600);
+ release_locality(chip, chip->vendor.locality, 0);
+ return rc;
+}
+
+static const struct file_operations tis_ops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .open = tpm_open,
+ .read = tpm_read,
+ .write = tpm_write,
+ .release = tpm_release,
+};
+
+static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
+static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
+static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
+static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
+static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
+static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
+static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
+static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
+
+static struct attribute *tis_attrs[] = {
+ &dev_attr_pubek.attr,
+ &dev_attr_pcrs.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_active.attr,
+ &dev_attr_owned.attr,
+ &dev_attr_temp_deactivated.attr,
+ &dev_attr_caps.attr,
+ &dev_attr_cancel.attr,
+ &dev_attr_durations.attr,
+ &dev_attr_timeouts.attr, NULL,
+};
+
+static struct attribute_group tis_attr_grp = {
+ .attrs = tis_attrs
+};
+
+static struct tpm_vendor_specific tpm_tis_i2c = {
+ .status = tpm_tis_i2c_status,
+ .recv = tpm_tis_i2c_recv,
+ .send = tpm_tis_i2c_send,
+ .cancel = tpm_tis_i2c_ready,
+ .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ .req_canceled = TPM_STS_COMMAND_READY,
+ .attr_group = &tis_attr_grp,
+ .miscdev = {
+ .fops = &tis_ops,},
+};
+
+static int tpm_tis_i2c_init(struct device *dev)
+{
+ u32 vendor;
+ int rc = 0;
+ struct tpm_chip *chip;
+
+ chip = tpm_register_hardware(dev, &tpm_tis_i2c);
+ if (!chip) {
+ rc = -ENODEV;
+ goto out_err;
+ }
+
+ /* Disable interrupts */
+ chip->vendor.irq = 0;
+
+ /* Default timeouts */
+ chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+ chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+ chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+
+ if (request_locality(chip, 0) != 0) {
+ rc = -ENODEV;
+ goto out_vendor;
+ }
+
+ /* read four bytes from DID_VID register */
+ if (iic_tpm_read(TPM_DID_VID(0), (u8 *) &vendor, 4) < 0) {
+ rc = -EIO;
+ goto out_vendor;
+ }
+
+ /* create DID_VID register value, after swapping to little-endian */
+ vendor = be32_to_cpu((__be32) vendor);
+
+ if (vendor != TPM_TIS_I2C_DID_VID) {
+ rc = -ENODEV;
+ goto out_release;
+ }
+
+ dev_info(dev, "1.2 TPM (device-id 0x%X)\n", vendor >> 16);
+
+ INIT_LIST_HEAD(&chip->vendor.list);
+ tpm_dev.chip = chip;
+
+ tpm_get_timeouts(chip);
+ tpm_do_selftest(chip);
+
+ return 0;
+
+out_release:
+ release_locality(chip, chip->vendor.locality, 1);
+
+out_vendor:
+ tpm_dev_vendor_release(chip);
+ tpm_remove_hardware(chip->dev);
+
+out_err:
+ return rc;
+}
+
+
+static const struct i2c_device_id tpm_i2c_tis_table[] = {
+ { "tpm_tis_i2c", 0 },
+ { },
+};
+
+#ifdef CONFIG_PM
+/* NOTE:
+ * Suspend does currently not work Nvidias Tegra2 Platform
+ * but works fine on Beagleboard (arm omap).
+ *
+ * This function will block System Suspend if TPM is not initialized,
+ * however the TPM is usually initialized by BIOS/u-boot or by sending
+ * a tpm startup command.
+ */
+static int tpm_tis_i2c_suspend(struct device *dev)
+{
+ return tpm_pm_suspend(dev, dev->power.power_state);
+}
+
+static int tpm_tis_i2c_resume(struct device *dev)
+{
+ return tpm_pm_resume(dev);
+}
+
+static const struct dev_pm_ops tpm_tis_i2c_ops = {
+ .suspend = tpm_tis_i2c_suspend,
+ .resume = tpm_tis_i2c_resume,
+};
+#endif
+
+
+static int dummy_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ return 0;
+}
+
+static int dummy_remove(struct i2c_client *client)
+{
+ return 0;
+}
+
+static struct i2c_driver tpm_i2c_tis_driver = {
+
+ .id_table = tpm_i2c_tis_table,
+ .probe = dummy_probe,
+ .remove = dummy_remove,
+ .driver = {
+ .name = "tpm_tis_i2c",
+ .owner = THIS_MODULE,
+#ifdef CONFIG_PM
+ .pm = &tpm_tis_i2c_ops,
+#endif
+ },
+};
+
+
+static struct i2c_adapter *adap;
+static struct i2c_client *client;
+static struct i2c_board_info info = {
+ I2C_BOARD_INFO("tpm_tis_i2c", 0x20),
+};
+
+
+static int addr = 0x20;
+module_param(addr, int, S_IRUGO);
+MODULE_PARM_DESC(addr, "TPM I2C Device Address (default: 0x20)");
+
+static int bus_id = 2;
+module_param(bus_id, int, S_IRUGO);
+MODULE_PARM_DESC(bus_id, "TPM I2C Bus Id (default: 2)");
+
+static int __init init_tis_i2c(void)
+{
+
+ int rc = 0;
+ info.addr = addr;
+
+
+ if (tpm_dev.client != NULL)
+ return -EBUSY;
+
+ adap = i2c_get_adapter(bus_id);
+ if (!adap)
+ return -ENODEV;
+
+ client = i2c_new_device(adap, &info);
+ if (!client) {
+ i2c_put_adapter(adap);
+ return -ENODEV;
+ }
+
+ rc = i2c_add_driver(&tpm_i2c_tis_driver);
+ if (rc != 0) {
+ i2c_del_driver(&tpm_i2c_tis_driver);
+ i2c_put_adapter(tpm_dev.client->adapter);
+ return -ENODEV;
+ }
+ client->driver = &tpm_i2c_tis_driver;
+
+ tpm_dev.client = client;
+
+ rc = tpm_tis_i2c_init(&client->dev);
+ if (rc < 0) {
+ i2c_del_driver(&tpm_i2c_tis_driver);
+ i2c_put_adapter(tpm_dev.client->adapter);
+ device_del(&(tpm_dev.client->dev));
+ }
+
+ return rc;
+}
+
+static void __exit cleanup_tis_i2c(void)
+{
+ struct tpm_chip *chip = tpm_dev.chip;
+ release_locality(chip, chip->vendor.locality, 1);
+
+ tpm_dev_vendor_release(chip);
+ tpm_remove_hardware(chip->dev);
+
+ i2c_del_driver(&tpm_i2c_tis_driver);
+
+ i2c_put_adapter(tpm_dev.client->adapter);
+
+ /*
+ * taken from core.c as workaround since
+ * tpm_remove_hardware requires device structure
+ */
+ pr_debug("device: '%s': %s\n",
+ dev_name(&(tpm_dev.client->dev)), __func__);
+ device_del(&(tpm_dev.client->dev));
+}
+
+module_init(init_tis_i2c);
+module_exit(cleanup_tis_i2c);
+MODULE_AUTHOR("Peter Huewe <[email protected]>");
+MODULE_DESCRIPTION("TPM TIS I2C Driver");
+MODULE_VERSION("2.1.3");
+MODULE_LICENSE("GPL");
--
1.7.6.msysgit.0
From: Bryan Freed <[email protected]>
This is derived from Peter Huewe's recommended fix:
On some ChromeOS systems, a TPM sharing the I2C bus with another device
gets confused when it sees I2C requests to that other device.
This change locks the I2C adapter for the duration of the full sequence
of I2C requests the TPM needs to complete.
smbus_xfer is not supported, but SMBUS is not supported by the original
driver, either.
Signed-off-by: Bryan Freed <[email protected]>
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/char/tpm/tpm_tis_i2c.c | 42 ++++++++++++++++++++++++++++++++++++---
1 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index 8975abf..e68f209 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -61,6 +61,28 @@ static struct tpm_inf_dev tpm_dev;
/*
+ * Copy i2c-core:i2c_transfer() as close as possible without the adapter locks
+ * and algorithm check. These are done by the caller for atomicity.
+ */
+static int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ unsigned long orig_jiffies;
+ int ret, try;
+
+ /* Retry automatically on arbitration loss */
+ orig_jiffies = jiffies;
+ for (ret = 0, try = 0; try <= adap->retries; try++) {
+ ret = adap->algo->master_xfer(adap, msgs, num);
+ if (ret != -EAGAIN)
+ break;
+ if (time_after(jiffies, orig_jiffies + adap->timeout))
+ break;
+ }
+ return ret;
+}
+
+/*
* iic_tpm_read() - read from TPM register
* @addr: register address to read from
* @buffer: provided by caller
@@ -83,8 +105,13 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
int rc;
int count;
+ /* Lock the adapter for the duration of the whole sequence. */
+ if (!tpm_dev.client->adapter->algo->master_xfer)
+ return -EOPNOTSUPP;
+ i2c_lock_adapter(tpm_dev.client->adapter);
+
for (count = 0; count < MAX_COUNT; count++) {
- rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
+ rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
if (rc > 0)
break; /* break here to skip sleep */
@@ -92,19 +119,21 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
}
if (rc <= 0)
- return -EIO;
+ goto out;
/* After the TPM has successfully received the register address it needs
* some time, thus we're sleeping here again, before retrieving the data
*/
for (count = 0; count < MAX_COUNT; count++) {
usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
- rc = i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
+ rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg2, 1);
if (rc > 0)
break;
}
+out:
+ i2c_unlock_adapter(tpm_dev.client->adapter);
if (rc <= 0)
return -EIO;
@@ -121,17 +150,22 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
struct i2c_msg msg1 = { tpm_dev.client->addr, 0, len + 1, tpm_dev.buf };
+ if (!tpm_dev.client->adapter->algo->master_xfer)
+ return -EOPNOTSUPP;
+ i2c_lock_adapter(tpm_dev.client->adapter);
+
tpm_dev.buf[0] = addr;
memcpy(&(tpm_dev.buf[1]), buffer, len);
for (count = 0; count < max_count; count++) {
- rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
+ rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
if (rc > 0)
break;
usleep_range(sleep_low, sleep_hi);
}
+ i2c_unlock_adapter(tpm_dev.client->adapter);
if (rc <= 0)
return -EIO;
--
1.7.6.msysgit.0
Hi Peter,
> +/*
> + * Copyright (C) 2012 Infineon Technologies
> + *
> + * Authors:
> + * Peter Huewe <[email protected]>
> + *
> + * Device driver for TCG/TCPA TPM (trusted platform module).
> + * Specifications at http://www.trustedcomputinggroup.org
> + *
> + * This device driver implements the TPM interface as defined in
> + * the TCG TPM Interface Spec version 1.2, revision 1.0 and the
> + * Infineon I2C Protocol Stack Specification v0.20.
> + *
> + * It is based on the original tpm_tis device driver from Leendert van
> + * Dorn and Kyleen Hall.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2 of the
> + * License.
> + *
> + *
> + */
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/wait.h>
> +#include "tpm.h"
> +
> +/* max. buffer size supported by our tpm */
> +#define TPM_BUFSIZE 1260
> +
> +/* max. number of iterations after i2c NAK */
> +#define MAX_COUNT 3
> +
> +#define SLEEP_DURATION_LOW 55
> +#define SLEEP_DURATION_HI 65
> +
> +/* max. number of iterations after i2c NAK for 'long' commands
> + * we need this especially for sending TPM_READY, since the cleanup after the
> + * transtion to the ready state may take some time, but it is unpredictable
> + * how long it will take.
> + */
> +#define MAX_COUNT_LONG 50
> +
> +#define SLEEP_DURATION_LONG_LOW 200
> +#define SLEEP_DURATION_LONG_HI 220
> +
> +/* expected value for DIDVID register */
> +#define TPM_TIS_I2C_DID_VID 0x000b15d1L
> +
> +/* Structure to store I2C TPM specific stuff */
> +struct tpm_inf_dev {
> + struct i2c_client *client;
> + u8 buf[TPM_BUFSIZE+sizeof(u8)]; /* max. buffer size + addr */
> + struct tpm_chip *chip;
> +};
> +
> +static struct tpm_inf_dev tpm_dev;
> +
> +
> +/*
> + * iic_tpm_read() - read from TPM register
> + * @addr: register address to read from
> + * @buffer: provided by caller
> + * @len: number of bytes to read
> + *
> + * Read len bytes from TPM register and put them into
> + * buffer (little-endian format, i.e. first byte is put into buffer[0]).
> + *
> + * NOTE: TPM is big-endian for multi-byte values. Multi-byte
> + * values have to be swapped.
> + *
> + * Return -EIO on error, 0 on success.
> + */
> +static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
> +{
> +
> + struct i2c_msg msg1 = { tpm_dev.client->addr, 0, 1, &addr };
> + struct i2c_msg msg2 = { tpm_dev.client->addr, I2C_M_RD, len, buffer };
> +
> + int rc;
> + int count;
> +
> + for (count = 0; count < MAX_COUNT; count++) {
> + rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
> + if (rc > 0)
> + break; /* break here to skip sleep */
> +
> + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> + }
Why don't you use the i2c_smbus* family function to read/write
from i2c. Everything you wrote here is already implemented there,
this is just overcode.
> +
> + if (rc <= 0)
> + return -EIO;
> +
> + /* After the TPM has successfully received the register address it needs
> + * some time, thus we're sleeping here again, before retrieving the data
> + */
> + for (count = 0; count < MAX_COUNT; count++) {
> + usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> + rc = i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
> + if (rc > 0)
> + break;
> +
> + }
... same here ...
> +
> + if (rc <= 0)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
> + unsigned int sleep_low,
> + unsigned int sleep_hi,
> + u8 max_count)
> +{
> + int rc = -1;
Haven't understood why here you are initializing -1
> + int count;
> +
> + struct i2c_msg msg1 = { tpm_dev.client->addr, 0, len + 1, tpm_dev.buf };
> +
> + tpm_dev.buf[0] = addr;
> + memcpy(&(tpm_dev.buf[1]), buffer, len);
> +
> + for (count = 0; count < max_count; count++) {
> + rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
> + if (rc > 0)
> + break;
> +
> + usleep_range(sleep_low, sleep_hi);
> + }
... same here ...
> +
> + if (rc <= 0)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +/*
> + * iic_tpm_write() - write to TPM register
> + * @addr: register address to write to
> + * @buffer: containing data to be written
> + * @len: number of bytes to write
> + *
> + * Write len bytes from provided buffer to TPM register (little
> + * endian format, i.e. buffer[0] is written as first byte).
> + *
> + * NOTE: TPM is big-endian for multi-byte values. Multi-byte
> + * values have to be swapped.
> + *
> + * NOTE: use this function instead of the iic_tpm_write_generic function.
> + *
> + * Return -EIO on error, 0 on success
> + */
> +static int iic_tpm_write(u8 addr, u8 *buffer, size_t len)
> +{
> + return iic_tpm_write_generic(addr, buffer, len, SLEEP_DURATION_LOW,
> + SLEEP_DURATION_HI, MAX_COUNT);
> +}
> +
> +/*
> + * This function is needed especially for the cleanup situation after
> + * sending TPM_READY
> + * */
> +static int iic_tpm_write_long(u8 addr, u8 *buffer, size_t len)
> +{
> + return iic_tpm_write_generic(addr, buffer, len, SLEEP_DURATION_LONG_LOW,
> + SLEEP_DURATION_LONG_HI, MAX_COUNT_LONG);
> +}
> +
> +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 buf;
> + int rc;
> +
> + rc = iic_tpm_read(TPM_ACCESS(loc), &buf, 1);
> + if (rc < 0)
> + return rc;
> +
> + if ((buf & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
> + (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
> + chip->vendor.locality = loc;
> + return loc;
> + }
> +
> + return -1;
Please choose a valid errno
> +}
> +
> +static void release_locality(struct tpm_chip *chip, int loc, int force)
> +{
> + u8 buf;
> + if (iic_tpm_read(TPM_ACCESS(loc), &buf, 1) < 0)
> + return;
> +
> + if (force || (buf & (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID)) ==
> + (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID)) {
> + buf = TPM_ACCESS_ACTIVE_LOCALITY;
> + iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
here you are ignoring the failure case
> + }
> +}
> +
> +static int request_locality(struct tpm_chip *chip, int loc)
> +{
> + unsigned long stop;
> + u8 buf = TPM_ACCESS_REQUEST_USE;
> +
> + if (check_locality(chip, loc) >= 0)
> + return loc;
> +
> + iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
also here the failure is ignored
> +
> + /* wait for burstcount */
> + stop = jiffies + chip->vendor.timeout_a;
> + do {
> + if (check_locality(chip, loc) >= 0)
> + return loc;
> + msleep(TPM_TIMEOUT);
> + } while (time_before(jiffies, stop));
> +
> + return -1;
Please choose a valid errno
> +}
> +
> +static u8 tpm_tis_i2c_status(struct tpm_chip *chip)
> +{
> + /* NOTE: since i2c read may fail, return 0 in this case --> time-out */
> + u8 buf;
> + if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
I think (but not sure) that this may upset
./scripts/checkpatch.pl
> + return 0;
> + else
> + return buf;
> +}
> +
> +static void tpm_tis_i2c_ready(struct tpm_chip *chip)
> +{
> + /* this causes the current command to be aborted */
> + u8 buf = TPM_STS_COMMAND_READY;
> + iic_tpm_write_long(TPM_STS(chip->vendor.locality), &buf, 1);
Still :) is there any reason you are ignoring the return value
from iic_tpm_write_long?
> +}
> +
> +static ssize_t get_burstcount(struct tpm_chip *chip)
> +{
> + unsigned long stop;
> + ssize_t burstcnt;
> + u8 buf[3];
> +
> + /* wait for burstcount */
> + /* which timeout value, spec has 2 answers (c & d) */
> + stop = jiffies + chip->vendor.timeout_d;
> + do {
> + /* Note: STS is little endian */
> + if (iic_tpm_read(TPM_STS(chip->vendor.locality) + 1, buf, 3) < 0)
> + burstcnt = 0;
> + else
> + burstcnt = (buf[2] << 16) + (buf[1] << 8) + buf[0];
> +
> + if (burstcnt)
> + return burstcnt;
> +
> + msleep(TPM_TIMEOUT);
> + } while (time_before(jiffies, stop));
> + return -EBUSY;
> +}
> +
> +static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> + int *status)
> +{
> + unsigned long stop;
> +
> + /* check current status */
> + *status = tpm_tis_i2c_status(chip);
> + if ((*status & mask) == mask)
> + return 0;
> +
> + stop = jiffies + timeout;
> + do {
> + msleep(TPM_TIMEOUT);
> + *status = tpm_tis_i2c_status(chip);
maybe you should sleep after the i2c_status()
> + if ((*status & mask) == mask)
> + return 0;
> +
> + } while (time_before(jiffies, stop));
> +
> + return -ETIME;
> +}
> +
> +static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + size_t size = 0;
> + ssize_t burstcnt;
> + int rc;
> +
> + while (size < count) {
> + burstcnt = get_burstcount(chip);
> +
> + /* burstcnt < 0 = tpm is busy */
> + if (burstcnt < 0)
> + return burstcnt;
> +
> + /* limit received data to max. left */
> + if (burstcnt > (count-size))
> + burstcnt = count-size;
> +
> + rc = iic_tpm_read(TPM_DATA_FIFO(chip->vendor.locality),
> + &(buf[size]),
> + burstcnt);
> + if (rc == 0)
> + size += burstcnt;
> +
> + }
> + return size;
> +}
> +
> +static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + int size = 0;
> + int expected, status;
> +
> + if (count < TPM_HEADER_SIZE) {
> + size = -EIO;
> + goto out;
> + }
> +
> + /* read first 10 bytes, including tag, paramsize, and result */
> + size = recv_data(chip, buf, TPM_HEADER_SIZE);
> + if (size < TPM_HEADER_SIZE) {
> + dev_err(chip->dev, "Unable to read header\n");
> + goto out;
> + }
> +
> + expected = be32_to_cpu(*(__be32 *) (buf + 2));
> + if ((size_t)expected > count) {
> + size = -EIO;
> + goto out;
> + }
> +
> + size += recv_data(chip, &buf[TPM_HEADER_SIZE],
> + expected - TPM_HEADER_SIZE);
> + if (size < expected) {
> + dev_err(chip->dev, "Unable to read remainder of result\n");
> + size = -ETIME;
> + goto out;
> + }
> +
> + wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c, &status);
> + if (status & TPM_STS_DATA_AVAIL) { /* retry? */
> + dev_err(chip->dev, "Error left over data\n");
> + size = -EIO;
> + goto out;
> + }
> +
> +out:
> + tpm_tis_i2c_ready(chip);
> + /* The TPM needs some time to clean up here,
> + * so we sleep rather than keeping the bus busy
> + */
> + usleep_range(2400, 2600);
> + release_locality(chip, chip->vendor.locality, 0);
> + return size;
> +}
> +
> +static int tpm_tis_i2c_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> + int rc, status;
> + ssize_t burstcnt;
> + size_t count = 0;
> + u8 sts = TPM_STS_GO;
> +
> + if (len > TPM_BUFSIZE)
> + return -E2BIG; /* command is too long for our tpm, sorry */
> +
> + if (request_locality(chip, 0) < 0)
> + return -EBUSY;
> +
> + status = tpm_tis_i2c_status(chip);
> + if ((status & TPM_STS_COMMAND_READY) == 0) {
> + tpm_tis_i2c_ready(chip);
> + if (wait_for_stat
> + (chip, TPM_STS_COMMAND_READY,
> + chip->vendor.timeout_b, &status) < 0) {
> + rc = -ETIME;
> + goto out_err;
> + }
> + }
> +
> + while (count < len - 1) {
> + burstcnt = get_burstcount(chip);
> + dev_dbg(chip->dev,
> + "send(): count=%zd, len=%zd, burstcount=%zd (plain)\n",
> + count, len, burstcnt);
> +
> + /* burstcnt < 0 = tpm is busy */
> + if (burstcnt < 0)
> + return burstcnt;
> +
> + if (burstcnt > (len-1-count))
> + burstcnt = len-1-count;
> +
> + dev_dbg(chip->dev, "send(): burstcount=%zd\n", burstcnt);
> +
> + rc = iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality),
> + &(buf[count]), burstcnt);
> + if (rc == 0)
> + count += burstcnt;
> +
> + wait_for_stat(chip, TPM_STS_VALID,
> + chip->vendor.timeout_c, &status);
> +
> + if ((status & TPM_STS_DATA_EXPECT) == 0) {
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + }
> +
> + dev_dbg(chip->dev, "send(): last byte @ count=%zd\n", count);
> +
> + /* write last byte */
> + iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality), &(buf[count]), 1);
> + wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c, &status);
> + if ((status & TPM_STS_DATA_EXPECT) != 0) {
> + rc = -EIO;
> + goto out_err;
> + }
> +
> + /* go and do it */
> + iic_tpm_write(TPM_STS(chip->vendor.locality), &sts, 1);
> +
> + return len;
> +out_err:
> + tpm_tis_i2c_ready(chip);
> + /* The TPM needs some time to clean up here,
> + * so we sleep rather than keeping the bus busy
> + */
> + usleep_range(2400, 2600);
> + release_locality(chip, chip->vendor.locality, 0);
> + return rc;
> +}
> +
> +static const struct file_operations tis_ops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .open = tpm_open,
> + .read = tpm_read,
> + .write = tpm_write,
> + .release = tpm_release,
> +};
> +
> +static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
> +static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
> +static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
> +static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
> +static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
> +static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
> +static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
> +static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
> +static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
> +static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
> +
> +static struct attribute *tis_attrs[] = {
> + &dev_attr_pubek.attr,
> + &dev_attr_pcrs.attr,
> + &dev_attr_enabled.attr,
> + &dev_attr_active.attr,
> + &dev_attr_owned.attr,
> + &dev_attr_temp_deactivated.attr,
> + &dev_attr_caps.attr,
> + &dev_attr_cancel.attr,
> + &dev_attr_durations.attr,
> + &dev_attr_timeouts.attr, NULL,
> +};
> +
> +static struct attribute_group tis_attr_grp = {
> + .attrs = tis_attrs
> +};
> +
> +static struct tpm_vendor_specific tpm_tis_i2c = {
> + .status = tpm_tis_i2c_status,
> + .recv = tpm_tis_i2c_recv,
> + .send = tpm_tis_i2c_send,
> + .cancel = tpm_tis_i2c_ready,
> + .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> + .req_canceled = TPM_STS_COMMAND_READY,
> + .attr_group = &tis_attr_grp,
> + .miscdev = {
> + .fops = &tis_ops,},
> +};
> +
> +static int tpm_tis_i2c_init(struct device *dev)
> +{
> + u32 vendor;
> + int rc = 0;
> + struct tpm_chip *chip;
> +
> + chip = tpm_register_hardware(dev, &tpm_tis_i2c);
> + if (!chip) {
> + rc = -ENODEV;
> + goto out_err;
> + }
> +
> + /* Disable interrupts */
> + chip->vendor.irq = 0;
> +
> + /* Default timeouts */
> + chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
> + chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> + chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
> +
> + if (request_locality(chip, 0) != 0) {
> + rc = -ENODEV;
> + goto out_vendor;
> + }
> +
> + /* read four bytes from DID_VID register */
> + if (iic_tpm_read(TPM_DID_VID(0), (u8 *) &vendor, 4) < 0) {
> + rc = -EIO;
> + goto out_vendor;
> + }
> +
> + /* create DID_VID register value, after swapping to little-endian */
> + vendor = be32_to_cpu((__be32) vendor);
> +
> + if (vendor != TPM_TIS_I2C_DID_VID) {
> + rc = -ENODEV;
> + goto out_release;
> + }
> +
> + dev_info(dev, "1.2 TPM (device-id 0x%X)\n", vendor >> 16);
> +
> + INIT_LIST_HEAD(&chip->vendor.list);
> + tpm_dev.chip = chip;
> +
> + tpm_get_timeouts(chip);
> + tpm_do_selftest(chip);
> +
> + return 0;
> +
> +out_release:
> + release_locality(chip, chip->vendor.locality, 1);
> +
> +out_vendor:
> + tpm_dev_vendor_release(chip);
> + tpm_remove_hardware(chip->dev);
> +
> +out_err:
> + return rc;
> +}
> +
> + <------
Just to be strict... in case you'll resend the patch, there is
one blank line more than necessary :)
> +static const struct i2c_device_id tpm_i2c_tis_table[] = {
> + { "tpm_tis_i2c", 0 },
> + { },
> +};
> +
> +#ifdef CONFIG_PM
and what if CONFIG_PM is not defined?
> +/* NOTE:
> + * Suspend does currently not work Nvidias Tegra2 Platform
> + * but works fine on Beagleboard (arm omap).
> + *
> + * This function will block System Suspend if TPM is not initialized,
> + * however the TPM is usually initialized by BIOS/u-boot or by sending
> + * a tpm startup command.
> + */
> +static int tpm_tis_i2c_suspend(struct device *dev)
> +{
> + return tpm_pm_suspend(dev, dev->power.power_state);
> +}
> +
> +static int tpm_tis_i2c_resume(struct device *dev)
> +{
> + return tpm_pm_resume(dev);
> +}
> +
> +static const struct dev_pm_ops tpm_tis_i2c_ops = {
> + .suspend = tpm_tis_i2c_suspend,
> + .resume = tpm_tis_i2c_resume,
> +};
> +#endif
> +
> +
> +static int dummy_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + return 0;
> +}
> +
> +static int dummy_remove(struct i2c_client *client)
> +{
> + return 0;
> +}
Ignoring probe and remove, is a matter of choice, but to me looks
awful.
> +static struct i2c_driver tpm_i2c_tis_driver = {
> +
> + .id_table = tpm_i2c_tis_table,
> + .probe = dummy_probe,
> + .remove = dummy_remove,
> + .driver = {
> + .name = "tpm_tis_i2c",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &tpm_tis_i2c_ops,
> +#endif
> + },
> +};
> +
> +
> +static struct i2c_adapter *adap;
> +static struct i2c_client *client;
> +static struct i2c_board_info info = {
> + I2C_BOARD_INFO("tpm_tis_i2c", 0x20),
> +};
> +
> +
> +static int addr = 0x20;
> +module_param(addr, int, S_IRUGO);
> +MODULE_PARM_DESC(addr, "TPM I2C Device Address (default: 0x20)");
> +
> +static int bus_id = 2;
> +module_param(bus_id, int, S_IRUGO);
> +MODULE_PARM_DESC(bus_id, "TPM I2C Bus Id (default: 2)");
> +
> +static int __init init_tis_i2c(void)
> +{
> +
> + int rc = 0;
> + info.addr = addr;
> +
> +
> + if (tpm_dev.client != NULL)
> + return -EBUSY;
> +
> + adap = i2c_get_adapter(bus_id);
> + if (!adap)
> + return -ENODEV;
> +
> + client = i2c_new_device(adap, &info);
> + if (!client) {
> + i2c_put_adapter(adap);
> + return -ENODEV;
> + }
> +
> + rc = i2c_add_driver(&tpm_i2c_tis_driver);
> + if (rc != 0) {
> + i2c_del_driver(&tpm_i2c_tis_driver);
> + i2c_put_adapter(tpm_dev.client->adapter);
> + return -ENODEV;
> + }
> + client->driver = &tpm_i2c_tis_driver;
> +
> + tpm_dev.client = client;
> +
> + rc = tpm_tis_i2c_init(&client->dev);
> + if (rc < 0) {
> + i2c_del_driver(&tpm_i2c_tis_driver);
> + i2c_put_adapter(tpm_dev.client->adapter);
> + device_del(&(tpm_dev.client->dev));
> + }
> +
> + return rc;
> +}
I think this should be done in the probe function. Moreover you
are also not checking the functionalities of the i2c drivers.
> +
> +static void __exit cleanup_tis_i2c(void)
> +{
> + struct tpm_chip *chip = tpm_dev.chip;
> + release_locality(chip, chip->vendor.locality, 1);
> +
> + tpm_dev_vendor_release(chip);
> + tpm_remove_hardware(chip->dev);
> +
> + i2c_del_driver(&tpm_i2c_tis_driver);
> +
> + i2c_put_adapter(tpm_dev.client->adapter);
> +
> + /*
> + * taken from core.c as workaround since
> + * tpm_remove_hardware requires device structure
> + */
> + pr_debug("device: '%s': %s\n",
> + dev_name(&(tpm_dev.client->dev)), __func__);
> + device_del(&(tpm_dev.client->dev));
> +}
Still this should be done in the remove function
> +
> +module_init(init_tis_i2c);
> +module_exit(cleanup_tis_i2c);
The correct way of doing this is by using module_i2c_driver()
> +MODULE_AUTHOR("Peter Huewe <[email protected]>");
> +MODULE_DESCRIPTION("TPM TIS I2C Driver");
> +MODULE_VERSION("2.1.3");
> +MODULE_LICENSE("GPL");
> --
> 1.7.6.msysgit.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi again :),
On Wed, May 02, 2012 at 04:03:36PM +0200, Peter Huewe wrote:
> From: Bryan Freed <[email protected]>
>
> This is derived from Peter Huewe's recommended fix:
>
> On some ChromeOS systems, a TPM sharing the I2C bus with another device
> gets confused when it sees I2C requests to that other device.
> This change locks the I2C adapter for the duration of the full sequence
> of I2C requests the TPM needs to complete.
>
> smbus_xfer is not supported, but SMBUS is not supported by the original
> driver, either.
>
> Signed-off-by: Bryan Freed <[email protected]>
> Signed-off-by: Peter Huewe <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_i2c.c | 42 ++++++++++++++++++++++++++++++++++++---
> 1 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> index 8975abf..e68f209 100644
> --- a/drivers/char/tpm/tpm_tis_i2c.c
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -61,6 +61,28 @@ static struct tpm_inf_dev tpm_dev;
>
>
> /*
> + * Copy i2c-core:i2c_transfer() as close as possible without the adapter locks
> + * and algorithm check. These are done by the caller for atomicity.
> + */
> +static int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + unsigned long orig_jiffies;
> + int ret, try;
> +
> + /* Retry automatically on arbitration loss */
> + orig_jiffies = jiffies;
> + for (ret = 0, try = 0; try <= adap->retries; try++) {
> + ret = adap->algo->master_xfer(adap, msgs, num);
> + if (ret != -EAGAIN)
> + break;
> + if (time_after(jiffies, orig_jiffies + adap->timeout))
> + break;
> + }
> + return ret;
> +}
> +
> +/*
> * iic_tpm_read() - read from TPM register
> * @addr: register address to read from
> * @buffer: provided by caller
> @@ -83,8 +105,13 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
> int rc;
> int count;
>
> + /* Lock the adapter for the duration of the whole sequence. */
> + if (!tpm_dev.client->adapter->algo->master_xfer)
> + return -EOPNOTSUPP;
> + i2c_lock_adapter(tpm_dev.client->adapter);
> +
> for (count = 0; count < MAX_COUNT; count++) {
> - rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
> + rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
> if (rc > 0)
> break; /* break here to skip sleep */
>
> @@ -92,19 +119,21 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
> }
>
> if (rc <= 0)
> - return -EIO;
> + goto out;
>
> /* After the TPM has successfully received the register address it needs
> * some time, thus we're sleeping here again, before retrieving the data
> */
> for (count = 0; count < MAX_COUNT; count++) {
> usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> - rc = i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
> + rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg2, 1);
> if (rc > 0)
> break;
>
> }
>
> +out:
> + i2c_unlock_adapter(tpm_dev.client->adapter);
> if (rc <= 0)
> return -EIO;
>
> @@ -121,17 +150,22 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
>
> struct i2c_msg msg1 = { tpm_dev.client->addr, 0, len + 1, tpm_dev.buf };
>
> + if (!tpm_dev.client->adapter->algo->master_xfer)
> + return -EOPNOTSUPP;
> + i2c_lock_adapter(tpm_dev.client->adapter);
> +
> tpm_dev.buf[0] = addr;
> memcpy(&(tpm_dev.buf[1]), buffer, len);
>
> for (count = 0; count < max_count; count++) {
> - rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
> + rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
> if (rc > 0)
> break;
>
> usleep_range(sleep_low, sleep_hi);
> }
>
> + i2c_unlock_adapter(tpm_dev.client->adapter);
> if (rc <= 0)
> return -EIO;
>
try to have a look to the i2c_smbus* function, you could avoid
lot of code
Andi
> --
> 1.7.6.msysgit.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
(Sorry for resending...)
Andi, it is not clear what i2c_smbus_* functions in particular will
improve upon this change.
All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
point will i2c_lock_adapter() for each request.
This is true for adapters that support SMBUS (where the lock occurs
directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
i2c_transfer() called through i2c_smbus_xfer_emulated()).
The goal of this change is for the tpm_tis_i2c driver to be able to
lock an adapter over the duration of several I2C requests.
Presumably, that is why i2c_lock_adapter() is exported.
bryan.
On Wed, May 2, 2012 at 8:17 AM, Andi Shyti <[email protected]> wrote:
> Hi again :),
>
> On Wed, May 02, 2012 at 04:03:36PM +0200, Peter Huewe wrote:
>> From: Bryan Freed <[email protected]>
>>
>> This is derived from Peter Huewe's recommended fix:
>>
>> On some ChromeOS systems, a TPM sharing the I2C bus with another device
>> gets confused when it sees I2C requests to that other device.
>> This change locks the I2C adapter for the duration of the full sequence
>> of I2C requests the TPM needs to complete.
>>
>> smbus_xfer is not supported, but SMBUS is not supported by the original
>> driver, either.
>>
>> Signed-off-by: Bryan Freed <[email protected]>
>> Signed-off-by: Peter Huewe <[email protected]>
>> ---
>> ?drivers/char/tpm/tpm_tis_i2c.c | ? 42 ++++++++++++++++++++++++++++++++++++---
>> ?1 files changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
>> index 8975abf..e68f209 100644
>> --- a/drivers/char/tpm/tpm_tis_i2c.c
>> +++ b/drivers/char/tpm/tpm_tis_i2c.c
>> @@ -61,6 +61,28 @@ static struct tpm_inf_dev tpm_dev;
>>
>>
>> ?/*
>> + * Copy i2c-core:i2c_transfer() as close as possible without the adapter locks
>> + * and algorithm check. ?These are done by the caller for atomicity.
>> + */
>> +static int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?int num)
>> +{
>> + ? ? unsigned long orig_jiffies;
>> + ? ? int ret, try;
>> +
>> + ? ? /* Retry automatically on arbitration loss */
>> + ? ? orig_jiffies = jiffies;
>> + ? ? for (ret = 0, try = 0; try <= adap->retries; try++) {
>> + ? ? ? ? ? ? ret = adap->algo->master_xfer(adap, msgs, num);
>> + ? ? ? ? ? ? if (ret != -EAGAIN)
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? if (time_after(jiffies, orig_jiffies + adap->timeout))
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? }
>> + ? ? return ret;
>> +}
>> +
>> +/*
>> ? * iic_tpm_read() - read from TPM register
>> ? * @addr: register address to read from
>> ? * @buffer: provided by caller
>> @@ -83,8 +105,13 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
>> ? ? ? int rc;
>> ? ? ? int count;
>>
>> + ? ? /* Lock the adapter for the duration of the whole sequence. */
>> + ? ? if (!tpm_dev.client->adapter->algo->master_xfer)
>> + ? ? ? ? ? ? return -EOPNOTSUPP;
>> + ? ? i2c_lock_adapter(tpm_dev.client->adapter);
>> +
>> ? ? ? for (count = 0; count < MAX_COUNT; count++) {
>> - ? ? ? ? ? ? rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
>> + ? ? ? ? ? ? rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
>> ? ? ? ? ? ? ? if (rc > 0)
>> ? ? ? ? ? ? ? ? ? ? ? break; /* break here to skip sleep */
>>
>> @@ -92,19 +119,21 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
>> ? ? ? }
>>
>> ? ? ? if (rc <= 0)
>> - ? ? ? ? ? ? return -EIO;
>> + ? ? ? ? ? ? goto out;
>>
>> ? ? ? /* After the TPM has successfully received the register address it needs
>> ? ? ? ?* some time, thus we're sleeping here again, before retrieving the data
>> ? ? ? ?*/
>> ? ? ? for (count = 0; count < MAX_COUNT; count++) {
>> ? ? ? ? ? ? ? usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> - ? ? ? ? ? ? rc = i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
>> + ? ? ? ? ? ? rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg2, 1);
>> ? ? ? ? ? ? ? if (rc > 0)
>> ? ? ? ? ? ? ? ? ? ? ? break;
>>
>> ? ? ? }
>>
>> +out:
>> + ? ? i2c_unlock_adapter(tpm_dev.client->adapter);
>> ? ? ? if (rc <= 0)
>> ? ? ? ? ? ? ? return -EIO;
>>
>> @@ -121,17 +150,22 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
>>
>> ? ? ? struct i2c_msg msg1 = { tpm_dev.client->addr, 0, len + 1, tpm_dev.buf };
>>
>> + ? ? if (!tpm_dev.client->adapter->algo->master_xfer)
>> + ? ? ? ? ? ? return -EOPNOTSUPP;
>> + ? ? i2c_lock_adapter(tpm_dev.client->adapter);
>> +
>> ? ? ? tpm_dev.buf[0] = addr;
>> ? ? ? memcpy(&(tpm_dev.buf[1]), buffer, len);
>>
>> ? ? ? for (count = 0; count < max_count; count++) {
>> - ? ? ? ? ? ? rc = i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
>> + ? ? ? ? ? ? rc = i2c_transfer_nolock(tpm_dev.client->adapter, &msg1, 1);
>> ? ? ? ? ? ? ? if (rc > 0)
>> ? ? ? ? ? ? ? ? ? ? ? break;
>>
>> ? ? ? ? ? ? ? usleep_range(sleep_low, sleep_hi);
>> ? ? ? }
>>
>> + ? ? i2c_unlock_adapter(tpm_dev.client->adapter);
>> ? ? ? if (rc <= 0)
>> ? ? ? ? ? ? ? return -EIO;
>>
>
>
>
> try to have a look to the i2c_smbus* function, you could avoid
> lot of code
>
> Andi
>
>> --
>> 1.7.6.msysgit.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at ?http://www.tux.org/lkml/
Hi Bryan,
> > try to have a look to the i2c_smbus* function, you could avoid
> > lot of code
On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote:
> (Sorry for resending...)
>
> Andi, it is not clear what i2c_smbus_* functions in particular will
> improve upon this change.
>
> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
> point will i2c_lock_adapter() for each request.
> This is true for adapters that support SMBUS (where the lock occurs
> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
> i2c_transfer() called through i2c_smbus_xfer_emulated()).
>
> The goal of this change is for the tpm_tis_i2c driver to be able to
> lock an adapter over the duration of several I2C requests.
> Presumably, that is why i2c_lock_adapter() is exported.
the i2c_smbus_* functions will not improve anything to the
driver, it will increase the readability and it will allow you to
do the same stuff with less code.
All the i2c communication implementation you wrote here, is
already written in the i2c-core.c file.
Andi
Hi Andi,
On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <[email protected]> wrote:
> Hi Bryan,
>
>> > try to have a look to the i2c_smbus* function, you could avoid
>> > lot of code
> On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote:
>> (Sorry for resending...)
>>
>> Andi, it is not clear what i2c_smbus_* functions in particular will
>> improve upon this change.
>>
>> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
>> point will i2c_lock_adapter() for each request.
>> This is true for adapters that support SMBUS (where the lock occurs
>> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
>> i2c_transfer() called through i2c_smbus_xfer_emulated()).
>>
>> The goal of this change is for the tpm_tis_i2c driver to be able to
>> lock an adapter over the duration of several I2C requests.
>> Presumably, that is why i2c_lock_adapter() is exported.
>
> the i2c_smbus_* functions will not improve anything to the
> driver, it will increase the readability and it will allow you to
> do the same stuff with less code.
I think I see what you are saying.
We could (in the unmodified version of this driver) replace all the
iic_tpm_read() calls of one byte (which sends an address byte before
reading a data byte) with an i2c_smbus_read_byte_command() call which
does the same thing.
Switching to the SMBUS calls in this driver will still work on
adapters that only support I2C because of i2c_smbus_xfer_emulated().
Right?
> All the i2c communication implementation you wrote here, is
> already written in the i2c-core.c file.
Right. Unfortunately, we cannot use any of the i2c_smbus_* functions
in this driver. The device will fail because the adapter lock is not
held long enough to prevent I2C traffic going to other devices on the
same bus. That other traffic to other devices confuses the i2c core
in this device. Our only driver solution is to lock the adapter for a
longer duration.
Yes, the code we have here is copied from the i2c-core.c file. In
fact, we comment this with, "Copy i2c-core:i2c_transfer() as close as
possible without the adapter locks
and algorithm check".
And that really is the problem with using the i2c-core.c calls. This
driver needs to lock the adapter for an extended duration.
> Andi
bryan.
Hi,
On Wed, May 02, 2012 at 12:06:14PM -0700, Bryan Freed wrote:
> Hi Andi,
>
> On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <[email protected]> wrote:
> > Hi Bryan,
> >
> >> > try to have a look to the i2c_smbus* function, you could avoid
> >> > lot of code
> > On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote:
> >> (Sorry for resending...)
> >>
> >> Andi, it is not clear what i2c_smbus_* functions in particular will
> >> improve upon this change.
> >>
> >> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
> >> point will i2c_lock_adapter() for each request.
> >> This is true for adapters that support SMBUS (where the lock occurs
> >> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
> >> i2c_transfer() called through i2c_smbus_xfer_emulated()).
> >>
> >> The goal of this change is for the tpm_tis_i2c driver to be able to
> >> lock an adapter over the duration of several I2C requests.
> >> Presumably, that is why i2c_lock_adapter() is exported.
> >
> > the i2c_smbus_* functions will not improve anything to the
> > driver, it will increase the readability and it will allow you to
> > do the same stuff with less code.
>
> I think I see what you are saying.
> We could (in the unmodified version of this driver) replace all the
> iic_tpm_read() calls of one byte (which sends an address byte before
> reading a data byte) with an i2c_smbus_read_byte_command() call which
> does the same thing.
> Switching to the SMBUS calls in this driver will still work on
> adapters that only support I2C because of i2c_smbus_xfer_emulated().
> Right?
> > All the i2c communication implementation you wrote here, is
> > already written in the i2c-core.c file.
>
> Right. Unfortunately, we cannot use any of the i2c_smbus_* functions
> in this driver. The device will fail because the adapter lock is not
> held long enough to prevent I2C traffic going to other devices on the
> same bus. That other traffic to other devices confuses the i2c core
> in this device. Our only driver solution is to lock the adapter for a
> longer duration.
>
> Yes, the code we have here is copied from the i2c-core.c file. In
> fact, we comment this with, "Copy i2c-core:i2c_transfer() as close as
> possible without the adapter locks
> and algorithm check".
>
> And that really is the problem with using the i2c-core.c calls. This
> driver needs to lock the adapter for an extended duration.
mmhhh... you still haven't convinced me. I always thought that
every dublicated code is useless.
You may have good reasons to do that, but I could still try to
find out a way on how to simplify it.
Thanks for the explanation,
Andi
Am Mittwoch 02 Mai 2012, 21:06:14 schrieb Bryan Freed:
> We could (in the unmodified version of this driver) replace all the
> iic_tpm_read() calls of one byte (which sends an address byte before
> reading a data byte) with an i2c_smbus_read_byte_command() call which
> does the same thing.
Unfortunately we have to use two seperate transfers (write/read) for a single
byte read as the device does not support "repeated start" conditions and will
simply fail to respond.
The device has some quirks that the driver has to workaround,
one is the repeated start problem, the other is the locking of the device.
If you can come up with a better solution I'd like to try it out if it's
compatible with the device.
Thanks,
Peter
Andi,
On Wed, May 2, 2012 at 1:18 PM, Andi Shyti <[email protected]> wrote:
> Hi,
>
> On Wed, May 02, 2012 at 12:06:14PM -0700, Bryan Freed wrote:
>> Hi Andi,
>>
>> On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <[email protected]> wrote:
>> > Hi Bryan,
>> >
>> >> > try to have a look to the i2c_smbus* function, you could avoid
>> >> > lot of code
>> > On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote:
>> >> (Sorry for resending...)
>> >>
>> >> Andi, it is not clear what i2c_smbus_* functions in particular will
>> >> improve upon this change.
>> >>
>> >> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
>> >> point will i2c_lock_adapter() for each request.
>> >> This is true for adapters that support SMBUS (where the lock occurs
>> >> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
>> >> i2c_transfer() called through i2c_smbus_xfer_emulated()).
>> >>
>> >> The goal of this change is for the tpm_tis_i2c driver to be able to
>> >> lock an adapter over the duration of several I2C requests.
>> >> Presumably, that is why i2c_lock_adapter() is exported.
>> >
>> > the i2c_smbus_* functions will not improve anything to the
>> > driver, it will increase the readability and it will allow you to
>> > do the same stuff with less code.
>>
>> I think I see what you are saying.
>> We could (in the unmodified version of this driver) replace all the
>> iic_tpm_read() calls of one byte (which sends an address byte before
>> reading a data byte) with an i2c_smbus_read_byte_command() call which
>> does the same thing.
>> Switching to the SMBUS calls in this driver will still work on
>> adapters that only support I2C because of i2c_smbus_xfer_emulated().
>> Right?
>> > All the i2c communication implementation you wrote here, is
>> > already written in the i2c-core.c file.
>>
>> Right. ?Unfortunately, we cannot use any of the i2c_smbus_* functions
>> in this driver. ?The device will fail because the adapter lock is not
>> held long enough to prevent I2C traffic going to other devices on the
>> same bus. ?That other traffic to other devices confuses the i2c core
>> in this device. ?Our only driver solution is to lock the adapter for a
>> longer duration.
>>
>> Yes, the code we have here is copied from the i2c-core.c file. ?In
>> fact, we comment this with, "Copy i2c-core:i2c_transfer() as close as
>> possible without the adapter locks
>> and algorithm check".
>>
>> And that really is the problem with using the i2c-core.c calls. ?This
>> driver needs to lock the adapter for an extended duration.
>
> mmhhh... you still haven't convinced me. I always thought that
> every dublicated code is useless.
Hey, I agree with you on that point. Duplicated code has its own problems.
A better solution would require i2c-core.c mods, mainly:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e9c1893..64cb9c2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1346,17 +1346,8 @@ int i2c_transfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
i2c_lock_adapter(adap);
}
- /* Retry automatically on arbitration loss */
- orig_jiffies = jiffies;
- for (ret = 0, try = 0; try <= adap->retries; try++) {
- ret = adap->algo->master_xfer(adap, msgs, num);
- if (ret != -EAGAIN)
- break;
- if (time_after(jiffies, orig_jiffies + adap->timeout))
- break;
- }
+ ret = i2c_transfer_nolock(&adap-, msgs, num);
i2c_unlock_adapter(adap);
-
return ret;
} else {
dev_dbg(&adap->dev, "I2C level transfers not supported\n");
@@ -1365,6 +1356,25 @@ int i2c_transfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
}
EXPORT_SYMBOL(i2c_transfer);
+int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num)
+{
+ unsigned long orig_jiffies;
+ int ret, try;
+
+ /* Retry automatically on arbitration loss */
+ orig_jiffies = jiffies;
+ for (ret = 0, try = 0; try <= adap->retries; try++) {
+ ret = adap->algo->master_xfer(adap, msgs, num);
+ if (ret != -EAGAIN)
+ break;
+ if (time_after(jiffies, orig_jiffies + adap->timeout))
+ break;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(i2c_transfer_nolock);
+
Then I would not need my own copy of i2c_transfer_nolock().
But making these ugly changes in the driver for one buggy device is
easier/safer than making it in the general I2C code.
bryan.
> You may have good reasons to do that, but I could still try to
> find out a way on how to simplify it.
>
> Thanks for the explanation,
> Andi
Hi Andi,
first of all thanks for the review! I really appreciate it.
If we can come up with solutions to improve the driver I'm more than happy to try them out.
For the moment, I still would appreciate if the driver gets merged in its current form and we do the (remaining) improvements later on,
as the driver is in heavy use for over a year now in Google Chromebooks.
> Why don't you use the i2c_smbus* family function to read/write
> from i2c. Everything you wrote here is already implemented there,
> this is just overcode.
The reason behind this is that the soft-i2c-tpm chip needs some special handling due to wake up, its "late acknowledge protocol" and unfortunately the not supported repeated start conditions, but let me explain a bit:
- No Repeated Starts:
We always have to send a stop bit to mark the end of a transfer, otherwise the tpm simply fails to respond.
So in order to read e.g. the access register byte we have to send:
S 0x20 W 0x00 P
S 0x20 R .... P
Thus we cannot use i2c_smbus*read* functions.
- Wakeup behavior:
After a successful transfer the soft i2c tpm goes back to sleep. When something toggles the SCL line the tpm wakes up and after some time (~50us) we can talk to it.
The i2c_smbus_xfer function does not sleep in between and thus the retry count would be pretty high (and bus speed dependent) in order to reach the 50us.
This would probably cause a lot of traffic on the i2c bus.
- "late acknowledge protocol"
The device does not support clock stretching and simply NAKs any transmission if it is not ready.
e.g. in the read case, after receiving the 'register address' to be read it needs some time to process the register address and prepare the data for transmission.
This is similar to the wakeup behavior but the sleep time might be a lot higher in case of writing.
Unfortunately I don't see any better solution at the moment for the i2c related topics.
I do acknowledge that we could have used i2c_master_send/recv but that would have saved only two or three lines (the setup of the i2c_msg struct).
About your other remarks:
>> +{
>> + int rc = -1;
> Haven't understood why here you are initializing -1
You're right this could have been done more elegantly. I'll probably clean this up in a separate patch.
>> +static int check_locality(struct tpm_chip *chip, int loc)
>> ...
>> + return -1;
> Please choose a valid errno
This is the same behavior as in the original tpm_tis driver:
git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/char/tpm/tpm_tis.c;hb=HEAD#l101
These functions e.g. check_locality can probably be generalized and reused by all tpm drivers, but I don't want to mix this up with this driver submission.
For the moment I'd leave it as it is.
>> +static void release_locality(struct tpm_chip *chip, int loc, int force)
>> ...
>> + iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
> here you are ignoring the failure case
Same applies to release_locality the semantics are the same in tpm_tis.
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/char/tpm/tpm_tis.c;hb=HEAD#l111
The reason behind ignoring the error code is more or less that we rely on the underlying TIS Protocol.
>> + if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
> I think (but not sure) that this may upset
> ./scripts/checkpatch.pl
checkpatch.pl -strict did not complain about this. It emits only one over 80 characters warning, which is 81 characters long.
>> +#ifdef CONFIG_PM
> and what if CONFIG_PM is not defined?
Then we don't need suspend and resume functions ;)
> I think this should be done in the probe function. ...
> Still this should be done in the remove function. ...
> The correct way of doing this is by using module_i2c_driver()
This is a valid point. I simply wasn't aware of this - the driver was initially created (and submitted) back in April 2011 where this macro did not yet exist.
I'd do a cleanup afterwards, probably with adding the probe/remove functionality - but I don't think it's a blocking point for the submission?
Thanks,
Peter
Infineon Technologies AG
CCS TI SWT SW ESW
Tel: +49 821 25851-86
Fax: +49 821 25851-40
[email protected]
****VISIT US AT: http://www.infineon.com *****
Infineon Technologies AG
Vorsitzender des Aufsichtsrats: Wolfgang Mayrhuber
Vorstand: Peter Bauer (Vorsitzender), Dominik Asam, Arunjai Mittal, Dr. Reinhard Ploss
Sitz der Gesellschaft: Neubiberg
Registergericht: M?nchen HRB 126492
Hi Peter,
sorry for the delay, finally I got some time to reply :)
On Thu, May 03, 2012 at 11:18:54AM +0000, [email protected] wrote:
> Hi Andi,
>
> first of all thanks for the review! I really appreciate it.
You're welcome, I hope it helps :)
> If we can come up with solutions to improve the driver I'm more than happy to try them out.
> For the moment, I still would appreciate if the driver gets merged in its current form and we do the (remaining) improvements later on,
> as the driver is in heavy use for over a year now in Google Chromebooks.
>
>
> > Why don't you use the i2c_smbus* family function to read/write
> > from i2c. Everything you wrote here is already implemented there,
> > this is just overcode.
>
> The reason behind this is that the soft-i2c-tpm chip needs some special handling due to wake up, its "late acknowledge protocol" and unfortunately the not supported repeated start conditions, but let me explain a bit:
>
> - No Repeated Starts:
> We always have to send a stop bit to mark the end of a transfer, otherwise the tpm simply fails to respond.
> So in order to read e.g. the access register byte we have to send:
> S 0x20 W 0x00 P
> S 0x20 R .... P
> Thus we cannot use i2c_smbus*read* functions.
>
>
> - Wakeup behavior:
> After a successful transfer the soft i2c tpm goes back to sleep. When something toggles the SCL line the tpm wakes up and after some time (~50us) we can talk to it.
> The i2c_smbus_xfer function does not sleep in between and thus the retry count would be pretty high (and bus speed dependent) in order to reach the 50us.
> This would probably cause a lot of traffic on the i2c bus.
>
>
> - "late acknowledge protocol"
> The device does not support clock stretching and simply NAKs any transmission if it is not ready.
> e.g. in the read case, after receiving the 'register address' to be read it needs some time to process the register address and prepare the data for transmission.
> This is similar to the wakeup behavior but the sleep time might be a lot higher in case of writing.
>
>
> Unfortunately I don't see any better solution at the moment for the i2c related topics.
> I do acknowledge that we could have used i2c_master_send/recv but that would have saved only two or three lines (the setup of the i2c_msg struct).
I understand, we had already a lively discussion on the other
patch on this driver about that.
I underlined there that personally I don't like code which is
written twice.
Which is exactly the i2c driver that you are using?
Do you think it could clean up the driver if you add to this bus
the smbus_xfer functions? At the end is a chip issue, not a
driver, so that implementing this function could help all the
device drivers that are using this chip.
> About your other remarks:
> >> +{
> >> + int rc = -1;
> > Haven't understood why here you are initializing -1
> You're right this could have been done more elegantly. I'll probably clean this up in a separate patch.
>
>
> >> +static int check_locality(struct tpm_chip *chip, int loc)
> >> ...
> >> + return -1;
> > Please choose a valid errno
> This is the same behavior as in the original tpm_tis driver:
> git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/char/tpm/tpm_tis.c;hb=HEAD#l101
> These functions e.g. check_locality can probably be generalized and reused by all tpm drivers, but I don't want to mix this up with this driver submission.
> For the moment I'd leave it as it is.
OK, but bear in mind '-1' is an awful error code :)
> >> +static void release_locality(struct tpm_chip *chip, int loc, int force)
> >> ...
> >> + iic_tpm_write(TPM_ACCESS(loc), &buf, 1);
> > here you are ignoring the failure case
>
> Same applies to release_locality the semantics are the same in tpm_tis.
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/char/tpm/tpm_tis.c;hb=HEAD#l111
>
>
> The reason behind ignoring the error code is more or less that we rely on the underlying TIS Protocol.
OK, but considering failures is never enough. If you will correct
this patch later on, it could be nice to consider every failure.
In my opinion who writes drivers should never rely 100% on the
above and below layers.
> >> + if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
> > I think (but not sure) that this may upset
> > ./scripts/checkpatch.pl
> checkpatch.pl -strict did not complain about this. It emits only one over 80 characters warning, which is 81 characters long.
I like to pander to checkpatch.pl, it's always better to have 0
error and 0 warnings
> >> +#ifdef CONFIG_PM
> > and what if CONFIG_PM is not defined?
> Then we don't need suspend and resume functions ;)
I think that considering the case that CONFIG_PM is not defined
could be a good use, something like
#ifdef CONFIG_PM
[...]
#else
[...] <--- here you can define the functions as null
#endif
this because, if in the future someone else will like to use
embedded in the code the power management functions can do it
easily without adding extra #ifdefs which pollute the code.
> > I think this should be done in the probe function. ...
> > Still this should be done in the remove function. ...
> > The correct way of doing this is by using module_i2c_driver()
>
> This is a valid point. I simply wasn't aware of this - the driver was initially created (and submitted) back in April 2011 where this macro did not yet exist.
> I'd do a cleanup afterwards, probably with adding the probe/remove functionality -
Yes, it's true the modle_i2c_driver is quite new as a define. In
order to use this macro definition, you should make all your
initialization in the probe function. The reason behind this is
that the module_i2c_driver, in his long queue of function calls,
comes across the probe, if the probe failes for some reasons,
then the kernel is able to manage it.
While if you make all the initializations on the __init function,
then is like cheating somehow.
> but I don't think it's a blocking point for the submission?
I don't know, I'm a reviewer by chance, this is up to the
maintainers. For sure the driver works, but I think that there is
still room for some improvements.
Andi