The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3].
[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async
---
Changes in v3:
- Fixed problem reported by 0-dey kbuild test robot around __exitcall.
It complained because there is a module_exit() in another file already.
Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
to the space to the struct file_priv to have access to it from the async job.
This is to avoid memory allocations on every write call. Now everything
what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.
Tadeusz Struk (2):
tpm: add ptr to the tpm_space struct to file_priv
tpm: add support for nonblocking operation
drivers/char/tpm/tpm-dev-common.c | 150 +++++++++++++++++++++++++++----------
drivers/char/tpm/tpm-dev.c | 22 +++--
drivers/char/tpm/tpm-dev.h | 19 +++--
drivers/char/tpm/tpm-interface.c | 1
drivers/char/tpm/tpm.h | 1
drivers/char/tpm/tpmrm-dev.c | 31 ++++----
6 files changed, 152 insertions(+), 72 deletions(-)
--
TS
Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.
Signed-off-by: Tadeusz Struk <[email protected]>
---
drivers/char/tpm/tpm-dev-common.c | 8 +++++---
drivers/char/tpm/tpm-dev.c | 10 ++--------
drivers/char/tpm/tpm-dev.h | 5 +++--
drivers/char/tpm/tpmrm-dev.c | 14 ++------------
4 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
}
void tpm_common_open(struct file *file, struct tpm_chip *chip,
- struct file_priv *priv)
+ struct file_priv *priv, struct tpm_space *space)
{
priv->chip = chip;
+ priv->space = space;
+
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
INIT_WORK(&priv->work, timeout_work);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
}
ssize_t tpm_common_write(struct file *file, const char __user *buf,
- size_t size, loff_t *off, struct tpm_space *space)
+ size_t size, loff_t *off)
{
struct file_priv *priv = file->private_data;
size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
mutex_unlock(&priv->buffer_mutex);
return -EPIPE;
}
- out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+ out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
sizeof(priv->data_buffer), 0);
tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
if (priv == NULL)
goto out;
- tpm_common_open(file, chip, priv);
+ tpm_common_open(file, chip, priv, NULL);
return 0;
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
-static ssize_t tpm_write(struct file *file, const char __user *buf,
- size_t size, loff_t *off)
-{
- return tpm_common_write(file, buf, size, off, NULL);
-}
-
/*
* Called on file close
*/
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
.llseek = no_llseek,
.open = tpm_open,
.read = tpm_common_read,
- .write = tpm_write,
+ .write = tpm_common_write,
.release = tpm_release,
};
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
struct file_priv {
struct tpm_chip *chip;
+ struct tpm_space *space;
/* Data passed to and from the tpm via the read/write calls */
size_t data_pending;
@@ -18,11 +19,11 @@ struct file_priv {
};
void tpm_common_open(struct file *file, struct tpm_chip *chip,
- struct file_priv *priv);
+ struct file_priv *priv, struct tpm_space *space);
ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
ssize_t tpm_common_write(struct file *file, const char __user *buf,
- size_t size, loff_t *off, struct tpm_space *space);
+ size_t size, loff_t *off);
void tpm_common_release(struct file *file, struct file_priv *priv);
#endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
}
- tpm_common_open(file, chip, &priv->priv);
+ tpm_common_open(file, chip, &priv->priv, &priv->space);
return 0;
}
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
return 0;
}
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
- size_t size, loff_t *off)
-{
- struct file_priv *fpriv = file->private_data;
- struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
- return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
const struct file_operations tpmrm_fops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
.open = tpmrm_open,
.read = tpm_common_read,
- .write = tpmrm_write,
+ .write = tpm_common_write,
.release = tpmrm_release,
};
-
Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.
Signed-off-by: Tadeusz Struk <[email protected]>
---
drivers/char/tpm/tpm-dev-common.c | 146 +++++++++++++++++++++++++++----------
drivers/char/tpm/tpm-dev.c | 16 +++-
drivers/char/tpm/tpm-dev.h | 16 +++-
drivers/char/tpm/tpm-interface.c | 1
drivers/char/tpm/tpm.h | 1
drivers/char/tpm/tpmrm-dev.c | 19 ++++-
6 files changed, 146 insertions(+), 53 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..f57c000fd84a 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,35 @@
* License.
*
*/
+#include <linux/poll.h>
+#include <linux/sched/signal.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/workqueue.h>
#include "tpm.h"
#include "tpm-dev.h"
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+ struct file_priv *priv =
+ container_of(work, struct file_priv, async_work);
+ ssize_t ret;
+
+ ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+ sizeof(priv->data_buffer), 0);
+
+ tpm_put_ops(priv->chip);
+ if (ret > 0) {
+ priv->data_pending = ret;
+ mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+ }
+ mutex_unlock(&priv->buffer_mutex);
+ wake_up_interruptible(&priv->async_wait);
+}
+
static void user_reader_timeout(struct timer_list *t)
{
struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +53,44 @@ static void user_reader_timeout(struct timer_list *t)
pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
task_tgid_nr(current));
- schedule_work(&priv->work);
+ schedule_work(&priv->timeout_work);
}
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
{
- struct file_priv *priv = container_of(work, struct file_priv, work);
+ struct file_priv *priv = container_of(work, struct file_priv,
+ timeout_work);
mutex_lock(&priv->buffer_mutex);
priv->data_pending = 0;
memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
mutex_unlock(&priv->buffer_mutex);
+ wake_up_interruptible(&priv->async_wait);
}
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
- struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+ struct file_priv *priv, struct tpm_space *space)
{
priv->chip = chip;
priv->space = space;
mutex_init(&priv->buffer_mutex);
timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
- INIT_WORK(&priv->work, timeout_work);
-
+ INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+ INIT_WORK(&priv->async_work, tpm_async_work);
+ init_waitqueue_head(&priv->async_wait);
file->private_data = priv;
+ mutex_lock(&tpm_dev_wq_lock);
+ if (!tpm_dev_wq) {
+ tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+ if (!tpm_dev_wq) {
+ mutex_unlock(&tpm_dev_wq_lock);
+ return -ENOMEM;
+ }
+ }
+
+ mutex_unlock(&tpm_dev_wq_lock);
+ return 0;
}
ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +101,17 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
int rc;
del_singleshot_timer_sync(&priv->user_read_timer);
- flush_work(&priv->work);
+ flush_work(&priv->timeout_work);
mutex_lock(&priv->buffer_mutex);
if (priv->data_pending) {
ret_size = min_t(ssize_t, size, priv->data_pending);
- rc = copy_to_user(buf, priv->data_buffer, ret_size);
- memset(priv->data_buffer, 0, priv->data_pending);
- if (rc)
- ret_size = -EFAULT;
+ if (ret_size > 0) {
+ rc = copy_to_user(buf, priv->data_buffer, ret_size);
+ memset(priv->data_buffer, 0, priv->data_pending);
+ if (rc)
+ ret_size = -EFAULT;
+ }
priv->data_pending = 0;
}
@@ -84,10 +124,9 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
size_t size, loff_t *off)
{
struct file_priv *priv = file->private_data;
- size_t in_size = size;
- ssize_t out_size;
+ int ret = 0;
- if (in_size > TPM_BUFSIZE)
+ if (size > TPM_BUFSIZE)
return -E2BIG;
mutex_lock(&priv->buffer_mutex);
@@ -97,20 +136,19 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
* buffered writes from blocking here.
*/
if (priv->data_pending != 0) {
- mutex_unlock(&priv->buffer_mutex);
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
- if (copy_from_user
- (priv->data_buffer, (void __user *) buf, in_size)) {
- mutex_unlock(&priv->buffer_mutex);
- return -EFAULT;
+ if (copy_from_user(priv->data_buffer, buf, size)) {
+ ret = -EFAULT;
+ goto out;
}
- if (in_size < 6 ||
- in_size < be32_to_cpu(*((__be32 *) (priv->data_buffer + 2)))) {
- mutex_unlock(&priv->buffer_mutex);
- return -EINVAL;
+ if (size < 6 ||
+ size < be32_to_cpu(*((__be32 *)(priv->data_buffer + 2)))) {
+ ret = -EINVAL;
+ goto out;
}
/* atomic tpm command send and result receive. We only hold the ops
@@ -118,25 +156,48 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
* the char dev is held open.
*/
if (tpm_try_get_ops(priv->chip)) {
- mutex_unlock(&priv->buffer_mutex);
- return -EPIPE;
+ ret = -EPIPE;
+ goto out;
}
- out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
- sizeof(priv->data_buffer), 0);
- tpm_put_ops(priv->chip);
- if (out_size < 0) {
- mutex_unlock(&priv->buffer_mutex);
- return out_size;
+ /*
+ * If in nonblocking mode schedule an async job to send
+ * the command return the size.
+ * In case of error the err code will be returned in
+ * the subsequent read call.
+ */
+ if (file->f_flags & O_NONBLOCK) {
+ queue_work(tpm_dev_wq, &priv->async_work);
+ return size;
}
- priv->data_pending = out_size;
+ ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+ sizeof(priv->data_buffer), 0);
+ tpm_put_ops(priv->chip);
+
+ if (ret > 0) {
+ priv->data_pending = ret;
+ mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+ ret = size;
+ }
+out:
mutex_unlock(&priv->buffer_mutex);
+ return ret;
+}
- /* Set a timeout by which the reader must come claim the result */
- mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+__poll_t tpm_common_poll(struct file *file, poll_table *wait)
+{
+ struct file_priv *priv = file->private_data;
+ __poll_t mask = 0;
- return in_size;
+ poll_wait(file, &priv->async_wait, wait);
+
+ if (priv->data_pending)
+ mask = EPOLLIN | EPOLLRDNORM;
+ else
+ mask = EPOLLOUT | EPOLLWRNORM;
+
+ return mask;
}
/*
@@ -144,8 +205,17 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
*/
void tpm_common_release(struct file *file, struct file_priv *priv)
{
+ flush_work(&priv->async_work);
del_singleshot_timer_sync(&priv->user_read_timer);
- flush_work(&priv->work);
+ flush_work(&priv->timeout_work);
file->private_data = NULL;
priv->data_pending = 0;
}
+
+void __exit tpm_dev_common_exit(void)
+{
+ if (tpm_dev_wq) {
+ destroy_workqueue(tpm_dev_wq);
+ tpm_dev_wq = NULL;
+ }
+}
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 98b9630c3a36..1e353a4f4b7e 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -24,6 +24,7 @@ static int tpm_open(struct inode *inode, struct file *file)
{
struct tpm_chip *chip;
struct file_priv *priv;
+ int rc = -ENOMEM;
chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
@@ -37,15 +38,21 @@ static int tpm_open(struct inode *inode, struct file *file)
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (priv == NULL)
- goto out;
+ goto out_clear;
- tpm_common_open(file, chip, priv, NULL);
+ rc = tpm_common_open(file, chip, priv, NULL);
+ if (rc)
+ goto out_free;
return 0;
- out:
+out_free:
+ kfree(priv);
+
+out_clear:
clear_bit(0, &chip->is_open);
- return -ENOMEM;
+
+ return rc;
}
/*
@@ -68,5 +75,6 @@ const struct file_operations tpm_fops = {
.open = tpm_open,
.read = tpm_common_read,
.write = tpm_common_write,
+ .poll = tpm_common_poll,
.release = tpm_release,
};
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index 4048677bbd78..fa49c885a2cb 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -2,28 +2,32 @@
#ifndef _TPM_DEV_H
#define _TPM_DEV_H
+#include <linux/poll.h>
#include "tpm.h"
struct file_priv {
struct tpm_chip *chip;
struct tpm_space *space;
- /* Data passed to and from the tpm via the read/write calls */
- size_t data_pending;
+ /* Holds the amount of data passed or an error code from async op */
+ ssize_t data_pending;
struct mutex buffer_mutex;
struct timer_list user_read_timer; /* user needs to claim result */
- struct work_struct work;
+ struct work_struct timeout_work;
+ struct work_struct async_work;
+ wait_queue_head_t async_wait;
u8 data_buffer[TPM_BUFSIZE];
};
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
- struct file_priv *priv, struct tpm_space *space);
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+ struct file_priv *priv, struct tpm_space *space);
ssize_t tpm_common_read(struct file *file, char __user *buf,
size_t size, loff_t *off);
ssize_t tpm_common_write(struct file *file, const char __user *buf,
size_t size, loff_t *off);
-void tpm_common_release(struct file *file, struct file_priv *priv);
+__poll_t tpm_common_poll(struct file *file, poll_table *wait);
+void tpm_common_release(struct file *file, struct file_priv *priv);
#endif
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e32f6e85dc6d..5fb29d58ca34 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1398,6 +1398,7 @@ static void __exit tpm_exit(void)
class_destroy(tpm_class);
class_destroy(tpmrm_class);
unregister_chrdev_region(tpm_devt, 2*TPM_NUM_DEVICES);
+ tpm_dev_common_exit();
}
subsys_initcall(tpm_init);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 4426649e431c..45d6f1eda680 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -595,4 +595,5 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
int tpm_bios_log_setup(struct tpm_chip *chip);
void tpm_bios_log_teardown(struct tpm_chip *chip);
+void tpm_dev_common_exit(void);
#endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 96006c6b9696..f131c97c1577 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -23,14 +23,22 @@ static int tpmrm_open(struct inode *inode, struct file *file)
return -ENOMEM;
rc = tpm2_init_space(&priv->space);
- if (rc) {
- kfree(priv);
- return -ENOMEM;
- }
+ if (rc)
+ goto out_free;
- tpm_common_open(file, chip, &priv->priv, &priv->space);
+ rc = tpm_common_open(file, chip, &priv->priv, &priv->space);
+ if (rc)
+ goto out_del_space;
return 0;
+
+out_del_space:
+ tpm2_del_space(chip, &priv->space);
+
+out_free:
+ kfree(priv);
+
+ return rc;
}
static int tpmrm_release(struct inode *inode, struct file *file)
@@ -51,5 +59,6 @@ const struct file_operations tpmrm_fops = {
.open = tpmrm_open,
.read = tpm_common_read,
.write = tpm_common_write,
+ .poll = tpm_common_poll,
.release = tpmrm_release,
};
On 6/12/18 10:58 AM, Tadeusz Struk wrote:
> Currently the TPM driver only supports blocking calls, which doesn't allow
> asynchronous IO operations to the TPM hardware.
> This patch changes it and adds support for nonblocking write and a new poll
> function to enable applications, which want to take advantage of this.
>
> Signed-off-by: Tadeusz Struk <[email protected]>
> ---
snip
.
.
.
> @@ -84,10 +124,9 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> size_t size, loff_t *off)
> {
> struct file_priv *priv = file->private_data;
> - size_t in_size = size;
> - ssize_t out_size;
> + int ret = 0;
>
> - if (in_size > TPM_BUFSIZE)
> + if (size > TPM_BUFSIZE)
> return -E2BIG;
>
> mutex_lock(&priv->buffer_mutex);
> @@ -97,20 +136,19 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> * buffered writes from blocking here.
> */
> if (priv->data_pending != 0) {
> - mutex_unlock(&priv->buffer_mutex);
> - return -EBUSY;
> + ret = -EBUSY;
> + goto out;
> }
>
> - if (copy_from_user
> - (priv->data_buffer, (void __user *) buf, in_size)) {
> - mutex_unlock(&priv->buffer_mutex);
> - return -EFAULT;
> + if (copy_from_user(priv->data_buffer, buf, size)) {
> + ret = -EFAULT;
> + goto out;
> }
>
> - if (in_size < 6 ||
> - in_size < be32_to_cpu(*((__be32 *) (priv->data_buffer + 2)))) {
> - mutex_unlock(&priv->buffer_mutex);
> - return -EINVAL;
> + if (size < 6 ||
> + size < be32_to_cpu(*((__be32 *)(priv->data_buffer + 2)))) {
> + ret = -EINVAL;
> + goto out;
> }
>
> /* atomic tpm command send and result receive. We only hold the ops
> @@ -118,25 +156,48 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> * the char dev is held open.
> */
> if (tpm_try_get_ops(priv->chip)) {
> - mutex_unlock(&priv->buffer_mutex);
> - return -EPIPE;
> + ret = -EPIPE;
> + goto out;
> }
> - out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
> - sizeof(priv->data_buffer), 0);
>
> - tpm_put_ops(priv->chip);
> - if (out_size < 0) {
> - mutex_unlock(&priv->buffer_mutex);
> - return out_size;
> + /*
> + * If in nonblocking mode schedule an async job to send
> + * the command return the size.
> + * In case of error the err code will be returned in
> + * the subsequent read call.
> + */
> + if (file->f_flags & O_NONBLOCK) {
> + queue_work(tpm_dev_wq, &priv->async_work);
> + return size;
Apologies for the question, but should there be a mutex_unlock() here?
It's about the only return statement I am seeing where I cannot tell if
a mutex_unlock() will be called before return or is needed before
return. The rest of the code is pretty obvious the return statements
are being re-factored to an out: block where the mutex_unlock() will
always be called before returning.
Thanks,
Jay
On 06/13/2018 10:55 AM, J Freyensee wrote:
>> + /*
>> + * If in nonblocking mode schedule an async job to send
>> + * the command return the size.
>> + * In case of error the err code will be returned in
>> + * the subsequent read call.
>> + */
>> + if (file->f_flags & O_NONBLOCK) {
>> + queue_work(tpm_dev_wq, &priv->async_work);
>> + return size;
>
> Apologies for the question, but should there be a mutex_unlock() here? It's about the only return statement I am seeing where I cannot tell if a mutex_unlock() will be called before return or is needed before return. The rest of the code is pretty obvious the return statements are being re-factored to an out: block where the mutex_unlock() will always be called before returning.
Hi Jay,
We need to hold the lock until the whole command is sent and the device is ready for next one.
In case of the async job the mutex in unlocked in tpm_async_work() just after the tpm_transmit() returns.
Thanks for reviewing.
--
Tadeusz
On Tue, Jun 12, 2018 at 10:58:26AM -0700, Tadeusz Struk wrote:
> The TCG SAPI specification [1] defines a set of functions, which allows
> applications to use the TPM device in either blocking or non-blocking fashion.
> Each command defined by the specification has a corresponding
> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> mode of operation. Currently the TPM driver supports only blocking calls,
> which doesn't allow asynchronous IO operations.
> This patch changes it and adds support for nonblocking write and a new poll
> function to enable applications, which want to take advantage of this feature.
> The new functionality can be tested using standard TPM tools implemented
> in [2], together with modified TCTI from [3].
>
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
> [2] https://github.com/tpm2-software/tpm2-tools
> [3] https://github.com/tstruk/tpm2-tss/tree/async
For me the value is still a bit questionable. The benchmark looks a bit
flakky to give much figures how this would work with real world workloads.
I read James response and I also have to question why not just a worker
thread in user space? TPM does only one command at a time anyways.
Cannot take this in before I know that user space will (1) adapt to
this and (2) gain value compared to a worker thread.
/Jarkko
On 06/19/2018 06:10 AM, Jarkko Sakkinen wrote:
> On Tue, Jun 12, 2018 at 10:58:26AM -0700, Tadeusz Struk wrote:
>> The TCG SAPI specification [1] defines a set of functions, which allows
>> applications to use the TPM device in either blocking or non-blocking fashion.
>> Each command defined by the specification has a corresponding
>> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
>> mode of operation. Currently the TPM driver supports only blocking calls,
>> which doesn't allow asynchronous IO operations.
>> This patch changes it and adds support for nonblocking write and a new poll
>> function to enable applications, which want to take advantage of this feature.
>> The new functionality can be tested using standard TPM tools implemented
>> in [2], together with modified TCTI from [3].
>>
>> [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
>> [2] https://github.com/tpm2-software/tpm2-tools
>> [3] https://github.com/tstruk/tpm2-tss/tree/async
>
> For me the value is still a bit questionable. The benchmark looks a bit
> flakky to give much figures how this would work with real world workloads.
>
> I read James response and I also have to question why not just a worker
> thread in user space? TPM does only one command at a time anyways.
>
> Cannot take this in before I know that user space will (1) adapt to
> this and (2) gain value compared to a worker thread.
>
Hi Jarkko,
Thanks for reviewing the patch.
There are applications/frameworks where a worker thread is not an option.
Take for example the IoT use-cases and frameworks like IoT.js, or "Node.js for IoT".
They are all single threaded, event-driven frameworks, using non-blocking I/O as the base of their processing model.
Similarly embedded applications, which are basically just a single threaded event loop, quite often don't use threads because of resources constrains.
If your concern is that user space will not adopt to this, I can say that TSS library [1] is currently blocked on this feature, and we can not enable some of the use-cases mentioned above because of this.
Thanks,
Tadeusz
[1] https://github.com/tpm2-software/tpm2-tss
On Tue, 2018-06-19 at 17:45 -0700, Tadeusz Struk wrote:
> On 06/19/2018 06:10 AM, Jarkko Sakkinen wrote:
> > On Tue, Jun 12, 2018 at 10:58:26AM -0700, Tadeusz Struk wrote:
> > > The TCG SAPI specification [1] defines a set of functions, which
> > > allows applications to use the TPM device in either blocking or
> > > non-blocking fashion. Each command defined by the specification
> > > has a corresponding Tss2_Sys_<COMMAND>_Prepare() and
> > > Tss2_Sys_<COMMAND>_Complete() call, which together with
> > > Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> > > mode of operation. Currently the TPM driver supports only
> > > blocking calls, which doesn't allow asynchronous IO operations.
> > > This patch changes it and adds support for nonblocking write and
> > > a new poll function to enable applications, which want to take
> > > advantage of this feature. The new functionality can be tested
> > > using standard TPM tools implemented in [2], together with
> > > modified TCTI from [3].
> > >
> > > [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI
> > > _Version-1.1_Revision-22_review_030918.pdf
> > > [2] https://github.com/tpm2-software/tpm2-tools
> > > [3] https://github.com/tstruk/tpm2-tss/tree/async
> >
> > For me the value is still a bit questionable. The benchmark looks a
> > bit flakky to give much figures how this would work with real world
> > workloads.
> >
> > I read James response and I also have to question why not just a
> > worker thread in user space? TPM does only one command at a time
> > anyways.
> >
> > Cannot take this in before I know that user space will (1) adapt to
> > this and (2) gain value compared to a worker thread.
> >
>
> Hi Jarkko,
> Thanks for reviewing the patch.
> There are applications/frameworks where a worker thread is not an
> option.
> Take for example the IoT use-cases and frameworks like IoT.js, or
> "Node.js for IoT".
> They are all single threaded, event-driven frameworks, using non-
> blocking I/O as the base of their processing model.
I'm slightly surprised by this statement. I thought IoT Node.js
runtimes (of which there are far too many, so I haven't looked at all
of them) use libuv or one of the forks:
http://libuv.org/
As the basis for their I/O handling? While libuv can do polling for
event driven interfaces it also support the worker thread model just as
easily:
http://docs.libuv.org/en/v1.x/threadpool.html
> Similarly embedded applications, which are basically just a single
> threaded event loop, quite often don't use threads because of
> resources constrains.
It's hard for me, as a kernel developer, to imagine any embedded
scenario using the Linux kernel that would not allow threads unless the
writers simply didn't bother with synchronization: The kernel schedules
at the threads level and can't be configured not to use them plus
threads are inherently more lightweight than processes so they're a
natural fit for resource constrained scenarios.
That's still not to say we shouldn't do this, but I've got to say I
think the only consumers would be old fashioned C code: the code we
used to write before we had thread libraries that did use signals and
poll() for a single threaded event driven monolith (think green
threads), because all the new webby languages use threading either
explicitly or at the core of their operation.
James
On 06/20/2018 04:59 PM, James Bottomley wrote:
> I'm slightly surprised by this statement. I thought IoT Node.js
> runtimes (of which there are far too many, so I haven't looked at all
> of them) use libuv or one of the forks:
>
> http://libuv.org/
>
> As the basis for their I/O handling? While libuv can do polling for
> event driven interfaces it also support the worker thread model just as
> easily:
>
> http://docs.libuv.org/en/v1.x/threadpool.html
Yes, it does polling:
http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop
>
>> Similarly embedded applications, which are basically just a single
>> threaded event loop, quite often don't use threads because of
>> resources constrains.
> It's hard for me, as a kernel developer, to imagine any embedded
> scenario using the Linux kernel that would not allow threads unless the
> writers simply didn't bother with synchronization: The kernel schedules
> at the threads level and can't be configured not to use them plus
> threads are inherently more lightweight than processes so they're a
> natural fit for resource constrained scenarios.
>
> That's still not to say we shouldn't do this, but I've got to say I
> think the only consumers would be old fashioned C code: the code we
> used to write before we had thread libraries that did use signals and
> poll() for a single threaded event driven monolith (think green
> threads), because all the new webby languages use threading either
> explicitly or at the core of their operation.
Regardless of how it actually might be used, I'm happy that we agree on
that this *is* the right thing to do.
Thanks,
Tadeusz
On Wed, 2018-06-20 at 18:24 -0700, Tadeusz Struk wrote:
> On 06/20/2018 04:59 PM, James Bottomley wrote:
> > I'm slightly surprised by this statement. I thought IoT Node.js
> > runtimes (of which there are far too many, so I haven't looked at
> > all of them) use libuv or one of the forks:
> >
> > http://libuv.org/
> >
> > As the basis for their I/O handling? While libuv can do polling
> > for event driven interfaces it also support the worker thread model
> > just as easily:
> >
> > http://docs.libuv.org/en/v1.x/threadpool.html
>
> Yes, it does polling:
> http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop
But that's for networking. You'll be talking to the TPM RM over the
file descriptor so that follows the thread pool model in
http://docs.libuv.org/en/v1.x/design.html#file-i-o
This precisely describes the current file descriptor abstraction we'd
use for the TPM.
> > > Similarly embedded applications, which are basically just a
> > > single threaded event loop, quite often don't use threads because
> > > of resources constrains.
> >
> > It's hard for me, as a kernel developer, to imagine any embedded
> > scenario using the Linux kernel that would not allow threads unless
> > the writers simply didn't bother with synchronization: The kernel
> > schedules at the threads level and can't be configured not to use
> > them plus threads are inherently more lightweight than processes so
> > they're a natural fit for resource constrained scenarios.
> >
> > That's still not to say we shouldn't do this, but I've got to say I
> > think the only consumers would be old fashioned C code: the code we
> > used to write before we had thread libraries that did use signals
> > and poll() for a single threaded event driven monolith (think green
> > threads), because all the new webby languages use threading either
> > explicitly or at the core of their operation.
>
> Regardless of how it actually might be used, I'm happy that we agree
> on that this *is* the right thing to do.
I didn't say that. I think using a single worker thread queue is the
correct abstraction for the TPM. If there's a legacy use case for
poll(), I don't see why not since the code seems to be fairly small and
self contained, but I don't really see it as correct or necessary to do
it that way.
James
On 06/20/2018 10:26 PM, James Bottomley wrote:
>> Yes, it does polling:
>> http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop
> But that's for networking. You'll be talking to the TPM RM over the
> file descriptor so that follows the thread pool model in
>
> http://docs.libuv.org/en/v1.x/design.html#file-i-o
>
> This precisely describes the current file descriptor abstraction we'd
> use for the TPM.
That is for the file IO that doesn't support non-blocking, because there
is no need for it as the operations are "fast".
Operations on the TPM would fall under the io loop model.
>
>> Regardless of how it actually might be used, I'm happy that we agree
>> on that this *is* the right thing to do.
> I didn't say that. I think using a single worker thread queue is the
> correct abstraction for the TPM. If there's a legacy use case for
> poll(), I don't see why not since the code seems to be fairly small and
> self contained, but I don't really see it as correct or necessary to do
> it that way.
This discussion starts to go around the circle. You don't agree, but you
also don't disagree? Is this what you are saying?
Thanks,
--
Tadeusz
On Tue, Jun 19, 2018 at 05:45:35PM -0700, Tadeusz Struk wrote:
> Hi Jarkko,
> Thanks for reviewing the patch.
> There are applications/frameworks where a worker thread is not an option.
> Take for example the IoT use-cases and frameworks like IoT.js, or "Node.js for IoT".
> They are all single threaded, event-driven frameworks, using non-blocking I/O as the base of their processing model.
> Similarly embedded applications, which are basically just a single threaded event loop, quite often don't use threads because of resources constrains.
>
> If your concern is that user space will not adopt to this, I can say that TSS library [1] is currently blocked on this feature, and we can not enable some of the use-cases mentioned above because of this.
>
> Thanks,
> Tadeusz
>
> [1] https://github.com/tpm2-software/tpm2-tss
I put this into "mathematical" terms. TPM is by nature is blocking. It
does not scale this way so you are essentially just simulating
non-blocking behaviour.
/Jarkko
On 06/21/2018 10:17 AM, Jarkko Sakkinen wrote:
> On Tue, Jun 19, 2018 at 05:45:35PM -0700, Tadeusz Struk wrote:
>> Hi Jarkko,
>> Thanks for reviewing the patch.
>> There are applications/frameworks where a worker thread is not an option.
>> Take for example the IoT use-cases and frameworks like IoT.js, or "Node.js for IoT".
>> They are all single threaded, event-driven frameworks, using non-blocking I/O as the base of their processing model.
>> Similarly embedded applications, which are basically just a single threaded event loop, quite often don't use threads because of resources constrains.
>>
>> If your concern is that user space will not adopt to this, I can say that TSS library [1] is currently blocked on this feature, and we can not enable some of the use-cases mentioned above because of this.
>>
>> Thanks,
>> Tadeusz
>>
>> [1] https://github.com/tpm2-software/tpm2-tss
>
> I put this into "mathematical" terms. TPM is by nature is blocking. It
> does not scale this way so you are essentially just simulating
> non-blocking behaviour.
>
That is correct, and this is exactly why we need this.
Thanks,
--
Tadeusz