[PATCH 0/5] TPM: Locking update

[PATCH 0/5] TPM: Locking update

[Rajiv Andrade]

1) Removes unnecessary BKL from tpm.c
2) Changes the num_opens variable name to is_open since it's a binary
state
3) Implements the use of RCU to protect tpm_chip_list
4) Insert .remove function in tis_pnp_driver structure
5) Puts the timer deletion before the flushing of unfinished jobs

TPM: update char dev BKL pushdown
TPM: num_opens to is_open variable change
TPM: rcu locking
TPM: pnp remove
TPM: tpm_release() timing

 drivers/char/tpm/tpm.c     |   92 ++++++++++++++++++++-----------------------
 drivers/char/tpm/tpm.h     |    3 +-
 drivers/char/tpm/tpm_tis.c |   14 ++++++-
 3 files changed, 58 insertions(+), 51 deletions(-)

[PATCH 1/5] TPM: update char dev BKL pushdown

[Rajiv Andrade]

This patch removes the BKL calls from the TPM driver, which
were added in the overall misc-char-dev-BKL-pushdown.patch,
as they are not needed.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ae766d8..ceba608 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -954,13 +954,16 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
 
 /*
  * Device file system interface to the TPM
+ *
+ * It's assured that the chip will be opened just once,
+ * by the check of is_open variable, which is protected
+ * by driver_lock.
  */
 int tpm_open(struct inode *inode, struct file *file)
 {
 	int rc = 0, minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
-	lock_kernel();
 	spin_lock(&driver_lock);
 
 	list_for_each_entry(pos, &tpm_chip_list, list) {
@@ -990,19 +993,16 @@ int tpm_open(struct inode *inode, struct file *file)
 	if (chip->data_buffer == NULL) {
 		chip->num_opens--;
 		put_device(chip->dev);
-		unlock_kernel();
 		return -ENOMEM;
 	}
 
 	atomic_set(&chip->data_pending, 0);
 
 	file->private_data = chip;
-	unlock_kernel();
 	return 0;
 
 err_out:
 	spin_unlock(&driver_lock);
-	unlock_kernel();
 	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);
-- 
1.5.6.3

[PATCH 2/5] TPM: num_opens to is_open variable change

[Rajiv Andrade]

Renames num_open to is_open, as only one process can open the file at a time.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |    7 +++----
 drivers/char/tpm/tpm.h |    2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ceba608..dfd4e7f 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -978,20 +978,19 @@ int tpm_open(struct inode *inode, struct file *file)
 		goto err_out;
 	}
 
-	if (chip->num_opens) {
+	if (test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(chip->dev, "Another process owns this TPM\n");
 		rc = -EBUSY;
 		goto err_out;
 	}
 
-	chip->num_opens++;
 	get_device(chip->dev);
 
 	spin_unlock(&driver_lock);
 
 	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
 	if (chip->data_buffer == NULL) {
-		chip->num_opens--;
+		clear_bit(0, &chip->is_open);
 		put_device(chip->dev);
 		return -ENOMEM;
 	}
@@ -1016,7 +1015,7 @@ int tpm_release(struct inode *inode, struct file *file)
 	file->private_data = NULL;
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	atomic_set(&chip->data_pending, 0);
-	chip->num_opens--;
+	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
 	kfree(chip->data_buffer);
 	spin_unlock(&driver_lock);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e885148..2756cab 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -90,7 +90,7 @@ struct tpm_chip {
 	struct device *dev;	/* Device stuff */
 
 	int dev_num;		/* /dev/tpm# */
-	int num_opens;		/* only one allowed */
+	unsigned long is_open;	/* only one allowed */
 	int time_expired;
 
 	/* Data passed to and from the tpm via the read/write calls */
-- 
1.5.6.3

[PATCH 3/5] TPM rcu locking

[Rajiv Andrade]

Protects tpm_chip_list when transversing it.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |   57 +++++++++++++++++------------------------------
 1 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index dfd4e7f..24fb7ab 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
  */
 int tpm_open(struct inode *inode, struct file *file)
 {
-	int rc = 0, minor = iminor(inode);
+	int minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
-	spin_lock(&driver_lock);
-
-	list_for_each_entry(pos, &tpm_chip_list, list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
 		if (pos->vendor.miscdev.minor == minor) {
 			chip = pos;
+			get_device(chip->dev);
 			break;
 		}
 	}
+	rcu_read_unlock();
 
-	if (chip == NULL) {
-		rc = -ENODEV;
-		goto err_out;
-	}
+	if (!chip)
+		return -ENODEV;
 
 	if (test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(chip->dev, "Another process owns this TPM\n");
-		rc = -EBUSY;
-		goto err_out;
+		put_device(chip->dev);
+		return -EBUSY;
 	}
 
-	get_device(chip->dev);
-
-	spin_unlock(&driver_lock);
-
 	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
 	if (chip->data_buffer == NULL) {
 		clear_bit(0, &chip->is_open);
@@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
 
 	file->private_data = chip;
 	return 0;
-
-err_out:
-	spin_unlock(&driver_lock);
-	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);
 
+/*
+ * Called on file close
+ */
 int tpm_release(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip = file->private_data;
 
 	flush_scheduled_work();
-	spin_lock(&driver_lock);
 	file->private_data = NULL;
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	atomic_set(&chip->data_pending, 0);
+	kfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
-	kfree(chip->data_buffer);
-	spin_unlock(&driver_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_release);
@@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
 	}
 
 	spin_lock(&driver_lock);
-
-	list_del(&chip->list);
-
+	list_del_rcu(&chip->list);
 	spin_unlock(&driver_lock);
+	synchronize_rcu();
 
 	misc_deregister(&chip->vendor.miscdev);
-
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
@@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
 /*
  * Once all references to platform device are down to 0,
  * release all allocated structures.
- * In case vendor provided release function,
- * call it too.
+ * In case vendor provided release function, call it too.
  */
 static void tpm_dev_release(struct device *dev)
 {
@@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
 
 	if (chip->vendor.release)
 		chip->vendor.release(dev);
-
 	chip->release(dev);
 
 	clear_bit(chip->dev_num, dev_mask);
@@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
  * 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_vendor_specific
-				       *entry)
+struct tpm_chip *tpm_register_hardware(struct device *dev,
+		const struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -1230,12 +1218,6 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 		return NULL;
 	}
 
-	spin_lock(&driver_lock);
-
-	list_add(&chip->list, &tpm_chip_list);
-
-	spin_unlock(&driver_lock);
-
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
 		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
@@ -1245,6 +1227,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->bios_dir = tpm_bios_log_setup(devname);
 
+	/* Make chip available */
+	list_add_rcu(&chip->list, &tpm_chip_list);
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
-- 
1.5.6.3

Re: [PATCH 3/5] TPM rcu locking

[Serge E. Hallyn]

Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
> Protects tpm_chip_list when transversing it.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm.c |   57 +++++++++++++++++------------------------------
>  1 files changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index dfd4e7f..24fb7ab 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
>   */
>  int tpm_open(struct inode *inode, struct file *file)
>  {
> -	int rc = 0, minor = iminor(inode);
> +	int minor = iminor(inode);
>  	struct tpm_chip *chip = NULL, *pos;
> 
> -	spin_lock(&driver_lock);
> -
> -	list_for_each_entry(pos, &tpm_chip_list, list) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
>  		if (pos->vendor.miscdev.minor == minor) {
>  			chip = pos;
> +			get_device(chip->dev);
>  			break;
>  		}
>  	}
> +	rcu_read_unlock();
> 
> -	if (chip == NULL) {
> -		rc = -ENODEV;
> -		goto err_out;
> -	}
> +	if (!chip)
> +		return -ENODEV;
> 
>  	if (test_and_set_bit(0, &chip->is_open)) {
>  		dev_dbg(chip->dev, "Another process owns this TPM\n");
> -		rc = -EBUSY;
> -		goto err_out;
> +		put_device(chip->dev);
> +		return -EBUSY;
>  	}
> 
> -	get_device(chip->dev);
> -
> -	spin_unlock(&driver_lock);
> -
>  	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
>  	if (chip->data_buffer == NULL) {
>  		clear_bit(0, &chip->is_open);
> @@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
> 
>  	file->private_data = chip;
>  	return 0;
> -
> -err_out:
> -	spin_unlock(&driver_lock);
> -	return rc;
>  }
>  EXPORT_SYMBOL_GPL(tpm_open);
> 
> +/*
> + * Called on file close
> + */
>  int tpm_release(struct inode *inode, struct file *file)
>  {
>  	struct tpm_chip *chip = file->private_data;
> 
>  	flush_scheduled_work();
> -	spin_lock(&driver_lock);
>  	file->private_data = NULL;
>  	del_singleshot_timer_sync(&chip->user_read_timer);
>  	atomic_set(&chip->data_pending, 0);
> +	kfree(chip->data_buffer);
>  	clear_bit(0, &chip->is_open);
>  	put_device(chip->dev);
> -	kfree(chip->data_buffer);
> -	spin_unlock(&driver_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_release);
> @@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
>  	}
> 
>  	spin_lock(&driver_lock);
> -
> -	list_del(&chip->list);
> -
> +	list_del_rcu(&chip->list);
>  	spin_unlock(&driver_lock);
> +	synchronize_rcu();
> 
>  	misc_deregister(&chip->vendor.miscdev);
> -
>  	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
>  	tpm_bios_log_teardown(chip->bios_dir);
> 
> @@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
>  /*
>   * Once all references to platform device are down to 0,
>   * release all allocated structures.
> - * In case vendor provided release function,
> - * call it too.
> + * In case vendor provided release function, call it too.
>   */
>  static void tpm_dev_release(struct device *dev)
>  {
> @@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
> 
>  	if (chip->vendor.release)
>  		chip->vendor.release(dev);
> -
>  	chip->release(dev);
> 
>  	clear_bit(chip->dev_num, dev_mask);
> @@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
>   * 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_vendor_specific
> -				       *entry)
> +struct tpm_chip *tpm_register_hardware(struct device *dev,
> +		const struct tpm_vendor_specific *entry)
>  {
>  #define DEVNAME_SIZE 7
> 
> @@ -1230,12 +1218,6 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
>  		return NULL;
>  	}
> 
> -	spin_lock(&driver_lock);
> -
> -	list_add(&chip->list, &tpm_chip_list);
> -
> -	spin_unlock(&driver_lock);
> -
>  	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
>  		list_del(&chip->list);
>  		misc_deregister(&chip->vendor.miscdev);
> @@ -1245,6 +1227,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> 
>  	chip->bios_dir = tpm_bios_log_setup(devname);
> 
> +	/* Make chip available */
> +	list_add_rcu(&chip->list, &tpm_chip_list);

Why don't you need the spinlock here?

> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);
> -- 
> 1.5.6.3

[PATCH 3/5][resubmit] TPM: rcu locking

[Rajiv Andrade]

This is a resubmission of rcu locking patch, posted some hours ago, addressing Serge Hallyn's comment.
In the previous patch there was missing the spin_lock/unlock protecting list_add_rcu() 
in tpm_register_hardware(). There were also an unnecessary list_del() inside the same function, 
that is now being removed.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c |   61 +++++++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index dfd4e7f..e96ca5a 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
  */
 int tpm_open(struct inode *inode, struct file *file)
 {
-	int rc = 0, minor = iminor(inode);
+	int minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
-	spin_lock(&driver_lock);
-
-	list_for_each_entry(pos, &tpm_chip_list, list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
 		if (pos->vendor.miscdev.minor == minor) {
 			chip = pos;
+			get_device(chip->dev);
 			break;
 		}
 	}
+	rcu_read_unlock();
 
-	if (chip == NULL) {
-		rc = -ENODEV;
-		goto err_out;
-	}
+	if (!chip)
+		return -ENODEV;
 
 	if (test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(chip->dev, "Another process owns this TPM\n");
-		rc = -EBUSY;
-		goto err_out;
+		put_device(chip->dev);
+		return -EBUSY;
 	}
 
-	get_device(chip->dev);
-
-	spin_unlock(&driver_lock);
-
 	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
 	if (chip->data_buffer == NULL) {
 		clear_bit(0, &chip->is_open);
@@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
 
 	file->private_data = chip;
 	return 0;
-
-err_out:
-	spin_unlock(&driver_lock);
-	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);
 
+/*
+ * Called on file close
+ */
 int tpm_release(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip = file->private_data;
 
 	flush_scheduled_work();
-	spin_lock(&driver_lock);
 	file->private_data = NULL;
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	atomic_set(&chip->data_pending, 0);
+	kfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
-	kfree(chip->data_buffer);
-	spin_unlock(&driver_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_release);
@@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
 	}
 
 	spin_lock(&driver_lock);
-
-	list_del(&chip->list);
-
+	list_del_rcu(&chip->list);
 	spin_unlock(&driver_lock);
+	synchronize_rcu();
 
 	misc_deregister(&chip->vendor.miscdev);
-
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
@@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
 /*
  * Once all references to platform device are down to 0,
  * release all allocated structures.
- * In case vendor provided release function,
- * call it too.
+ * In case vendor provided release function, call it too.
  */
 static void tpm_dev_release(struct device *dev)
 {
@@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
 
 	if (chip->vendor.release)
 		chip->vendor.release(dev);
-
 	chip->release(dev);
 
 	clear_bit(chip->dev_num, dev_mask);
@@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
  * 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_vendor_specific
-				       *entry)
+struct tpm_chip *tpm_register_hardware(struct device *dev,
+					const struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -1230,21 +1218,20 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 		return NULL;
 	}
 
-	spin_lock(&driver_lock);
-
-	list_add(&chip->list, &tpm_chip_list);
-
-	spin_unlock(&driver_lock);
-
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
-		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
 		put_device(chip->dev);
+
 		return NULL;
 	}
 
 	chip->bios_dir = tpm_bios_log_setup(devname);
 
+	/* Make chip available */
+	spin_lock(&driver_lock);
+	list_add_rcu(&chip->list, &tpm_chip_list);
+	spin_lock(&driver_lock);
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
-- 
1.5.6.3

Re: [PATCH 3/5][resubmit] TPM: rcu locking

[Serge E. Hallyn]

Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
> This is a resubmission of rcu locking patch, posted some hours ago, addressing Serge Hallyn's comment.
> In the previous patch there was missing the spin_lock/unlock protecting list_add_rcu() 
> in tpm_register_hardware(). There were also an unnecessary list_del() inside the same function, 
> that is now being removed.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serue@us.ibm.com>

which also applies to 1 and 2.

thanks,
-serge

> ---
>  drivers/char/tpm/tpm.c |   61 +++++++++++++++++++-----------------------------
>  1 files changed, 24 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index dfd4e7f..e96ca5a 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
>   */
>  int tpm_open(struct inode *inode, struct file *file)
>  {
> -	int rc = 0, minor = iminor(inode);
> +	int minor = iminor(inode);
>  	struct tpm_chip *chip = NULL, *pos;
> 
> -	spin_lock(&driver_lock);
> -
> -	list_for_each_entry(pos, &tpm_chip_list, list) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
>  		if (pos->vendor.miscdev.minor == minor) {
>  			chip = pos;
> +			get_device(chip->dev);
>  			break;
>  		}
>  	}
> +	rcu_read_unlock();
> 
> -	if (chip == NULL) {
> -		rc = -ENODEV;
> -		goto err_out;
> -	}
> +	if (!chip)
> +		return -ENODEV;
> 
>  	if (test_and_set_bit(0, &chip->is_open)) {
>  		dev_dbg(chip->dev, "Another process owns this TPM\n");
> -		rc = -EBUSY;
> -		goto err_out;
> +		put_device(chip->dev);
> +		return -EBUSY;
>  	}
> 
> -	get_device(chip->dev);
> -
> -	spin_unlock(&driver_lock);
> -
>  	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
>  	if (chip->data_buffer == NULL) {
>  		clear_bit(0, &chip->is_open);
> @@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
> 
>  	file->private_data = chip;
>  	return 0;
> -
> -err_out:
> -	spin_unlock(&driver_lock);
> -	return rc;
>  }
>  EXPORT_SYMBOL_GPL(tpm_open);
> 
> +/*
> + * Called on file close
> + */
>  int tpm_release(struct inode *inode, struct file *file)
>  {
>  	struct tpm_chip *chip = file->private_data;
> 
>  	flush_scheduled_work();
> -	spin_lock(&driver_lock);
>  	file->private_data = NULL;
>  	del_singleshot_timer_sync(&chip->user_read_timer);
>  	atomic_set(&chip->data_pending, 0);
> +	kfree(chip->data_buffer);
>  	clear_bit(0, &chip->is_open);
>  	put_device(chip->dev);
> -	kfree(chip->data_buffer);
> -	spin_unlock(&driver_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_release);
> @@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
>  	}
> 
>  	spin_lock(&driver_lock);
> -
> -	list_del(&chip->list);
> -
> +	list_del_rcu(&chip->list);
>  	spin_unlock(&driver_lock);
> +	synchronize_rcu();
> 
>  	misc_deregister(&chip->vendor.miscdev);
> -
>  	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
>  	tpm_bios_log_teardown(chip->bios_dir);
> 
> @@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
>  /*
>   * Once all references to platform device are down to 0,
>   * release all allocated structures.
> - * In case vendor provided release function,
> - * call it too.
> + * In case vendor provided release function, call it too.
>   */
>  static void tpm_dev_release(struct device *dev)
>  {
> @@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
> 
>  	if (chip->vendor.release)
>  		chip->vendor.release(dev);
> -
>  	chip->release(dev);
> 
>  	clear_bit(chip->dev_num, dev_mask);
> @@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
>   * 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_vendor_specific
> -				       *entry)
> +struct tpm_chip *tpm_register_hardware(struct device *dev,
> +					const struct tpm_vendor_specific *entry)
>  {
>  #define DEVNAME_SIZE 7
> 
> @@ -1230,21 +1218,20 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
>  		return NULL;
>  	}
> 
> -	spin_lock(&driver_lock);
> -
> -	list_add(&chip->list, &tpm_chip_list);
> -
> -	spin_unlock(&driver_lock);
> -
>  	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> -		list_del(&chip->list);
>  		misc_deregister(&chip->vendor.miscdev);
>  		put_device(chip->dev);
> +
>  		return NULL;
>  	}
> 
>  	chip->bios_dir = tpm_bios_log_setup(devname);
> 
> +	/* Make chip available */
> +	spin_lock(&driver_lock);
> +	list_add_rcu(&chip->list, &tpm_chip_list);
> +	spin_lock(&driver_lock);
> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);
> -- 
> 1.5.6.3
> 
> 

Re: [PATCH 3/5][resubmit][BUG] TPM: rcu locking

[Rajiv Andrade]

Please, do not consider this patch, it's wrong...

On Fri, 2008-10-03 at 16:12 -0300, Rajiv Andrade wrote:
> This is a resubmission of rcu locking patch, posted some hours ago, addressing Serge Hallyn's comment.
> In the previous patch there was missing the spin_lock/unlock protecting list_add_rcu() 
> in tpm_register_hardware(). There were also an unnecessary list_del() inside the same function, 
> that is now being removed.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm.c |   61 +++++++++++++++++++-----------------------------
>  1 files changed, 24 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index dfd4e7f..e96ca5a 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
>   */
>  int tpm_open(struct inode *inode, struct file *file)
>  {
> -	int rc = 0, minor = iminor(inode);
> +	int minor = iminor(inode);
>  	struct tpm_chip *chip = NULL, *pos;
> 
> -	spin_lock(&driver_lock);
> -
> -	list_for_each_entry(pos, &tpm_chip_list, list) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
>  		if (pos->vendor.miscdev.minor == minor) {
>  			chip = pos;
> +			get_device(chip->dev);
>  			break;
>  		}
>  	}
> +	rcu_read_unlock();
> 
> -	if (chip == NULL) {
> -		rc = -ENODEV;
> -		goto err_out;
> -	}
> +	if (!chip)
> +		return -ENODEV;
> 
>  	if (test_and_set_bit(0, &chip->is_open)) {
>  		dev_dbg(chip->dev, "Another process owns this TPM\n");
> -		rc = -EBUSY;
> -		goto err_out;
> +		put_device(chip->dev);
> +		return -EBUSY;
>  	}
> 
> -	get_device(chip->dev);
> -
> -	spin_unlock(&driver_lock);
> -
>  	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
>  	if (chip->data_buffer == NULL) {
>  		clear_bit(0, &chip->is_open);
> @@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
> 
>  	file->private_data = chip;
>  	return 0;
> -
> -err_out:
> -	spin_unlock(&driver_lock);
> -	return rc;
>  }
>  EXPORT_SYMBOL_GPL(tpm_open);
> 
> +/*
> + * Called on file close
> + */
>  int tpm_release(struct inode *inode, struct file *file)
>  {
>  	struct tpm_chip *chip = file->private_data;
> 
>  	flush_scheduled_work();
> -	spin_lock(&driver_lock);
>  	file->private_data = NULL;
>  	del_singleshot_timer_sync(&chip->user_read_timer);
>  	atomic_set(&chip->data_pending, 0);
> +	kfree(chip->data_buffer);
>  	clear_bit(0, &chip->is_open);
>  	put_device(chip->dev);
> -	kfree(chip->data_buffer);
> -	spin_unlock(&driver_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_release);
> @@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
>  	}
> 
>  	spin_lock(&driver_lock);
> -
> -	list_del(&chip->list);
> -
> +	list_del_rcu(&chip->list);
>  	spin_unlock(&driver_lock);
> +	synchronize_rcu();
> 
>  	misc_deregister(&chip->vendor.miscdev);
> -
>  	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
>  	tpm_bios_log_teardown(chip->bios_dir);
> 
> @@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
>  /*
>   * Once all references to platform device are down to 0,
>   * release all allocated structures.
> - * In case vendor provided release function,
> - * call it too.
> + * In case vendor provided release function, call it too.
>   */
>  static void tpm_dev_release(struct device *dev)
>  {
> @@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
> 
>  	if (chip->vendor.release)
>  		chip->vendor.release(dev);
> -
>  	chip->release(dev);
> 
>  	clear_bit(chip->dev_num, dev_mask);
> @@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
>   * 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_vendor_specific
> -				       *entry)
> +struct tpm_chip *tpm_register_hardware(struct device *dev,
> +					const struct tpm_vendor_specific *entry)
>  {
>  #define DEVNAME_SIZE 7
> 
> @@ -1230,21 +1218,20 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
>  		return NULL;
>  	}
> 
> -	spin_lock(&driver_lock);
> -
> -	list_add(&chip->list, &tpm_chip_list);
> -
> -	spin_unlock(&driver_lock);
> -
>  	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> -		list_del(&chip->list);
>  		misc_deregister(&chip->vendor.miscdev);
>  		put_device(chip->dev);
> +
>  		return NULL;
>  	}
> 
>  	chip->bios_dir = tpm_bios_log_setup(devname);
> 
> +	/* Make chip available */
> +	spin_lock(&driver_lock);
> +	list_add_rcu(&chip->list, &tpm_chip_list);
> +	spin_lock(&driver_lock);
> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);

Re: [PATCH 3/5][resubmit][FIXED] TPM: rcu locking

[Rajiv Andrade]

This is a resubmission of rcu locking patch, posted some hours ago, addressing Serge Hallyn's comment.
In the previous patch there was missing the spin_lock/unlock protecting list_add_rcu()
in tpm_register_hardware(). There were also an unnecessary list_del() inside the same function,
that is now being removed.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Acked-by: Serge E. Hallyn <serue@us.ibm.com>
---
 drivers/char/tpm/tpm.c |   61 +++++++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index dfd4e7f..e96ca5a 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -961,33 +961,28 @@ EXPORT_SYMBOL_GPL(tpm_store_cancel);
  */
 int tpm_open(struct inode *inode, struct file *file)
 {
-	int rc = 0, minor = iminor(inode);
+	int minor = iminor(inode);
 	struct tpm_chip *chip = NULL, *pos;
 
-	spin_lock(&driver_lock);
-
-	list_for_each_entry(pos, &tpm_chip_list, list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
 		if (pos->vendor.miscdev.minor == minor) {
 			chip = pos;
+			get_device(chip->dev);
 			break;
 		}
 	}
+	rcu_read_unlock();
 
-	if (chip == NULL) {
-		rc = -ENODEV;
-		goto err_out;
-	}
+	if (!chip)
+		return -ENODEV;
 
 	if (test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(chip->dev, "Another process owns this TPM\n");
-		rc = -EBUSY;
-		goto err_out;
+		put_device(chip->dev);
+		return -EBUSY;
 	}
 
-	get_device(chip->dev);
-
-	spin_unlock(&driver_lock);
-
 	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
 	if (chip->data_buffer == NULL) {
 		clear_bit(0, &chip->is_open);
@@ -999,26 +994,23 @@ int tpm_open(struct inode *inode, struct file *file)
 
 	file->private_data = chip;
 	return 0;
-
-err_out:
-	spin_unlock(&driver_lock);
-	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm_open);
 
+/*
+ * Called on file close
+ */
 int tpm_release(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip = file->private_data;
 
 	flush_scheduled_work();
-	spin_lock(&driver_lock);
 	file->private_data = NULL;
 	del_singleshot_timer_sync(&chip->user_read_timer);
 	atomic_set(&chip->data_pending, 0);
+	kfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
 	put_device(chip->dev);
-	kfree(chip->data_buffer);
-	spin_unlock(&driver_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_release);
@@ -1092,13 +1084,11 @@ void tpm_remove_hardware(struct device *dev)
 	}
 
 	spin_lock(&driver_lock);
-
-	list_del(&chip->list);
-
+	list_del_rcu(&chip->list);
 	spin_unlock(&driver_lock);
+	synchronize_rcu();
 
 	misc_deregister(&chip->vendor.miscdev);
-
 	sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
 	tpm_bios_log_teardown(chip->bios_dir);
 
@@ -1146,8 +1136,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
 /*
  * Once all references to platform device are down to 0,
  * release all allocated structures.
- * In case vendor provided release function,
- * call it too.
+ * In case vendor provided release function, call it too.
  */
 static void tpm_dev_release(struct device *dev)
 {
@@ -1155,7 +1144,6 @@ static void tpm_dev_release(struct device *dev)
 
 	if (chip->vendor.release)
 		chip->vendor.release(dev);
-
 	chip->release(dev);
 
 	clear_bit(chip->dev_num, dev_mask);
@@ -1170,8 +1158,8 @@ static void tpm_dev_release(struct device *dev)
  * 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_vendor_specific
-				       *entry)
+struct tpm_chip *tpm_register_hardware(struct device *dev,
+					const struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -1230,21 +1218,20 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 		return NULL;
 	}
 
-	spin_lock(&driver_lock);
-
-	list_add(&chip->list, &tpm_chip_list);
-
-	spin_unlock(&driver_lock);
-
 	if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
-		list_del(&chip->list);
 		misc_deregister(&chip->vendor.miscdev);
 		put_device(chip->dev);
+
 		return NULL;
 	}
 
 	chip->bios_dir = tpm_bios_log_setup(devname);
 
+	/* Make chip available */
+	spin_lock(&driver_lock);
+	list_add_rcu(&chip->list, &tpm_chip_list);
+	spin_unlock(&driver_lock);
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
-- 
1.5.6.3

[PATCH 4/5] TPM: addition of pnp_remove()

[Rajiv Andrade]

The tpm_dev_release function is only called for platform devices, not pnp devices, so we
implemented the .remove function for pnp ones.
Since it's code is very similar to the one inside tpm_dev_release, we've created a helper
function tpm_dev_vendor_release, which is called by both.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c     |   22 ++++++++++++++++------
 drivers/char/tpm/tpm.h     |    1 +
 drivers/char/tpm/tpm_tis.c |   14 +++++++++++++-
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 24fb7ab..ab03b4d 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1133,23 +1133,33 @@ int tpm_pm_resume(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
+/* In case vendor provided release function, call it too.*/
+
+void tpm_dev_vendor_release(struct tpm_chip *chip)
+{
+	if (chip->vendor.release)
+	 	chip->vendor.release(chip->dev);
+	
+	clear_bit(chip->dev_num, dev_mask);
+	kfree(chip->vendor.miscdev.name);
+}
+EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
+
+
 /*
  * Once all references to platform device are down to 0,
  * release all allocated structures.
- * In case vendor provided release function, call it too.
  */
 static void tpm_dev_release(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 
-	if (chip->vendor.release)
-		chip->vendor.release(dev);
-	chip->release(dev);
+	tpm_dev_vendor_release(chip);
 
-	clear_bit(chip->dev_num, dev_mask);
-	kfree(chip->vendor.miscdev.name);
+	chip->release(dev);
 	kfree(chip);
 }
+EXPORT_SYMBOL_GPL(tpm_dev_release);
 
 /*
  * Called from tpm_<specific>.c probe function only for devices 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2756cab..8e30df4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -132,6 +132,7 @@ extern struct tpm_chip* tpm_register_hardware(struct device *,
 				 const struct tpm_vendor_specific *);
 extern int tpm_open(struct inode *, struct file *);
 extern int tpm_release(struct inode *, struct file *);
+extern void tpm_dev_vendor_release(struct tpm_chip *);
 extern ssize_t tpm_write(struct file *, const char __user *, size_t,
 			 loff_t *);
 extern ssize_t tpm_read(struct file *, char __user *, size_t, loff_t *);
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index ed1879c..3491d70 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -630,12 +630,23 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
 	{"", 0}			/* Terminator */
 };
 
+static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
+{
+	struct tpm_chip *chip = pnp_get_drvdata(dev);
+	
+	tpm_dev_vendor_release(chip);
+
+	kfree(chip);
+}
+
+
 static struct pnp_driver tis_pnp_driver = {
 	.name = "tpm_tis",
 	.id_table = tpm_pnp_tbl,
 	.probe = tpm_tis_pnp_init,
 	.suspend = tpm_tis_pnp_suspend,
 	.resume = tpm_tis_pnp_resume,
+	.remove = tpm_tis_pnp_remove,
 };
 
 #define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
@@ -683,6 +694,7 @@ static void __exit cleanup_tis(void)
 	spin_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.
@@ -694,9 +706,9 @@ static void __exit cleanup_tis(void)
 			free_irq(chip->vendor.irq, chip);
 		iounmap(i->iobase);
 		list_del(&i->list);
-		tpm_remove_hardware(chip->dev);
 	}
 	spin_unlock(&tis_lock);
+	
 	if (force) {
 		platform_device_unregister(pdev);
 		driver_unregister(&tis_drv);
-- 
1.5.6.3

Re: [PATCH 4/5] TPM: addition of pnp_remove()

[Serge E. Hallyn]

Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
> The tpm_dev_release function is only called for platform devices, not pnp devices, so we
> implemented the .remove function for pnp ones.
> Since it's code is very similar to the one inside tpm_dev_release, we've created a helper
> function tpm_dev_vendor_release, which is called by both.

Should tpm_infineon also be switched over to this?

> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm.c     |   22 ++++++++++++++++------
>  drivers/char/tpm/tpm.h     |    1 +
>  drivers/char/tpm/tpm_tis.c |   14 +++++++++++++-
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 24fb7ab..ab03b4d 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1133,23 +1133,33 @@ int tpm_pm_resume(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(tpm_pm_resume);
> 
> +/* In case vendor provided release function, call it too.*/
> +
> +void tpm_dev_vendor_release(struct tpm_chip *chip)
> +{
> +	if (chip->vendor.release)
> +	 	chip->vendor.release(chip->dev);
> +	
> +	clear_bit(chip->dev_num, dev_mask);
> +	kfree(chip->vendor.miscdev.name);
> +}
> +EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
> +
> +
>  /*
>   * Once all references to platform device are down to 0,
>   * release all allocated structures.
> - * In case vendor provided release function, call it too.
>   */
>  static void tpm_dev_release(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> 
> -	if (chip->vendor.release)
> -		chip->vendor.release(dev);
> -	chip->release(dev);
> +	tpm_dev_vendor_release(chip);
> 
> -	clear_bit(chip->dev_num, dev_mask);
> -	kfree(chip->vendor.miscdev.name);
> +	chip->release(dev);
>  	kfree(chip);
>  }
> +EXPORT_SYMBOL_GPL(tpm_dev_release);
> 
>  /*
>   * Called from tpm_<specific>.c probe function only for devices 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 2756cab..8e30df4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -132,6 +132,7 @@ extern struct tpm_chip* tpm_register_hardware(struct device *,
>  				 const struct tpm_vendor_specific *);
>  extern int tpm_open(struct inode *, struct file *);
>  extern int tpm_release(struct inode *, struct file *);
> +extern void tpm_dev_vendor_release(struct tpm_chip *);
>  extern ssize_t tpm_write(struct file *, const char __user *, size_t,
>  			 loff_t *);
>  extern ssize_t tpm_read(struct file *, char __user *, size_t, loff_t *);
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index ed1879c..3491d70 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -630,12 +630,23 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
>  	{"", 0}			/* Terminator */
>  };
> 
> +static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
> +{
> +	struct tpm_chip *chip = pnp_get_drvdata(dev);
> +	
> +	tpm_dev_vendor_release(chip);
> +
> +	kfree(chip);
> +}
> +
> +
>  static struct pnp_driver tis_pnp_driver = {
>  	.name = "tpm_tis",
>  	.id_table = tpm_pnp_tbl,
>  	.probe = tpm_tis_pnp_init,
>  	.suspend = tpm_tis_pnp_suspend,
>  	.resume = tpm_tis_pnp_resume,
> +	.remove = tpm_tis_pnp_remove,
>  };
> 
>  #define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
> @@ -683,6 +694,7 @@ static void __exit cleanup_tis(void)
>  	spin_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.
> @@ -694,9 +706,9 @@ static void __exit cleanup_tis(void)
>  			free_irq(chip->vendor.irq, chip);
>  		iounmap(i->iobase);
>  		list_del(&i->list);
> -		tpm_remove_hardware(chip->dev);
>  	}
>  	spin_unlock(&tis_lock);
> +	
>  	if (force) {
>  		platform_device_unregister(pdev);
>  		driver_unregister(&tis_drv);
> -- 
> 1.5.6.3

Re: [PATCH 4/5] TPM: addition of pnp_remove()

[Rajiv Andrade]

Unfortunately we don't have an old 1.1 infineon tpm chip in order to
test this legacy driver, but probably we'd need to add a call to
tpm_dev_vendor_release() in tpm_infineon.c

Marcel, do you have any clue?

On Fri, 2008-10-03 at 15:05 -0500, Serge E. Hallyn wrote:
> Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
> > The tpm_dev_release function is only called for platform devices, not pnp devices, so we
> > implemented the .remove function for pnp ones.
> > Since it's code is very similar to the one inside tpm_dev_release, we've created a helper
> > function tpm_dev_vendor_release, which is called by both.
> 
> Should tpm_infineon also be switched over to this?
> 
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> > ---
> >  drivers/char/tpm/tpm.c     |   22 ++++++++++++++++------
> >  drivers/char/tpm/tpm.h     |    1 +
> >  drivers/char/tpm/tpm_tis.c |   14 +++++++++++++-
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index 24fb7ab..ab03b4d 100644
> > --- a/drivers/char/tpm/tpm.c
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -1133,23 +1133,33 @@ int tpm_pm_resume(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(tpm_pm_resume);
> > 
> > +/* In case vendor provided release function, call it too.*/
> > +
> > +void tpm_dev_vendor_release(struct tpm_chip *chip)
> > +{
> > +	if (chip->vendor.release)
> > +	 	chip->vendor.release(chip->dev);
> > +	
> > +	clear_bit(chip->dev_num, dev_mask);
> > +	kfree(chip->vendor.miscdev.name);
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
> > +
> > +
> >  /*
> >   * Once all references to platform device are down to 0,
> >   * release all allocated structures.
> > - * In case vendor provided release function, call it too.
> >   */
> >  static void tpm_dev_release(struct device *dev)
> >  {
> >  	struct tpm_chip *chip = dev_get_drvdata(dev);
> > 
> > -	if (chip->vendor.release)
> > -		chip->vendor.release(dev);
> > -	chip->release(dev);
> > +	tpm_dev_vendor_release(chip);
> > 
> > -	clear_bit(chip->dev_num, dev_mask);
> > -	kfree(chip->vendor.miscdev.name);
> > +	chip->release(dev);
> >  	kfree(chip);
> >  }
> > +EXPORT_SYMBOL_GPL(tpm_dev_release);
> > 
> >  /*
> >   * Called from tpm_<specific>.c probe function only for devices 
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 2756cab..8e30df4 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -132,6 +132,7 @@ extern struct tpm_chip* tpm_register_hardware(struct device *,
> >  				 const struct tpm_vendor_specific *);
> >  extern int tpm_open(struct inode *, struct file *);
> >  extern int tpm_release(struct inode *, struct file *);
> > +extern void tpm_dev_vendor_release(struct tpm_chip *);
> >  extern ssize_t tpm_write(struct file *, const char __user *, size_t,
> >  			 loff_t *);
> >  extern ssize_t tpm_read(struct file *, char __user *, size_t, loff_t *);
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index ed1879c..3491d70 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -630,12 +630,23 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
> >  	{"", 0}			/* Terminator */
> >  };
> > 
> > +static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
> > +{
> > +	struct tpm_chip *chip = pnp_get_drvdata(dev);
> > +	
> > +	tpm_dev_vendor_release(chip);
> > +
> > +	kfree(chip);
> > +}
> > +
> > +
> >  static struct pnp_driver tis_pnp_driver = {
> >  	.name = "tpm_tis",
> >  	.id_table = tpm_pnp_tbl,
> >  	.probe = tpm_tis_pnp_init,
> >  	.suspend = tpm_tis_pnp_suspend,
> >  	.resume = tpm_tis_pnp_resume,
> > +	.remove = tpm_tis_pnp_remove,
> >  };
> > 
> >  #define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
> > @@ -683,6 +694,7 @@ static void __exit cleanup_tis(void)
> >  	spin_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.
> > @@ -694,9 +706,9 @@ static void __exit cleanup_tis(void)
> >  			free_irq(chip->vendor.irq, chip);
> >  		iounmap(i->iobase);
> >  		list_del(&i->list);
> > -		tpm_remove_hardware(chip->dev);
> >  	}
> >  	spin_unlock(&tis_lock);
> > +	
> >  	if (force) {
> >  		platform_device_unregister(pdev);
> >  		driver_unregister(&tis_drv);
> > -- 
> > 1.5.6.3

Re: [PATCH 4/5] TPM: addition of pnp_remove()

[Marcel Selhorst]

Hi,

sorry for the delay in my response.
As far as I understand, there is no need to add something to the Infineon
driver, since it already calls the according pnp device release functions on its
own (see code snippets below).
Or do you want to remove the tpm_inf_pnp_remove-function calls from the Infineon
device driver and let the TPM-framework handle the pnp release?

I still have one or two Infineon 1.1b here, so in case you want me to test a
patch, I am happy to do so.

Thanks in advance,
Marcel

static __devexit void tpm_inf_pnp_remove(struct pnp_dev *dev)
{
        struct tpm_chip *chip = pnp_get_drvdata(dev);
        if (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,
                                       tpm_dev.config_size);
                } else {
                        iounmap(tpm_dev.mem_base);
                        release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
                }
                tpm_remove_hardware(chip->dev);
        }
}

static struct pnp_driver tpm_inf_pnp_driver = {
[...]
        .remove = __devexit_p(tpm_inf_pnp_remove),
};


Rajiv Andrade schrieb:
> Unfortunately we don't have an old 1.1 infineon tpm chip in order to
> test this legacy driver, but probably we'd need to add a call to
> tpm_dev_vendor_release() in tpm_infineon.c
> 
> Marcel, do you have any clue?
> 
> On Fri, 2008-10-03 at 15:05 -0500, Serge E. Hallyn wrote:
>> Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
>>> The tpm_dev_release function is only called for platform devices, not pnp devices, so we
>>> implemented the .remove function for pnp ones.
>>> Since it's code is very similar to the one inside tpm_dev_release, we've created a helper
>>> function tpm_dev_vendor_release, which is called by both.
>> Should tpm_infineon also be switched over to this?
>>
>>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>>> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
>>> ---
>>>  drivers/char/tpm/tpm.c     |   22 ++++++++++++++++------
>>>  drivers/char/tpm/tpm.h     |    1 +
>>>  drivers/char/tpm/tpm_tis.c |   14 +++++++++++++-
>>>  3 files changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>>> index 24fb7ab..ab03b4d 100644
>>> --- a/drivers/char/tpm/tpm.c
>>> +++ b/drivers/char/tpm/tpm.c
>>> @@ -1133,23 +1133,33 @@ int tpm_pm_resume(struct device *dev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(tpm_pm_resume);
>>>
>>> +/* In case vendor provided release function, call it too.*/
>>> +
>>> +void tpm_dev_vendor_release(struct tpm_chip *chip)
>>> +{
>>> +	if (chip->vendor.release)
>>> +	 	chip->vendor.release(chip->dev);
>>> +	
>>> +	clear_bit(chip->dev_num, dev_mask);
>>> +	kfree(chip->vendor.miscdev.name);
>>> +}
>>> +EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
>>> +
>>> +
>>>  /*
>>>   * Once all references to platform device are down to 0,
>>>   * release all allocated structures.
>>> - * In case vendor provided release function, call it too.
>>>   */
>>>  static void tpm_dev_release(struct device *dev)
>>>  {
>>>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>>>
>>> -	if (chip->vendor.release)
>>> -		chip->vendor.release(dev);
>>> -	chip->release(dev);
>>> +	tpm_dev_vendor_release(chip);
>>>
>>> -	clear_bit(chip->dev_num, dev_mask);
>>> -	kfree(chip->vendor.miscdev.name);
>>> +	chip->release(dev);
>>>  	kfree(chip);
>>>  }
>>> +EXPORT_SYMBOL_GPL(tpm_dev_release);
>>>
>>>  /*
>>>   * Called from tpm_<specific>.c probe function only for devices 
>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>> index 2756cab..8e30df4 100644
>>> --- a/drivers/char/tpm/tpm.h
>>> +++ b/drivers/char/tpm/tpm.h
>>> @@ -132,6 +132,7 @@ extern struct tpm_chip* tpm_register_hardware(struct device *,
>>>  				 const struct tpm_vendor_specific *);
>>>  extern int tpm_open(struct inode *, struct file *);
>>>  extern int tpm_release(struct inode *, struct file *);
>>> +extern void tpm_dev_vendor_release(struct tpm_chip *);
>>>  extern ssize_t tpm_write(struct file *, const char __user *, size_t,
>>>  			 loff_t *);
>>>  extern ssize_t tpm_read(struct file *, char __user *, size_t, loff_t *);
>>> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
>>> index ed1879c..3491d70 100644
>>> --- a/drivers/char/tpm/tpm_tis.c
>>> +++ b/drivers/char/tpm/tpm_tis.c
>>> @@ -630,12 +630,23 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
>>>  	{"", 0}			/* Terminator */
>>>  };
>>>
>>> +static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
>>> +{
>>> +	struct tpm_chip *chip = pnp_get_drvdata(dev);
>>> +	
>>> +	tpm_dev_vendor_release(chip);
>>> +
>>> +	kfree(chip);
>>> +}
>>> +
>>> +
>>>  static struct pnp_driver tis_pnp_driver = {
>>>  	.name = "tpm_tis",
>>>  	.id_table = tpm_pnp_tbl,
>>>  	.probe = tpm_tis_pnp_init,
>>>  	.suspend = tpm_tis_pnp_suspend,
>>>  	.resume = tpm_tis_pnp_resume,
>>> +	.remove = tpm_tis_pnp_remove,
>>>  };
>>>
>>>  #define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
>>> @@ -683,6 +694,7 @@ static void __exit cleanup_tis(void)
>>>  	spin_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.
>>> @@ -694,9 +706,9 @@ static void __exit cleanup_tis(void)
>>>  			free_irq(chip->vendor.irq, chip);
>>>  		iounmap(i->iobase);
>>>  		list_del(&i->list);
>>> -		tpm_remove_hardware(chip->dev);
>>>  	}
>>>  	spin_unlock(&tis_lock);
>>> +	
>>>  	if (force) {
>>>  		platform_device_unregister(pdev);
>>>  		driver_unregister(&tis_drv);
>>> -- 
>>> 1.5.6.3
> 
> 

[PATCH 5/5] TPM: Fixed tpm_release() timing

[Rajiv Andrade]

As pointed out by Jonathan Corbet, the timer must be deleted before flushing
the work queue in order to avoid a job being submitted after 
the chip had been released.

Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
CC: Jonathan Corbet <corbet@lwn.net>
---
 drivers/char/tpm/tpm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ab03b4d..8d090d3 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1004,9 +1004,9 @@ int tpm_release(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip = file->private_data;
 
+	del_singleshot_timer_sync(&chip->user_read_timer);
 	flush_scheduled_work();
 	file->private_data = NULL;
-	del_singleshot_timer_sync(&chip->user_read_timer);
 	atomic_set(&chip->data_pending, 0);
 	kfree(chip->data_buffer);
 	clear_bit(0, &chip->is_open);
-- 
1.5.6.3