This patch set fixes two race conditions in the TPM subsystem:
* Two-phase initialization for struct tpm_chip so that device can
initialize fully initialize before exposing itself to the user
space. Also, in future TPM2 devices must be flagged before they
can be registered.
* Machines where there are two TPM devices exposed by ACPI have
a racy lookup for the PPI interface. This patch set fixes this
issues
In addition, transmit_cmd() is renamed as tpm_transmit_cmd() and
made opaque so that separate command structure can be introduced
for TPM2.
Comments about v1:
* I think this could be pulled to 3.18 because this clearly fixes
bugs in the current implementation.
Jarkko Sakkinen (3):
tpm: merge duplicate transmit_cmd() functions
tpm: two-phase chip management functions
tpm: fix multiple race conditions in tpm_ppi.c
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-chip.c | 190 ++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm-dev.c | 4 +-
drivers/char/tpm/tpm-interface.c | 201 ++++++------------------------------
drivers/char/tpm/tpm-sysfs.c | 23 +----
drivers/char/tpm/tpm.h | 32 +++---
drivers/char/tpm/tpm_atmel.c | 11 +-
drivers/char/tpm/tpm_i2c_atmel.c | 33 ++----
drivers/char/tpm/tpm_i2c_infineon.c | 37 ++-----
drivers/char/tpm/tpm_i2c_nuvoton.c | 44 +++-----
drivers/char/tpm/tpm_i2c_stm_st33.c | 22 ++--
drivers/char/tpm/tpm_ibmvtpm.c | 17 ++-
drivers/char/tpm/tpm_infineon.c | 13 ++-
drivers/char/tpm/tpm_nsc.c | 11 +-
drivers/char/tpm/tpm_ppi.c | 137 ++++++++++++++----------
drivers/char/tpm/tpm_tis.c | 94 ++++++++---------
drivers/char/tpm/xen-tpmfront.c | 14 +--
17 files changed, 458 insertions(+), 427 deletions(-)
create mode 100644 drivers/char/tpm/tpm-chip.c
--
2.1.0
Merged transmit_cmd() functions in tpm-interface.c and tpm-sysfs.c.
Added "tpm_" prefix for consistency sake. Changed cmd parameter as
opaque. This enables to use separate command structures for TPM1
and TPM2 commands in future. Loose coupling works fine here.
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm-dev.c | 4 +--
drivers/char/tpm/tpm-interface.c | 53 +++++++++++++++++++++-------------------
drivers/char/tpm/tpm-sysfs.c | 23 ++---------------
drivers/char/tpm/tpm.h | 5 ++--
4 files changed, 34 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index d9b774e..bd79d33 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -140,8 +140,8 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
}
/* atomic tpm command send and result receive */
- out_size = tpm_transmit(priv->chip, priv->data_buffer,
- sizeof(priv->data_buffer));
+ out_size = tpm_transmit_cmd(priv->chip, priv->data_buffer,
+ sizeof(priv->data_buffer), NULL);
if (out_size < 0) {
mutex_unlock(&priv->buffer_mutex);
return out_size;
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 6af1700..fedb4d5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -331,8 +331,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
/*
* Internal kernel interface to transmit TPM commands
*/
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
- size_t bufsiz)
+static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+ size_t bufsiz)
{
ssize_t rc;
u32 count, ordinal;
@@ -398,9 +398,10 @@ out:
#define TPM_DIGEST_SIZE 20
#define TPM_RET_CODE_IDX 6
-static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
- int len, const char *desc)
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+ int len, const char *desc)
{
+ struct tpm_output_header *header;
int err;
len = tpm_transmit(chip, (u8 *) cmd, len);
@@ -409,7 +410,9 @@ static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
else if (len < TPM_HEADER_SIZE)
return -EFAULT;
- err = be32_to_cpu(cmd->header.out.return_code);
+ header = (struct tpm_output_header *) cmd;
+
+ err = be32_to_cpu(header->return_code);
if (err != 0 && desc)
dev_err(chip->dev, "A TPM error (%d) occurred %s\n", err, desc);
@@ -448,7 +451,7 @@ ssize_t tpm_getcap(struct device *dev, __be32 subcap_id, cap_t *cap,
tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
tpm_cmd.params.getcap_in.subcap = subcap_id;
}
- rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, desc);
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, desc);
if (!rc)
*cap = tpm_cmd.params.getcap_out.cap;
return rc;
@@ -464,8 +467,8 @@ void tpm_gen_interrupt(struct tpm_chip *chip)
tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
- rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
- "attempting to determine the timeouts");
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
+ "attempting to determine the timeouts");
}
EXPORT_SYMBOL_GPL(tpm_gen_interrupt);
@@ -484,8 +487,8 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
struct tpm_cmd_t start_cmd;
start_cmd.header.in = tpm_startup_header;
start_cmd.params.startup_in.startup_type = startup_type;
- return transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE,
- "attempting to start the TPM");
+ return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE,
+ "attempting to start the TPM");
}
int tpm_get_timeouts(struct tpm_chip *chip)
@@ -500,7 +503,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
- rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
if (rc == TPM_ERR_INVALID_POSTINIT) {
/* The TPM is not started, we are the first to talk to it.
@@ -513,7 +516,7 @@ int tpm_get_timeouts(struct tpm_chip *chip)
tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
- rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
NULL);
}
if (rc) {
@@ -575,8 +578,8 @@ duration:
tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION;
- rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
- "attempting to determine the durations");
+ rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
+ "attempting to determine the durations");
if (rc)
return rc;
@@ -631,8 +634,8 @@ static int tpm_continue_selftest(struct tpm_chip *chip)
struct tpm_cmd_t cmd;
cmd.header.in = continue_selftest_header;
- rc = transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE,
- "continue selftest");
+ rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE,
+ "continue selftest");
return rc;
}
@@ -672,8 +675,8 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
cmd.header.in = pcrread_header;
cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
- rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
- "attempting to read a pcr value");
+ rc = tpm_transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
+ "attempting to read a pcr value");
if (rc == 0)
memcpy(res_buf, cmd.params.pcrread_out.pcr_result,
@@ -737,8 +740,8 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
cmd.header.in = pcrextend_header;
cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
- rc = transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
- "attempting extend a PCR value");
+ rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
+ "attempting extend a PCR value");
tpm_chip_put(chip);
return rc;
@@ -817,7 +820,7 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen)
if (chip == NULL)
return -ENODEV;
- rc = transmit_cmd(chip, cmd, buflen, "attempting tpm_cmd");
+ rc = tpm_transmit_cmd(chip, cmd, buflen, "attempting tpm_cmd");
tpm_chip_put(chip);
return rc;
@@ -938,14 +941,14 @@ int tpm_pm_suspend(struct device *dev)
cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(tpm_suspend_pcr);
memcpy(cmd.params.pcrextend_in.hash, dummy_hash,
TPM_DIGEST_SIZE);
- rc = transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
- "extending dummy pcr before suspend");
+ rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
+ "extending dummy pcr before suspend");
}
/* now do the actual savestate */
for (try = 0; try < TPM_RETRY; try++) {
cmd.header.in = savestate_header;
- rc = transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, NULL);
+ rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, NULL);
/*
* If the TPM indicates that it is too busy to respond to
@@ -1022,7 +1025,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
tpm_cmd.header.in = tpm_getrandom_header;
tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
- err = transmit_cmd(chip, &tpm_cmd,
+ err = tpm_transmit_cmd(chip, &tpm_cmd,
TPM_GETRANDOM_RESULT_SIZE + num_bytes,
"attempting get random");
if (err)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 01730a2..8ecb052 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -20,25 +20,6 @@
#include <linux/device.h>
#include "tpm.h"
-/* XXX for now this helper is duplicated in tpm-interface.c */
-static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
- int len, const char *desc)
-{
- int err;
-
- len = tpm_transmit(chip, (u8 *) cmd, len);
- if (len < 0)
- return len;
- else if (len < TPM_HEADER_SIZE)
- return -EFAULT;
-
- err = be32_to_cpu(cmd->header.out.return_code);
- if (err != 0 && desc)
- dev_err(chip->dev, "A TPM error (%d) occurred %s\n", err, desc);
-
- return err;
-}
-
#define READ_PUBEK_RESULT_SIZE 314
#define TPM_ORD_READPUBEK cpu_to_be32(124)
static struct tpm_input_header tpm_readpubek_header = {
@@ -58,8 +39,8 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
struct tpm_chip *chip = dev_get_drvdata(dev);
tpm_cmd.header.in = tpm_readpubek_header;
- err = transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
- "attempting to read the PUBEK");
+ err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
+ "attempting to read the PUBEK");
if (err)
goto out;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e4d0888..12326e1 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -314,9 +314,8 @@ struct tpm_cmd_t {
} __packed;
ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);
-
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
- size_t bufsiz);
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
+ const char *desc);
extern int tpm_get_timeouts(struct tpm_chip *);
extern void tpm_gen_interrupt(struct tpm_chip *);
extern int tpm_do_selftest(struct tpm_chip *);
--
2.1.0
tpm_register_hardware() and tpm_remove_hardware() are called often
before initializing the device. This is wrong order since it could
be that main TPM driver needs a fully initialized chip to be able to
do its job. For example, now it is impossible to move common startup
functions such as tpm_do_selftest() to tpm_register_hardware().
Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc()
reserves memory resources and tpm_chip_register() initializes the
device driver. This way it is possible to alter struct tpm_chip
attributes and initialize the device driver before passing it to
tpm_chip_register().
The framework takes care of freeing struct tpm_chip by using devres
API. The broken release callback has been wiped. For example, ACPI
drivers do not ever get this callback.
This is a interm step to get proper life-cycle for TPM device drivers.
The next steps are adding proper ref counting and locking to tpm_chip
that is used in every place in the TPM driver.
Big thank you to Jason Gunthorpe for carefully reviewing this part
of the code. Without his contribution reaching the best quality would
not have been possible.
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-chip.c | 190 ++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm-interface.c | 148 +---------------------------
drivers/char/tpm/tpm.h | 11 ++-
drivers/char/tpm/tpm_atmel.c | 11 ++-
drivers/char/tpm/tpm_i2c_atmel.c | 33 +++----
drivers/char/tpm/tpm_i2c_infineon.c | 37 ++-----
drivers/char/tpm/tpm_i2c_nuvoton.c | 44 +++------
drivers/char/tpm/tpm_i2c_stm_st33.c | 22 ++---
drivers/char/tpm/tpm_ibmvtpm.c | 17 ++--
drivers/char/tpm/tpm_infineon.c | 13 ++-
drivers/char/tpm/tpm_nsc.c | 11 ++-
drivers/char/tpm/tpm_tis.c | 79 ++++++---------
drivers/char/tpm/xen-tpmfront.c | 14 +--
14 files changed, 316 insertions(+), 316 deletions(-)
create mode 100644 drivers/char/tpm/tpm-chip.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 4d85dd6..837da04 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -2,7 +2,7 @@
# Makefile for the kernel tpm device drivers.
#
obj-$(CONFIG_TCG_TPM) += tpm.o
-tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o
+tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o
ifdef CONFIG_ACPI
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
new file mode 100644
index 0000000..4a29421
--- /dev/null
+++ b/drivers/char/tpm/tpm-chip.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * Authors:
+ * Jarkko Sakkinen <[email protected]>
+ * Leendert van Doorn <[email protected]>
+ * Dave Safford <[email protected]>
+ * Reiner Sailer <[email protected]>
+ * Kylene Hall <[email protected]>
+ *
+ * Maintained by: <[email protected]>
+ *
+ * TPM chip management routines.
+ *
+ * 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/poll.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/freezer.h>
+#include "tpm.h"
+#include "tpm_eventlog.h"
+
+static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
+static LIST_HEAD(tpm_chip_list);
+static DEFINE_SPINLOCK(driver_lock);
+
+/*
+ * tpm_chip_find_get - return tpm_chip for a given chip number
+ * @chip_num the device number for the chip
+ */
+struct tpm_chip *tpm_chip_find_get(int chip_num)
+{
+ struct tpm_chip *pos, *chip = NULL;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
+ if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
+ continue;
+
+ if (try_module_get(pos->dev->driver->owner)) {
+ chip = pos;
+ break;
+ }
+ }
+ rcu_read_unlock();
+ return chip;
+}
+
+/**
+ * tpmm_chip_remove() - free chip memory and device number
+ * @data: points to struct tpm_chip instance
+ *
+ * This is used internally by tpmm_chip_alloc() and called by devres
+ * when the device is released. This funcion does the opposite of
+ * tpmm_chip_alloc() freeing memory and the device number.
+ */
+static void tpmm_chip_remove(void *data)
+{
+ struct tpm_chip *chip = (struct tpm_chip *) data;
+ dev_dbg(chip->dev, "%s\n", __func__);
+ clear_bit(chip->dev_num, dev_mask);
+ kfree(chip);
+}
+
+/**
+ * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
+ * @dev: device to which the chip is associated
+ * @ops: struct tpm_class_ops instance
+ *
+ * Allocates a new struct tpm_chip instance and assigns a free
+ * device number for it. Caller does not have to worry about
+ * freeing the allocated resources. When the devices is removed
+ * devres calls tpmm_chip_remove() to do the job.
+ */
+struct tpm_chip *tpmm_chip_alloc(struct device *dev,
+ const struct tpm_class_ops *ops)
+{
+ struct tpm_chip *chip;
+
+ chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+ if (chip == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_init(&chip->tpm_mutex);
+ INIT_LIST_HEAD(&chip->list);
+
+ chip->ops = ops;
+ chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
+
+ if (chip->dev_num >= TPM_NUM_DEVICES) {
+ dev_err(dev, "No available tpm device numbers\n");
+ kfree(chip);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ set_bit(chip->dev_num, dev_mask);
+
+ scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
+ chip->dev_num);
+
+ chip->dev = dev;
+ devm_add_action(dev, tpmm_chip_remove, chip);
+ dev_set_drvdata(dev, chip);
+
+ return chip;
+}
+EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
+
+/*
+ * tpm_chip_register() - create a misc driver for the TPM chip
+ * @chip: TPM chip to use.
+ *
+ * Creates a misc driver for the TPM chip and adds sysfs interfaces for
+ * the device, PPI and TCPA. As the last step this function adds the
+ * chip to the list of TPM chips available for use.
+ *
+ * NOTE: This function should be only called after the chip initialization
+ * is complete.
+ *
+ * Called from tpm_<specific>.c probe function only for devices
+ * the driver has determined it should claim. Prior to calling
+ * this function the specific probe function has called pci_enable_device
+ * upon errant exit from this function specific probe function should call
+ * pci_disable_device
+ */
+int tpm_chip_register(struct tpm_chip *chip)
+{
+ int rc;
+
+ rc = tpm_dev_add_device(chip);
+ if (rc)
+ return rc;
+
+ rc = tpm_sysfs_add_device(chip);
+ if (rc)
+ goto del_misc;
+
+ rc = tpm_add_ppi(&chip->dev->kobj);
+ if (rc)
+ goto del_sysfs;
+
+ chip->bios_dir = tpm_bios_log_setup(chip->devname);
+
+ /* Make the chip available. */
+ spin_lock(&driver_lock);
+ list_add_rcu(&chip->list, &tpm_chip_list);
+ spin_unlock(&driver_lock);
+
+ return 0;
+del_sysfs:
+ tpm_sysfs_del_device(chip);
+del_misc:
+ tpm_dev_del_device(chip);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_chip_register);
+
+/*
+ * tpm_chip_unregister() - release the TPM driver
+ * @chip: TPM chip to use.
+ *
+ * Takes the chip first away from the list of available TPM chips and then
+ * cleans up all the resources reserved by tpm_chip_register().
+ *
+ * NOTE: This function should be only called before deinitializing chip
+ * resources.
+ */
+void tpm_chip_unregister(struct tpm_chip *chip)
+{
+ dev_dbg(chip->dev, "%s\n", __func__);
+
+ spin_lock(&driver_lock);
+ list_del_rcu(&chip->list);
+ spin_unlock(&driver_lock);
+ synchronize_rcu();
+
+ tpm_sysfs_del_device(chip);
+ tpm_remove_ppi(&chip->dev->kobj);
+ tpm_bios_log_teardown(chip->bios_dir);
+ tpm_dev_del_device(chip);
+}
+EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index fedb4d5..59d6654 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2014 Intel Corporation
*
* Authors:
* Leendert van Doorn <[email protected]>
@@ -47,10 +48,6 @@ module_param_named(suspend_pcr, tpm_suspend_pcr, uint, 0644);
MODULE_PARM_DESC(suspend_pcr,
"PCR to use for dummy writes to faciltate flush on suspend.");
-static LIST_HEAD(tpm_chip_list);
-static DEFINE_SPINLOCK(driver_lock);
-static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
-
/*
* Array with one entry per ordinal defining the maximum amount
* of time the chip could take to return the result. The ordinal
@@ -639,27 +636,6 @@ static int tpm_continue_selftest(struct tpm_chip *chip)
return rc;
}
-/*
- * tpm_chip_find_get - return tpm_chip for given chip number
- */
-static struct tpm_chip *tpm_chip_find_get(int chip_num)
-{
- struct tpm_chip *pos, *chip = NULL;
-
- rcu_read_lock();
- list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
- if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
- continue;
-
- if (try_module_get(pos->dev->driver->owner)) {
- chip = pos;
- break;
- }
- }
- rcu_read_unlock();
- return chip;
-}
-
#define TPM_ORDINAL_PCRREAD cpu_to_be32(21)
#define READ_PCR_RESULT_SIZE 30
static struct tpm_input_header pcrread_header = {
@@ -887,30 +863,6 @@ again:
}
EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
-void tpm_remove_hardware(struct device *dev)
-{
- struct tpm_chip *chip = dev_get_drvdata(dev);
-
- if (chip == NULL) {
- dev_err(dev, "No device data found\n");
- return;
- }
-
- spin_lock(&driver_lock);
- list_del_rcu(&chip->list);
- spin_unlock(&driver_lock);
- synchronize_rcu();
-
- tpm_dev_del_device(chip);
- tpm_sysfs_del_device(chip);
- tpm_remove_ppi(&dev->kobj);
- tpm_bios_log_teardown(chip->bios_dir);
-
- /* write it this way to be explicit (chip->dev == dev) */
- put_device(chip->dev);
-}
-EXPORT_SYMBOL_GPL(tpm_remove_hardware);
-
#define TPM_ORD_SAVESTATE cpu_to_be32(152)
#define SAVESTATE_RESULT_SIZE 10
@@ -1044,104 +996,6 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
}
EXPORT_SYMBOL_GPL(tpm_get_random);
-/* In case vendor provided release function, call it too.*/
-
-void tpm_dev_vendor_release(struct tpm_chip *chip)
-{
- if (!chip)
- return;
-
- clear_bit(chip->dev_num, dev_mask);
-}
-EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
-
-
-/*
- * Once all references to platform device are down to 0,
- * release all allocated structures.
- */
-static void tpm_dev_release(struct device *dev)
-{
- struct tpm_chip *chip = dev_get_drvdata(dev);
-
- if (!chip)
- return;
-
- tpm_dev_vendor_release(chip);
-
- chip->release(dev);
- kfree(chip);
-}
-
-/*
- * Called from tpm_<specific>.c probe function only for devices
- * the driver has determined it should claim. Prior to calling
- * this function the specific probe function has called pci_enable_device
- * upon errant exit from this function specific probe function should call
- * pci_disable_device
- */
-struct tpm_chip *tpm_register_hardware(struct device *dev,
- const struct tpm_class_ops *ops)
-{
- struct tpm_chip *chip;
-
- /* Driver specific per-device data */
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-
- if (chip == NULL)
- return NULL;
-
- mutex_init(&chip->tpm_mutex);
- INIT_LIST_HEAD(&chip->list);
-
- chip->ops = ops;
- chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
-
- if (chip->dev_num >= TPM_NUM_DEVICES) {
- dev_err(dev, "No available tpm device numbers\n");
- goto out_free;
- }
-
- set_bit(chip->dev_num, dev_mask);
-
- scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
- chip->dev_num);
-
- chip->dev = get_device(dev);
- chip->release = dev->release;
- dev->release = tpm_dev_release;
- dev_set_drvdata(dev, chip);
-
- if (tpm_dev_add_device(chip))
- goto put_device;
-
- if (tpm_sysfs_add_device(chip))
- goto del_misc;
-
- if (tpm_add_ppi(&dev->kobj))
- goto del_sysfs;
-
- chip->bios_dir = tpm_bios_log_setup(chip->devname);
-
- /* Make chip available */
- spin_lock(&driver_lock);
- list_add_rcu(&chip->list, &tpm_chip_list);
- spin_unlock(&driver_lock);
-
- return chip;
-
-del_sysfs:
- tpm_sysfs_del_device(chip);
-del_misc:
- tpm_dev_del_device(chip);
-put_device:
- put_device(chip->dev);
-out_free:
- kfree(chip);
- return NULL;
-}
-EXPORT_SYMBOL_GPL(tpm_register_hardware);
-
MODULE_AUTHOR("Leendert van Doorn ([email protected])");
MODULE_DESCRIPTION("TPM Driver");
MODULE_VERSION("2.0");
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 12326e1..fa0974c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -110,7 +110,6 @@ struct tpm_chip {
struct dentry **bios_dir;
struct list_head list;
- void (*release) (struct device *);
};
#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
@@ -320,15 +319,17 @@ extern int tpm_get_timeouts(struct tpm_chip *);
extern void tpm_gen_interrupt(struct tpm_chip *);
extern int tpm_do_selftest(struct tpm_chip *);
extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
-extern struct tpm_chip* tpm_register_hardware(struct device *,
- const struct tpm_class_ops *ops);
-extern void tpm_dev_vendor_release(struct tpm_chip *);
-extern void tpm_remove_hardware(struct device *);
extern int tpm_pm_suspend(struct device *);
extern int tpm_pm_resume(struct device *);
extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
wait_queue_head_t *, bool);
+struct tpm_chip *tpm_chip_find_get(int chip_num);
+extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
+ const struct tpm_class_ops *ops);
+extern int tpm_chip_register(struct tpm_chip *chip);
+extern void tpm_chip_unregister(struct tpm_chip *chip);
+
int tpm_dev_add_device(struct tpm_chip *chip);
void tpm_dev_del_device(struct tpm_chip *chip);
int tpm_sysfs_add_device(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index 6069d13..8e2576a 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -138,11 +138,11 @@ static void atml_plat_remove(void)
struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
if (chip) {
+ tpm_chip_unregister(chip);
if (chip->vendor.have_region)
atmel_release_region(chip->vendor.base,
chip->vendor.region_size);
atmel_put_base_addr(chip->vendor.iobase);
- tpm_remove_hardware(chip->dev);
platform_device_unregister(pdev);
}
}
@@ -184,8 +184,9 @@ static int __init init_atmel(void)
goto err_rel_reg;
}
- if (!(chip = tpm_register_hardware(&pdev->dev, &tpm_atmel))) {
- rc = -ENODEV;
+ chip = tpmm_chip_alloc(&pdev->dev, &tpm_atmel);
+ if (IS_ERR(chip)) {
+ rc = PTR_ERR(chip);
goto err_unreg_dev;
}
@@ -194,6 +195,10 @@ static int __init init_atmel(void)
chip->vendor.have_region = have_region;
chip->vendor.region_size = region_size;
+ rc = tpm_chip_register(chip);
+ if (rc)
+ goto err_unreg_dev;
+
return 0;
err_unreg_dev:
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index 7727292..8af3b4a 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -160,11 +160,9 @@ static int i2c_atmel_probe(struct i2c_client *client,
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
return -ENODEV;
- chip = tpm_register_hardware(dev, &i2c_atmel);
- if (!chip) {
- dev_err(dev, "%s() error in tpm_register_hardware\n", __func__);
- return -ENODEV;
- }
+ chip = tpmm_chip_alloc(dev, &i2c_atmel);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
chip->vendor.priv = devm_kzalloc(dev, sizeof(struct priv_data),
GFP_KERNEL);
@@ -179,21 +177,16 @@ static int i2c_atmel_probe(struct i2c_client *client,
/* There is no known way to probe for this device, and all version
* information seems to be read via TPM commands. Thus we rely on the
* TPM startup process in the common code to detect the device. */
- if (tpm_get_timeouts(chip)) {
- rc = -ENODEV;
- goto out_err;
- }
+ if (tpm_get_timeouts(chip))
+ return -ENODEV;
- if (tpm_do_selftest(chip)) {
- rc = -ENODEV;
- goto out_err;
- }
+ if (tpm_do_selftest(chip))
+ return -ENODEV;
- return 0;
+ rc = tpm_chip_register(chip);
+ if (rc)
+ return rc;
-out_err:
- tpm_dev_vendor_release(chip);
- tpm_remove_hardware(chip->dev);
return rc;
}
@@ -201,11 +194,7 @@ static int i2c_atmel_remove(struct i2c_client *client)
{
struct device *dev = &(client->dev);
struct tpm_chip *chip = dev_get_drvdata(dev);
-
- if (chip)
- tpm_dev_vendor_release(chip);
- tpm_remove_hardware(dev);
- kfree(chip);
+ tpm_chip_unregister(chip);
return 0;
}
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 472af4b..6f00bc3 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -581,10 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev)
int rc = 0;
struct tpm_chip *chip;
- chip = tpm_register_hardware(dev, &tpm_tis_i2c);
- if (!chip) {
- dev_err(dev, "could not register hardware\n");
- rc = -ENODEV;
+ chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
+ if (IS_ERR(chip)) {
+ rc = PTR_ERR(chip);
goto out_err;
}
@@ -600,7 +599,7 @@ static int tpm_tis_i2c_init(struct device *dev)
if (request_locality(chip, 0) != 0) {
dev_err(dev, "could not request locality\n");
rc = -ENODEV;
- goto out_vendor;
+ goto out_err;
}
/* read four bytes from DID_VID register */
@@ -628,21 +627,13 @@ static int tpm_tis_i2c_init(struct device *dev)
tpm_get_timeouts(chip);
tpm_do_selftest(chip);
- return 0;
+ rc = tpm_chip_register(chip);
+ if (rc)
+ goto out_release;
+ return 0;
out_release:
release_locality(chip, chip->vendor.locality, 1);
-
-out_vendor:
- /* close file handles */
- tpm_dev_vendor_release(chip);
-
- /* remove hardware */
- tpm_remove_hardware(chip->dev);
-
- /* reset these pointers, otherwise we oops */
- chip->dev->release = NULL;
- chip->release = NULL;
tpm_dev.client = NULL;
out_err:
return rc;
@@ -712,17 +703,9 @@ static int tpm_tis_i2c_probe(struct i2c_client *client,
static int tpm_tis_i2c_remove(struct i2c_client *client)
{
struct tpm_chip *chip = tpm_dev.chip;
- release_locality(chip, chip->vendor.locality, 1);
- /* close file handles */
- tpm_dev_vendor_release(chip);
-
- /* remove hardware */
- tpm_remove_hardware(chip->dev);
-
- /* reset these pointers, otherwise we oops */
- chip->dev->release = NULL;
- chip->release = NULL;
+ tpm_chip_unregister(chip);
+ release_locality(chip, chip->vendor.locality, 1);
tpm_dev.client = NULL;
return 0;
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 7b158ef..09f0c46 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -530,11 +530,9 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
dev_info(dev, "VID: %04X DID: %02X RID: %02X\n", (u16) vid,
(u8) (vid >> 16), (u8) (vid >> 24));
- chip = tpm_register_hardware(dev, &tpm_i2c);
- if (!chip) {
- dev_err(dev, "%s() error in tpm_register_hardware\n", __func__);
- return -ENODEV;
- }
+ chip = tpmm_chip_alloc(dev, &tpm_i2c);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
chip->vendor.priv = devm_kzalloc(dev, sizeof(struct priv_data),
GFP_KERNEL);
@@ -584,7 +582,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
TPM_DATA_FIFO_W,
1, (u8 *) (&rc));
if (rc < 0)
- goto out_err;
+ return rc;
/* TPM_STS <- 0x40 (commandReady) */
i2c_nuvoton_ready(chip);
} else {
@@ -594,45 +592,33 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
* only TPM_STS_VALID should be set
*/
if (i2c_nuvoton_read_status(chip) !=
- TPM_STS_VALID) {
- rc = -EIO;
- goto out_err;
- }
+ TPM_STS_VALID)
+ return -EIO;
}
}
}
- if (tpm_get_timeouts(chip)) {
- rc = -ENODEV;
- goto out_err;
- }
+ if (tpm_get_timeouts(chip))
+ return -ENODEV;
- if (tpm_do_selftest(chip)) {
- rc = -ENODEV;
- goto out_err;
- }
+ if (tpm_do_selftest(chip))
+ return -ENODEV;
- return 0;
+ rc = tpm_chip_register(chip);
+ if (rc)
+ return rc;
-out_err:
- tpm_dev_vendor_release(chip);
- tpm_remove_hardware(chip->dev);
- return rc;
+ return 0;
}
static int i2c_nuvoton_remove(struct i2c_client *client)
{
struct device *dev = &(client->dev);
struct tpm_chip *chip = dev_get_drvdata(dev);
-
- if (chip)
- tpm_dev_vendor_release(chip);
- tpm_remove_hardware(dev);
- kfree(chip);
+ tpm_chip_unregister(chip);
return 0;
}
-
static const struct i2c_device_id i2c_nuvoton_id[] = {
{I2C_DRIVER_NAME, 0},
{}
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 4669e37..dd32429 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -619,10 +619,9 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto end;
}
- chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
- if (!chip) {
- dev_info(&client->dev, "fail chip\n");
- err = -ENODEV;
+ chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm);
+ if (IS_ERR(chip)) {
+ err = PTR_ERR(chip);
goto end;
}
@@ -631,14 +630,14 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (!platform_data) {
dev_info(&client->dev, "chip not available\n");
err = -ENODEV;
- goto _tpm_clean_answer;
+ goto end;
}
platform_data->tpm_i2c_buffer[0] =
kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
if (platform_data->tpm_i2c_buffer[0] == NULL) {
err = -ENOMEM;
- goto _tpm_clean_answer;
+ goto end;
}
platform_data->tpm_i2c_buffer[1] =
kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
@@ -716,7 +715,10 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
tpm_get_timeouts(chip);
tpm_do_selftest(chip);
- dev_info(chip->dev, "TPM I2C Initialized\n");
+ err = tpm_chip_register(chip);
+ if (err)
+ goto _irq_set;
+
return 0;
_irq_set:
free_irq(gpio_to_irq(platform_data->io_serirq), (void *)chip);
@@ -732,8 +734,6 @@ _tpm_clean_response2:
_tpm_clean_response1:
kzfree(platform_data->tpm_i2c_buffer[0]);
platform_data->tpm_i2c_buffer[0] = NULL;
-_tpm_clean_answer:
- tpm_remove_hardware(chip->dev);
end:
pr_info("TPM I2C initialisation fail\n");
return err;
@@ -752,13 +752,13 @@ static int tpm_st33_i2c_remove(struct i2c_client *client)
((struct i2c_client *)TPM_VPRIV(chip))->dev.platform_data;
if (pin_infos != NULL) {
+ tpm_chip_unregister(chip);
+
free_irq(pin_infos->io_serirq, chip);
gpio_free(pin_infos->io_serirq);
gpio_free(pin_infos->io_lpcpd);
- tpm_remove_hardware(chip->dev);
-
if (pin_infos->tpm_i2c_buffer[1] != NULL) {
kzfree(pin_infos->tpm_i2c_buffer[1]);
pin_infos->tpm_i2c_buffer[1] = NULL;
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index af74c57..eb95796 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -270,8 +270,11 @@ static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
{
struct ibmvtpm_dev *ibmvtpm = ibmvtpm_get_data(&vdev->dev);
+ struct tpm_chip *chip = dev_get_drvdata(ibmvtpm->dev);
int rc = 0;
+ tpm_chip_unregister(chip);
+
free_irq(vdev->irq, ibmvtpm);
do {
@@ -290,8 +293,6 @@ static int tpm_ibmvtpm_remove(struct vio_dev *vdev)
kfree(ibmvtpm->rtce_buf);
}
- tpm_remove_hardware(ibmvtpm->dev);
-
kfree(ibmvtpm);
return 0;
@@ -555,11 +556,9 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
struct tpm_chip *chip;
int rc = -ENOMEM, rc1;
- chip = tpm_register_hardware(dev, &tpm_ibmvtpm);
- if (!chip) {
- dev_err(dev, "tpm_register_hardware failed\n");
- return -ENODEV;
- }
+ chip = tpmm_chip_alloc(dev, &tpm_ibmvtpm);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
ibmvtpm = kzalloc(sizeof(struct ibmvtpm_dev), GFP_KERNEL);
if (!ibmvtpm) {
@@ -629,7 +628,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
if (rc)
goto init_irq_cleanup;
- return rc;
+ return tpm_chip_register(chip);
init_irq_cleanup:
do {
rc1 = plpar_hcall_norets(H_FREE_CRQ, vio_dev->unit_address);
@@ -644,8 +643,6 @@ cleanup:
kfree(ibmvtpm);
}
- tpm_remove_hardware(dev);
-
return rc;
}
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index dc0a255..d1559f6 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -546,7 +546,14 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
vendorid[0], vendorid[1],
productid[0], productid[1], chipname);
- if (!(chip = tpm_register_hardware(&dev->dev, &tpm_inf)))
+ chip = tpmm_chip_alloc(&dev->dev, &tpm_inf);
+ if (IS_ERR(chip)) {
+ rc = PTR_ERR(chip);
+ goto err_release_region;
+ }
+
+ rc = tpm_chip_register(chip);
+ if (rc)
goto err_release_region;
return 0;
@@ -573,6 +580,8 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
struct tpm_chip *chip = pnp_get_drvdata(dev);
if (chip) {
+ tpm_chip_unregister(chip);
+
if (tpm_dev.iotype == TPM_INF_IO_PORT) {
release_region(tpm_dev.data_regs, tpm_dev.data_size);
release_region(tpm_dev.config_port,
@@ -581,8 +590,6 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
iounmap(tpm_dev.mem_base);
release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
}
- tpm_dev_vendor_release(chip);
- tpm_remove_hardware(chip->dev);
}
}
diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index 3179ec9..151f96a 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -247,9 +247,9 @@ static struct platform_device *pdev = NULL;
static void tpm_nsc_remove(struct device *dev)
{
struct tpm_chip *chip = dev_get_drvdata(dev);
- if ( chip ) {
+ if (chip) {
+ tpm_chip_unregister(chip);
release_region(chip->vendor.base, 2);
- tpm_remove_hardware(chip->dev);
}
}
@@ -308,11 +308,16 @@ static int __init init_nsc(void)
goto err_del_dev;
}
- if (!(chip = tpm_register_hardware(&pdev->dev, &tpm_nsc))) {
+ chip = tpmm_chip_alloc(&pdev->dev, &tpm_nsc);
+ if (IS_ERR(chip)) {
rc = -ENODEV;
goto err_rel_reg;
}
+ rc = tpm_chip_register(chip);
+ if (rc)
+ goto err_rel_reg;
+
dev_dbg(&pdev->dev, "NSC TPM detected\n");
dev_dbg(&pdev->dev,
"NSC LDN 0x%x, SID 0x%x, SRID 0x%x\n",
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 2c46734..e2cb04d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,9 +75,6 @@ enum tis_defaults {
#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
#define TPM_RID(l) (0x0F04 | ((l) << 12))
-static LIST_HEAD(tis_chips);
-static DEFINE_MUTEX(tis_lock);
-
#if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
static int is_itpm(struct pnp_dev *dev)
{
@@ -528,6 +525,17 @@ static bool interrupts = true;
module_param(interrupts, bool, 0444);
MODULE_PARM_DESC(interrupts, "Enable interrupts");
+static void tpm_tis_remove(struct tpm_chip *chip)
+{
+ iowrite32(~TPM_GLOBAL_INT_ENABLE &
+ ioread32(chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.
+ locality)),
+ chip->vendor.iobase +
+ TPM_INT_ENABLE(chip->vendor.locality));
+ release_locality(chip, chip->vendor.locality, 1);
+}
+
static int tpm_tis_init(struct device *dev, resource_size_t start,
resource_size_t len, unsigned int irq)
{
@@ -535,14 +543,13 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
int rc, i, irq_s, irq_e, probe;
struct tpm_chip *chip;
- if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
- return -ENODEV;
+ chip = tpmm_chip_alloc(dev, &tpm_tis);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
- chip->vendor.iobase = ioremap(start, len);
- if (!chip->vendor.iobase) {
- rc = -EIO;
- goto out_err;
- }
+ chip->vendor.iobase = devm_ioremap(dev, start, len);
+ if (!chip->vendor.iobase)
+ return -EIO;
/* Default timeouts */
chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
@@ -649,8 +656,8 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
iowrite8(i, chip->vendor.iobase +
TPM_INT_VECTOR(chip->vendor.locality));
- if (request_irq
- (i, tis_int_probe, IRQF_SHARED,
+ if (devm_request_irq
+ (dev, i, tis_int_probe, IRQF_SHARED,
chip->vendor.miscdev.name, chip) != 0) {
dev_info(chip->dev,
"Unable to request irq: %d for probe\n",
@@ -690,15 +697,14 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
iowrite32(intmask,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
- free_irq(i, chip);
}
}
if (chip->vendor.irq) {
iowrite8(chip->vendor.irq,
chip->vendor.iobase +
TPM_INT_VECTOR(chip->vendor.locality));
- if (request_irq
- (chip->vendor.irq, tis_int_handler, IRQF_SHARED,
+ if (devm_request_irq
+ (dev, chip->vendor.irq, tis_int_handler, IRQF_SHARED,
chip->vendor.miscdev.name, chip) != 0) {
dev_info(chip->dev,
"Unable to request irq: %d for use\n",
@@ -719,17 +725,9 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
}
}
- INIT_LIST_HEAD(&chip->vendor.list);
- mutex_lock(&tis_lock);
- list_add(&chip->vendor.list, &tis_chips);
- mutex_unlock(&tis_lock);
-
-
- return 0;
+ return tpm_chip_register(chip);
out_err:
- if (chip->vendor.iobase)
- iounmap(chip->vendor.iobase);
- tpm_remove_hardware(chip->dev);
+ tpm_tis_remove(chip);
return rc;
}
@@ -811,13 +809,10 @@ MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
static void tpm_tis_pnp_remove(struct pnp_dev *dev)
{
struct tpm_chip *chip = pnp_get_drvdata(dev);
-
- tpm_dev_vendor_release(chip);
-
- kfree(chip);
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
}
-
static struct pnp_driver tis_pnp_driver = {
.name = "tpm_tis",
.id_table = tpm_pnp_tbl,
@@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
static struct platform_driver tis_drv = {
.driver = {
- .name = "tpm_tis",
+ .name = "tpm_tis",
.owner = THIS_MODULE,
.pm = &tpm_tis_pm,
},
@@ -876,31 +871,17 @@ err_dev:
static void __exit cleanup_tis(void)
{
- struct tpm_vendor_specific *i, *j;
struct tpm_chip *chip;
- mutex_lock(&tis_lock);
- list_for_each_entry_safe(i, j, &tis_chips, list) {
- chip = to_tpm_chip(i);
- tpm_remove_hardware(chip->dev);
- iowrite32(~TPM_GLOBAL_INT_ENABLE &
- ioread32(chip->vendor.iobase +
- TPM_INT_ENABLE(chip->vendor.
- locality)),
- chip->vendor.iobase +
- TPM_INT_ENABLE(chip->vendor.locality));
- release_locality(chip, chip->vendor.locality, 1);
- if (chip->vendor.irq)
- free_irq(chip->vendor.irq, chip);
- iounmap(i->iobase);
- list_del(&i->list);
- }
- mutex_unlock(&tis_lock);
#ifdef CONFIG_PNP
if (!force) {
pnp_unregister_driver(&tis_pnp_driver);
return;
}
#endif
+ chip = dev_get_drvdata(&pdev->dev);
+ /* FIXME might be broken when using force */
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
platform_device_unregister(pdev);
platform_driver_unregister(&tis_drv);
}
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 441b44e..c3b4f5a 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -175,9 +175,9 @@ static int setup_chip(struct device *dev, struct tpm_private *priv)
{
struct tpm_chip *chip;
- chip = tpm_register_hardware(dev, &tpm_vtpm);
- if (!chip)
- return -ENODEV;
+ chip = tpmm_chip_alloc(dev, &tpm_vtpm);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
init_waitqueue_head(&chip->vendor.read_queue);
@@ -286,6 +286,7 @@ static int tpmfront_probe(struct xenbus_device *dev,
const struct xenbus_device_id *id)
{
struct tpm_private *priv;
+ struct tpm_chip *chip;
int rv;
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -302,21 +303,22 @@ static int tpmfront_probe(struct xenbus_device *dev,
rv = setup_ring(dev, priv);
if (rv) {
- tpm_remove_hardware(&dev->dev);
+ chip = dev_get_drvdata(&dev->dev);
+ tpm_chip_unregister(chip);
ring_free(priv);
return rv;
}
tpm_get_timeouts(priv->chip);
- return rv;
+ return tpm_chip_register(priv->chip);
}
static int tpmfront_remove(struct xenbus_device *dev)
{
struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
struct tpm_private *priv = TPM_VPRIV(chip);
- tpm_remove_hardware(&dev->dev);
+ tpm_chip_unregister(chip);
ring_free(priv);
TPM_VPRIV(chip) = NULL;
return 0;
--
2.1.0
Traversal of the ACPI device tree was not done right. It should lookup
PPI only under the ACPI device that it is associated. Otherwise, it could
match to a wrong PPI interface if there are two TPM devices in the device
tree.
Removed global ACPI handle and version string from tpm_ppi.c as this
is racy. Instead they should be associated with the chip.
Moved code just a tiny bit towards two-phase allocation to implement
fix for the PPI race conditions.
Added missing copyright platter to tpm_ppi.c.
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 4 +-
drivers/char/tpm/tpm.h | 16 ++++--
drivers/char/tpm/tpm_ppi.c | 137 +++++++++++++++++++++++++++-----------------
drivers/char/tpm/tpm_tis.c | 15 +++--
4 files changed, 110 insertions(+), 62 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 4a29421..2446422 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -143,7 +143,7 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
goto del_misc;
- rc = tpm_add_ppi(&chip->dev->kobj);
+ rc = tpm_add_ppi(chip);
if (rc)
goto del_sysfs;
@@ -183,7 +183,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
synchronize_rcu();
tpm_sysfs_del_device(chip);
- tpm_remove_ppi(&chip->dev->kobj);
+ tpm_remove_ppi(chip);
tpm_bios_log_teardown(chip->bios_dir);
tpm_dev_del_device(chip);
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index fa0974c..16fab4f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -27,6 +27,7 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/tpm.h>
+#include <linux/acpi.h>
enum tpm_const {
TPM_MINOR = 224, /* officially assigned */
@@ -94,6 +95,8 @@ struct tpm_vendor_specific {
#define TPM_VID_WINBOND 0x1050
#define TPM_VID_STM 0x104A
+#define TPM_PPI_VERSION_LEN 3
+
struct tpm_chip {
struct device *dev; /* Device stuff */
const struct tpm_class_ops *ops;
@@ -109,6 +112,11 @@ struct tpm_chip {
struct dentry **bios_dir;
+#ifdef CONFIG_ACPI
+ acpi_handle acpi_dev_handle;
+ char ppi_version[TPM_PPI_VERSION_LEN + 1];
+#endif /* CONFIG_ACPI */
+
struct list_head list;
};
@@ -338,15 +346,15 @@ void tpm_sysfs_del_device(struct tpm_chip *chip);
int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
#ifdef CONFIG_ACPI
-extern int tpm_add_ppi(struct kobject *);
-extern void tpm_remove_ppi(struct kobject *);
+extern int tpm_add_ppi(struct tpm_chip *chip);
+extern void tpm_remove_ppi(struct tpm_chip *chip);
#else
-static inline int tpm_add_ppi(struct kobject *parent)
+static inline int tpm_add_ppi(struct tpm_chip *chip)
{
return 0;
}
-static inline void tpm_remove_ppi(struct kobject *parent)
+static inline void tpm_remove_ppi(struct tpm_chip *chip)
{
}
#endif
diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
index 61dcc80..f8b2326 100644
--- a/drivers/char/tpm/tpm_ppi.c
+++ b/drivers/char/tpm/tpm_ppi.c
@@ -1,3 +1,22 @@
+/*
+ * Copyright (C) 2012-2014 Intel Corporation
+ *
+ * Authors:
+ * Xiaoyan Zhang <[email protected]>
+ * Jiang Liu <[email protected]>
+ * Jarkko Sakkinen <[email protected]>
+ *
+ * Maintained by: <[email protected]>
+ *
+ * This file contains implementation of the sysfs interface for PPI.
+ *
+ * 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/acpi.h>
#include "tpm.h"
@@ -22,45 +41,22 @@ static const u8 tpm_ppi_uuid[] = {
0x8D, 0x10, 0x08, 0x9D, 0x16, 0x53
};
-static char tpm_ppi_version[PPI_VERSION_LEN + 1];
-static acpi_handle tpm_ppi_handle;
-
-static acpi_status ppi_callback(acpi_handle handle, u32 level, void *context,
- void **return_value)
-{
- union acpi_object *obj;
-
- if (!acpi_check_dsm(handle, tpm_ppi_uuid, TPM_PPI_REVISION_ID,
- 1 << TPM_PPI_FN_VERSION))
- return AE_OK;
-
- /* Cache version string */
- obj = acpi_evaluate_dsm_typed(handle, tpm_ppi_uuid,
- TPM_PPI_REVISION_ID, TPM_PPI_FN_VERSION,
- NULL, ACPI_TYPE_STRING);
- if (obj) {
- strlcpy(tpm_ppi_version, obj->string.pointer,
- PPI_VERSION_LEN + 1);
- ACPI_FREE(obj);
- }
-
- *return_value = handle;
-
- return AE_CTRL_TERMINATE;
-}
-
static inline union acpi_object *
-tpm_eval_dsm(int func, acpi_object_type type, union acpi_object *argv4)
+tpm_eval_dsm(acpi_handle dev_handle, int func, acpi_object_type type,
+ union acpi_object *argv4)
{
- BUG_ON(!tpm_ppi_handle);
- return acpi_evaluate_dsm_typed(tpm_ppi_handle, tpm_ppi_uuid,
- TPM_PPI_REVISION_ID, func, argv4, type);
+ BUG_ON(!dev_handle);
+ return acpi_evaluate_dsm_typed(dev_handle, tpm_ppi_uuid,
+ TPM_PPI_REVISION_ID,
+ func, argv4, type);
}
static ssize_t tpm_show_ppi_version(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return scnprintf(buf, PAGE_SIZE, "%s\n", tpm_ppi_version);
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ return scnprintf(buf, PAGE_SIZE, "%s\n", chip->ppi_version);
}
static ssize_t tpm_show_ppi_request(struct device *dev,
@@ -68,8 +64,10 @@ static ssize_t tpm_show_ppi_request(struct device *dev,
{
ssize_t size = -EINVAL;
union acpi_object *obj;
+ struct tpm_chip *chip = dev_get_drvdata(dev);
- obj = tpm_eval_dsm(TPM_PPI_FN_GETREQ, ACPI_TYPE_PACKAGE, NULL);
+ obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETREQ,
+ ACPI_TYPE_PACKAGE, NULL);
if (!obj)
return -ENXIO;
@@ -103,14 +101,15 @@ static ssize_t tpm_store_ppi_request(struct device *dev,
int func = TPM_PPI_FN_SUBREQ;
union acpi_object *obj, tmp;
union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
+ struct tpm_chip *chip = dev_get_drvdata(dev);
/*
* the function to submit TPM operation request to pre-os environment
* is updated with function index from SUBREQ to SUBREQ2 since PPI
* version 1.1
*/
- if (acpi_check_dsm(tpm_ppi_handle, tpm_ppi_uuid, TPM_PPI_REVISION_ID,
- 1 << TPM_PPI_FN_SUBREQ2))
+ if (acpi_check_dsm(chip->acpi_dev_handle, tpm_ppi_uuid,
+ TPM_PPI_REVISION_ID, 1 << TPM_PPI_FN_SUBREQ2))
func = TPM_PPI_FN_SUBREQ2;
/*
@@ -119,7 +118,7 @@ static ssize_t tpm_store_ppi_request(struct device *dev,
* string/package type. For PPI version 1.0 and 1.1, use buffer type
* for compatibility, and use package type since 1.2 according to spec.
*/
- if (strcmp(tpm_ppi_version, "1.2") < 0) {
+ if (strcmp(chip->ppi_version, "1.2") < 0) {
if (sscanf(buf, "%d", &req) != 1)
return -EINVAL;
argv4.type = ACPI_TYPE_BUFFER;
@@ -131,7 +130,8 @@ static ssize_t tpm_store_ppi_request(struct device *dev,
return -EINVAL;
}
- obj = tpm_eval_dsm(func, ACPI_TYPE_INTEGER, &argv4);
+ obj = tpm_eval_dsm(chip->acpi_dev_handle, func, ACPI_TYPE_INTEGER,
+ &argv4);
if (!obj) {
return -ENXIO;
} else {
@@ -157,6 +157,7 @@ static ssize_t tpm_show_ppi_transition_action(struct device *dev,
.buffer.length = 0,
.buffer.pointer = NULL
};
+ struct tpm_chip *chip = dev_get_drvdata(dev);
static char *info[] = {
"None",
@@ -171,9 +172,10 @@ static ssize_t tpm_show_ppi_transition_action(struct device *dev,
* (e.g. Capella with PPI 1.0) need integer/string/buffer type, so for
* compatibility, define params[3].type as buffer, if PPI version < 1.2
*/
- if (strcmp(tpm_ppi_version, "1.2") < 0)
+ if (strcmp(chip->ppi_version, "1.2") < 0)
obj = &tmp;
- obj = tpm_eval_dsm(TPM_PPI_FN_GETACT, ACPI_TYPE_INTEGER, obj);
+ obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETACT,
+ ACPI_TYPE_INTEGER, obj);
if (!obj) {
return -ENXIO;
} else {
@@ -196,8 +198,10 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
acpi_status status = -EINVAL;
union acpi_object *obj, *ret_obj;
u64 req, res;
+ struct tpm_chip *chip = dev_get_drvdata(dev);
- obj = tpm_eval_dsm(TPM_PPI_FN_GETRSP, ACPI_TYPE_PACKAGE, NULL);
+ obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETRSP,
+ ACPI_TYPE_PACKAGE, NULL);
if (!obj)
return -ENXIO;
@@ -248,7 +252,8 @@ cleanup:
return status;
}
-static ssize_t show_ppi_operations(char *buf, u32 start, u32 end)
+static ssize_t show_ppi_operations(acpi_handle dev_handle, char *buf, u32 start,
+ u32 end)
{
int i;
u32 ret;
@@ -264,14 +269,15 @@ static ssize_t show_ppi_operations(char *buf, u32 start, u32 end)
"User not required",
};
- if (!acpi_check_dsm(tpm_ppi_handle, tpm_ppi_uuid, TPM_PPI_REVISION_ID,
+ if (!acpi_check_dsm(dev_handle, tpm_ppi_uuid, TPM_PPI_REVISION_ID,
1 << TPM_PPI_FN_GETOPR))
return -EPERM;
tmp.integer.type = ACPI_TYPE_INTEGER;
for (i = start; i <= end; i++) {
tmp.integer.value = i;
- obj = tpm_eval_dsm(TPM_PPI_FN_GETOPR, ACPI_TYPE_INTEGER, &argv);
+ obj = tpm_eval_dsm(dev_handle, TPM_PPI_FN_GETOPR,
+ ACPI_TYPE_INTEGER, &argv);
if (!obj) {
return -ENOMEM;
} else {
@@ -291,14 +297,20 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- return show_ppi_operations(buf, 0, PPI_TPM_REQ_MAX);
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
+ PPI_TPM_REQ_MAX);
}
static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- return show_ppi_operations(buf, PPI_VS_REQ_START, PPI_VS_REQ_END);
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ return show_ppi_operations(chip->acpi_dev_handle, buf, PPI_VS_REQ_START,
+ PPI_VS_REQ_END);
}
static DEVICE_ATTR(version, S_IRUGO, tpm_show_ppi_version, NULL);
@@ -323,16 +335,37 @@ static struct attribute_group ppi_attr_grp = {
.attrs = ppi_attrs
};
-int tpm_add_ppi(struct kobject *parent)
+int tpm_add_ppi(struct tpm_chip *chip)
{
- /* Cache TPM ACPI handle and version string */
- acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
- ppi_callback, NULL, NULL, &tpm_ppi_handle);
- return tpm_ppi_handle ? sysfs_create_group(parent, &ppi_attr_grp) : 0;
+ union acpi_object *obj;
+ struct kobject *parent = &chip->dev->kobj;
+
+ if (!chip->acpi_dev_handle)
+ return 0;
+
+ if (!acpi_check_dsm(chip->acpi_dev_handle, tpm_ppi_uuid,
+ TPM_PPI_REVISION_ID, 1 << TPM_PPI_FN_VERSION))
+ return 0;
+
+ /* Cache PPI version string. */
+ obj = acpi_evaluate_dsm_typed(chip->acpi_dev_handle, tpm_ppi_uuid,
+ TPM_PPI_REVISION_ID, TPM_PPI_FN_VERSION,
+ NULL, ACPI_TYPE_STRING);
+ if (obj) {
+ strlcpy(chip->ppi_version, obj->string.pointer,
+ PPI_VERSION_LEN + 1);
+ ACPI_FREE(obj);
+ } else
+ return -ENOMEM;
+
+ return chip->acpi_dev_handle ?
+ sysfs_create_group(parent, &ppi_attr_grp) : 0;
}
-void tpm_remove_ppi(struct kobject *parent)
+void tpm_remove_ppi(struct tpm_chip *chip)
{
- if (tpm_ppi_handle)
+ struct kobject *parent = &chip->dev->kobj;
+
+ if (chip->ppi_version[0] != '\0')
sysfs_remove_group(parent, &ppi_attr_grp);
}
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e2cb04d..c427da2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -536,8 +536,9 @@ static void tpm_tis_remove(struct tpm_chip *chip)
release_locality(chip, chip->vendor.locality, 1);
}
-static int tpm_tis_init(struct device *dev, resource_size_t start,
- resource_size_t len, unsigned int irq)
+static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
+ resource_size_t start, resource_size_t len,
+ unsigned int irq)
{
u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
@@ -547,6 +548,8 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ chip->acpi_dev_handle = acpi_dev_handle;
+
chip->vendor.iobase = devm_ioremap(dev, start, len);
if (!chip->vendor.iobase)
return -EIO;
@@ -777,6 +780,7 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
{
resource_size_t start, len;
unsigned int irq = 0;
+ acpi_handle acpi_dev_handle = 0;
start = pnp_mem_start(pnp_dev, 0);
len = pnp_mem_len(pnp_dev, 0);
@@ -789,7 +793,10 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
if (is_itpm(pnp_dev))
itpm = true;
- return tpm_tis_init(&pnp_dev->dev, start, len, irq);
+ if (pnp_acpi_device(pnp_dev))
+ acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
+
+ return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
}
static struct pnp_device_id tpm_pnp_tbl[] = {
@@ -858,7 +865,7 @@ static int __init init_tis(void)
rc = PTR_ERR(pdev);
goto err_dev;
}
- rc = tpm_tis_init(&pdev->dev, TIS_MEM_BASE, TIS_MEM_LEN, 0);
+ rc = tpm_tis_init(&pdev->dev, 0, TIS_MEM_BASE, TIS_MEM_LEN, 0);
if (rc)
goto err_init;
return 0;
--
2.1.0
Hi Jarkko,
> Jarkko Sakkinen (3):
> tpm: merge duplicate transmit_cmd() functions
> tpm: two-phase chip management functions
> tpm: fix multiple race conditions in tpm_ppi.c
the patchset introduces a sparse error:
CHECK /home/phuewe/linux-2.6-host/drivers/char/tpm/tpm_tis.c
/home/phuewe/linux-2.6-host/drivers/char/tpm/tpm_tis.c:783:39: warning: Using plain integer as NULL pointer
/home/phuewe/linux-2.6-host/drivers/char/tpm/tpm_tis.c:868:39: warning: Using plain integer as NULL pointer
apart from that I'll review it this evening.
Thanks,
Peter
On Wed, Oct 22, 2014 at 07:23:54PM +0300, Jarkko Sakkinen wrote:
> Merged transmit_cmd() functions in tpm-interface.c and tpm-sysfs.c.
> Added "tpm_" prefix for consistency sake. Changed cmd parameter as
> opaque. This enables to use separate command structures for TPM1
> and TPM2 commands in future. Loose coupling works fine here.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-By: Jason Gunthorpe <[email protected]>
Thanks Jarkko!
Jason
On Wed, Oct 22, 2014 at 07:23:55PM +0300, Jarkko Sakkinen wrote:
> tpm_register_hardware() and tpm_remove_hardware() are called often
> before initializing the device. This is wrong order since it could
> be that main TPM driver needs a fully initialized chip to be able to
> do its job. For example, now it is impossible to move common startup
> functions such as tpm_do_selftest() to tpm_register_hardware().
> Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc()
It is called tpmm_chip_alloc() in this version..
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-By: Jason Gunthorpe <[email protected]>
Looks fine, if you have to spin this again you can incorporate the
nits.
> +
> +static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
Not for this patch, but while this area of code is being looked at
this should probably be an IDR/IDA like other subsytems?
> + if (try_module_get(pos->dev->driver->owner)) {
> + chip = pos;
> + break;
> + }
Not for this patch, this module stuff should be wiped in favor of chip->ops
locking.
> +static void tpmm_chip_remove(void *data)
> +{
> + struct tpm_chip *chip = (struct tpm_chip *) data;
> + dev_dbg(chip->dev, "%s\n", __func__);
This print is silent in the default compile, right?
> + chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
[..]
> + set_bit(chip->dev_num, dev_mask);
Not for this patch but somehow there is no locking for dev_mask
here. I guess it should use the driver_lock spinlock?
> + chip->bios_dir = tpm_bios_log_setup(chip->devname);
Not for this patch, but tpm_bios_log_setup can return NULL if
securityfs setup fails.
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 6069d13..8e2576a 100644
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -138,11 +138,11 @@ static void atml_plat_remove(void)
> struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
>
> if (chip) {
> + tpm_chip_unregister(chip);
> if (chip->vendor.have_region)
> atmel_release_region(chip->vendor.base,
> chip->vendor.region_size);
> atmel_put_base_addr(chip->vendor.iobase);
> - tpm_remove_hardware(chip->dev);
> platform_device_unregister(pdev);
Missed this before, the Atmel driver is the same as the TIS driver in
force mode, ie it isn't going through the driver APIs, but instead
force creating platform devices. Same comment as for TIS - I'm not
sure devm works properly like this.
I guess just add the same comment as for TIS?
> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> index 472af4b..6f00bc3 100644
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -581,10 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev)
> int rc = 0;
> struct tpm_chip *chip;
>
> - chip = tpm_register_hardware(dev, &tpm_tis_i2c);
> - if (!chip) {
> - dev_err(dev, "could not register hardware\n");
> - rc = -ENODEV;
> + chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
> + if (IS_ERR(chip)) {
> + rc = PTR_ERR(chip);
> goto out_err;
Nit: out_err is synonymous with 'return rc;', so all the goto out_err
in this driver can just return;
> + rc = tpm_chip_register(chip);
> + if (rc)
> + return rc;
> + return 0;
Nit: could just be return tpm_chip_register(chip);
> @@ -619,10 +619,9 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
> goto end;
> }
>
> - chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
> - if (!chip) {
> - dev_info(&client->dev, "fail chip\n");
> - err = -ENODEV;
> + chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm);
> + if (IS_ERR(chip)) {
> + err = PTR_ERR(chip);
> goto end;
> }
Nit: Same comment, 'goto end' can just be 'return rc'
> - dev_info(chip->dev, "TPM I2C Initialized\n");
> + err = tpm_chip_register(chip);
> + if (err)
> + goto _irq_set;
> +
Nit: Same comment
> end:
> pr_info("TPM I2C initialisation fail\n");
This pr_info should be deleted
> @@ -573,6 +580,8 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
> struct tpm_chip *chip = pnp_get_drvdata(dev);
>
> if (chip) {
Nit: This if in the remove callback is an anti-pattern and should be
globally removed. If remove is being called then probe succeeded,
there is no way for probe to succed and drvdata to be unset.
> static void tpm_nsc_remove(struct device *dev)
> {
> struct tpm_chip *chip = dev_get_drvdata(dev);
> - if ( chip ) {
> + if (chip) {
Same comment
> @@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
>
> static struct platform_driver tis_drv = {
> .driver = {
> - .name = "tpm_tis",
> + .name = "tpm_tis",
> .owner = THIS_MODULE,
> .pm = &tpm_tis_pm,
> },
There is no remove method here in this platform_driver, this ties into
the question if force works or not. The tpm_tis_remove call in the
cleanup_hardware should be done through the .remove method of this
driver structure..
Jason
On Wed, Oct 22, 2014 at 07:23:56PM +0300, Jarkko Sakkinen wrote:
> Traversal of the ACPI device tree was not done right. It should lookup
> PPI only under the ACPI device that it is associated. Otherwise, it could
> match to a wrong PPI interface if there are two TPM devices in the device
> tree.
>
> Removed global ACPI handle and version string from tpm_ppi.c as this
> is racy. Instead they should be associated with the chip.
>
> Moved code just a tiny bit towards two-phase allocation to implement
> fix for the PPI race conditions.
Not this version..
> Added missing copyright platter to tpm_ppi.c.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
I like this one the most of the three I've seen :)
Did you also look in tpm_acpi.c to see if it needs to use
acpi_dev_handle somehow too?
> + union acpi_object *obj;
> + struct kobject *parent = &chip->dev->kobj;
Nit, this variable is only used once, it would be clearer to inline
> + /* Cache PPI version string. */
> + obj = acpi_evaluate_dsm_typed(chip->acpi_dev_handle, tpm_ppi_uuid,
> + TPM_PPI_REVISION_ID, TPM_PPI_FN_VERSION,
> + NULL, ACPI_TYPE_STRING);
> + if (obj) {
> + strlcpy(chip->ppi_version, obj->string.pointer,
> + PPI_VERSION_LEN + 1);
> + ACPI_FREE(obj);
> + } else
> + return -ENOMEM;
> +
> + return chip->acpi_dev_handle ?
> + sysfs_create_group(parent, &ppi_attr_grp) : 0;
The above sequence can just be:
if (!obj)
return -ENOMEM;
strlcpy(chip->ppi_version, obj->string.pointer, sizeof(chip->ppi_version));
ACPI_FREE(obj);
return sysfs_create_group(&chip->dev->kobj, &ppi_attr_grp);
Which is more idiomatic. Also remove TPM_PPI_VERSION_LEN, sizeof is better.
I know nothing about acpi, but is ENOMEM the right code? I would think
acpi_evalute_dsm_typed would also fail if tpm_ppi_uuid is not found??
> + return chip->acpi_dev_handle ?
> + sysfs_create_group(parent, &ppi_attr_grp) : 0;
dev_handle is already checked to be non 0
> +void tpm_remove_ppi(struct tpm_chip *chip)
> + struct kobject *parent = &chip->dev->kobj;
Also used only once
Jason
On Wed, Oct 22, 2014 at 11:16:03AM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 22, 2014 at 07:23:55PM +0300, Jarkko Sakkinen wrote:
> > tpm_register_hardware() and tpm_remove_hardware() are called often
> > before initializing the device. This is wrong order since it could
> > be that main TPM driver needs a fully initialized chip to be able to
> > do its job. For example, now it is impossible to move common startup
> > functions such as tpm_do_selftest() to tpm_register_hardware().
>
> > Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc()
>
> It is called tpmm_chip_alloc() in this version..
>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
>
> Reviewed-By: Jason Gunthorpe <[email protected]>
>
> Looks fine, if you have to spin this again you can incorporate the
> nits.
Thanks for the review! I'll try to incorporate most (if not all of
them). I was going to comment some of the points you made but they
would have mostly been "ACK".
> > +
> > +static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
>
> Not for this patch, but while this area of code is being looked at
> this should probably be an IDR/IDA like other subsytems?
Yes, it should use IDR. I'll put this into my backlog but don't
include into this patch set.
> > + if (try_module_get(pos->dev->driver->owner)) {
> > + chip = pos;
> > + break;
> > + }
>
> Not for this patch, this module stuff should be wiped in favor of chip->ops
> locking.
Definitely, horrible stuff, putting into my backlog (kref + mutex
should be a better solution).
> > +static void tpmm_chip_remove(void *data)
> > +{
> > + struct tpm_chip *chip = (struct tpm_chip *) data;
> > + dev_dbg(chip->dev, "%s\n", __func__);
>
> This print is silent in the default compile, right?
Forgotten clutter, removing it, thanks.
> > + chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
> [..]
> > + set_bit(chip->dev_num, dev_mask);
>
> Not for this patch but somehow there is no locking for dev_mask
> here. I guess it should use the driver_lock spinlock?
I'll add it anyway, thanks.
> > + chip->bios_dir = tpm_bios_log_setup(chip->devname);
>
> Not for this patch, but tpm_bios_log_setup can return NULL if
> securityfs setup fails.
Easy to fix so I'll just fix it.
> > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > index 6069d13..8e2576a 100644
> > +++ b/drivers/char/tpm/tpm_atmel.c
> > @@ -138,11 +138,11 @@ static void atml_plat_remove(void)
> > struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> >
> > if (chip) {
> > + tpm_chip_unregister(chip);
> > if (chip->vendor.have_region)
> > atmel_release_region(chip->vendor.base,
> > chip->vendor.region_size);
> > atmel_put_base_addr(chip->vendor.iobase);
> > - tpm_remove_hardware(chip->dev);
> > platform_device_unregister(pdev);
>
> Missed this before, the Atmel driver is the same as the TIS driver in
> force mode, ie it isn't going through the driver APIs, but instead
> force creating platform devices. Same comment as for TIS - I'm not
> sure devm works properly like this.
>
> I guess just add the same comment as for TIS?
Yup, makes sense.
> > diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> > index 472af4b..6f00bc3 100644
> > +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> > @@ -581,10 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev)
> > int rc = 0;
> > struct tpm_chip *chip;
> >
> > - chip = tpm_register_hardware(dev, &tpm_tis_i2c);
> > - if (!chip) {
> > - dev_err(dev, "could not register hardware\n");
> > - rc = -ENODEV;
> > + chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
> > + if (IS_ERR(chip)) {
> > + rc = PTR_ERR(chip);
> > goto out_err;
>
> Nit: out_err is synonymous with 'return rc;', so all the goto out_err
> in this driver can just return;
>
> > + rc = tpm_chip_register(chip);
> > + if (rc)
> > + return rc;
> > + return 0;
>
> Nit: could just be return tpm_chip_register(chip);
>
> > @@ -619,10 +619,9 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > goto end;
> > }
> >
> > - chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
> > - if (!chip) {
> > - dev_info(&client->dev, "fail chip\n");
> > - err = -ENODEV;
> > + chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm);
> > + if (IS_ERR(chip)) {
> > + err = PTR_ERR(chip);
> > goto end;
> > }
>
> Nit: Same comment, 'goto end' can just be 'return rc'
>
> > - dev_info(chip->dev, "TPM I2C Initialized\n");
> > + err = tpm_chip_register(chip);
> > + if (err)
> > + goto _irq_set;
> > +
>
> Nit: Same comment
>
> > end:
> > pr_info("TPM I2C initialisation fail\n");
>
> This pr_info should be deleted
Agreed.
> > @@ -573,6 +580,8 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
> > struct tpm_chip *chip = pnp_get_drvdata(dev);
> >
> > if (chip) {
>
> Nit: This if in the remove callback is an anti-pattern and should be
> globally removed. If remove is being called then probe succeeded,
> there is no way for probe to succed and drvdata to be unset.
>
> > static void tpm_nsc_remove(struct device *dev)
> > {
> > struct tpm_chip *chip = dev_get_drvdata(dev);
> > - if ( chip ) {
> > + if (chip) {
>
> Same comment
>
> > @@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
> >
> > static struct platform_driver tis_drv = {
> > .driver = {
> > - .name = "tpm_tis",
> > + .name = "tpm_tis",
> > .owner = THIS_MODULE,
> > .pm = &tpm_tis_pm,
> > },
>
> There is no remove method here in this platform_driver, this ties into
> the question if force works or not. The tpm_tis_remove call in the
> cleanup_hardware should be done through the .remove method of this
> driver structure..
I'll try to get this tested.
> Jason
/Jarkko
Thanks for the excellent review comments. I'll do another spin an try to
incorporate most them.
/Jarkko
On Wed, Oct 22, 2014 at 11:26:46AM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 22, 2014 at 07:23:56PM +0300, Jarkko Sakkinen wrote:
> > Traversal of the ACPI device tree was not done right. It should lookup
> > PPI only under the ACPI device that it is associated. Otherwise, it could
> > match to a wrong PPI interface if there are two TPM devices in the device
> > tree.
> >
> > Removed global ACPI handle and version string from tpm_ppi.c as this
> > is racy. Instead they should be associated with the chip.
> >
> > Moved code just a tiny bit towards two-phase allocation to implement
> > fix for the PPI race conditions.
>
> Not this version..
>
> > Added missing copyright platter to tpm_ppi.c.
> >
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
>
> Reviewed-by: Jason Gunthorpe <[email protected]>
>
> I like this one the most of the three I've seen :)
>
> Did you also look in tpm_acpi.c to see if it needs to use
> acpi_dev_handle somehow too?
>
> > + union acpi_object *obj;
> > + struct kobject *parent = &chip->dev->kobj;
>
> Nit, this variable is only used once, it would be clearer to inline
>
> > + /* Cache PPI version string. */
> > + obj = acpi_evaluate_dsm_typed(chip->acpi_dev_handle, tpm_ppi_uuid,
> > + TPM_PPI_REVISION_ID, TPM_PPI_FN_VERSION,
> > + NULL, ACPI_TYPE_STRING);
> > + if (obj) {
> > + strlcpy(chip->ppi_version, obj->string.pointer,
> > + PPI_VERSION_LEN + 1);
> > + ACPI_FREE(obj);
> > + } else
> > + return -ENOMEM;
> > +
> > + return chip->acpi_dev_handle ?
> > + sysfs_create_group(parent, &ppi_attr_grp) : 0;
>
> The above sequence can just be:
>
> if (!obj)
> return -ENOMEM;
>
> strlcpy(chip->ppi_version, obj->string.pointer, sizeof(chip->ppi_version));
> ACPI_FREE(obj);
>
> return sysfs_create_group(&chip->dev->kobj, &ppi_attr_grp);
>
> Which is more idiomatic. Also remove TPM_PPI_VERSION_LEN, sizeof is better.
>
> I know nothing about acpi, but is ENOMEM the right code? I would think
> acpi_evalute_dsm_typed would also fail if tpm_ppi_uuid is not found??
>
> > + return chip->acpi_dev_handle ?
> > + sysfs_create_group(parent, &ppi_attr_grp) : 0;
>
> dev_handle is already checked to be non 0
>
> > +void tpm_remove_ppi(struct tpm_chip *chip)
> > + struct kobject *parent = &chip->dev->kobj;
>
> Also used only once
>
> Jason