[tpmdd-devel,v6,06/11] tpm: Replace device number bitmap with IDR
diff mbox

Message ID 1457545170-30120-7-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger March 9, 2016, 5:39 p.m. UTC
Replace the device number bitmap with IDR. Extend the number of devices we
can create to 64k.
Since an IDR allows us to associate a pointer with an ID, we use this now
to rewrite tpm_chip_find_get() to simply look up the chip pointer by the
given device ID.

Protect the IDR calls with a mutex.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c      | 84 +++++++++++++++++++++-------------------
 drivers/char/tpm/tpm-interface.c |  1 +
 drivers/char/tpm/tpm.h           |  5 +--
 3 files changed, 48 insertions(+), 42 deletions(-)

Comments

Jarkko Sakkinen March 10, 2016, 1:21 p.m. UTC | #1
Hi Stefan

On Wed, Mar 09, 2016 at 12:39:25PM -0500, Stefan Berger wrote:
> Replace the device number bitmap with IDR. Extend the number of devices we
> can create to 64k.
> Since an IDR allows us to associate a pointer with an ID, we use this now
> to rewrite tpm_chip_find_get() to simply look up the chip pointer by the
> given device ID.
> 
> Protect the IDR calls with a mutex.

Could you make a separate patch set of first six patches and CC them to
linux-kernel@vger.kernel.org and linux-security-module@vger.kernel.org.
They are obvious improvements and could be soon merged to 'next' but
they do need more wider review first.

This also make reviewers easier as the first six patches kind of form
separate entity from remaining four patches.

For the patch that takes the module lock away it would be a good idea
to better document the behavioral change.

/Jarkko


> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-chip.c      | 84 +++++++++++++++++++++-------------------
>  drivers/char/tpm/tpm-interface.c |  1 +
>  drivers/char/tpm/tpm.h           |  5 +--
>  3 files changed, 48 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 5880377..f62c851 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -29,9 +29,8 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
> -static LIST_HEAD(tpm_chip_list);
> -static DEFINE_SPINLOCK(driver_lock);
> +DEFINE_IDR(dev_nums_idr);
> +static DEFINE_MUTEX(idr_lock);
>  
>  struct class *tpm_class;
>  dev_t tpm_devt;
> @@ -88,20 +87,30 @@ EXPORT_SYMBOL_GPL(tpm_put_ops);
>    */
>  struct tpm_chip *tpm_chip_find_get(int chip_num)
>  {
> -	struct tpm_chip *pos, *chip = NULL;
> +	struct tpm_chip *chip, *res = NULL;
> +	int chip_prev;
> +
> +	mutex_lock(&idr_lock);
> +
> +	if (chip_num == TPM_ANY_NUM) {
> +		chip_num = 0;
> +		do {
> +			chip_prev = chip_num;
> +			chip = idr_get_next(&dev_nums_idr, &chip_num);
> +			if (chip && !tpm_try_get_ops(chip)) {
> +				res = chip;
> +				break;
> +			}
> +		} while (chip_prev != chip_num);
> +	} else {
> +		chip = idr_find_slowpath(&dev_nums_idr, chip_num);
> +		if (chip && !tpm_try_get_ops(chip))
> +			res = chip;
> +	}
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
> -		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
> -			continue;
> +	mutex_unlock(&idr_lock);
>  
> -		/* rcu prevents chip from being free'd */
> -		if (!tpm_try_get_ops(pos))
> -			chip = pos;
> -		break;
> -	}
> -	rcu_read_unlock();
> -	return chip;
> +	return res;
>  }
>  
>  /**
> @@ -114,9 +123,10 @@ static void tpm_dev_release(struct device *dev)
>  {
>  	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
>  
> -	spin_lock(&driver_lock);
> -	clear_bit(chip->dev_num, dev_mask);
> -	spin_unlock(&driver_lock);
> +	mutex_lock(&idr_lock);
> +	idr_remove(&dev_nums_idr, chip->dev_num);
> +	mutex_unlock(&idr_lock);
> +
>  	kfree(chip);
>  }
>  
> @@ -142,21 +152,18 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  
>  	mutex_init(&chip->tpm_mutex);
>  	init_rwsem(&chip->ops_sem);
> -	INIT_LIST_HEAD(&chip->list);
>  
>  	chip->ops = ops;
>  
> -	spin_lock(&driver_lock);
> -	chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
> -	spin_unlock(&driver_lock);
> -
> -	if (chip->dev_num >= TPM_NUM_DEVICES) {
> +	mutex_lock(&idr_lock);
> +	rc = idr_alloc(&dev_nums_idr, NULL, 0, TPM_NUM_DEVICES, GFP_KERNEL);
> +	mutex_unlock(&idr_lock);
> +	if (rc < 0) {
>  		dev_err(dev, "No available tpm device numbers\n");
>  		kfree(chip);
> -		return ERR_PTR(-ENOMEM);
> +		return ERR_PTR(rc);
>  	}
> -
> -	set_bit(chip->dev_num, dev_mask);
> +	chip->dev_num = rc;
>  
>  	device_initialize(&chip->dev);
>  
> @@ -242,19 +249,28 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	/* Make the chip available. */
> +	mutex_lock(&idr_lock);
> +	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> +	mutex_unlock(&idr_lock);
> +
>  	return rc;
>  }
>  
>  static void tpm_del_char_device(struct tpm_chip *chip)
>  {
>  	cdev_del(&chip->cdev);
> +	device_del(&chip->dev);
> +
> +	/* Make the chip unavailable. */
> +	mutex_lock(&idr_lock);
> +	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> +	mutex_unlock(&idr_lock);
>  
>  	/* Make the driver uncallable. */
>  	down_write(&chip->ops_sem);
>  	chip->ops = NULL;
>  	up_write(&chip->ops_sem);
> -
> -	device_del(&chip->dev);
>  }
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
> @@ -309,11 +325,6 @@ int tpm_chip_register(struct tpm_chip *chip)
>  	if (rc)
>  		goto out_err;
>  
> -	/* Make the chip available. */
> -	spin_lock(&driver_lock);
> -	list_add_tail_rcu(&chip->list, &tpm_chip_list);
> -	spin_unlock(&driver_lock);
> -
>  	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
>  
>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> @@ -350,11 +361,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
>  		return;
>  
> -	spin_lock(&driver_lock);
> -	list_del_rcu(&chip->list);
> -	spin_unlock(&driver_lock);
> -	synchronize_rcu();
> -
>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>  		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5caf154..5397b64 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1139,6 +1139,7 @@ static int __init tpm_init(void)
>  
>  static void __exit tpm_exit(void)
>  {
> +	idr_destroy(&dev_nums_idr);
>  	class_destroy(tpm_class);
>  	unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
>  }
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 5fcf788..928b47f 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -34,7 +34,7 @@
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
>  	TPM_BUFSIZE = 4096,
> -	TPM_NUM_DEVICES = 256,
> +	TPM_NUM_DEVICES = 65536,
>  	TPM_RETRY = 50,		/* 5 seconds */
>  };
>  
> @@ -195,8 +195,6 @@ struct tpm_chip {
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>  #endif /* CONFIG_ACPI */
> -
> -	struct list_head list;
>  };
>  
>  #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
> @@ -492,6 +490,7 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
>  extern struct class *tpm_class;
>  extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
> +extern struct idr dev_nums_idr;
>  
>  ssize_t	tpm_getcap(struct device *, __be32, cap_t *, const char *);
>  ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -- 
> 2.4.3
> 

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
Stefan Berger March 10, 2016, 4:26 p.m. UTC | #2
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 03/10/2016 
08:21:56 AM:

> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> To: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Cc: tpmdd-devel@lists.sourceforge.net
> Date: 03/10/2016 08:22 AM
> Subject: Re: [tpmdd-devel] [PATCH v6 06/11] tpm: Replace device 
> number bitmap with IDR
> 
> Hi Stefan
> 
> On Wed, Mar 09, 2016 at 12:39:25PM -0500, Stefan Berger wrote:
> > Replace the device number bitmap with IDR. Extend the number of 
devices we
> > can create to 64k.
> > Since an IDR allows us to associate a pointer with an ID, we use this 
now
> > to rewrite tpm_chip_find_get() to simply look up the chip pointer by 
the
> > given device ID.
> > 
> > Protect the IDR calls with a mutex.
> 
> Could you make a separate patch set of first six patches and CC them to
> linux-kernel@vger.kernel.org and linux-security-module@vger.kernel.org.

I'd rather post v7 of the whole series so that we don't have too many 
different series flying around. I can add a comment into the cover letter 
to request particular care for patches 1 to 6 or 7?

   Stefan
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
Jarkko Sakkinen March 10, 2016, 4:40 p.m. UTC | #3
On Thu, Mar 10, 2016 at 11:26:38AM -0500, Stefan Berger wrote:
>    Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 03/10/2016
>    08:21:56 AM:
> 
>    > From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>    > To: Stefan Berger <stefanb@linux.vnet.ibm.com>
>    > Cc: tpmdd-devel@lists.sourceforge.net
>    > Date: 03/10/2016 08:22 AM
>    > Subject: Re: [tpmdd-devel] [PATCH v6 06/11] tpm: Replace device
>    > number bitmap with IDR
>    >
>    > Hi Stefan
>    >
>    > On Wed, Mar 09, 2016 at 12:39:25PM -0500, Stefan Berger wrote:
>    > > Replace the device number bitmap with IDR. Extend the number of
>    devices we
>    > > can create to 64k.
>    > > Since an IDR allows us to associate a pointer with an ID, we use this
>    now
>    > > to rewrite tpm_chip_find_get() to simply look up the chip pointer by
>    the
>    > > given device ID.
>    > >
>    > > Protect the IDR calls with a mutex.
>    >
>    > Could you make a separate patch set of first six patches and CC them to
>    > linux-kernel@vger.kernel.org and linux-security-module@vger.kernel.org.
> 
>    I'd rather post v7 of the whole series so that we don't have too many
>    different series flying around. I can add a comment into the cover letter
>    to request particular care for patches 1 to 6 or 7?

Makes sense! It doesn't prevent me merging the 'head' of the series :)

>       Stefan

/Jarkko

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 5880377..f62c851 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -29,9 +29,8 @@ 
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
-static LIST_HEAD(tpm_chip_list);
-static DEFINE_SPINLOCK(driver_lock);
+DEFINE_IDR(dev_nums_idr);
+static DEFINE_MUTEX(idr_lock);
 
 struct class *tpm_class;
 dev_t tpm_devt;
@@ -88,20 +87,30 @@  EXPORT_SYMBOL_GPL(tpm_put_ops);
   */
 struct tpm_chip *tpm_chip_find_get(int chip_num)
 {
-	struct tpm_chip *pos, *chip = NULL;
+	struct tpm_chip *chip, *res = NULL;
+	int chip_prev;
+
+	mutex_lock(&idr_lock);
+
+	if (chip_num == TPM_ANY_NUM) {
+		chip_num = 0;
+		do {
+			chip_prev = chip_num;
+			chip = idr_get_next(&dev_nums_idr, &chip_num);
+			if (chip && !tpm_try_get_ops(chip)) {
+				res = chip;
+				break;
+			}
+		} while (chip_prev != chip_num);
+	} else {
+		chip = idr_find_slowpath(&dev_nums_idr, chip_num);
+		if (chip && !tpm_try_get_ops(chip))
+			res = chip;
+	}
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
-		if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num)
-			continue;
+	mutex_unlock(&idr_lock);
 
-		/* rcu prevents chip from being free'd */
-		if (!tpm_try_get_ops(pos))
-			chip = pos;
-		break;
-	}
-	rcu_read_unlock();
-	return chip;
+	return res;
 }
 
 /**
@@ -114,9 +123,10 @@  static void tpm_dev_release(struct device *dev)
 {
 	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
 
-	spin_lock(&driver_lock);
-	clear_bit(chip->dev_num, dev_mask);
-	spin_unlock(&driver_lock);
+	mutex_lock(&idr_lock);
+	idr_remove(&dev_nums_idr, chip->dev_num);
+	mutex_unlock(&idr_lock);
+
 	kfree(chip);
 }
 
@@ -142,21 +152,18 @@  struct tpm_chip *tpm_chip_alloc(struct device *dev,
 
 	mutex_init(&chip->tpm_mutex);
 	init_rwsem(&chip->ops_sem);
-	INIT_LIST_HEAD(&chip->list);
 
 	chip->ops = ops;
 
-	spin_lock(&driver_lock);
-	chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
-	spin_unlock(&driver_lock);
-
-	if (chip->dev_num >= TPM_NUM_DEVICES) {
+	mutex_lock(&idr_lock);
+	rc = idr_alloc(&dev_nums_idr, NULL, 0, TPM_NUM_DEVICES, GFP_KERNEL);
+	mutex_unlock(&idr_lock);
+	if (rc < 0) {
 		dev_err(dev, "No available tpm device numbers\n");
 		kfree(chip);
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(rc);
 	}
-
-	set_bit(chip->dev_num, dev_mask);
+	chip->dev_num = rc;
 
 	device_initialize(&chip->dev);
 
@@ -242,19 +249,28 @@  static int tpm_add_char_device(struct tpm_chip *chip)
 		return rc;
 	}
 
+	/* Make the chip available. */
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, chip, chip->dev_num);
+	mutex_unlock(&idr_lock);
+
 	return rc;
 }
 
 static void tpm_del_char_device(struct tpm_chip *chip)
 {
 	cdev_del(&chip->cdev);
+	device_del(&chip->dev);
+
+	/* Make the chip unavailable. */
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
+	mutex_unlock(&idr_lock);
 
 	/* Make the driver uncallable. */
 	down_write(&chip->ops_sem);
 	chip->ops = NULL;
 	up_write(&chip->ops_sem);
-
-	device_del(&chip->dev);
 }
 
 static int tpm1_chip_register(struct tpm_chip *chip)
@@ -309,11 +325,6 @@  int tpm_chip_register(struct tpm_chip *chip)
 	if (rc)
 		goto out_err;
 
-	/* Make the chip available. */
-	spin_lock(&driver_lock);
-	list_add_tail_rcu(&chip->list, &tpm_chip_list);
-	spin_unlock(&driver_lock);
-
 	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
 
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
@@ -350,11 +361,6 @@  void tpm_chip_unregister(struct tpm_chip *chip)
 	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
 		return;
 
-	spin_lock(&driver_lock);
-	list_del_rcu(&chip->list);
-	spin_unlock(&driver_lock);
-	synchronize_rcu();
-
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
 		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5caf154..5397b64 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1139,6 +1139,7 @@  static int __init tpm_init(void)
 
 static void __exit tpm_exit(void)
 {
+	idr_destroy(&dev_nums_idr);
 	class_destroy(tpm_class);
 	unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 5fcf788..928b47f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -34,7 +34,7 @@ 
 enum tpm_const {
 	TPM_MINOR = 224,	/* officially assigned */
 	TPM_BUFSIZE = 4096,
-	TPM_NUM_DEVICES = 256,
+	TPM_NUM_DEVICES = 65536,
 	TPM_RETRY = 50,		/* 5 seconds */
 };
 
@@ -195,8 +195,6 @@  struct tpm_chip {
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
 #endif /* CONFIG_ACPI */
-
-	struct list_head list;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
@@ -492,6 +490,7 @@  static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 extern struct class *tpm_class;
 extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
+extern struct idr dev_nums_idr;
 
 ssize_t	tpm_getcap(struct device *, __be32, cap_t *, const char *);
 ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,