diff mbox

[tpmdd-devel,RFC,v4,4/5] tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c

Message ID 20170122234438.12102-5-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Jan. 22, 2017, 11:44 p.m. UTC
From: James Bottomley <James.Bottomley@HansenPartnership.com>

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/Makefile         |   2 +-
 drivers/char/tpm/tpm-dev-common.c | 145 ++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm-dev.c        | 140 ++++--------------------------------
 drivers/char/tpm/tpm-dev.h        |  27 +++++++
 4 files changed, 187 insertions(+), 127 deletions(-)
 create mode 100644 drivers/char/tpm/tpm-dev-common.c
 create mode 100644 drivers/char/tpm/tpm-dev.h

Comments

Jason Gunthorpe Jan. 23, 2017, 4:47 p.m. UTC | #1
On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> Signed-off-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>

I really think we should not use the ugly read/write interface for any
new things.

Still unconvinced we should add a new cdev at this point.. But seeing
seesion support certainl is encouraging..

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 23, 2017, 10:28 p.m. UTC | #2
On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > Signed-off-by: James Bottomley
> > <James.Bottomley@HansenPartnership.com>
> 
> I really think we should not use the ugly read/write interface for 
> any new things.

The R/W interface is needed for backward compat, so we don't really
have a choice (well, it could go in for long term deprecation, but I
found in SCSI that "long term" == "never").  I think no-one objects to
the ioctl interface ... it's just no-one feels strongly enough to build
and test it.  I'm sure if you send patches, Jarkko will include them.

James

> Still unconvinced we should add a new cdev at this point.. But seeing
> seesion support certainl is encouraging..



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 23, 2017, 10:49 p.m. UTC | #3
On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote:
> On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > 
> > > Signed-off-by: James Bottomley
> > > <James.Bottomley@HansenPartnership.com>
> > 
> > I really think we should not use the ugly read/write interface for 
> > any new things.
> 
> The R/W interface is needed for backward compat,

With what? This is a new cdev with different semantics.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 23, 2017, 10:57 p.m. UTC | #4
On Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote:
> > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > 
> > > > Signed-off-by: James Bottomley
> > > > <James.Bottomley@HansenPartnership.com>
> > > 
> > > I really think we should not use the ugly read/write interface
> > > for 
> > > any new things.
> > 
> > The R/W interface is needed for backward compat,
> 
> With what? This is a new cdev with different semantics.

If you set TPM_DEVICE=/dev/tpms0 the old software just works.  If we
remove the R/W interface, nothing will work.  The point being the new
cdev has the same interface semantics, it just has different global
behavour.

James



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 23, 2017, 11:04 p.m. UTC | #5
On Mon, Jan 23, 2017 at 02:57:11PM -0800, James Bottomley wrote:
> On Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> > > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > > > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > 
> > > > > Signed-off-by: James Bottomley
> > > > > <James.Bottomley@HansenPartnership.com>
> > > > 
> > > > I really think we should not use the ugly read/write interface
> > > > for 
> > > > any new things.
> > > 
> > > The R/W interface is needed for backward compat,
> > 
> > With what? This is a new cdev with different semantics.
> 
> If you set TPM_DEVICE=/dev/tpms0 the old software just works.  If we
> remove the R/W interface, nothing will work.  The point being the new
> cdev has the same interface semantics, it just has different global
> behavour.

So you are saying there is so much already deployed TPM2 software that
has this TPM_DEVICE env var convention that we need to support it with
compat?

I'm really surprised by that.. But OK.

Can you at least remove the 'user_read_timer' junk from the new cdev?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 23, 2017, 11:20 p.m. UTC | #6
On Mon, 2017-01-23 at 16:04 -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 02:57:11PM -0800, James Bottomley wrote:
> > On Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote:
> > > On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> > > > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen
> > > > > wrote:
> > > > > > From: James Bottomley <
> > > > > > James.Bottomley@HansenPartnership.com>
> > > > > > 
> > > > > > Signed-off-by: James Bottomley
> > > > > > <James.Bottomley@HansenPartnership.com>
> > > > > 
> > > > > I really think we should not use the ugly read/write 
> > > > > interface for any new things.
> > > > 
> > > > The R/W interface is needed for backward compat,
> > > 
> > > With what? This is a new cdev with different semantics.
> > 
> > If you set TPM_DEVICE=/dev/tpms0 the old software just works.  If 
> > we remove the R/W interface, nothing will work.  The point being 
> > the new cdev has the same interface semantics, it just has 
> > different global behavour.
> 
> So you are saying there is so much already deployed TPM2 software 
> that has this TPM_DEVICE env var convention that we need to support 
> it with compat?
> 
> I'm really surprised by that.. But OK.
> 
> Can you at least remove the 'user_read_timer' junk from the new cdev?

What's the problem with it?  Can we not just fix whatever the issue is?

I'd rather reuse all the R/W machinery as is.  If I start trying to
special case it so that we only use some parts on some control flows,
the chances are I'll introduce additional bugs as well.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 23, 2017, 11:30 p.m. UTC | #7
On Mon, Jan 23, 2017 at 03:20:12PM -0800, James Bottomley wrote:

> > So you are saying there is so much already deployed TPM2 software 
> > that has this TPM_DEVICE env var convention that we need to support 
> > it with compat?
> > 
> > I'm really surprised by that.. But OK.
> > 
> > Can you at least remove the 'user_read_timer' junk from the new cdev?
> 
> What's the problem with it?  Can we not just fix whatever the issue is?

The issue is that it exists at all.

I've been unwilling to remove it because some crazy userspace might
rely on it, but I really don't want to see it continue in any new
stuff.

If you know the existing TPM1 userspace is safe then lets just delete
it entirely. Otherwise lets be sure no new users crop up by disabling
it.

> I'd rather reuse all the R/W machinery as is.  If I start trying to
> special case it so that we only use some parts on some control flows,
> the chances are I'll introduce additional bugs as well.

Sure, this is part of the pain of compat..

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley Jan. 23, 2017, 11:45 p.m. UTC | #8
On Mon, 2017-01-23 at 16:30 -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 03:20:12PM -0800, James Bottomley wrote:
> 
> > > So you are saying there is so much already deployed TPM2 software
> > > that has this TPM_DEVICE env var convention that we need to 
> > > support it with compat?
> > > 
> > > I'm really surprised by that.. But OK.
> > > 
> > > Can you at least remove the 'user_read_timer' junk from the new
> > > cdev?
> > 
> > What's the problem with it?  Can we not just fix whatever the issue
> > is?
> 
> The issue is that it exists at all.
> 
> I've been unwilling to remove it because some crazy userspace might
> rely on it, but I really don't want to see it continue in any new
> stuff.

All it does is clear the pending read after 60s ... like you, I suspect
it could just be removed but I don't think having it present causes
problems.

> If you know the existing TPM1 userspace is safe then lets just delete
> it entirely. Otherwise lets be sure no new users crop up by disabling
> it.
> 
> > I'd rather reuse all the R/W machinery as is.  If I start trying to
> > special case it so that we only use some parts on some control 
> > flows, the chances are I'll introduce additional bugs as well.
> 
> Sure, this is part of the pain of compat..

Except for the added complexity and possibility of extra bugs, nothing
is gained by the special casing.  That tells me we should either remove
this interface behaviour globally or not at all.  Removing it globally
would be independent of the space patches, because they'd simply
inherit whatever was done.

Why don't you start by doubling the timeout?  If nothing notices,
chances are nothing relies on this aspect of the interface and it can
be easily removed.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Jan. 24, 2017, 12:04 a.m. UTC | #9
On Mon, Jan 23, 2017 at 03:45:58PM -0800, James Bottomley wrote:

> Why don't you start by doubling the timeout?  If nothing notices,
> chances are nothing relies on this aspect of the interface and it can
> be easily removed.

Okay, fair enough, with a print I think it solves my concern. I sent a patch.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Jan. 24, 2017, 2:28 p.m. UTC | #10
On Mon, Jan 23, 2017 at 09:47:54AM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > Signed-off-by: James Bottomley
> > <James.Bottomley@HansenPartnership.com>
> 
> I really think we should not use the ugly read/write interface for any
> new things.
> 
> Still unconvinced we should add a new cdev at this point.. But seeing
> seesion support certainl is encouraging..
> 
> Jason

We can then revisit the interface once my patch set includes
the full functionality inluding the session management.

It's only a tiny portion of the implementation and can be
switched at any point.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Jan. 24, 2017, 2:30 p.m. UTC | #11
On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote:
> On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote:
> > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > 
> > > Signed-off-by: James Bottomley
> > > <James.Bottomley@HansenPartnership.com>
> > 
> > I really think we should not use the ugly read/write interface for 
> > any new things.
> 
> The R/W interface is needed for backward compat, so we don't really
> have a choice (well, it could go in for long term deprecation, but I
> found in SCSI that "long term" == "never").  I think no-one objects to
> the ioctl interface ... it's just no-one feels strongly enough to build
> and test it.  I'm sure if you send patches, Jarkko will include them.
> 
> James

I feel that it is incorrect to speak backwards compatibility because we
do no touch /dev/tpm0.

We can only speak about backwards compatibility only after the API for
tpms0 is in a kernel release. If someone uses that device, she must know
the constraints that it has.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
diff mbox

Patch

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 251d0ed..13ff5da 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -3,7 +3,7 @@ 
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-	 tpm_eventlog.o tpm2-space.o
+	 tpm_eventlog.o tpm2-space.o tpm-dev-common.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
new file mode 100644
index 0000000..0156562
--- /dev/null
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -0,0 +1,145 @@ 
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Authors:
+ * Leendert van Doorn <leendert@watson.ibm.com>
+ * Dave Safford <safford@watson.ibm.com>
+ * Reiner Sailer <sailer@watson.ibm.com>
+ * Kylene Hall <kjhall@us.ibm.com>
+ *
+ * Copyright (C) 2013 Obsidian Research Corp
+ * Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
+ *
+ * Device file system interface to the TPM
+ *
+ * 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/slab.h>
+#include <linux/uaccess.h>
+#include "tpm.h"
+#include "tpm-dev.h"
+
+static void user_reader_timeout(unsigned long ptr)
+{
+	struct file_priv *priv = (struct file_priv *)ptr;
+
+	schedule_work(&priv->work);
+}
+
+static void timeout_work(struct work_struct *work)
+{
+	struct file_priv *priv = container_of(work, struct file_priv, work);
+
+	mutex_lock(&priv->buffer_mutex);
+	atomic_set(&priv->data_pending, 0);
+	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
+	mutex_unlock(&priv->buffer_mutex);
+}
+
+void tpm_common_open(struct file *file, struct tpm_chip *chip,
+		     struct file_priv *priv)
+{
+	priv->chip = chip;
+	atomic_set(&priv->data_pending, 0);
+	mutex_init(&priv->buffer_mutex);
+	setup_timer(&priv->user_read_timer, user_reader_timeout,
+			(unsigned long)priv);
+	INIT_WORK(&priv->work, timeout_work);
+
+	file->private_data = priv;
+}
+
+ssize_t tpm_common_read(struct file *file, char __user *buf,
+			size_t size, loff_t *off)
+{
+	struct file_priv *priv = file->private_data;
+	ssize_t ret_size;
+	ssize_t orig_ret_size;
+	int rc;
+
+	del_singleshot_timer_sync(&priv->user_read_timer);
+	flush_work(&priv->work);
+	ret_size = atomic_read(&priv->data_pending);
+	if (ret_size > 0) {	/* relay data */
+		orig_ret_size = ret_size;
+		if (size < ret_size)
+			ret_size = size;
+
+		mutex_lock(&priv->buffer_mutex);
+		rc = copy_to_user(buf, priv->data_buffer, ret_size);
+		memset(priv->data_buffer, 0, orig_ret_size);
+		if (rc)
+			ret_size = -EFAULT;
+
+		mutex_unlock(&priv->buffer_mutex);
+	}
+
+	atomic_set(&priv->data_pending, 0);
+
+	return ret_size;
+}
+
+ssize_t tpm_common_write(struct file *file, const char __user *buf,
+			 size_t size, loff_t *off, struct tpm_space *space)
+{
+	struct file_priv *priv = file->private_data;
+	size_t in_size = size;
+	ssize_t out_size;
+
+	/* Cannot perform a write until the read has cleared either via
+	 * tpm_read or a user_read_timer timeout. This also prevents split
+	 * buffered writes from blocking here.
+	 */
+	if (atomic_read(&priv->data_pending) != 0)
+		return -EBUSY;
+
+	if (in_size > TPM_BUFSIZE)
+		return -E2BIG;
+
+	mutex_lock(&priv->buffer_mutex);
+
+	if (copy_from_user
+	    (priv->data_buffer, (void __user *) buf, in_size)) {
+		mutex_unlock(&priv->buffer_mutex);
+		return -EFAULT;
+	}
+
+	/* atomic tpm command send and result receive. We only hold the ops
+	 * lock during this period so that the tpm can be unregistered even if
+	 * the char dev is held open.
+	 */
+	if (tpm_try_get_ops(priv->chip)) {
+		mutex_unlock(&priv->buffer_mutex);
+		return -EPIPE;
+	}
+	out_size = tpm_transmit(priv->chip, 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;
+	}
+
+	atomic_set(&priv->data_pending, out_size);
+	mutex_unlock(&priv->buffer_mutex);
+
+	/* Set a timeout by which the reader must come claim the result */
+	mod_timer(&priv->user_read_timer, jiffies + (60 * HZ));
+
+	return in_size;
+}
+
+/*
+ * Called on file close
+ */
+void tpm_common_release(struct file *file, struct file_priv *priv)
+{
+	del_singleshot_timer_sync(&priv->user_read_timer);
+	flush_work(&priv->work);
+	file->private_data = NULL;
+	atomic_set(&priv->data_pending, 0);
+}
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 249eeb0..ebd74ab 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -18,45 +18,15 @@ 
  *
  */
 #include <linux/slab.h>
-#include <linux/uaccess.h>
-#include "tpm.h"
-
-struct file_priv {
-	struct tpm_chip *chip;
-
-	/* Data passed to and from the tpm via the read/write calls */
-	atomic_t data_pending;
-	struct mutex buffer_mutex;
-
-	struct timer_list user_read_timer;      /* user needs to claim result */
-	struct work_struct work;
-
-	u8 data_buffer[TPM_BUFSIZE];
-};
-
-static void user_reader_timeout(unsigned long ptr)
-{
-	struct file_priv *priv = (struct file_priv *)ptr;
-
-	schedule_work(&priv->work);
-}
-
-static void timeout_work(struct work_struct *work)
-{
-	struct file_priv *priv = container_of(work, struct file_priv, work);
-
-	mutex_lock(&priv->buffer_mutex);
-	atomic_set(&priv->data_pending, 0);
-	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
-	mutex_unlock(&priv->buffer_mutex);
-}
+#include "tpm-dev.h"
 
 static int tpm_open(struct inode *inode, struct file *file)
 {
-	struct tpm_chip *chip =
-		container_of(inode->i_cdev, struct tpm_chip, cdev);
+	struct tpm_chip *chip;
 	struct file_priv *priv;
 
+	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
+
 	/* It's assured that the chip will be opened just once,
 	 * by the check of is_open variable, which is protected
 	 * by driver_lock. */
@@ -66,100 +36,22 @@  static int tpm_open(struct inode *inode, struct file *file)
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (priv == NULL) {
-		clear_bit(0, &chip->is_open);
-		return -ENOMEM;
-	}
+	if (priv == NULL)
+		goto out;
 
-	priv->chip = chip;
-	atomic_set(&priv->data_pending, 0);
-	mutex_init(&priv->buffer_mutex);
-	setup_timer(&priv->user_read_timer, user_reader_timeout,
-			(unsigned long)priv);
-	INIT_WORK(&priv->work, timeout_work);
+	tpm_common_open(file, chip, priv);
 
-	file->private_data = priv;
 	return 0;
-}
-
-static ssize_t tpm_read(struct file *file, char __user *buf,
-			size_t size, loff_t *off)
-{
-	struct file_priv *priv = file->private_data;
-	ssize_t ret_size;
-	int rc;
-
-	del_singleshot_timer_sync(&priv->user_read_timer);
-	flush_work(&priv->work);
-	ret_size = atomic_read(&priv->data_pending);
-	if (ret_size > 0) {	/* relay data */
-		ssize_t orig_ret_size = ret_size;
-		if (size < ret_size)
-			ret_size = size;
 
-		mutex_lock(&priv->buffer_mutex);
-		rc = copy_to_user(buf, priv->data_buffer, ret_size);
-		memset(priv->data_buffer, 0, orig_ret_size);
-		if (rc)
-			ret_size = -EFAULT;
-
-		mutex_unlock(&priv->buffer_mutex);
-	}
-
-	atomic_set(&priv->data_pending, 0);
-
-	return ret_size;
+ out:
+	clear_bit(0, &chip->is_open);
+	return -ENOMEM;
 }
 
 static ssize_t tpm_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;
-
-	/* cannot perform a write until the read has cleared
-	   either via tpm_read or a user_read_timer timeout.
-	   This also prevents splitted buffered writes from blocking here.
-	*/
-	if (atomic_read(&priv->data_pending) != 0)
-		return -EBUSY;
-
-	if (in_size > TPM_BUFSIZE)
-		return -E2BIG;
-
-	mutex_lock(&priv->buffer_mutex);
-
-	if (copy_from_user
-	    (priv->data_buffer, (void __user *) buf, in_size)) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EFAULT;
-	}
-
-	/* atomic tpm command send and result receive. We only hold the ops
-	 * lock during this period so that the tpm can be unregistered even if
-	 * the char dev is held open.
-	 */
-	if (tpm_try_get_ops(priv->chip)) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EPIPE;
-	}
-	out_size = tpm_transmit(priv->chip, NULL, 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;
-	}
-
-	atomic_set(&priv->data_pending, out_size);
-	mutex_unlock(&priv->buffer_mutex);
-
-	/* Set a timeout by which the reader must come claim the result */
-	mod_timer(&priv->user_read_timer, jiffies + (60 * HZ));
-
-	return in_size;
+	return tpm_common_write(file, buf, size, off, NULL);
 }
 
 /*
@@ -169,12 +61,10 @@  static int tpm_release(struct inode *inode, struct file *file)
 {
 	struct file_priv *priv = file->private_data;
 
-	del_singleshot_timer_sync(&priv->user_read_timer);
-	flush_work(&priv->work);
-	file->private_data = NULL;
-	atomic_set(&priv->data_pending, 0);
+	tpm_common_release(file, priv);
 	clear_bit(0, &priv->chip->is_open);
 	kfree(priv);
+
 	return 0;
 }
 
@@ -182,9 +72,7 @@  const struct file_operations tpm_fops = {
 	.owner = THIS_MODULE,
 	.llseek = no_llseek,
 	.open = tpm_open,
-	.read = tpm_read,
+	.read = tpm_common_read,
 	.write = tpm_write,
 	.release = tpm_release,
 };
-
-
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
new file mode 100644
index 0000000..ff15cf7
--- /dev/null
+++ b/drivers/char/tpm/tpm-dev.h
@@ -0,0 +1,27 @@ 
+#ifndef _TPM_DEV_H
+#define _TPM_DEV_H
+
+#include "tpm.h"
+
+struct file_priv {
+	struct tpm_chip *chip;
+
+	/* Data passed to and from the tpm via the read/write calls */
+	atomic_t data_pending;
+	struct mutex buffer_mutex;
+
+	struct timer_list user_read_timer;      /* user needs to claim result */
+	struct work_struct work;
+
+	u8 data_buffer[TPM_BUFSIZE];
+};
+
+void tpm_common_open(struct file *file, struct tpm_chip *chip,
+		     struct file_priv *priv);
+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);
+void tpm_common_release(struct file *file, struct file_priv *priv);
+
+#endif