[prev in list] [next in list] [prev in thread] [next in thread] 

List:       tpmdd-devel
Subject:    Re: [tpmdd-devel] TPM 2.0 device driver blocking open
From:       James Bottomley <jejb () linux ! vnet ! ibm ! com>
Date:       2016-12-30 18:46:49
Message-ID: 1483123609.2712.1.camel () linux ! vnet ! ibm ! com
[Download RAW message or body]

On Fri, 2016-12-30 at 08:22 -0800, James Bottomley wrote:
> On Fri, 2016-12-30 at 10:53 -0500, Ken Goldman wrote:
> > Is there any way to change it.  I didn't set O_NOBLOCK.  Is there 
> > perhaps an ioctl()?
> > Is this something that should be added?
> 
> I think for the 2.0 model of every application getting direct access,
> we should make it so that every open gets a separate read/write 
> stream to the tpm which we send in via the locked version of 
> tpm_transmit() and just let the chip->tpm_mutex sort out the
> accesses.
> 
> I can code up a patch if no-one's already done it.

Just FYI this is the temporary hack I'm using to fix my multiple access
problem while Jarkko is working on the RM as the final solution.  All
it does is block a second and subsequent open until the first one
completes.

James

---

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 912ad30..cce67d5 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -51,23 +51,44 @@ static void timeout_work(struct work_struct *work)
 	mutex_unlock(&priv->buffer_mutex);
 }
 
+static int tpm_open_lock(struct tpm_chip *chip, int lock)
+{
+	static DECLARE_COMPLETION(wait);
+
+	if (lock) {
+		while (test_and_set_bit(0, &chip->is_open)) {
+			int ret;
+
+			dev_dbg(&chip->dev, "Another process owns this TPM\n");
+			ret = wait_for_completion_interruptible(&wait);
+			if (ret)
+				return ret;
+		}
+	} else {
+		clear_bit(0, &chip->is_open);
+		complete(&wait);
+	}
+
+	return 0;
+}
+
 static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip =
 		container_of(inode->i_cdev, struct tpm_chip, cdev);
 	struct file_priv *priv;
+	int ret;
 
 	/* It's assured that the chip will be opened just once,
 	 * by the check of is_open variable, which is protected
 	 * by driver_lock. */
-	if (test_and_set_bit(0, &chip->is_open)) {
-		dev_dbg(&chip->dev, "Another process owns this TPM\n");
-		return -EBUSY;
-	}
+	ret = tpm_open_lock(chip, 1);
+	if (ret)
+		return ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL) {
-		clear_bit(0, &chip->is_open);
+		tpm_open_lock(chip, 0);
 		return -ENOMEM;
 	}
 
@@ -173,7 +194,7 @@ static int tpm_release(struct inode *inode, struct file *file)
 	flush_work(&priv->work);
 	file->private_data = NULL;
 	atomic_set(&priv->data_pending, 0);
-	clear_bit(0, &priv->chip->is_open);
+	tpm_open_lock(priv->chip, 0);
 	kfree(priv);
 	return 0;
 }


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
[prev in list] [next in list] [prev in thread] [next in thread]