Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755072AbaJGSE0 (ORCPT ); Tue, 7 Oct 2014 14:04:26 -0400 Received: from mga14.intel.com ([192.55.52.115]:65081 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753176AbaJGSEY (ORCPT ); Tue, 7 Oct 2014 14:04:24 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,672,1406617200"; d="scan'208";a="482590984" Date: Tue, 7 Oct 2014 21:04:17 +0300 From: Jarkko Sakkinen To: Jason Gunthorpe Cc: Peter Huewe , Ashley Lai , Marcel Selhorst , linux-api@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [tpmdd-devel] [PATCH v2 2/7] tpm: two-phase chip management functions Message-ID: <20141007180417.GA29459@intel.com> References: <1412701277-27794-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1412701277-27794-3-git-send-email-jarkko.sakkinen@linux.intel.com> <20141007175017.GA10432@obsidianresearch.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141007175017.GA10432@obsidianresearch.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi And thanks for the feedback. Change requests look very reasonable. On Tue, Oct 07, 2014 at 11:50:17AM -0600, Jason Gunthorpe wrote: > On Tue, Oct 07, 2014 at 08:01:12PM +0300, Jarkko Sakkinen wrote: > > Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc() > > reserves memory resources and tpm_chip_register() initializes the > > device driver. This way it is possible to alter struct tpm_chip > > attributes before passing it to tpm_chip_register(). > > This looks broadly reasonable to me > > Please add a note to the commit that this is known to still have > problems with resource reference counting, but they are less severe > than what existed before, and this is only an interm step. > > > +/** > > + * tpm_chip_alloc() - allocate a new struct tpm_chip instance > > This is using devm so it should be called 'tpmm_chip_alloc()' for > clarity > > > I know that was there before, but it sure is racy: > > > + chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES); > [..] > > + set_bit(chip->dev_num, dev_mask); > > Someday it should use IDR. > > > > @@ -896,18 +872,7 @@ void tpm_remove_hardware(struct device *dev) > > return; > > } > > > > - spin_lock(&driver_lock); > > - list_del_rcu(&chip->list); > > - spin_unlock(&driver_lock); > > - synchronize_rcu(); > > - > > - tpm_dev_del_device(chip); > > - tpm_sysfs_del_device(chip); > > - tpm_remove_ppi(&dev->kobj); > > - tpm_bios_log_teardown(chip->bios_dir); > > - > > - /* write it this way to be explicit (chip->dev == dev) */ > > - put_device(chip->dev); > > + tpm_chip_unregister(chip); > > } > > EXPORT_SYMBOL_GPL(tpm_remove_hardware); > > This can move to tpm-chip too, same with tpm_register_hardware > > > @@ -714,15 +709,10 @@ static int tpm_tis_i2c_remove(struct i2c_client *client) > > struct tpm_chip *chip = tpm_dev.chip; > > release_locality(chip, chip->vendor.locality, 1); > > > > - /* close file handles */ > > - tpm_dev_vendor_release(chip); > > - > > /* remove hardware */ > > tpm_remove_hardware(chip->dev); > > Wrong ordering here, tpm_remove_hardware should always be first - > drivers should not tear down internal state before calling it, so > release_locality should be second. > > Noting that since we use devm the kfree will not happen until > remove returns, so the chip pointer is still valid. > > > /* reset these pointers, otherwise we oops */ > > - chip->dev->release = NULL; > > - chip->release = NULL; > > tpm_dev.client = NULL; > > The comment can go too > > Note: tpm_dev should be driver private data, but that is not your > problem.. > > Did you test compile all the drivers? One of my git commits on github > has some hackery to make that possible on x86. Yeah, I compiled all the drivers: $ grep CONFIG_TCG .config CONFIG_TCG_TPM=m CONFIG_TCG_TIS=m CONFIG_TCG_TIS_I2C_ATMEL=m CONFIG_TCG_TIS_I2C_INFINEON=m CONFIG_TCG_TIS_I2C_NUVOTON=m CONFIG_TCG_NSC=m CONFIG_TCG_ATMEL=m CONFIG_TCG_INFINEON=m CONFIG_TCG_ST33_I2C=m CONFIG_TCG_XEN=m CONFIG_TCG_CRB=m > Jason /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/