| Message ID | 1455885728-10315-8-git-send-email-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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,
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(-)