diff mbox

[tpmdd-devel,v2,3/7] tpm: clean up tpm_tis driver life-cycle

Message ID 1412701277-27794-4-git-send-email-jarkko.sakkinen@linux.intel.com
State Superseded, archived
Headers show

Commit Message

Jarkko Sakkinen Oct. 7, 2014, 5:01 p.m. UTC 
Updated tpm_tis to properly use tpm-chip API instead of using ad hoc
methods. tpm_chip_unregister() is called on remove event when PNP driver
is used and on module removal when platform driver is used.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 64 ++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 36 deletions(-)

Comments

Jason Gunthorpe Oct. 7, 2014, 5:53 p.m. UTC | #1 
On Tue, Oct 07, 2014 at 08:01:13PM +0300, Jarkko Sakkinen wrote:

> +	chip = tpm_chip_alloc(dev, &tpm_tis);
> +	if (!chip)
>  		return -ENODEV;

Needs to use ERR_PTR
  
> +	rc = tpm_chip_register(chip);
> +	if (rc)
> +		return -ENODEV;

Wrong ordering, this needs to be last in the probe function

Return rc not -ENODEV
  
> +static void tpm_tis_chip_remove(struct tpm_chip *chip)
> +{
> +	iowrite32(~TPM_GLOBAL_INT_ENABLE &
> +		  ioread32(chip->vendor.iobase +
> +			   TPM_INT_ENABLE(chip->vendor.
> +					  locality)),
> +		  chip->vendor.iobase +
> +		  TPM_INT_ENABLE(chip->vendor.locality));
> +	release_locality(chip, chip->vendor.locality, 1);
> +	if (chip->vendor.irq)
> +		free_irq(chip->vendor.irq, chip);
> +
> +	tpm_chip_unregister(chip);
> +}

Wrong ordering, tpm_chip_unregister needs to be first

> +	chip = dev_get_drvdata(&pdev->dev);
> +	tpm_tis_chip_remove(chip);
>  	platform_device_unregister(pdev);

I'm under the impression devm does not work outside a device driver
context, so adding devm breaks force mode in this driver. Do you see
differently?

AFAIK the two options are to fix force mode so that it attaches the
dummy platform driver (that is what it is for after all) or remove
force mode.

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index a2780df..df04ce6 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -75,9 +75,6 @@  enum tis_defaults {
 #define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
 #define	TPM_RID(l)			(0x0F04 | ((l) << 12))
 
-static LIST_HEAD(tis_chips);
-static DEFINE_MUTEX(tis_lock);
-
 #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
 static int is_itpm(struct pnp_dev *dev)
 {
@@ -535,14 +532,17 @@  static int tpm_tis_init(struct device *dev, resource_size_t start,
 	int rc, i, irq_s, irq_e, probe;
 	struct tpm_chip *chip;
 
-	if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
+	chip = tpm_chip_alloc(dev, &tpm_tis);
+	if (!chip)
 		return -ENODEV;
 
-	chip->vendor.iobase = ioremap(start, len);
-	if (!chip->vendor.iobase) {
-		rc = -EIO;
-		goto out_err;
-	}
+	chip->vendor.iobase = devm_ioremap(dev, start, len);
+	if (!chip->vendor.iobase)
+		return -EIO;
+
+	rc = tpm_chip_register(chip);
+	if (rc)
+		return -ENODEV;
 
 	/* Default timeouts */
 	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
@@ -720,16 +720,10 @@  static int tpm_tis_init(struct device *dev, resource_size_t start,
 	}
 
 	INIT_LIST_HEAD(&chip->vendor.list);
-	mutex_lock(&tis_lock);
-	list_add(&chip->vendor.list, &tis_chips);
-	mutex_unlock(&tis_lock);
-
 
 	return 0;
 out_err:
-	if (chip->vendor.iobase)
-		iounmap(chip->vendor.iobase);
-	tpm_remove_hardware(chip->dev);
+	tpm_chip_unregister(chip);
 	return rc;
 }
 
@@ -808,13 +802,27 @@  static struct pnp_device_id tpm_pnp_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
 
+static void tpm_tis_chip_remove(struct tpm_chip *chip)
+{
+	iowrite32(~TPM_GLOBAL_INT_ENABLE &
+		  ioread32(chip->vendor.iobase +
+			   TPM_INT_ENABLE(chip->vendor.
+					  locality)),
+		  chip->vendor.iobase +
+		  TPM_INT_ENABLE(chip->vendor.locality));
+	release_locality(chip, chip->vendor.locality, 1);
+	if (chip->vendor.irq)
+		free_irq(chip->vendor.irq, chip);
+
+	tpm_chip_unregister(chip);
+}
+
 static void tpm_tis_pnp_remove(struct pnp_dev *dev)
 {
 	struct tpm_chip *chip = pnp_get_drvdata(dev);
-	tpm_remove_hardware(chip->dev);
+	tpm_tis_chip_remove(chip);
 }
 
-
 static struct pnp_driver tis_pnp_driver = {
 	.name = "tpm_tis",
 	.id_table = tpm_pnp_tbl,
@@ -873,31 +881,15 @@  err_dev:
 
 static void __exit cleanup_tis(void)
 {
-	struct tpm_vendor_specific *i, *j;
 	struct tpm_chip *chip;
-	mutex_lock(&tis_lock);
-	list_for_each_entry_safe(i, j, &tis_chips, list) {
-		chip = to_tpm_chip(i);
-		tpm_remove_hardware(chip->dev);
-		iowrite32(~TPM_GLOBAL_INT_ENABLE &
-			  ioread32(chip->vendor.iobase +
-				   TPM_INT_ENABLE(chip->vendor.
-						  locality)),
-			  chip->vendor.iobase +
-			  TPM_INT_ENABLE(chip->vendor.locality));
-		release_locality(chip, chip->vendor.locality, 1);
-		if (chip->vendor.irq)
-			free_irq(chip->vendor.irq, chip);
-		iounmap(i->iobase);
-		list_del(&i->list);
-	}
-	mutex_unlock(&tis_lock);
 #ifdef CONFIG_PNP
 	if (!force) {
 		pnp_unregister_driver(&tis_pnp_driver);
 		return;
 	}
 #endif
+	chip = dev_get_drvdata(&pdev->dev);
+	tpm_tis_chip_remove(chip);
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&tis_drv);
 }