From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754601AbYIYNaA (ORCPT ); Thu, 25 Sep 2008 09:30:00 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753379AbYIYN3v (ORCPT ); Thu, 25 Sep 2008 09:29:51 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:37200 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753035AbYIYN3u (ORCPT ); Thu, 25 Sep 2008 09:29:50 -0400 Date: Thu, 25 Sep 2008 08:29:32 -0500 From: "Serge E. Hallyn" To: Rajiv Andrade Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, jmoriss@namei.org, serue@linux.vnet.ibm.com, zohar@linux.vnet.ibm.com, safford@watson.ibm.com, paulmck@linux.vnet.ibm.com, debora@linux.vnet.ibm.com Subject: Re: [PATCH 3/4] TPM: rcu locking Message-ID: <20080925132932.GA13414@us.ibm.com> References: <1222190371-23814-3-git-send-email-srajiv@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1222190371-23814-3-git-send-email-srajiv@linux.vnet.ibm.com> User-Agent: Mutt/1.5.17+20080114 (2008-01-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Rajiv Andrade (srajiv@linux.vnet.ibm.com): > Protects tpm_chip_list when transversing it. > > Signed-off-by: Mimi Zohar > Signed-off-by: Rajiv Andrade > > --- > 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