Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753889AbaJVRQO (ORCPT ); Wed, 22 Oct 2014 13:16:14 -0400 Received: from quartz.orcorp.ca ([184.70.90.242]:42241 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750930AbaJVRQM (ORCPT ); Wed, 22 Oct 2014 13:16:12 -0400 Date: Wed, 22 Oct 2014 11:16:03 -0600 From: Jason Gunthorpe To: Jarkko Sakkinen Cc: Peter Huewe , Ashley Lai , Marcel Selhorst , tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, josh.triplett@intel.com, christophe.ricard@gmail.com, jason.gunthorpe@obsidianresearch.com Subject: Re: [PATCH v1 2/3] tpm: two-phase chip management functions Message-ID: <20141022171603.GC12775@obsidianresearch.com> References: <1413995036-22497-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1413995036-22497-3-git-send-email-jarkko.sakkinen@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1413995036-22497-3-git-send-email-jarkko.sakkinen@linux.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.161 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 22, 2014 at 07:23:55PM +0300, Jarkko Sakkinen wrote: > tpm_register_hardware() and tpm_remove_hardware() are called often > before initializing the device. This is wrong order since it could > be that main TPM driver needs a fully initialized chip to be able to > do its job. For example, now it is impossible to move common startup > functions such as tpm_do_selftest() to tpm_register_hardware(). > Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc() It is called tpmm_chip_alloc() in this version.. > Signed-off-by: Jarkko Sakkinen Reviewed-By: Jason Gunthorpe Looks fine, if you have to spin this again you can incorporate the nits. > + > +static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES); Not for this patch, but while this area of code is being looked at this should probably be an IDR/IDA like other subsytems? > + if (try_module_get(pos->dev->driver->owner)) { > + chip = pos; > + break; > + } Not for this patch, this module stuff should be wiped in favor of chip->ops locking. > +static void tpmm_chip_remove(void *data) > +{ > + struct tpm_chip *chip = (struct tpm_chip *) data; > + dev_dbg(chip->dev, "%s\n", __func__); This print is silent in the default compile, right? > + chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES); [..] > + set_bit(chip->dev_num, dev_mask); Not for this patch but somehow there is no locking for dev_mask here. I guess it should use the driver_lock spinlock? > + chip->bios_dir = tpm_bios_log_setup(chip->devname); Not for this patch, but tpm_bios_log_setup can return NULL if securityfs setup fails. > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c > index 6069d13..8e2576a 100644 > +++ b/drivers/char/tpm/tpm_atmel.c > @@ -138,11 +138,11 @@ static void atml_plat_remove(void) > struct tpm_chip *chip = dev_get_drvdata(&pdev->dev); > > if (chip) { > + tpm_chip_unregister(chip); > if (chip->vendor.have_region) > atmel_release_region(chip->vendor.base, > chip->vendor.region_size); > atmel_put_base_addr(chip->vendor.iobase); > - tpm_remove_hardware(chip->dev); > platform_device_unregister(pdev); Missed this before, the Atmel driver is the same as the TIS driver in force mode, ie it isn't going through the driver APIs, but instead force creating platform devices. Same comment as for TIS - I'm not sure devm works properly like this. I guess just add the same comment as for TIS? > diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c > index 472af4b..6f00bc3 100644 > +++ b/drivers/char/tpm/tpm_i2c_infineon.c > @@ -581,10 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev) > int rc = 0; > struct tpm_chip *chip; > > - chip = tpm_register_hardware(dev, &tpm_tis_i2c); > - if (!chip) { > - dev_err(dev, "could not register hardware\n"); > - rc = -ENODEV; > + chip = tpmm_chip_alloc(dev, &tpm_tis_i2c); > + if (IS_ERR(chip)) { > + rc = PTR_ERR(chip); > goto out_err; Nit: out_err is synonymous with 'return rc;', so all the goto out_err in this driver can just return; > + rc = tpm_chip_register(chip); > + if (rc) > + return rc; > + return 0; Nit: could just be return tpm_chip_register(chip); > @@ -619,10 +619,9 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) > goto end; > } > > - chip = tpm_register_hardware(&client->dev, &st_i2c_tpm); > - if (!chip) { > - dev_info(&client->dev, "fail chip\n"); > - err = -ENODEV; > + chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm); > + if (IS_ERR(chip)) { > + err = PTR_ERR(chip); > goto end; > } Nit: Same comment, 'goto end' can just be 'return rc' > - dev_info(chip->dev, "TPM I2C Initialized\n"); > + err = tpm_chip_register(chip); > + if (err) > + goto _irq_set; > + Nit: Same comment > end: > pr_info("TPM I2C initialisation fail\n"); This pr_info should be deleted > @@ -573,6 +580,8 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev) > struct tpm_chip *chip = pnp_get_drvdata(dev); > > if (chip) { Nit: This if in the remove callback is an anti-pattern and should be globally removed. If remove is being called then probe succeeded, there is no way for probe to succed and drvdata to be unset. > static void tpm_nsc_remove(struct device *dev) > { > struct tpm_chip *chip = dev_get_drvdata(dev); > - if ( chip ) { > + if (chip) { Same comment > @@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe"); > > static struct platform_driver tis_drv = { > .driver = { > - .name = "tpm_tis", > + .name = "tpm_tis", > .owner = THIS_MODULE, > .pm = &tpm_tis_pm, > }, There is no remove method here in this platform_driver, this ties into the question if force works or not. The tpm_tis_remove call in the cleanup_hardware should be done through the .remove method of this driver structure.. Jason -- 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/