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

Message ID 1455885728-10315-8-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger Feb. 19, 2016, 12:42 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>
---
 drivers/char/tpm/tpm-chip.c      | 81 +++++++++++++++++++++-------------------
 drivers/char/tpm/tpm-interface.c |  1 +
 drivers/char/tpm/tpm.h           |  5 +--
 3 files changed, 46 insertions(+), 41 deletions(-)

Comments

Jason Gunthorpe Feb. 22, 2016, 7:06 p.m. UTC | #1
On Fri, Feb 19, 2016 at 07:42:04AM -0500, Stefan Berger wrote:
> +	if (chip_num == TPM_ANY_NUM)
> +		chip_next = 0;
> +
> +	mutex_lock(&idr_lock);
> +
> +	do {
> +		if (chip_num == TPM_ANY_NUM) {
> +			chip_prev = chip_next;
> +			chip = idr_get_next(&dev_nums_idr, &chip_next);
> +		} else
> +			chip = idr_find_slowpath(&dev_nums_idr, chip_num);
> +
> +		if (chip && !tpm_try_get_ops(chip))
> +			break;
> +	} while (chip_num == TPM_ANY_NUM && chip_prev != chip_next);

This while loop doesn't look very good if tpm_try_get_ops fails?

Maybe like this?

struct tpm_chip *tpm_chip_find_get(int chip_num)
{
	struct tpm_chip *res = NULL;

	mutex_lock(&idr_lock);

	if (chip_num == TPM_ANY_NUM) {
		struct tpm_chip *chip;

		chip_num = 0;
		do {
			chip = idr_get_next(&dev_nums_idr, &chip_num);
			if (res && !tpm_try_get_ops(chip)) {
				res = chip;
				break;
			}
		}
		while (chip);
	} else {
		res = idr_find_slowpath(&dev_nums_idr, chip_num);
		if (res && tpm_try_get_ops(chip))
			res = NULL;
	}

	mutex_unlock(&idr_lock);

	return res;
}

> +	if (err < 0) {
>  		dev_err(dev, "No available tpm device numbers\n");

I would drop the dev_err too

> @@ -247,12 +253,6 @@ static int tpm_dev_add_device(struct tpm_chip *chip)
>  static void tpm_dev_del_device(struct tpm_chip *chip)
>  {
>  	cdev_del(&chip->cdev);
> -
> -	/* Make the driver uncallable. */
> -	down_write(&chip->ops_sem);
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -
>  	device_del(&chip->dev);

Hum.. The ordering here is very important, I guess I got it slightly
wrong as well in my patch adding ops.

Lets go for this:

- Tear down /dev/tpmX, sysfs and all other user accessible entry
  points
- Do device_del (get rid of all the sysfs stuff)
- NULL the IDR (disables kAPI access and allow reuse of the ID)
- NULL the ops (flush in-progress access now that new access is impossible)
- put_device to kfree (via devm)

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 23, 2016, 1:15 a.m. UTC | #2
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/22/2016 
02:06:29 PM:

> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> To: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Cc: tpmdd-devel@lists.sourceforge.net
> Date: 02/22/2016 02:06 PM
> Subject: Re: [tpmdd-devel] [PATCH v3 07/11] tpm: Replace device 
> number bitmap with IDR
> 
> On Fri, Feb 19, 2016 at 07:42:04AM -0500, Stefan Berger wrote:
> > +   if (chip_num == TPM_ANY_NUM)
> > +      chip_next = 0;
> > +
> > +   mutex_lock(&idr_lock);
> > +
> > +   do {
> > +      if (chip_num == TPM_ANY_NUM) {
> > +         chip_prev = chip_next;
> > +         chip = idr_get_next(&dev_nums_idr, &chip_next);
> > +      } else
> > +         chip = idr_find_slowpath(&dev_nums_idr, chip_num);
> > +
> > +      if (chip && !tpm_try_get_ops(chip))
> > +         break;
> > +   } while (chip_num == TPM_ANY_NUM && chip_prev != chip_next);
> 
> This while loop doesn't look very good if tpm_try_get_ops fails?
> 
> Maybe like this?
> 
> struct tpm_chip *tpm_chip_find_get(int chip_num)
> {
>    struct tpm_chip *res = NULL;
> 
>    mutex_lock(&idr_lock);
> 
>    if (chip_num == TPM_ANY_NUM) {
>       struct tpm_chip *chip;
> 
>       chip_num = 0;
>       do {
>          chip = idr_get_next(&dev_nums_idr, &chip_num);
>          if (res && !tpm_try_get_ops(chip)) {
>             res = chip;
>             break;
>          }
>       }
>       while (chip);
>    } else {
>       res = idr_find_slowpath(&dev_nums_idr, chip_num);
>       if (res && tpm_try_get_ops(chip))
>          res = NULL;
>    }
> 
>    mutex_unlock(&idr_lock);
> 
>    return res;
> }
> 
> > +   if (err < 0) {
> >        dev_err(dev, "No available tpm device numbers\n");
> 
> I would drop the dev_err too
> 
> > @@ -247,12 +253,6 @@ static int tpm_dev_add_device(struct tpm_chip 
*chip)
> >  static void tpm_dev_del_device(struct tpm_chip *chip)
> >  {
> >     cdev_del(&chip->cdev);
> > -
> > -   /* Make the driver uncallable. */
> > -   down_write(&chip->ops_sem);
> > -   chip->ops = NULL;
> > -   up_write(&chip->ops_sem);
> > -
> >     device_del(&chip->dev);
> 
> Hum.. The ordering here is very important, I guess I got it slightly
> wrong as well in my patch adding ops.
> 
> Lets go for this:
> 
> - Tear down /dev/tpmX, sysfs and all other user accessible entry
>   points
> - Do device_del (get rid of all the sysfs stuff)
> - NULL the IDR (disables kAPI access and allow reuse of the ID)
> - NULL the ops (flush in-progress access now that new access is 
impossible)

Why NULL the ops so late and allow access even after device_del? I'd do 
that at the beginning.

Also the IDR has a two-stages:
 one making the chip inaccessible on the idr : idr_replace(&dev_nums_idr, 
NULL, chip->dev_num)
 the other one by freeing the IDR for re-use : idr_remove(&dev_nums_ir, 
chip->dev_num)

> - put_device to kfree (via devm)


My propsal:

- Tear down /dev/tpmX
- NULL the IDR (disable kAPI access since chip cannot be found anymore)
- NULL the ops (flush in-progress access now that new access is 
impossible)
- Tear down sysfs and all other user accessible entry points
- put_device
- Free the chip's device number from the IDR (makes IDR re-usable; must 
happen after /dev/tpmX is released to avoid clashes

   Stefan

> 
> Jason
> 
> 
------------------------------------------------------------------------------
> Site24x7 APM Insight: Get Deep Visibility into Application Performance
> APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
> Monitor end-to-end web transactions and take corrective actions now
> Troubleshoot faster and improve end-user experience. Signup Now!
> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 23, 2016, 2:16 a.m. UTC | #3
On Mon, Feb 22, 2016 at 08:15:38PM -0500, Stefan Berger wrote:
> > - Tear down /dev/tpmX, sysfs and all other user accessible entry
> >   points
> > - Do device_del (get rid of all the sysfs stuff)
> > - NULL the IDR (disables kAPI access and allow reuse of the ID)
> > - NULL the ops (flush in-progress access now that new access is impossible)
> 
> Why NULL the ops so late and allow access even after device_del? I'd do that at
> the beginning.

It absolutely has to be done as the very last thing.

None of the sysfs code checks ops, and thus can't fail on null ops
(see the comment I added in the ops patch in tpm-sysfs). All sysfs
must be rendered inaccessible and all in-progress accesses must be
completed before we attempt to drop ops.

Since we are moving to using chip->groups to register sysfs a full
device_del seems necessary to fully fence sysfs access.

> Also the IDR has a two-stages:
>  one making the chip inaccessible on the idr : idr_replace(&dev_nums_idr, NULL,
> chip->dev_num)
>  the other one by freeing the IDR for re-use : idr_remove(&dev_nums_ir, chip->
> dev_num)

Oh, I missed that, yes, that is the way to go for the IDR part,
do the idr_remove after device_del.

If you set to null I expect that means the while loop example I gave
will need adjusting.

> - Free the chip's device number from the IDR (makes IDR re-usable; must happen
> after /dev/tpmX is released to avoid clashes

With this new arrangment the idr_remove can happen directly after
device_del and we can avoid the intermediate state of a NULL'd entry,
which seems slightly simplifying. Before there was the problem because
vtpm was using a struct device, but that is gone now.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 23, 2016, 2:16 a.m. UTC | #4
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/22/2016 
02:06:29 PM:


> 
> On Fri, Feb 19, 2016 at 07:42:04AM -0500, Stefan Berger wrote:
> > +   if (chip_num == TPM_ANY_NUM)
> > +      chip_next = 0;
> > +
> > +   mutex_lock(&idr_lock);
> > +
> > +   do {
> > +      if (chip_num == TPM_ANY_NUM) {
> > +         chip_prev = chip_next;
> > +         chip = idr_get_next(&dev_nums_idr, &chip_next);
> > +      } else
> > +         chip = idr_find_slowpath(&dev_nums_idr, chip_num);
> > +
> > +      if (chip && !tpm_try_get_ops(chip))
> > +         break;
> > +   } while (chip_num == TPM_ANY_NUM && chip_prev != chip_next);
> 
> This while loop doesn't look very good if tpm_try_get_ops fails?
> 
> Maybe like this?
> 
> struct tpm_chip *tpm_chip_find_get(int chip_num)
> {
>    struct tpm_chip *res = NULL;
> 
>    mutex_lock(&idr_lock);
> 
>    if (chip_num == TPM_ANY_NUM) {
>       struct tpm_chip *chip;
> 
>       chip_num = 0;
>       do {
>          chip = idr_get_next(&dev_nums_idr, &chip_num);
>          if (res && !tpm_try_get_ops(chip)) {
>             res = chip;
>             break;
>          }
>       }
>       while (chip);
>    } else {
>       res = idr_find_slowpath(&dev_nums_idr, chip_num);
>       if (res && tpm_try_get_ops(chip))
>          res = NULL;
>    }
> 
>    mutex_unlock(&idr_lock);
> 
>    return res;
> }

        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;
        }

        mutex_unlock(&idr_lock);



FYI: idr_get_next will return NULL for a chip->dev_num that has nothing 
(NULL) registered; we *may* find something after that again.

   Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 23, 2016, 2:18 a.m. UTC | #5
On Mon, Feb 22, 2016 at 09:16:53PM -0500, Stefan Berger wrote:

> FYI: idr_get_next will return NULL for a chip->dev_num that has nothing (NULL)
> registered; we *may* find something after that again.

Yes, I see that now, if you decide to get rid of that then simplify
the search again, but your version looks nice either way.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 23, 2016, 11:04 p.m. UTC | #6
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/22/2016 
09:16:06 PM:

> 
> On Mon, Feb 22, 2016 at 08:15:38PM -0500, Stefan Berger wrote:
> > > - Tear down /dev/tpmX, sysfs and all other user accessible entry
> > >   points
> > > - Do device_del (get rid of all the sysfs stuff)
> > > - NULL the IDR (disables kAPI access and allow reuse of the ID)
> > > - NULL the ops (flush in-progress access now that new access is 
> impossible)
> > 

> 
> > - Free the chip's device number from the IDR (makes IDR re-usable;
> must happen
> > after /dev/tpmX is released to avoid clashes
> 
> With this new arrangment the idr_remove can happen directly after
> device_del and we can avoid the intermediate state of a NULL'd entry,
> which seems slightly simplifying. Before there was the problem because
> vtpm was using a struct device, but that is gone now.

Let's stay with the 2 stage removal unrolled in the functions that are the 
opposite where the do the staged adding.

I just ran into the problem that when tpm_chip_register wasn't called due 
to me killing the process while the driver task attempted to get the 
durations, the tpm_chip_unregister didn't get called either. So we lost 
ID's in the IDR. So we allocate the id in tpm_chip_alloc and free it 
before the kfree(chip) in tpm_dev_release. We put the chip on the IDR in 
tpm_dev_add_device and remove it in tpm_dev_del_device -- nice and 
symmetric.

  Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 23, 2016, 11:18 p.m. UTC | #7
On Tue, Feb 23, 2016 at 06:04:38PM -0500, Stefan Berger wrote:

>    I just ran into the problem that when tpm_chip_register wasn't called
>    due to me killing the process while the driver task attempted to get
>    the durations, the tpm_chip_unregister didn't get called either.

Hmm. Okay. We can't move the idr alloc out of tpm_chip_alloc to combat
that, so we are stuck with the nulls.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 2270e47..4fd36ba 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,18 +87,27 @@  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;
+	int chip_next, chip_prev;
 
-	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;
+	if (chip_num == TPM_ANY_NUM)
+		chip_next = 0;
+
+	mutex_lock(&idr_lock);
+
+	do {
+		if (chip_num == TPM_ANY_NUM) {
+			chip_prev = chip_next;
+			chip = idr_get_next(&dev_nums_idr, &chip_next);
+		} else
+			chip = idr_find_slowpath(&dev_nums_idr, chip_num);
+
+		if (chip && !tpm_try_get_ops(chip))
+			break;
+	} while (chip_num == TPM_ANY_NUM && chip_prev != chip_next);
+
+	mutex_unlock(&idr_lock);
 
-		if (!tpm_try_get_ops(pos))
-			chip = pos;
-		break;
-	}
-	rcu_read_unlock();
 	return chip;
 }
 
@@ -113,9 +121,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);
 }
 
@@ -141,19 +150,16 @@  struct tpm_chip *tpm_chip_alloc(struct device *dev,
 
 	init_rwsem(&chip->ops_sem);
 	mutex_init(&chip->tpm_mutex);
-	INIT_LIST_HEAD(&chip->list);
-
-	spin_lock(&driver_lock);
-	chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
-	if (chip->dev_num < TPM_NUM_DEVICES)
-		set_bit(chip->dev_num, dev_mask);
-	spin_unlock(&driver_lock);
 
-	if (chip->dev_num >= TPM_NUM_DEVICES) {
+	mutex_lock(&idr_lock);
+	err = idr_alloc(&dev_nums_idr, NULL, 0, TPM_NUM_DEVICES, GFP_KERNEL);
+	mutex_unlock(&idr_lock);
+	if (err < 0) {
 		dev_err(dev, "No available tpm device numbers\n");
 		kfree(chip);
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(err);
 	}
+	chip->dev_num = err;
 
 	chip->ops = ops;
 
@@ -247,12 +253,6 @@  static int tpm_dev_add_device(struct tpm_chip *chip)
 static void tpm_dev_del_device(struct tpm_chip *chip)
 {
 	cdev_del(&chip->cdev);
-
-	/* Make the driver uncallable. */
-	down_write(&chip->ops_sem);
-	chip->ops = NULL;
-	up_write(&chip->ops_sem);
-
 	device_del(&chip->dev);
 }
 
@@ -312,9 +312,9 @@  int tpm_chip_register(struct tpm_chip *chip)
 		goto out_err;
 
 	/* Make the chip available. */
-	spin_lock(&driver_lock);
-	list_add_tail_rcu(&chip->list, &tpm_chip_list);
-	spin_unlock(&driver_lock);
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, chip, chip->dev_num);
+	mutex_unlock(&idr_lock);
 
 	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
 
@@ -349,10 +349,15 @@  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();
+	/* Make the driver uncallable. */
+	down_write(&chip->ops_sem);
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
+
+	/* hide the chip now */
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
+	mutex_unlock(&idr_lock);
 
 	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 1dfe2ce..f2ae217 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1121,6 +1121,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 25efe8f..2ca5fb4 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 */
 };
 
@@ -199,8 +199,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)
@@ -496,6 +494,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,