[PATCH 0/4] BKL removed, RCU addition and pnp remove

[PATCH 0/4] BKL removed, RCU addition and pnp remove

[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

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

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

[PATCH 1/4] 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/4] 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/4] 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 |   58 ++++++++++++++++++-----------------------------
 1 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index dfd4e7f..ac8982e 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -961,33 +961,27 @@ 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;
+		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 +993,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 +1083,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 +1135,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 +1143,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 +1157,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 +1217,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 +1226,11 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	chip->bios_dir = tpm_bios_log_setup(devname);
 
+	/* Make chip available */
+	spin_lock(&driver_lock);
+	list_add(&chip->list, &tpm_chip_list);
+	spin_unlock(&driver_lock);
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
-- 
1.5.6.3

[PATCH 4/4] 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 ac8982e..a771ceb 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1132,23 +1132,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

about kernel_physical_mapping_init

[Joilnen Leite]

I wondering if inside kernel_physical_mapping_init, we could put the line
  pmd = one_md_table_init(pgd);
after
  if (pfn >= max_low_pfn)
    continue;
and if this could to be a improve, one time it dont go to one_md_table_init in pfn >= max_low_pfn case

Joilnen


      Novos endereços, o Yahoo! que você conhece. Crie um email novo com a sua cara @ymail.com ou @rocketmail.com.
http://br.new.mail.yahoo.com/addresses

Re: [PATCH 3/4] TPM: rcu locking

[Paul E. McKenney]

On Tue, Sep 23, 2008 at 02:19:28PM -0300, Rajiv Andrade wrote:
> 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>

Question below...

> ---
>  drivers/char/tpm/tpm.c |   58 ++++++++++++++++++-----------------------------
>  1 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index dfd4e7f..ac8982e 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -961,33 +961,27 @@ 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) {

Here we are traversing starting at a global tpm_chip_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;
> +		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 +993,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 +1083,11 @@ void tpm_remove_hardware(struct device *dev)
>  	}
> 
>  	spin_lock(&driver_lock);
> -
> -	list_del(&chip->list);
> -
> +	list_del_rcu(&chip->list);

But here we are deleting from what appears to be some other list.
And I don't see any insertiong into either list.

What am I missing here?

							Thanx, Paul

>  	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 +1135,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 +1143,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 +1157,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 +1217,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 +1226,11 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> 
>  	chip->bios_dir = tpm_bios_log_setup(devname);
> 
> +	/* Make chip available */
> +	spin_lock(&driver_lock);
> +	list_add(&chip->list, &tpm_chip_list);
> +	spin_unlock(&driver_lock);
> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);
> -- 
> 1.5.6.3
> 

Re: [PATCH 3/4] TPM: rcu locking

[Rajiv Andrade]

Paul,

On Tue, 2008-09-23 at 11:19 -0700, Paul E. McKenney wrote:
> 
> But here we are deleting from what appears to be some other list.
> And I don't see any insertiong into either list.
> 
> What am I missing here?
> 
>                                                         Thanx, Paul

Sorry, forgot to change list_add() to list_add_rcu() in the code section
below:

> > +     /* Make chip available */
> > +     spin_lock(&driver_lock);
> > +     list_add(&chip->list, &tpm_chip_list);
> > +     spin_unlock(&driver_lock);

I'll resubmit.

Thanks,
Rajiv Andrade
IBM Linux Technology Center
Security Dev.

[PATCH 3/4][resubmit] 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 |   58 ++++++++++++++++++-----------------------------
 1 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index dfd4e7f..ac8982e 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -961,33 +961,27 @@ 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;
+		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 +993,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 +1083,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 +1135,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 +1143,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 +1157,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 +1217,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 +1226,11 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
 
 	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

Re: [PATCH 3/4] TPM: rcu locking

[Paul E. McKenney]

On Tue, Sep 23, 2008 at 05:18:17PM -0300, Rajiv Andrade wrote:
> Paul,
> 
> On Tue, 2008-09-23 at 11:19 -0700, Paul E. McKenney wrote:
> > 
> > But here we are deleting from what appears to be some other list.
> > And I don't see any insertiong into either list.
> > 
> > What am I missing here?
> > 
> >                                                         Thanx, Paul
> 
> Sorry, forgot to change list_add() to list_add_rcu() in the code section
> below:
> 
> > > +     /* Make chip available */
> > > +     spin_lock(&driver_lock);
> > > +     list_add(&chip->list, &tpm_chip_list);
> > > +     spin_unlock(&driver_lock);
> 
> I'll resubmit.

Cool!

So tpm_chip_list and the not-obviously-identical list manipulated
in tpm_remove_hardware() really are the same list?

							Thanx, Paul

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

[Jonathan Corbet]

On Tue, 23 Sep 2008 14:19:26 -0300
Rajiv Andrade <srajiv@linux.vnet.ibm.com> wrote:

> + * It's assured that the chip will be opened just once,
> + * by the check of is_open variable, which is protected
> + * by driver_lock.

Taking a look at the code, I'm convinced.  BKL removal seems
appropriate.

While I was in the neighborhood, though, something caught my eye:

int tpm_release(struct inode *inode, struct file *file)
{
	struct tpm_chip *chip = file->private_data;

	flush_scheduled_work();

Here you have waited until you've got nothing in the workqueue.

	spin_lock(&driver_lock);
	file->private_data = NULL;
	del_singleshot_timer_sync(&chip->user_read_timer);

But, until you get here, your timer could have resubmitted a job into
the workqueue - job which could run after you've freed "chip" and
forgotten all about it. I think you need either a "don't resubmit" flag,
or you need to delete the timer first.

jon

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

[Jonathan Corbet]

On Tue, 23 Sep 2008 17:19:33 -0300
Rajiv Andrade <srajiv@linux.vnet.ibm.com> wrote:

> Protects tpm_chip_list when transversing it.

It all looks like it should work (though it might make sense to include
<linux/rcupdate.h> and <linux/rculist.h>.  But I have to ask: do you
really need the added complexity of RCU here?  I suspect that TPM
devices don't come and go very often...once most of us have ripped it
out, we tend to leave it out...:)

jon

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

[Mimi Zohar]

On Tue, 2008-09-23 at 15:01 -0600, Jonathan Corbet wrote:
> On Tue, 23 Sep 2008 17:19:33 -0300
> Rajiv Andrade <srajiv@linux.vnet.ibm.com> wrote:
> 
> > Protects tpm_chip_list when transversing it.
> 
> It all looks like it should work (though it might make sense to include
> <linux/rcupdate.h> and <linux/rculist.h>.  But I have to ask: do you
> really need the added complexity of RCU here?  I suspect that TPM
> devices don't come and go very often...once most of us have ripped it
> out, we tend to leave it out...:)
> 
> jon

True, the RCU locking is overkill for the current TPM code. It will be
used in the integrity-tpm-internal-interface patch to resolve the
locking issue that Christoph Helwig noted for the case when the TPM
driver is built as a module, which it shouldn't be, instead of being
built-in.

Mimi Zohar

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

[Rajiv Andrade]

Reading the code again, we noticed that there's a put_device() missing
right inside this test:

>  	if (test_and_set_bit(0, &chip->is_open)) {
>  		dev_dbg(chip->dev, "Another process owns this TPM\n");
> -		rc = -EBUSY;
> -		goto err_out;
> +		return -EBUSY;
>  	}
> 
Since this was moved to a section before.
> -	get_device(chip->dev);
> -

I'll submit a patch to be applied on top of this one.

Rajiv Andrade
IBM Linux Technology Center
Security Development

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

[Rajiv Andrade]

On Tue, 2008-09-23 at 14:58 -0600, Jonathan Corbet wrote:
> On Tue, 23 Sep 2008 14:19:26 -0300
> Rajiv Andrade <srajiv@linux.vnet.ibm.com> wrote:
> 
> > + * It's assured that the chip will be opened just once,
> > + * by the check of is_open variable, which is protected
> > + * by driver_lock.
> 
> Taking a look at the code, I'm convinced.  BKL removal seems
> appropriate.
> 
> While I was in the neighborhood, though, something caught my eye:
> 
> int tpm_release(struct inode *inode, struct file *file)
> {
> 	struct tpm_chip *chip = file->private_data;
> 
> 	flush_scheduled_work();
> 
> Here you have waited until you've got nothing in the workqueue.
> 
> 	spin_lock(&driver_lock);
> 	file->private_data = NULL;
> 	del_singleshot_timer_sync(&chip->user_read_timer);
> 
> But, until you get here, your timer could have resubmitted a job into
> the workqueue - job which could run after you've freed "chip" and
> forgotten all about it. I think you need either a "don't resubmit" flag,
> or you need to delete the timer first.
> 
> jon

Yes, like in tpm_read(), the timer must be deleted before
flush_scheduled_work(). Since it's a fix to a new issue, I'm going to
submit a another patch.

Thanks,
Rajiv Andrade
IBM Linux Technology Center
Security Development

Re: [PATCH 3/4] 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 |   58 ++++++++++++++++++-----------------------------
>  1 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index dfd4e7f..ac8982e 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -961,33 +961,27 @@ 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;

but don't you need to 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 +993,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);

And so chip->data_buffer (and data_pending) may *only* be accessed by
the owner of the chip, who has set chip->is_open?

Could you comment that in tpm.h for future reference?  I've been trying
to push for better commenting of precisely how the locking is expected
to work, but I think that was misunderstood as asking for email
explanations, when I really want it commented in the code.

>  	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 +1083,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 +1135,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 +1143,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 +1157,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 +1217,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 +1226,11 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> 
>  	chip->bios_dir = tpm_bios_log_setup(devname);
> 
> +	/* Make chip available */
> +	spin_lock(&driver_lock);
> +	list_add(&chip->list, &tpm_chip_list);
> +	spin_unlock(&driver_lock);
> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);
> -- 
> 1.5.6.3

Re: [PATCH 3/4] TPM: rcu locking

[Serge E. Hallyn]

Quoting Paul E. McKenney (paulmck@linux.vnet.ibm.com):
> On Tue, Sep 23, 2008 at 05:18:17PM -0300, Rajiv Andrade wrote:
> > Paul,
> > 
> > On Tue, 2008-09-23 at 11:19 -0700, Paul E. McKenney wrote:
> > > 
> > > But here we are deleting from what appears to be some other list.
> > > And I don't see any insertiong into either list.
> > > 
> > > What am I missing here?
> > > 
> > >                                                         Thanx, Paul
> > 
> > Sorry, forgot to change list_add() to list_add_rcu() in the code section
> > below:
> > 
> > > > +     /* Make chip available */
> > > > +     spin_lock(&driver_lock);
> > > > +     list_add(&chip->list, &tpm_chip_list);
> > > > +     spin_unlock(&driver_lock);
> > 
> > I'll resubmit.
> 
> Cool!
> 
> So tpm_chip_list and the not-obviously-identical list manipulated
> in tpm_remove_hardware() really are the same list?
> 
> 							Thanx, Paul

Hey Paul,

curious, why do they not look like the same list?

-serge

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

[Serge E. Hallyn]

Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com):
> Reading the code again, we noticed that there's a put_device() missing
> right inside this test:

Oops.  So ignore my other comment  :)

Sorry, I'm behind on some email.

-serge

> >  	if (test_and_set_bit(0, &chip->is_open)) {
> >  		dev_dbg(chip->dev, "Another process owns this TPM\n");
> > -		rc = -EBUSY;
> > -		goto err_out;
> > +		return -EBUSY;
> >  	}
> > 
> Since this was moved to a section before.
> > -	get_device(chip->dev);
> > -
> 
> I'll submit a patch to be applied on top of this one.
> 
> Rajiv Andrade
> IBM Linux Technology Center
> Security Development
> 

Re: [PATCH 3/4] TPM: rcu locking

[Paul E. McKenney]

On Thu, Sep 25, 2008 at 08:36:45AM -0500, Serge E. Hallyn wrote:
> Quoting Paul E. McKenney (paulmck@linux.vnet.ibm.com):
> > On Tue, Sep 23, 2008 at 05:18:17PM -0300, Rajiv Andrade wrote:
> > > Paul,
> > > 
> > > On Tue, 2008-09-23 at 11:19 -0700, Paul E. McKenney wrote:
> > > > 
> > > > But here we are deleting from what appears to be some other list.
> > > > And I don't see any insertiong into either list.
> > > > 
> > > > What am I missing here?
> > > > 
> > > >                                                         Thanx, Paul
> > > 
> > > Sorry, forgot to change list_add() to list_add_rcu() in the code section
> > > below:
> > > 
> > > > > +     /* Make chip available */
> > > > > +     spin_lock(&driver_lock);
> > > > > +     list_add(&chip->list, &tpm_chip_list);
> > > > > +     spin_unlock(&driver_lock);
> > > 
> > > I'll resubmit.
> > 
> > Cool!
> > 
> > So tpm_chip_list and the not-obviously-identical list manipulated
> > in tpm_remove_hardware() really are the same list?
> > 
> > 							Thanx, Paul
> 
> Hey Paul,
> 
> curious, why do they not look like the same list?

Because one of them is named &tpm_chip_list, a global variable, and the
other seemed to be returned from a function taking a struct device as an
argument.  This is indeed consistent with an element in this list being
hung off of the struct device, so perhaps I was just being insufficiently
persistent in tracking things down.

							Thanx, Paul

Re: [PATCH 3/4] TPM: rcu locking

[Serge E. Hallyn]

Quoting Paul E. McKenney (paulmck@linux.vnet.ibm.com):
> On Thu, Sep 25, 2008 at 08:36:45AM -0500, Serge E. Hallyn wrote:
> > Quoting Paul E. McKenney (paulmck@linux.vnet.ibm.com):
> > > On Tue, Sep 23, 2008 at 05:18:17PM -0300, Rajiv Andrade wrote:
> > > > Paul,
> > > > 
> > > > On Tue, 2008-09-23 at 11:19 -0700, Paul E. McKenney wrote:
> > > > > 
> > > > > But here we are deleting from what appears to be some other list.
> > > > > And I don't see any insertiong into either list.
> > > > > 
> > > > > What am I missing here?
> > > > > 
> > > > >                                                         Thanx, Paul
> > > > 
> > > > Sorry, forgot to change list_add() to list_add_rcu() in the code section
> > > > below:
> > > > 
> > > > > > +     /* Make chip available */
> > > > > > +     spin_lock(&driver_lock);
> > > > > > +     list_add(&chip->list, &tpm_chip_list);
> > > > > > +     spin_unlock(&driver_lock);
> > > > 
> > > > I'll resubmit.
> > > 
> > > Cool!
> > > 
> > > So tpm_chip_list and the not-obviously-identical list manipulated
> > > in tpm_remove_hardware() really are the same list?
> > > 
> > > 							Thanx, Paul
> > 
> > Hey Paul,
> > 
> > curious, why do they not look like the same list?
> 
> Because one of them is named &tpm_chip_list, a global variable, and the
> other seemed to be returned from a function taking a struct device as an
> argument.  This is indeed consistent with an element in this list being
> hung off of the struct device, so perhaps I was just being insufficiently
> persistent in tracking things down.
> 
> 							Thanx, Paul

Right, tpm_register_hardware both does
	dev_set_drvdata(dev, chip)
to set dev->driver_data = chip, and list_add(chip->list, &tpm_chip_list).
The tpm_remove_hardware(), then, gets the device, gets the chip from
dev->driver_data,  and removes it from the tpm_chip_list.

It does look kosher.

Though once this is all applied to some public git tree, I want to take
some time to look at the full result.

-serge