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