lkml.org 
[lkml]   [2008]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] TPM: rcu locking
    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


    \
     
     \ /
      Last update: 2008-09-25 15:33    [W:0.034 / U:29.984 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site