[PATCH] Infineon TPM: move infineon driver off pci_dev

[PATCH] Infineon TPM: move infineon driver off pci_dev

[Marcel Selhorst]

Dear all,

the following patch moves the Infineon TPM driver off pci device
and makes it a pure pnp-driver. It was tested with IFX0101 and
IFX0102 and is now based on the tpm patchset (1 to 5) from Kylene
Hall submitted yesterday.

Best regards,

Marcel Selhorst

Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
---

diff -pruN linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c
linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c
--- linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c	2005-10-26
15:21:53.000000000 +0200
+++ linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c	2005-10-26
15:21:42.000000000 +0200
@@ -5,6 +5,7 @@
  * Specifications at www.trustedcomputinggroup.org
  *
  * Copyright (C) 2005, Marcel Selhorst <selhorst@crypto.rub.de>
+ * Sirrix AG - security technologies, http://www.sirrix.com and
  * Applied Data Security Group, Ruhr-University Bochum, Germany
  * Project-Homepage: http://www.prosec.rub.de/tpm
  *
@@ -31,7 +32,7 @@
 /* These values will be filled after PnP-call */
 static int TPM_INF_DATA = 0;
 static int TPM_INF_ADDR = 0;
-static int pnp_registered = 0;
+static int TPM_INF_BASE = 0;

 /* TPM header definitions */
 enum infineon_tpm_header {
@@ -143,11 +144,9 @@ static int wait(struct tpm_chip *chip, i
 	}
 	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
 		if (wait_for_bit == STAT_XFE)
-			dev_err(chip->dev,
-				"Timeout in wait(STAT_XFE)\n");
+			dev_err(chip->dev, "Timeout in wait(STAT_XFE)\n");
 		if (wait_for_bit == STAT_RDA)
-			dev_err(chip->dev,
-				"Timeout in wait(STAT_RDA)\n");
+			dev_err(chip->dev, "Timeout in wait(STAT_RDA)\n");
 		return -EIO;
 	}
 	return 0;
@@ -196,7 +195,7 @@ static int tpm_inf_recv(struct tpm_chip
 	int ret;
 	u32 size = 0;

-recv_begin:
+      recv_begin:
 	/* start receiving header */
 	for (i = 0; i < 4; i++) {
 		ret = wait(chip, STAT_RDA);
@@ -221,8 +220,7 @@ recv_begin:
 		}

 		if ((size == 0x6D00) && (buf[1] == 0x80)) {
-			dev_err(chip->dev,
-				"Error handling on vendor layer!\n");
+			dev_err(chip->dev, "Error handling on vendor layer!\n");
 			return -EIO;
 		}

@@ -362,30 +360,11 @@ static const struct pnp_device_id tpm_pn
 	{"IFX0102", 0},
 	{"", 0}
 };
+
 MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);

 static int __devinit tpm_inf_pnp_probe(struct pnp_dev *dev,
-					const struct pnp_device_id *dev_id)
-{
-	if (pnp_port_valid(dev, 0)) {
-		TPM_INF_ADDR = (pnp_port_start(dev, 0) & 0xff);
-		TPM_INF_DATA = ((TPM_INF_ADDR + 1) & 0xff);
-		tpm_inf.base = pnp_port_start(dev, 1);
-		dev_info(&dev->dev, "Found %s with ID %s\n",
-		dev->name, dev_id->id);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-static struct pnp_driver tpm_inf_pnp = {
-	.name = "tpm_inf_pnp",
-	.id_table = tpm_pnp_tbl,
-	.probe = tpm_inf_pnp_probe,
-};
-
-static int __devinit tpm_inf_probe(struct pci_dev *pci_dev,
-				   const struct pci_device_id *pci_id)
+				       const struct pnp_device_id *dev_id)
 {
 	int rc = 0;
 	u8 iol, ioh;
@@ -394,30 +373,23 @@ static int __devinit tpm_inf_probe(struc
 	int productid[2];
 	char chipname[20];

-	rc = pci_enable_device(pci_dev);
-	if (rc)
-		return rc;
-
-	dev_info(&pci_dev->dev, "LPC-bus found at 0x%x\n", pci_id->device);
-
-	/* read IO-ports from PnP */
-	rc = pnp_register_driver(&tpm_inf_pnp);
-	if (rc < 0) {
-		dev_err(&pci_dev->dev,
-			"Error %x from pnp_register_driver!\n",rc);
-		goto error2;
-	}
-	if (!rc) {
-		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
-		goto error;
+	/* read IO-ports through PnP */
+	if (pnp_port_valid(dev, 0) &&
+	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
+		TPM_INF_ADDR = pnp_port_start(dev, 0);
+		TPM_INF_DATA = (TPM_INF_ADDR + 1);
+		TPM_INF_BASE = pnp_port_start(dev, 1);
+		dev_info(&dev->dev, "Found %s with ID %s\n",
+			 dev->name, dev_id->id);
+		if (!((TPM_INF_BASE >> 8) & 0xff)) {
+			tpm_inf.base = 0;
+		} else {
+			/* publish the base address to the tpm-driver
+			   and use tpm_inf.base from now on */
+			tpm_inf.base = TPM_INF_BASE;
+		}
 	} else {
-		pnp_registered = 1;
-	}
-
-	/* Make sure, we have received valid config ports */
-	if (!TPM_INF_ADDR) {
-		dev_err(&pci_dev->dev, "No valid IO-ports received!\n");
-		goto error;
+		return -EINVAL;
 	}

 	/* query chip for its vendor, its version number a.s.o. */
@@ -450,8 +422,8 @@ static int __devinit tpm_inf_probe(struc
 	if ((vendorid[0] << 8 | vendorid[1]) == (TPM_INFINEON_DEV_VEN_VALUE)) {

 		if (tpm_inf.base == 0) {
-			dev_err(&pci_dev->dev, "No IO-ports found!\n");
-			goto error;
+			dev_err(&dev->dev, "No IO-ports found!\n");
+			return -EIO;
 		}
 		/* configure TPM with IO-ports */
 		outb(IOLIMH, TPM_INF_ADDR);
@@ -466,10 +438,10 @@ static int __devinit tpm_inf_probe(struc
 		iol = inb(TPM_INF_DATA);

 		if ((ioh << 8 | iol) != tpm_inf.base) {
-			dev_err(&pci_dev->dev,
+			dev_err(&dev->dev,
 				"Could not set IO-ports to %04x\n",
 				tpm_inf.base);
-			goto error;
+			return -EIO;
 		}

 		/* activate register */
@@ -481,7 +453,7 @@ static int __devinit tpm_inf_probe(struc
 		outb(RESET_LP_IRQC_DISABLE, tpm_inf.base + CMD);

 		/* Finally, we're done, print some infos */
-		dev_info(&pci_dev->dev, "TPM found: "
+		dev_info(&dev->dev, "TPM found: "
 			 "config base 0x%x, "
 			 "io base 0x%x, "
 			 "chip version %02x%02x, "
@@ -489,67 +461,48 @@ static int __devinit tpm_inf_probe(struc
 			 "product id %02x%02x"
 			 "%s\n",
 			 TPM_INF_ADDR,
-			 tpm_inf.base,
+			 TPM_INF_BASE,
 			 version[0], version[1],
 			 vendorid[0], vendorid[1],
 			 productid[0], productid[1], chipname);

-		rc = tpm_register_hardware(&pci_dev->dev, &tpm_inf);
-		if (rc < 0)
-			goto error;
+		rc = tpm_register_hardware(&dev->dev, &tpm_inf);
+		if (rc < 0) {
+			return -ENODEV;
+		}
 		return 0;
 	} else {
-		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
-error:
-		pnp_unregister_driver(&tpm_inf_pnp);
-error2:
-		pci_disable_device(pci_dev);
-		pnp_registered = 0;
+		dev_info(&dev->dev, "No Infineon TPM found!\n");
 		return -ENODEV;
 	}
 }

-static __devexit void tpm_inf_remove(struct pci_dev* pci_dev)
+static __devexit void tpm_inf_pnp_remove(struct pnp_dev *dev)
 {
-	struct tpm_chip* chip = pci_get_drvdata(pci_dev);
-	
-	if( chip )
+	struct tpm_chip *chip = pnp_get_drvdata(dev);
+
+	if (chip)
 		tpm_remove_hardware(chip->dev);
 }

-static struct pci_device_id tpm_pci_tbl[] __devinitdata = {
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_12)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_2)},
-	{0,}
-};
-
-MODULE_DEVICE_TABLE(pci, tpm_pci_tbl);
-
-static struct pci_driver inf_pci_driver = {
-	.name = "tpm_inf",
-	.id_table = tpm_pci_tbl,
-	.probe = tpm_inf_probe,
-	.remove = __devexit_p(tpm_inf_remove),
-	.suspend = tpm_pm_suspend,
-	.resume = tpm_pm_resume,
+static struct pnp_driver tpm_inf_pnp = {
+	.name = "tpm_inf_pnp",
+	.driver.owner = THIS_MODULE,
+	.id_table = tpm_pnp_tbl,
+	.probe = tpm_inf_pnp_probe,
+	.remove = tpm_inf_pnp_remove,
+	.driver.suspend = tpm_pm_suspend,
+	.driver.resume = tpm_pm_resume,
 };

 static int __init init_inf(void)
 {
-	return pci_register_driver(&inf_pci_driver);
+	return pnp_register_driver(&tpm_inf_pnp);
 }

 static void __exit cleanup_inf(void)
 {
-	if (pnp_registered)
-		pnp_unregister_driver(&tpm_inf_pnp);
-	pci_unregister_driver(&inf_pci_driver);
+	pnp_unregister_driver(&tpm_inf_pnp);
 }

 module_init(init_inf);
@@ -557,5 +510,5 @@ module_exit(cleanup_inf);

 MODULE_AUTHOR("Marcel Selhorst <selhorst@crypto.rub.de>");
 MODULE_DESCRIPTION("Driver for Infineon TPM SLD 9630 TT 1.1 / SLB 9635 TT 1.2");
-MODULE_VERSION("1.5");
+MODULE_VERSION("1.6");
 MODULE_LICENSE("GPL");

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Kylene Jo Hall]

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>

On Wed, 2005-10-26 at 19:11 +0200, Marcel Selhorst wrote:
> Dear all,
> 
> the following patch moves the Infineon TPM driver off pci device
> and makes it a pure pnp-driver. It was tested with IFX0101 and
> IFX0102 and is now based on the tpm patchset (1 to 5) from Kylene
> Hall submitted yesterday.
> 
> Best regards,
> 
> Marcel Selhorst
> 
> Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
> ---
> 
> diff -pruN linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c
> linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c
> --- linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c	2005-10-26
> 15:21:53.000000000 +0200
> +++ linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c	2005-10-26
> 15:21:42.000000000 +0200
> @@ -5,6 +5,7 @@
>   * Specifications at www.trustedcomputinggroup.org
>   *
>   * Copyright (C) 2005, Marcel Selhorst <selhorst@crypto.rub.de>
> + * Sirrix AG - security technologies, http://www.sirrix.com and
>   * Applied Data Security Group, Ruhr-University Bochum, Germany
>   * Project-Homepage: http://www.prosec.rub.de/tpm
>   *
> @@ -31,7 +32,7 @@
>  /* These values will be filled after PnP-call */
>  static int TPM_INF_DATA = 0;
>  static int TPM_INF_ADDR = 0;
> -static int pnp_registered = 0;
> +static int TPM_INF_BASE = 0;
> 
>  /* TPM header definitions */
>  enum infineon_tpm_header {
> @@ -143,11 +144,9 @@ static int wait(struct tpm_chip *chip, i
>  	}
>  	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
>  		if (wait_for_bit == STAT_XFE)
> -			dev_err(chip->dev,
> -				"Timeout in wait(STAT_XFE)\n");
> +			dev_err(chip->dev, "Timeout in wait(STAT_XFE)\n");
>  		if (wait_for_bit == STAT_RDA)
> -			dev_err(chip->dev,
> -				"Timeout in wait(STAT_RDA)\n");
> +			dev_err(chip->dev, "Timeout in wait(STAT_RDA)\n");
>  		return -EIO;
>  	}
>  	return 0;
> @@ -196,7 +195,7 @@ static int tpm_inf_recv(struct tpm_chip
>  	int ret;
>  	u32 size = 0;
> 
> -recv_begin:
> +      recv_begin:
>  	/* start receiving header */
>  	for (i = 0; i < 4; i++) {
>  		ret = wait(chip, STAT_RDA);
> @@ -221,8 +220,7 @@ recv_begin:
>  		}
> 
>  		if ((size == 0x6D00) && (buf[1] == 0x80)) {
> -			dev_err(chip->dev,
> -				"Error handling on vendor layer!\n");
> +			dev_err(chip->dev, "Error handling on vendor layer!\n");
>  			return -EIO;
>  		}
> 
> @@ -362,30 +360,11 @@ static const struct pnp_device_id tpm_pn
>  	{"IFX0102", 0},
>  	{"", 0}
>  };
> +
>  MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
> 
>  static int __devinit tpm_inf_pnp_probe(struct pnp_dev *dev,
> -					const struct pnp_device_id *dev_id)
> -{
> -	if (pnp_port_valid(dev, 0)) {
> -		TPM_INF_ADDR = (pnp_port_start(dev, 0) & 0xff);
> -		TPM_INF_DATA = ((TPM_INF_ADDR + 1) & 0xff);
> -		tpm_inf.base = pnp_port_start(dev, 1);
> -		dev_info(&dev->dev, "Found %s with ID %s\n",
> -		dev->name, dev_id->id);
> -		return 0;
> -	}
> -	return -ENODEV;
> -}
> -
> -static struct pnp_driver tpm_inf_pnp = {
> -	.name = "tpm_inf_pnp",
> -	.id_table = tpm_pnp_tbl,
> -	.probe = tpm_inf_pnp_probe,
> -};
> -
> -static int __devinit tpm_inf_probe(struct pci_dev *pci_dev,
> -				   const struct pci_device_id *pci_id)
> +				       const struct pnp_device_id *dev_id)
>  {
>  	int rc = 0;
>  	u8 iol, ioh;
> @@ -394,30 +373,23 @@ static int __devinit tpm_inf_probe(struc
>  	int productid[2];
>  	char chipname[20];
> 
> -	rc = pci_enable_device(pci_dev);
> -	if (rc)
> -		return rc;
> -
> -	dev_info(&pci_dev->dev, "LPC-bus found at 0x%x\n", pci_id->device);
> -
> -	/* read IO-ports from PnP */
> -	rc = pnp_register_driver(&tpm_inf_pnp);
> -	if (rc < 0) {
> -		dev_err(&pci_dev->dev,
> -			"Error %x from pnp_register_driver!\n",rc);
> -		goto error2;
> -	}
> -	if (!rc) {
> -		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
> -		goto error;
> +	/* read IO-ports through PnP */
> +	if (pnp_port_valid(dev, 0) &&
> +	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
> +		TPM_INF_ADDR = pnp_port_start(dev, 0);
> +		TPM_INF_DATA = (TPM_INF_ADDR + 1);
> +		TPM_INF_BASE = pnp_port_start(dev, 1);
> +		dev_info(&dev->dev, "Found %s with ID %s\n",
> +			 dev->name, dev_id->id);
> +		if (!((TPM_INF_BASE >> 8) & 0xff)) {
> +			tpm_inf.base = 0;
> +		} else {
> +			/* publish the base address to the tpm-driver
> +			   and use tpm_inf.base from now on */
> +			tpm_inf.base = TPM_INF_BASE;
> +		}
>  	} else {
> -		pnp_registered = 1;
> -	}
> -
> -	/* Make sure, we have received valid config ports */
> -	if (!TPM_INF_ADDR) {
> -		dev_err(&pci_dev->dev, "No valid IO-ports received!\n");
> -		goto error;
> +		return -EINVAL;
>  	}
> 
>  	/* query chip for its vendor, its version number a.s.o. */
> @@ -450,8 +422,8 @@ static int __devinit tpm_inf_probe(struc
>  	if ((vendorid[0] << 8 | vendorid[1]) == (TPM_INFINEON_DEV_VEN_VALUE)) {
> 
>  		if (tpm_inf.base == 0) {
> -			dev_err(&pci_dev->dev, "No IO-ports found!\n");
> -			goto error;
> +			dev_err(&dev->dev, "No IO-ports found!\n");
> +			return -EIO;
>  		}
>  		/* configure TPM with IO-ports */
>  		outb(IOLIMH, TPM_INF_ADDR);
> @@ -466,10 +438,10 @@ static int __devinit tpm_inf_probe(struc
>  		iol = inb(TPM_INF_DATA);
> 
>  		if ((ioh << 8 | iol) != tpm_inf.base) {
> -			dev_err(&pci_dev->dev,
> +			dev_err(&dev->dev,
>  				"Could not set IO-ports to %04x\n",
>  				tpm_inf.base);
> -			goto error;
> +			return -EIO;
>  		}
> 
>  		/* activate register */
> @@ -481,7 +453,7 @@ static int __devinit tpm_inf_probe(struc
>  		outb(RESET_LP_IRQC_DISABLE, tpm_inf.base + CMD);
> 
>  		/* Finally, we're done, print some infos */
> -		dev_info(&pci_dev->dev, "TPM found: "
> +		dev_info(&dev->dev, "TPM found: "
>  			 "config base 0x%x, "
>  			 "io base 0x%x, "
>  			 "chip version %02x%02x, "
> @@ -489,67 +461,48 @@ static int __devinit tpm_inf_probe(struc
>  			 "product id %02x%02x"
>  			 "%s\n",
>  			 TPM_INF_ADDR,
> -			 tpm_inf.base,
> +			 TPM_INF_BASE,
>  			 version[0], version[1],
>  			 vendorid[0], vendorid[1],
>  			 productid[0], productid[1], chipname);
> 
> -		rc = tpm_register_hardware(&pci_dev->dev, &tpm_inf);
> -		if (rc < 0)
> -			goto error;
> +		rc = tpm_register_hardware(&dev->dev, &tpm_inf);
> +		if (rc < 0) {
> +			return -ENODEV;
> +		}
>  		return 0;
>  	} else {
> -		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
> -error:
> -		pnp_unregister_driver(&tpm_inf_pnp);
> -error2:
> -		pci_disable_device(pci_dev);
> -		pnp_registered = 0;
> +		dev_info(&dev->dev, "No Infineon TPM found!\n");
>  		return -ENODEV;
>  	}
>  }
> 
> -static __devexit void tpm_inf_remove(struct pci_dev* pci_dev)
> +static __devexit void tpm_inf_pnp_remove(struct pnp_dev *dev)
>  {
> -	struct tpm_chip* chip = pci_get_drvdata(pci_dev);
> -	
> -	if( chip )
> +	struct tpm_chip *chip = pnp_get_drvdata(dev);
> +
> +	if (chip)
>  		tpm_remove_hardware(chip->dev);
>  }
> 
> -static struct pci_device_id tpm_pci_tbl[] __devinitdata = {
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_12)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_2)},
> -	{0,}
> -};
> -
> -MODULE_DEVICE_TABLE(pci, tpm_pci_tbl);
> -
> -static struct pci_driver inf_pci_driver = {
> -	.name = "tpm_inf",
> -	.id_table = tpm_pci_tbl,
> -	.probe = tpm_inf_probe,
> -	.remove = __devexit_p(tpm_inf_remove),
> -	.suspend = tpm_pm_suspend,
> -	.resume = tpm_pm_resume,
> +static struct pnp_driver tpm_inf_pnp = {
> +	.name = "tpm_inf_pnp",
> +	.driver.owner = THIS_MODULE,
> +	.id_table = tpm_pnp_tbl,
> +	.probe = tpm_inf_pnp_probe,
> +	.remove = tpm_inf_pnp_remove,
> +	.driver.suspend = tpm_pm_suspend,
> +	.driver.resume = tpm_pm_resume,
>  };
> 
>  static int __init init_inf(void)
>  {
> -	return pci_register_driver(&inf_pci_driver);
> +	return pnp_register_driver(&tpm_inf_pnp);
>  }
> 
>  static void __exit cleanup_inf(void)
>  {
> -	if (pnp_registered)
> -		pnp_unregister_driver(&tpm_inf_pnp);
> -	pci_unregister_driver(&inf_pci_driver);
> +	pnp_unregister_driver(&tpm_inf_pnp);
>  }
> 
>  module_init(init_inf);
> @@ -557,5 +510,5 @@ module_exit(cleanup_inf);
> 
>  MODULE_AUTHOR("Marcel Selhorst <selhorst@crypto.rub.de>");
>  MODULE_DESCRIPTION("Driver for Infineon TPM SLD 9630 TT 1.1 / SLB 9635 TT 1.2");
> -MODULE_VERSION("1.5");
> +MODULE_VERSION("1.6");
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[matthieu castet]

Hi,

Marcel Selhorst wrote:
> Dear all,
> 
> the following patch moves the Infineon TPM driver off pci device
> and makes it a pure pnp-driver. It was tested with IFX0101 and
> IFX0102 and is now based on the tpm patchset (1 to 5) from Kylene
> Hall submitted yesterday.
> 
> Best regards,
> 
> Marcel Selhorst
> 
> Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
> ---
> 
> diff -pruN linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c
> linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c
> --- linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c	2005-10-26
> 15:21:53.000000000 +0200
> +++ linux-2.6.14-rc5.infineon_v1.6/drivers/char/tpm/tpm_infineon.c	2005-10-26
> 15:21:42.000000000 +0200
> @@ -5,6 +5,7 @@
>   * Specifications at www.trustedcomputinggroup.org
>   *
>   * Copyright (C) 2005, Marcel Selhorst <selhorst@crypto.rub.de>
> + * Sirrix AG - security technologies, http://www.sirrix.com and
>   * Applied Data Security Group, Ruhr-University Bochum, Germany
>   * Project-Homepage: http://www.prosec.rub.de/tpm
>   *
> @@ -31,7 +32,7 @@
>  /* These values will be filled after PnP-call */

> +	/* read IO-ports through PnP */
> +	if (pnp_port_valid(dev, 0) &&
> +	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
> +		TPM_INF_ADDR = pnp_port_start(dev, 0);
> +		TPM_INF_DATA = (TPM_INF_ADDR + 1);
> +		TPM_INF_BASE = pnp_port_start(dev, 1);
You should add a pnp_port_valid(dev, 1) check.
If you are paranoid, you could also check the port len.

I don't remember if it is done somewhere, but a request_region should be 
used.

The others parts are ok for me.

Matthieu

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Marcel Selhorst]

Hi Matthieu,

>> +    /* read IO-ports through PnP */
>> +    if (pnp_port_valid(dev, 0) &&
>> +        !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
>> +        TPM_INF_ADDR = pnp_port_start(dev, 0);
>> +        TPM_INF_DATA = (TPM_INF_ADDR + 1);
>> +        TPM_INF_BASE = pnp_port_start(dev, 1);
> 
> You should add a pnp_port_valid(dev, 1) check.
> If you are paranoid, you could also check the port len.
> 
> I don't remember if it is done somewhere, but a request_region should be
> used.

Thanks for that good hint, I'll take care of that tomorrow
and submit a new patch here.

Marcel Selhorst

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Marcel Selhorst]

Dear all,

the following patch moves the Infineon TPM driver off pci device
and makes it a pure pnp-driver. It was tested with IFX0101 and
IFX0102 and is now based on the new tpm patchset (1 to 5) from Kylene
Hall submitted two days ago. It now also includes pnp-port validation
and region requesting.

Best regards,

Marcel Selhorst

Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
---

diff -pruN linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c
linux-2.6.14-rc5.infineon-v1.6/drivers/char/tpm/tpm_infineon.c
--- linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c	2005-10-27
13:28:54.000000000 +0200
+++ linux-2.6.14-rc5.infineon-v1.6/drivers/char/tpm/tpm_infineon.c	2005-10-27
13:25:19.000000000 +0200
@@ -5,6 +5,7 @@
  * Specifications at www.trustedcomputinggroup.org
  *
  * Copyright (C) 2005, Marcel Selhorst <selhorst@crypto.rub.de>
+ * Sirrix AG - security technologies, http://www.sirrix.com and
  * Applied Data Security Group, Ruhr-University Bochum, Germany
  * Project-Homepage: http://www.prosec.rub.de/tpm
  *
@@ -31,7 +32,8 @@
 /* These values will be filled after PnP-call */
 static int TPM_INF_DATA = 0;
 static int TPM_INF_ADDR = 0;
-static int pnp_registered = 0;
+static int TPM_INF_BASE = 0;
+static int TPM_INF_PORT_LEN = 0;

 /* TPM header definitions */
 enum infineon_tpm_header {
@@ -143,11 +145,9 @@ static int wait(struct tpm_chip *chip, i
 	}
 	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
 		if (wait_for_bit == STAT_XFE)
-			dev_err(chip->dev,
-				"Timeout in wait(STAT_XFE)\n");
+			dev_err(chip->dev, "Timeout in wait(STAT_XFE)\n");
 		if (wait_for_bit == STAT_RDA)
-			dev_err(chip->dev,
-				"Timeout in wait(STAT_RDA)\n");
+			dev_err(chip->dev, "Timeout in wait(STAT_RDA)\n");
 		return -EIO;
 	}
 	return 0;
@@ -221,8 +221,7 @@ recv_begin:
 		}

 		if ((size == 0x6D00) && (buf[1] == 0x80)) {
-			dev_err(chip->dev,
-				"Error handling on vendor layer!\n");
+			dev_err(chip->dev, "Error handling on vendor layer!\n");
 			return -EIO;
 		}

@@ -318,7 +317,7 @@ static void tpm_inf_cancel(struct tpm_ch

 static u8 tpm_inf_status(struct tpm_chip *chip)
 {
-	return inb(chip->vendor->base + 1);
+	return inb(chip->vendor->base + STAT);
 }

 static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
@@ -362,30 +361,11 @@ static const struct pnp_device_id tpm_pn
 	{"IFX0102", 0},
 	{"", 0}
 };
+
 MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);

 static int __devinit tpm_inf_pnp_probe(struct pnp_dev *dev,
-					const struct pnp_device_id *dev_id)
-{
-	if (pnp_port_valid(dev, 0)) {
-		TPM_INF_ADDR = (pnp_port_start(dev, 0) & 0xff);
-		TPM_INF_DATA = ((TPM_INF_ADDR + 1) & 0xff);
-		tpm_inf.base = pnp_port_start(dev, 1);
-		dev_info(&dev->dev, "Found %s with ID %s\n",
-		dev->name, dev_id->id);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-static struct pnp_driver tpm_inf_pnp = {
-	.name = "tpm_inf_pnp",
-	.id_table = tpm_pnp_tbl,
-	.probe = tpm_inf_pnp_probe,
-};
-
-static int __devinit tpm_inf_probe(struct pci_dev *pci_dev,
-				   const struct pci_device_id *pci_id)
+				       const struct pnp_device_id *dev_id)
 {
 	int rc = 0;
 	u8 iol, ioh;
@@ -394,30 +374,28 @@ static int __devinit tpm_inf_probe(struc
 	int productid[2];
 	char chipname[20];

-	rc = pci_enable_device(pci_dev);
-	if (rc)
-		return rc;
-
-	dev_info(&pci_dev->dev, "LPC-bus found at 0x%x\n", pci_id->device);
-
-	/* read IO-ports from PnP */
-	rc = pnp_register_driver(&tpm_inf_pnp);
-	if (rc < 0) {
-		dev_err(&pci_dev->dev,
-			"Error %x from pnp_register_driver!\n",rc);
-		goto error2;
-	}
-	if (!rc) {
-		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
-		goto error;
+	/* read IO-ports through PnP */
+	if (pnp_port_valid(dev, 0) && pnp_port_valid(dev, 1) &&
+	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
+		TPM_INF_ADDR = pnp_port_start(dev, 0);
+		TPM_INF_DATA = (TPM_INF_ADDR + 1);
+		TPM_INF_BASE = pnp_port_start(dev, 1);
+		TPM_INF_PORT_LEN = pnp_port_len(dev, 1);
+		if (!TPM_INF_PORT_LEN)
+			return -EINVAL;
+		dev_info(&dev->dev, "Found %s with ID %s\n",
+			 dev->name, dev_id->id);
+		if (!((TPM_INF_BASE >> 8) & 0xff))
+			return -EINVAL;
+		/* publish my base address and request region */
+		tpm_inf.base = TPM_INF_BASE;
+		if (request_region
+		    (tpm_inf.base, TPM_INF_PORT_LEN, "tpm_infineon0") == NULL) {
+			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
+			return -EINVAL;
+		}
 	} else {
-		pnp_registered = 1;
-	}
-
-	/* Make sure, we have received valid config ports */
-	if (!TPM_INF_ADDR) {
-		dev_err(&pci_dev->dev, "No valid IO-ports received!\n");
-		goto error;
+		return -EINVAL;
 	}

 	/* query chip for its vendor, its version number a.s.o. */
@@ -449,10 +427,6 @@ static int __devinit tpm_inf_probe(struc

 	if ((vendorid[0] << 8 | vendorid[1]) == (TPM_INFINEON_DEV_VEN_VALUE)) {

-		if (tpm_inf.base == 0) {
-			dev_err(&pci_dev->dev, "No IO-ports found!\n");
-			goto error;
-		}
 		/* configure TPM with IO-ports */
 		outb(IOLIMH, TPM_INF_ADDR);
 		outb(((tpm_inf.base >> 8) & 0xff), TPM_INF_DATA);
@@ -466,10 +440,11 @@ static int __devinit tpm_inf_probe(struc
 		iol = inb(TPM_INF_DATA);

 		if ((ioh << 8 | iol) != tpm_inf.base) {
-			dev_err(&pci_dev->dev,
+			dev_err(&dev->dev,
 				"Could not set IO-ports to %04x\n",
 				tpm_inf.base);
-			goto error;
+			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
+			return -EIO;
 		}

 		/* activate register */
@@ -481,7 +456,7 @@ static int __devinit tpm_inf_probe(struc
 		outb(RESET_LP_IRQC_DISABLE, tpm_inf.base + CMD);

 		/* Finally, we're done, print some infos */
-		dev_info(&pci_dev->dev, "TPM found: "
+		dev_info(&dev->dev, "TPM found: "
 			 "config base 0x%x, "
 			 "io base 0x%x, "
 			 "chip version %02x%02x, "
@@ -489,67 +464,51 @@ static int __devinit tpm_inf_probe(struc
 			 "product id %02x%02x"
 			 "%s\n",
 			 TPM_INF_ADDR,
-			 tpm_inf.base,
+			 TPM_INF_BASE,
 			 version[0], version[1],
 			 vendorid[0], vendorid[1],
 			 productid[0], productid[1], chipname);

-		rc = tpm_register_hardware(&pci_dev->dev, &tpm_inf);
-		if (rc < 0)
-			goto error;
+		rc = tpm_register_hardware(&dev->dev, &tpm_inf);
+		if (rc < 0) {
+			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
+			return -ENODEV;
+		}
 		return 0;
 	} else {
-		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
-error:
-		pnp_unregister_driver(&tpm_inf_pnp);
-error2:
-		pci_disable_device(pci_dev);
-		pnp_registered = 0;
+		dev_info(&dev->dev, "No Infineon TPM found!\n");
 		return -ENODEV;
 	}
 }

-static __devexit void tpm_inf_remove(struct pci_dev* pci_dev)
+static __devexit void tpm_inf_pnp_remove(struct pnp_dev *dev)
 {
-	struct tpm_chip* chip = pci_get_drvdata(pci_dev);
-	
-	if( chip )
+	struct tpm_chip *chip = pnp_get_drvdata(dev);
+
+	if (chip) {
+		release_region(chip->vendor->base, TPM_INF_PORT_LEN);
 		tpm_remove_hardware(chip->dev);
+	}
 }

-static struct pci_device_id tpm_pci_tbl[] __devinitdata = {
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_12)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1)},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_2)},
-	{0,}
-};
-
-MODULE_DEVICE_TABLE(pci, tpm_pci_tbl);
-
-static struct pci_driver inf_pci_driver = {
-	.name = "tpm_inf",
-	.id_table = tpm_pci_tbl,
-	.probe = tpm_inf_probe,
-	.remove = __devexit_p(tpm_inf_remove),
-	.suspend = tpm_pm_suspend,
-	.resume = tpm_pm_resume,
+static struct pnp_driver tpm_inf_pnp = {
+	.name = "tpm_inf_pnp",
+	.driver.owner = THIS_MODULE,
+	.id_table = tpm_pnp_tbl,
+	.probe = tpm_inf_pnp_probe,
+	.remove = tpm_inf_pnp_remove,
+	.driver.suspend = tpm_pm_suspend,
+	.driver.resume = tpm_pm_resume,
 };

 static int __init init_inf(void)
 {
-	return pci_register_driver(&inf_pci_driver);
+	return pnp_register_driver(&tpm_inf_pnp);
 }

 static void __exit cleanup_inf(void)
 {
-	if (pnp_registered)
-		pnp_unregister_driver(&tpm_inf_pnp);
-	pci_unregister_driver(&inf_pci_driver);
+	pnp_unregister_driver(&tpm_inf_pnp);
 }

 module_init(init_inf);
@@ -557,5 +516,5 @@ module_exit(cleanup_inf);

 MODULE_AUTHOR("Marcel Selhorst <selhorst@crypto.rub.de>");
 MODULE_DESCRIPTION("Driver for Infineon TPM SLD 9630 TT 1.1 / SLB 9635 TT 1.2");
-MODULE_VERSION("1.5");
+MODULE_VERSION("1.6");
 MODULE_LICENSE("GPL");

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Kylene Jo Hall]

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>

On Thu, 2005-10-27 at 13:22 +0200, Marcel Selhorst wrote:
> Dear all,
> 
> the following patch moves the Infineon TPM driver off pci device
> and makes it a pure pnp-driver. It was tested with IFX0101 and
> IFX0102 and is now based on the new tpm patchset (1 to 5) from Kylene
> Hall submitted two days ago. It now also includes pnp-port validation
> and region requesting.
> 
> Best regards,
> 
> Marcel Selhorst
> 
> Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
> ---
> 
> diff -pruN linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c
> linux-2.6.14-rc5.infineon-v1.6/drivers/char/tpm/tpm_infineon.c
> --- linux-2.6.14-rc5.ibm/drivers/char/tpm/tpm_infineon.c	2005-10-27
> 13:28:54.000000000 +0200
> +++ linux-2.6.14-rc5.infineon-v1.6/drivers/char/tpm/tpm_infineon.c	2005-10-27
> 13:25:19.000000000 +0200
> @@ -5,6 +5,7 @@
>   * Specifications at www.trustedcomputinggroup.org
>   *
>   * Copyright (C) 2005, Marcel Selhorst <selhorst@crypto.rub.de>
> + * Sirrix AG - security technologies, http://www.sirrix.com and
>   * Applied Data Security Group, Ruhr-University Bochum, Germany
>   * Project-Homepage: http://www.prosec.rub.de/tpm
>   *
> @@ -31,7 +32,8 @@
>  /* These values will be filled after PnP-call */
>  static int TPM_INF_DATA = 0;
>  static int TPM_INF_ADDR = 0;
> -static int pnp_registered = 0;
> +static int TPM_INF_BASE = 0;
> +static int TPM_INF_PORT_LEN = 0;
> 
>  /* TPM header definitions */
>  enum infineon_tpm_header {
> @@ -143,11 +145,9 @@ static int wait(struct tpm_chip *chip, i
>  	}
>  	if (i == TPM_MAX_TRIES) {	/* timeout occurs */
>  		if (wait_for_bit == STAT_XFE)
> -			dev_err(chip->dev,
> -				"Timeout in wait(STAT_XFE)\n");
> +			dev_err(chip->dev, "Timeout in wait(STAT_XFE)\n");
>  		if (wait_for_bit == STAT_RDA)
> -			dev_err(chip->dev,
> -				"Timeout in wait(STAT_RDA)\n");
> +			dev_err(chip->dev, "Timeout in wait(STAT_RDA)\n");
>  		return -EIO;
>  	}
>  	return 0;
> @@ -221,8 +221,7 @@ recv_begin:
>  		}
> 
>  		if ((size == 0x6D00) && (buf[1] == 0x80)) {
> -			dev_err(chip->dev,
> -				"Error handling on vendor layer!\n");
> +			dev_err(chip->dev, "Error handling on vendor layer!\n");
>  			return -EIO;
>  		}
> 
> @@ -318,7 +317,7 @@ static void tpm_inf_cancel(struct tpm_ch
> 
>  static u8 tpm_inf_status(struct tpm_chip *chip)
>  {
> -	return inb(chip->vendor->base + 1);
> +	return inb(chip->vendor->base + STAT);
>  }
> 
>  static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
> @@ -362,30 +361,11 @@ static const struct pnp_device_id tpm_pn
>  	{"IFX0102", 0},
>  	{"", 0}
>  };
> +
>  MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
> 
>  static int __devinit tpm_inf_pnp_probe(struct pnp_dev *dev,
> -					const struct pnp_device_id *dev_id)
> -{
> -	if (pnp_port_valid(dev, 0)) {
> -		TPM_INF_ADDR = (pnp_port_start(dev, 0) & 0xff);
> -		TPM_INF_DATA = ((TPM_INF_ADDR + 1) & 0xff);
> -		tpm_inf.base = pnp_port_start(dev, 1);
> -		dev_info(&dev->dev, "Found %s with ID %s\n",
> -		dev->name, dev_id->id);
> -		return 0;
> -	}
> -	return -ENODEV;
> -}
> -
> -static struct pnp_driver tpm_inf_pnp = {
> -	.name = "tpm_inf_pnp",
> -	.id_table = tpm_pnp_tbl,
> -	.probe = tpm_inf_pnp_probe,
> -};
> -
> -static int __devinit tpm_inf_probe(struct pci_dev *pci_dev,
> -				   const struct pci_device_id *pci_id)
> +				       const struct pnp_device_id *dev_id)
>  {
>  	int rc = 0;
>  	u8 iol, ioh;
> @@ -394,30 +374,28 @@ static int __devinit tpm_inf_probe(struc
>  	int productid[2];
>  	char chipname[20];
> 
> -	rc = pci_enable_device(pci_dev);
> -	if (rc)
> -		return rc;
> -
> -	dev_info(&pci_dev->dev, "LPC-bus found at 0x%x\n", pci_id->device);
> -
> -	/* read IO-ports from PnP */
> -	rc = pnp_register_driver(&tpm_inf_pnp);
> -	if (rc < 0) {
> -		dev_err(&pci_dev->dev,
> -			"Error %x from pnp_register_driver!\n",rc);
> -		goto error2;
> -	}
> -	if (!rc) {
> -		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
> -		goto error;
> +	/* read IO-ports through PnP */
> +	if (pnp_port_valid(dev, 0) && pnp_port_valid(dev, 1) &&
> +	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
> +		TPM_INF_ADDR = pnp_port_start(dev, 0);
> +		TPM_INF_DATA = (TPM_INF_ADDR + 1);
> +		TPM_INF_BASE = pnp_port_start(dev, 1);
> +		TPM_INF_PORT_LEN = pnp_port_len(dev, 1);
> +		if (!TPM_INF_PORT_LEN)
> +			return -EINVAL;
> +		dev_info(&dev->dev, "Found %s with ID %s\n",
> +			 dev->name, dev_id->id);
> +		if (!((TPM_INF_BASE >> 8) & 0xff))
> +			return -EINVAL;
> +		/* publish my base address and request region */
> +		tpm_inf.base = TPM_INF_BASE;
> +		if (request_region
> +		    (tpm_inf.base, TPM_INF_PORT_LEN, "tpm_infineon0") == NULL) {
> +			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
> +			return -EINVAL;
> +		}
>  	} else {
> -		pnp_registered = 1;
> -	}
> -
> -	/* Make sure, we have received valid config ports */
> -	if (!TPM_INF_ADDR) {
> -		dev_err(&pci_dev->dev, "No valid IO-ports received!\n");
> -		goto error;
> +		return -EINVAL;
>  	}
> 
>  	/* query chip for its vendor, its version number a.s.o. */
> @@ -449,10 +427,6 @@ static int __devinit tpm_inf_probe(struc
> 
>  	if ((vendorid[0] << 8 | vendorid[1]) == (TPM_INFINEON_DEV_VEN_VALUE)) {
> 
> -		if (tpm_inf.base == 0) {
> -			dev_err(&pci_dev->dev, "No IO-ports found!\n");
> -			goto error;
> -		}
>  		/* configure TPM with IO-ports */
>  		outb(IOLIMH, TPM_INF_ADDR);
>  		outb(((tpm_inf.base >> 8) & 0xff), TPM_INF_DATA);
> @@ -466,10 +440,11 @@ static int __devinit tpm_inf_probe(struc
>  		iol = inb(TPM_INF_DATA);
> 
>  		if ((ioh << 8 | iol) != tpm_inf.base) {
> -			dev_err(&pci_dev->dev,
> +			dev_err(&dev->dev,
>  				"Could not set IO-ports to %04x\n",
>  				tpm_inf.base);
> -			goto error;
> +			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
> +			return -EIO;
>  		}
> 
>  		/* activate register */
> @@ -481,7 +456,7 @@ static int __devinit tpm_inf_probe(struc
>  		outb(RESET_LP_IRQC_DISABLE, tpm_inf.base + CMD);
> 
>  		/* Finally, we're done, print some infos */
> -		dev_info(&pci_dev->dev, "TPM found: "
> +		dev_info(&dev->dev, "TPM found: "
>  			 "config base 0x%x, "
>  			 "io base 0x%x, "
>  			 "chip version %02x%02x, "
> @@ -489,67 +464,51 @@ static int __devinit tpm_inf_probe(struc
>  			 "product id %02x%02x"
>  			 "%s\n",
>  			 TPM_INF_ADDR,
> -			 tpm_inf.base,
> +			 TPM_INF_BASE,
>  			 version[0], version[1],
>  			 vendorid[0], vendorid[1],
>  			 productid[0], productid[1], chipname);
> 
> -		rc = tpm_register_hardware(&pci_dev->dev, &tpm_inf);
> -		if (rc < 0)
> -			goto error;
> +		rc = tpm_register_hardware(&dev->dev, &tpm_inf);
> +		if (rc < 0) {
> +			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
> +			return -ENODEV;
> +		}
>  		return 0;
>  	} else {
> -		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
> -error:
> -		pnp_unregister_driver(&tpm_inf_pnp);
> -error2:
> -		pci_disable_device(pci_dev);
> -		pnp_registered = 0;
> +		dev_info(&dev->dev, "No Infineon TPM found!\n");
>  		return -ENODEV;
>  	}
>  }
> 
> -static __devexit void tpm_inf_remove(struct pci_dev* pci_dev)
> +static __devexit void tpm_inf_pnp_remove(struct pnp_dev *dev)
>  {
> -	struct tpm_chip* chip = pci_get_drvdata(pci_dev);
> -	
> -	if( chip )
> +	struct tpm_chip *chip = pnp_get_drvdata(dev);
> +
> +	if (chip) {
> +		release_region(chip->vendor->base, TPM_INF_PORT_LEN);
>  		tpm_remove_hardware(chip->dev);
> +	}
>  }
> 
> -static struct pci_device_id tpm_pci_tbl[] __devinitdata = {
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_12)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_0)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_1)},
> -	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_2)},
> -	{0,}
> -};
> -
> -MODULE_DEVICE_TABLE(pci, tpm_pci_tbl);
> -
> -static struct pci_driver inf_pci_driver = {
> -	.name = "tpm_inf",
> -	.id_table = tpm_pci_tbl,
> -	.probe = tpm_inf_probe,
> -	.remove = __devexit_p(tpm_inf_remove),
> -	.suspend = tpm_pm_suspend,
> -	.resume = tpm_pm_resume,
> +static struct pnp_driver tpm_inf_pnp = {
> +	.name = "tpm_inf_pnp",
> +	.driver.owner = THIS_MODULE,
> +	.id_table = tpm_pnp_tbl,
> +	.probe = tpm_inf_pnp_probe,
> +	.remove = tpm_inf_pnp_remove,
> +	.driver.suspend = tpm_pm_suspend,
> +	.driver.resume = tpm_pm_resume,
>  };
> 
>  static int __init init_inf(void)
>  {
> -	return pci_register_driver(&inf_pci_driver);
> +	return pnp_register_driver(&tpm_inf_pnp);
>  }
> 
>  static void __exit cleanup_inf(void)
>  {
> -	if (pnp_registered)
> -		pnp_unregister_driver(&tpm_inf_pnp);
> -	pci_unregister_driver(&inf_pci_driver);
> +	pnp_unregister_driver(&tpm_inf_pnp);
>  }
> 
>  module_init(init_inf);
> @@ -557,5 +516,5 @@ module_exit(cleanup_inf);
> 
>  MODULE_AUTHOR("Marcel Selhorst <selhorst@crypto.rub.de>");
>  MODULE_DESCRIPTION("Driver for Infineon TPM SLD 9630 TT 1.1 / SLB 9635 TT 1.2");
> -MODULE_VERSION("1.5");
> +MODULE_VERSION("1.6");
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Andrew Morton]

Have just taken a wander through the TPM driver.  I couldn't help myself - are
you OK with the below trivial changes?

Other things:

a)

ssize_t tpm_read(struct file * file, char __user *buf,
		 size_t size, loff_t * off)
{
	struct tpm_chip *chip = file->private_data;
	int ret_size;

	del_singleshot_timer_sync(&chip->user_read_timer);
	ret_size = atomic_read(&chip->data_pending);
	atomic_set(&chip->data_pending, 0);
	if (ret_size > 0) {	/* relay data */
		if (size < ret_size)
			ret_size = size;

		down(&chip->buffer_mutex);
		if (copy_to_user
		    ((void __user *) buf, chip->data_buffer, ret_size))
			ret_size = -EFAULT;

Why is the first arg to copy_to_user() typecast in this manner?  I don't
think the cast needs to be there?


b) user_reader_timeout does down() from within a timer handler!  That's
   deadlocky and is illegal - timer handlers are run from interrupt
   context.

   This should have generated a storm of runtime warnings if tested with
   CONFIG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP.  Developers really should
   enable all the kernel debug options during development - they find bugs.

   Suggest you convert this to using schedule_work() or
   schedule_delayed_work(). 

c) In tpm_remove_hardware():

	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));

   Is that `!' supposed to be there?   Looks odd.

d) How did this happen?

   int tpm_pm_suspend(struct device *dev, pm_message_t pm_state, u32 level)

   device_driver.suspend() only takes two arguments nowadays.


e)

	int tpm_pm_resume(struct device *dev, u32 level)

   Ditto



diff -puN drivers/char/tpm/tpm_atmel.c~tpm-tidies drivers/char/tpm/tpm_atmel.c
--- 25/drivers/char/tpm/tpm_atmel.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_atmel.c	Thu Oct 27 14:52:43 2005
@@ -40,7 +40,7 @@ enum tpm_atmel_read_status {
 	ATML_STATUS_READY = 0x08
 };
 
-static int tpm_atml_recv(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	u8 status, *hdr = buf;
 	u32 size;
@@ -100,7 +100,7 @@ static int tpm_atml_recv(struct tpm_chip
 	return size;
 }
 
-static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_send(struct tpm_chip *chip, u8  *buf, size_t count)
 {
 	int i;
 
@@ -159,12 +159,12 @@ static struct tpm_vendor_specific tpm_at
 	.miscdev = { .fops = &atmel_ops, },
 };
 
-static struct platform_device *pdev = NULL;
+static struct platform_device *pdev;
 
 static void __devexit tpm_atml_remove(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	if ( chip ) {
+	if (chip) {
 		release_region(chip->vendor->base, 2);
 		tpm_remove_hardware(chip->dev);
 	}
@@ -201,19 +201,17 @@ static int __init init_atmel(void)
 		(tpm_read_index(TPM_ADDR, 0x01) != 0x01 ))
 		return -ENODEV;
 
-	pdev = kmalloc(sizeof(struct platform_device), GFP_KERNEL);
+	pdev = kzalloc(sizeof(struct platform_device), GFP_KERNEL);
 	if ( !pdev )
 		return -ENOMEM;
 
-	memset(pdev, 0, sizeof(struct platform_device));
-
 	pdev->name = "tpm_atmel0";
 	pdev->id = -1;
 	pdev->num_resources = 0;
 	pdev->dev.release = tpm_atml_remove;
 	pdev->dev.driver = &atml_drv;
 
-	if ((rc=platform_device_register(pdev)) < 0) {
+	if ((rc = platform_device_register(pdev)) < 0) {
 		kfree(pdev);
 		pdev = NULL;
 		return rc;
@@ -234,7 +232,8 @@ static int __init init_atmel(void)
 		return rc;
 	}
 
-	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n", tpm_atmel.base);
+	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n",
+			tpm_atmel.base);
 	return 0;
 }
 
diff -puN drivers/char/tpm/tpm.c~tpm-tidies drivers/char/tpm/tpm.c
--- 25/drivers/char/tpm/tpm.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm.c	Thu Oct 27 14:52:43 2005
@@ -162,7 +162,8 @@ ssize_t tpm_show_pcrs(struct device *dev
 		    < READ_PCR_RESULT_SIZE){
 			dev_dbg(chip->dev, "A TPM error (%d) occurred"
 				" attempting to read PCR %d of %d\n",
-				be32_to_cpu(*((__be32 *) (data + 6))), i, num_pcrs);
+				be32_to_cpu(*((__be32 *) (data + 6))),
+				i, num_pcrs);
 			goto out;
 		}
 		str += sprintf(str, "PCR-%02d: ", i);
@@ -194,12 +195,11 @@ ssize_t tpm_show_pubek(struct device *de
 	if (chip == NULL)
 		return -ENODEV;
 
-	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
+	data = kxalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	memcpy(data, readpubek, sizeof(readpubek));
-	memset(data + sizeof(readpubek), 0, 20);	/* zero nonce */
 
 	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <
 	    READ_PUBEK_RESULT_SIZE) {
@@ -243,7 +243,6 @@ out:
 	kfree(data);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_show_pubek);
 
 #define CAP_VER_RESULT_SIZE 18
@@ -312,7 +311,6 @@ ssize_t tpm_store_cancel(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_store_cancel);
 
-
 /*
  * Device file system interface to the TPM
  */
@@ -336,8 +334,7 @@ int tpm_open(struct inode *inode, struct
 	}
 
 	if (chip->num_opens) {
-		dev_dbg(chip->dev,
-			"Another process owns this TPM\n");
+		dev_dbg(chip->dev, "Another process owns this TPM\n");
 		rc = -EBUSY;
 		goto err_out;
 	}
@@ -363,7 +360,6 @@ err_out:
 	spin_unlock(&driver_lock);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_open);
 
 int tpm_release(struct inode *inode, struct file *file)
@@ -380,10 +376,9 @@ int tpm_release(struct inode *inode, str
 	spin_unlock(&driver_lock);
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_release);
 
-ssize_t tpm_write(struct file * file, const char __user * buf,
+ssize_t tpm_write(struct file * file, const char __user *buf,
 		  size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -419,7 +414,7 @@ ssize_t tpm_write(struct file * file, co
 
 EXPORT_SYMBOL_GPL(tpm_write);
 
-ssize_t tpm_read(struct file * file, char __user * buf,
+ssize_t tpm_read(struct file * file, char __user *buf,
 		 size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -441,7 +436,6 @@ ssize_t tpm_read(struct file * file, cha
 
 	return ret_size;
 }
-
 EXPORT_SYMBOL_GPL(tpm_read);
 
 void tpm_remove_hardware(struct device *dev)
@@ -465,13 +459,13 @@ void tpm_remove_hardware(struct device *
 
 	sysfs_remove_group(&dev->kobj, chip->vendor->attr_group);
 
-	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
+	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &=
+		!(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
 
 	kfree(chip);
 
 	put_device(dev);
 }
-
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
 static u8 savestate[] = {
@@ -493,7 +487,6 @@ int tpm_pm_suspend(struct device *dev, p
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 
 /*
@@ -509,7 +502,6 @@ int tpm_pm_resume(struct device *dev, u3
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
@@ -519,8 +511,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
  * upon errant exit from this function specific probe function should call
  * pci_disable_device
  */
-int tpm_register_hardware(struct device *dev,
-			  struct tpm_vendor_specific *entry)
+int tpm_register_hardware(struct device *dev, struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -529,12 +520,10 @@ int tpm_register_hardware(struct device 
 	int i, j;
 
 	/* Driver specific per-device data */
-	chip = kmalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
 		return -ENOMEM;
 
-	memset(chip, 0, sizeof(struct tpm_chip));
-
 	init_MUTEX(&chip->buffer_mutex);
 	init_MUTEX(&chip->tpm_mutex);
 	INIT_LIST_HEAD(&chip->list);
@@ -558,8 +547,7 @@ int tpm_register_hardware(struct device 
 
 dev_num_search_complete:
 	if (chip->dev_num < 0) {
-		dev_err(dev,
-			"No available tpm device numbers\n");
+		dev_err(dev, "No available tpm device numbers\n");
 		kfree(chip);
 		return -ENODEV;
 	} else if (chip->dev_num == 0)
@@ -597,7 +585,6 @@ dev_num_search_complete:
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
 
 MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)");
diff -puN drivers/char/tpm/tpm_infineon.c~tpm-tidies drivers/char/tpm/tpm_infineon.c
--- 25/drivers/char/tpm/tpm_infineon.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_infineon.c	Thu Oct 27 14:52:43 2005
@@ -30,10 +30,10 @@
 #define	TPM_INFINEON_DEV_VEN_VALUE	0x15D1
 
 /* These values will be filled after PnP-call */
-static int TPM_INF_DATA = 0;
-static int TPM_INF_ADDR = 0;
-static int TPM_INF_BASE = 0;
-static int TPM_INF_PORT_LEN = 0;
+static int TPM_INF_DATA;
+static int TPM_INF_ADDR;
+static int TPM_INF_BASE;
+static int TPM_INF_PORT_LEN;
 
 /* TPM header definitions */
 enum infineon_tpm_header {
_

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Roland Dreier]

Andrew, a few comments on your trivial comments:

    > -static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
    > +static int tpm_atml_send(struct tpm_chip *chip, u8  *buf, size_t count)

There's still an extra space there I think.

    > -	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
    > +	data = kxalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);

When did we add kxalloc()?  And what does it do?

    > -ssize_t tpm_write(struct file * file, const char __user * buf,
    > +ssize_t tpm_write(struct file * file, const char __user *buf,

Why delete one space between * and buf but not the one between * and file?

 - R.

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Andrew Morton]

Roland Dreier <rolandd@cisco.com> wrote:
>
> Andrew, a few comments on your trivial comments:
> 
>     > -static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
>     > +static int tpm_atml_send(struct tpm_chip *chip, u8  *buf, size_t count)
> 
> There's still an extra space there I think.

yup.

>     > -	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
>     > +	data = kxalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
> 
> When did we add kxalloc()?  And what does it do?

I type with my heels.

>     > -ssize_t tpm_write(struct file * file, const char __user * buf,
>     > +ssize_t tpm_write(struct file * file, const char __user *buf,
> 
> Why delete one space between * and buf but not the one between * and file?


diff -puN drivers/char/tpm/tpm_atmel.c~tpm-tidies drivers/char/tpm/tpm_atmel.c
--- 25/drivers/char/tpm/tpm_atmel.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_atmel.c	Thu Oct 27 14:52:43 2005
@@ -40,7 +40,7 @@ enum tpm_atmel_read_status {
 	ATML_STATUS_READY = 0x08
 };
 
-static int tpm_atml_recv(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	u8 status, *hdr = buf;
 	u32 size;
@@ -100,7 +100,7 @@ static int tpm_atml_recv(struct tpm_chip
 	return size;
 }
 
-static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_send(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	int i;
 
@@ -159,12 +159,12 @@ static struct tpm_vendor_specific tpm_at
 	.miscdev = { .fops = &atmel_ops, },
 };
 
-static struct platform_device *pdev = NULL;
+static struct platform_device *pdev;
 
 static void __devexit tpm_atml_remove(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	if ( chip ) {
+	if (chip) {
 		release_region(chip->vendor->base, 2);
 		tpm_remove_hardware(chip->dev);
 	}
@@ -201,19 +201,17 @@ static int __init init_atmel(void)
 		(tpm_read_index(TPM_ADDR, 0x01) != 0x01 ))
 		return -ENODEV;
 
-	pdev = kmalloc(sizeof(struct platform_device), GFP_KERNEL);
+	pdev = kzalloc(sizeof(struct platform_device), GFP_KERNEL);
 	if ( !pdev )
 		return -ENOMEM;
 
-	memset(pdev, 0, sizeof(struct platform_device));
-
 	pdev->name = "tpm_atmel0";
 	pdev->id = -1;
 	pdev->num_resources = 0;
 	pdev->dev.release = tpm_atml_remove;
 	pdev->dev.driver = &atml_drv;
 
-	if ((rc=platform_device_register(pdev)) < 0) {
+	if ((rc = platform_device_register(pdev)) < 0) {
 		kfree(pdev);
 		pdev = NULL;
 		return rc;
@@ -234,7 +232,8 @@ static int __init init_atmel(void)
 		return rc;
 	}
 
-	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n", tpm_atmel.base);
+	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n",
+			tpm_atmel.base);
 	return 0;
 }
 
diff -puN drivers/char/tpm/tpm.c~tpm-tidies drivers/char/tpm/tpm.c
--- 25/drivers/char/tpm/tpm.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm.c	Thu Oct 27 14:52:43 2005
@@ -162,7 +162,8 @@ ssize_t tpm_show_pcrs(struct device *dev
 		    < READ_PCR_RESULT_SIZE){
 			dev_dbg(chip->dev, "A TPM error (%d) occurred"
 				" attempting to read PCR %d of %d\n",
-				be32_to_cpu(*((__be32 *) (data + 6))), i, num_pcrs);
+				be32_to_cpu(*((__be32 *) (data + 6))),
+				i, num_pcrs);
 			goto out;
 		}
 		str += sprintf(str, "PCR-%02d: ", i);
@@ -194,12 +195,11 @@ ssize_t tpm_show_pubek(struct device *de
 	if (chip == NULL)
 		return -ENODEV;
 
-	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
+	data = kzalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	memcpy(data, readpubek, sizeof(readpubek));
-	memset(data + sizeof(readpubek), 0, 20);	/* zero nonce */
 
 	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <
 	    READ_PUBEK_RESULT_SIZE) {
@@ -243,7 +243,6 @@ out:
 	kfree(data);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_show_pubek);
 
 #define CAP_VER_RESULT_SIZE 18
@@ -312,7 +311,6 @@ ssize_t tpm_store_cancel(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_store_cancel);
 
-
 /*
  * Device file system interface to the TPM
  */
@@ -336,8 +334,7 @@ int tpm_open(struct inode *inode, struct
 	}
 
 	if (chip->num_opens) {
-		dev_dbg(chip->dev,
-			"Another process owns this TPM\n");
+		dev_dbg(chip->dev, "Another process owns this TPM\n");
 		rc = -EBUSY;
 		goto err_out;
 	}
@@ -363,7 +360,6 @@ err_out:
 	spin_unlock(&driver_lock);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_open);
 
 int tpm_release(struct inode *inode, struct file *file)
@@ -380,10 +376,9 @@ int tpm_release(struct inode *inode, str
 	spin_unlock(&driver_lock);
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_release);
 
-ssize_t tpm_write(struct file * file, const char __user * buf,
+ssize_t tpm_write(struct file *file, const char __user *buf,
 		  size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -419,7 +414,7 @@ ssize_t tpm_write(struct file * file, co
 
 EXPORT_SYMBOL_GPL(tpm_write);
 
-ssize_t tpm_read(struct file * file, char __user * buf,
+ssize_t tpm_read(struct file * file, char __user *buf,
 		 size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -441,7 +436,6 @@ ssize_t tpm_read(struct file * file, cha
 
 	return ret_size;
 }
-
 EXPORT_SYMBOL_GPL(tpm_read);
 
 void tpm_remove_hardware(struct device *dev)
@@ -465,13 +459,13 @@ void tpm_remove_hardware(struct device *
 
 	sysfs_remove_group(&dev->kobj, chip->vendor->attr_group);
 
-	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
+	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &=
+		!(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
 
 	kfree(chip);
 
 	put_device(dev);
 }
-
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
 static u8 savestate[] = {
@@ -493,7 +487,6 @@ int tpm_pm_suspend(struct device *dev, p
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 
 /*
@@ -509,7 +502,6 @@ int tpm_pm_resume(struct device *dev, u3
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
@@ -519,8 +511,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
  * upon errant exit from this function specific probe function should call
  * pci_disable_device
  */
-int tpm_register_hardware(struct device *dev,
-			  struct tpm_vendor_specific *entry)
+int tpm_register_hardware(struct device *dev, struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -529,12 +520,10 @@ int tpm_register_hardware(struct device 
 	int i, j;
 
 	/* Driver specific per-device data */
-	chip = kmalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
 		return -ENOMEM;
 
-	memset(chip, 0, sizeof(struct tpm_chip));
-
 	init_MUTEX(&chip->buffer_mutex);
 	init_MUTEX(&chip->tpm_mutex);
 	INIT_LIST_HEAD(&chip->list);
@@ -558,8 +547,7 @@ int tpm_register_hardware(struct device 
 
 dev_num_search_complete:
 	if (chip->dev_num < 0) {
-		dev_err(dev,
-			"No available tpm device numbers\n");
+		dev_err(dev, "No available tpm device numbers\n");
 		kfree(chip);
 		return -ENODEV;
 	} else if (chip->dev_num == 0)
@@ -597,7 +585,6 @@ dev_num_search_complete:
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
 
 MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)");
diff -puN drivers/char/tpm/tpm_infineon.c~tpm-tidies drivers/char/tpm/tpm_infineon.c
--- 25/drivers/char/tpm/tpm_infineon.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_infineon.c	Thu Oct 27 14:52:43 2005
@@ -30,10 +30,10 @@
 #define	TPM_INFINEON_DEV_VEN_VALUE	0x15D1
 
 /* These values will be filled after PnP-call */
-static int TPM_INF_DATA = 0;
-static int TPM_INF_ADDR = 0;
-static int TPM_INF_BASE = 0;
-static int TPM_INF_PORT_LEN = 0;
+static int TPM_INF_DATA;
+static int TPM_INF_ADDR;
+static int TPM_INF_BASE;
+static int TPM_INF_PORT_LEN;
 
 /* TPM header definitions */
 enum infineon_tpm_header {
_

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Kylene Jo Hall]

Acked-by: Kylene Hall <kjhall@us.ibm.com>

On Thu, 2005-10-27 at 15:26 -0700, Andrew Morton wrote:
> Roland Dreier <rolandd@cisco.com> wrote:
> >
> > Andrew, a few comments on your trivial comments:
> > 
> >     > -static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
> >     > +static int tpm_atml_send(struct tpm_chip *chip, u8  *buf, size_t count)
> > 
> > There's still an extra space there I think.
> 
> yup.
> 
> >     > -	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
> >     > +	data = kxalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
> > 
> > When did we add kxalloc()?  And what does it do?
> 
> I type with my heels.
> 
> >     > -ssize_t tpm_write(struct file * file, const char __user * buf,
> >     > +ssize_t tpm_write(struct file * file, const char __user *buf,
> > 
> > Why delete one space between * and buf but not the one between * and file?
> 
> 
> diff -puN drivers/char/tpm/tpm_atmel.c~tpm-tidies drivers/char/tpm/tpm_atmel.c
> --- 25/drivers/char/tpm/tpm_atmel.c~tpm-tidies	Thu Oct 27 14:52:43 2005
> +++ 25-akpm/drivers/char/tpm/tpm_atmel.c	Thu Oct 27 14:52:43 2005
> @@ -40,7 +40,7 @@ enum tpm_atmel_read_status {
>  	ATML_STATUS_READY = 0x08
>  };
>  
> -static int tpm_atml_recv(struct tpm_chip *chip, u8 * buf, size_t count)
> +static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  {
>  	u8 status, *hdr = buf;
>  	u32 size;
> @@ -100,7 +100,7 @@ static int tpm_atml_recv(struct tpm_chip
>  	return size;
>  }
>  
> -static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
> +static int tpm_atml_send(struct tpm_chip *chip, u8 *buf, size_t count)
>  {
>  	int i;
>  
> @@ -159,12 +159,12 @@ static struct tpm_vendor_specific tpm_at
>  	.miscdev = { .fops = &atmel_ops, },
>  };
>  
> -static struct platform_device *pdev = NULL;
> +static struct platform_device *pdev;
>  
>  static void __devexit tpm_atml_remove(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> -	if ( chip ) {
> +	if (chip) {
>  		release_region(chip->vendor->base, 2);
>  		tpm_remove_hardware(chip->dev);
>  	}
> @@ -201,19 +201,17 @@ static int __init init_atmel(void)
>  		(tpm_read_index(TPM_ADDR, 0x01) != 0x01 ))
>  		return -ENODEV;
>  
> -	pdev = kmalloc(sizeof(struct platform_device), GFP_KERNEL);
> +	pdev = kzalloc(sizeof(struct platform_device), GFP_KERNEL);
>  	if ( !pdev )
>  		return -ENOMEM;
>  
> -	memset(pdev, 0, sizeof(struct platform_device));
> -
>  	pdev->name = "tpm_atmel0";
>  	pdev->id = -1;
>  	pdev->num_resources = 0;
>  	pdev->dev.release = tpm_atml_remove;
>  	pdev->dev.driver = &atml_drv;
>  
> -	if ((rc=platform_device_register(pdev)) < 0) {
> +	if ((rc = platform_device_register(pdev)) < 0) {
>  		kfree(pdev);
>  		pdev = NULL;
>  		return rc;
> @@ -234,7 +232,8 @@ static int __init init_atmel(void)
>  		return rc;
>  	}
>  
> -	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n", tpm_atmel.base);
> +	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n",
> +			tpm_atmel.base);
>  	return 0;
>  }
>  
> diff -puN drivers/char/tpm/tpm.c~tpm-tidies drivers/char/tpm/tpm.c
> --- 25/drivers/char/tpm/tpm.c~tpm-tidies	Thu Oct 27 14:52:43 2005
> +++ 25-akpm/drivers/char/tpm/tpm.c	Thu Oct 27 14:52:43 2005
> @@ -162,7 +162,8 @@ ssize_t tpm_show_pcrs(struct device *dev
>  		    < READ_PCR_RESULT_SIZE){
>  			dev_dbg(chip->dev, "A TPM error (%d) occurred"
>  				" attempting to read PCR %d of %d\n",
> -				be32_to_cpu(*((__be32 *) (data + 6))), i, num_pcrs);
> +				be32_to_cpu(*((__be32 *) (data + 6))),
> +				i, num_pcrs);
>  			goto out;
>  		}
>  		str += sprintf(str, "PCR-%02d: ", i);
> @@ -194,12 +195,11 @@ ssize_t tpm_show_pubek(struct device *de
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
> +	data = kzalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
>  	memcpy(data, readpubek, sizeof(readpubek));
> -	memset(data + sizeof(readpubek), 0, 20);	/* zero nonce */
>  
>  	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <
>  	    READ_PUBEK_RESULT_SIZE) {
> @@ -243,7 +243,6 @@ out:
>  	kfree(data);
>  	return rc;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_show_pubek);
>  
>  #define CAP_VER_RESULT_SIZE 18
> @@ -312,7 +311,6 @@ ssize_t tpm_store_cancel(struct device *
>  }
>  EXPORT_SYMBOL_GPL(tpm_store_cancel);
>  
> -
>  /*
>   * Device file system interface to the TPM
>   */
> @@ -336,8 +334,7 @@ int tpm_open(struct inode *inode, struct
>  	}
>  
>  	if (chip->num_opens) {
> -		dev_dbg(chip->dev,
> -			"Another process owns this TPM\n");
> +		dev_dbg(chip->dev, "Another process owns this TPM\n");
>  		rc = -EBUSY;
>  		goto err_out;
>  	}
> @@ -363,7 +360,6 @@ err_out:
>  	spin_unlock(&driver_lock);
>  	return rc;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_open);
>  
>  int tpm_release(struct inode *inode, struct file *file)
> @@ -380,10 +376,9 @@ int tpm_release(struct inode *inode, str
>  	spin_unlock(&driver_lock);
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_release);
>  
> -ssize_t tpm_write(struct file * file, const char __user * buf,
> +ssize_t tpm_write(struct file *file, const char __user *buf,
>  		  size_t size, loff_t * off)
>  {
>  	struct tpm_chip *chip = file->private_data;
> @@ -419,7 +414,7 @@ ssize_t tpm_write(struct file * file, co
>  
>  EXPORT_SYMBOL_GPL(tpm_write);
>  
> -ssize_t tpm_read(struct file * file, char __user * buf,
> +ssize_t tpm_read(struct file * file, char __user *buf,
>  		 size_t size, loff_t * off)
>  {
>  	struct tpm_chip *chip = file->private_data;
> @@ -441,7 +436,6 @@ ssize_t tpm_read(struct file * file, cha
>  
>  	return ret_size;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_read);
>  
>  void tpm_remove_hardware(struct device *dev)
> @@ -465,13 +459,13 @@ void tpm_remove_hardware(struct device *
>  
>  	sysfs_remove_group(&dev->kobj, chip->vendor->attr_group);
>  
> -	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
> +	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &=
> +		!(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
>  
>  	kfree(chip);
>  
>  	put_device(dev);
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
>  
>  static u8 savestate[] = {
> @@ -493,7 +487,6 @@ int tpm_pm_suspend(struct device *dev, p
>  	tpm_transmit(chip, savestate, sizeof(savestate));
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_pm_suspend);
>  
>  /*
> @@ -509,7 +502,6 @@ int tpm_pm_resume(struct device *dev, u3
>  
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_pm_resume);
>  
>  /*
> @@ -519,8 +511,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
>   * upon errant exit from this function specific probe function should call
>   * pci_disable_device
>   */
> -int tpm_register_hardware(struct device *dev,
> -			  struct tpm_vendor_specific *entry)
> +int tpm_register_hardware(struct device *dev, struct tpm_vendor_specific *entry)
>  {
>  #define DEVNAME_SIZE 7
>  
> @@ -529,12 +520,10 @@ int tpm_register_hardware(struct device 
>  	int i, j;
>  
>  	/* Driver specific per-device data */
> -	chip = kmalloc(sizeof(*chip), GFP_KERNEL);
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>  	if (chip == NULL)
>  		return -ENOMEM;
>  
> -	memset(chip, 0, sizeof(struct tpm_chip));
> -
>  	init_MUTEX(&chip->buffer_mutex);
>  	init_MUTEX(&chip->tpm_mutex);
>  	INIT_LIST_HEAD(&chip->list);
> @@ -558,8 +547,7 @@ int tpm_register_hardware(struct device 
>  
>  dev_num_search_complete:
>  	if (chip->dev_num < 0) {
> -		dev_err(dev,
> -			"No available tpm device numbers\n");
> +		dev_err(dev, "No available tpm device numbers\n");
>  		kfree(chip);
>  		return -ENODEV;
>  	} else if (chip->dev_num == 0)
> @@ -597,7 +585,6 @@ dev_num_search_complete:
>  
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL_GPL(tpm_register_hardware);
>  
>  MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)");
> diff -puN drivers/char/tpm/tpm_infineon.c~tpm-tidies drivers/char/tpm/tpm_infineon.c
> --- 25/drivers/char/tpm/tpm_infineon.c~tpm-tidies	Thu Oct 27 14:52:43 2005
> +++ 25-akpm/drivers/char/tpm/tpm_infineon.c	Thu Oct 27 14:52:43 2005
> @@ -30,10 +30,10 @@
>  #define	TPM_INFINEON_DEV_VEN_VALUE	0x15D1
>  
>  /* These values will be filled after PnP-call */
> -static int TPM_INF_DATA = 0;
> -static int TPM_INF_ADDR = 0;
> -static int TPM_INF_BASE = 0;
> -static int TPM_INF_PORT_LEN = 0;
> +static int TPM_INF_DATA;
> +static int TPM_INF_ADDR;
> +static int TPM_INF_BASE;
> +static int TPM_INF_PORT_LEN;
>  
>  /* TPM header definitions */
>  enum infineon_tpm_header {
> _
> 
> 

[PATCH] TPM: cleanups

[Kylene Jo Hall]

Patch to clean up a couple of issues recently pointed out with the
driver.  The fix for issue (c) was also pointed out by Steve Tate on the
tpm-devel list.


On Thu, 2005-10-27 at 14:55 -0700, Andrew Morton wrote: 
> Have just taken a wander through the TPM driver.  I couldn't help myself - are
> you OK with the below trivial changes?
> 
> Other things:
> 
> a)
> 
> ssize_t tpm_read(struct file * file, char __user *buf,
> 		 size_t size, loff_t * off)
> {
> f	struct tpm_chip *chip = file->private_data;
> 	int ret_size;
> 
> 	del_singleshot_timer_sync(&chip->user_read_timer);
> 	ret_size = atomic_read(&chip->data_pending);
> 	atomic_set(&chip->data_pending, 0);
> 	if (ret_size > 0) {	/* relay data */
> 		if (size < ret_size)
> 			ret_size = size;
> 
> 		down(&chip->buffer_mutex);
> 		if (copy_to_user
> 		    ((void __user *) buf, chip->data_buffer, ret_size))
> 			ret_size = -EFAULT;
> 
> Why is the first arg to copy_to_user() typecast in this manner?  I don't
> think the cast needs to be there?
> 
Fixed in the patch below.

> 
> b) user_reader_timeout does down() from within a timer handler!  That's
>    deadlocky and is illegal - timer handlers are run from interrupt
>    context.
> 
>    This should have generated a storm of runtime warnings if tested with
>    CONFIG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP.  Developers really should
>    enable all the kernel debug options during development - they find bugs.
> 
>    Suggest you convert this to using schedule_work() or
>    schedule_delayed_work(). 
> 
I'll look into this.

> c) In tpm_remove_hardware():
> 
> 	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
> 
>    Is that `!' supposed to be there?   Looks odd.
> 
Fixed in the patch below.

> d) How did this happen?
> 
>    int tpm_pm_suspend(struct device *dev, pm_message_t pm_state, u32 level)
> 
>    device_driver.suspend() only takes two arguments nowadays.
> 
> 
> e)
> 
> 	int tpm_pm_resume(struct device *dev, u32 level)
> 
>    Ditto
> 

>

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>
---
--- linux-2.6.14/drivers/char/tpm/tpm.c.orig	2005-11-11 14:08:48.000000000 -0600
+++ linux-2.6.14/drivers/char/tpm/tpm.c	2005-11-11 14:09:47.000000000
-0600
@@ -428,8 +428,7 @@ ssize_t tpm_read(struct file * file, cha
 			ret_size = size;
 
 		down(&chip->buffer_mutex);
-		if (copy_to_user
-		    ((void __user *) buf, chip->data_buffer, ret_size))
+		if (copy_to_user(buf, chip->data_buffer, ret_size))
 			ret_size = -EFAULT;
 		up(&chip->buffer_mutex);
 	}
@@ -460,7 +459,7 @@ void tpm_remove_hardware(struct device *
 	sysfs_remove_group(&dev->kobj, chip->vendor->attr_group);
 
 	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &=
-		!(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
+		~(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
 
 	kfree(chip);
 

Re: [PATCH] TPM: cleanups

[Kylene Jo Hall]

> > 
> > b) user_reader_timeout does down() from within a timer handler!  That's
> >    deadlocky and is illegal - timer handlers are run from interrupt
> >    context.
> > 
> >    This should have generated a storm of runtime warnings if tested with
> >    CONFIG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP.  Developers really should
> >    enable all the kernel debug options during development - they find bugs.
> > 
> >    Suggest you convert this to using schedule_work() or
> >    schedule_delayed_work(). 
> > 
> I'll look into this.

Addressed this timer/interrupt/spinlock issue with schedule_work.

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>
---
diff -urpN --exclude='*.o' --exclude='*.ko' --exclude='*.orig' --exclude='*mod*' --exclude='.*' --exclude='tpm_*' --exclude='*~' --exclude='*.rej' linux-2.6.14/drivers/char/tpm/tpm.c linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.14/drivers/char/tpm/tpm.c	2005-11-11 14:09:47.000000000 -0600
+++ linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.c	2005-11-11 15:40:34.000000000 -0600
@@ -43,6 +43,13 @@ static void user_reader_timeout(unsigned
 {
 	struct tpm_chip *chip = (struct tpm_chip *) ptr;
 
+	schedule_work(&chip->work);
+}
+
+static void timeout_work(void * ptr)
+{
+	struct tpm_chip *chip = (struct tpm_chip*) ptr;
+
 	down(&chip->buffer_mutex);
 	atomic_set(&chip->data_pending, 0);
 	memset(chip->data_buffer, 0, TPM_BUFSIZE);
@@ -527,6 +535,8 @@ int tpm_register_hardware(struct device 
 	init_MUTEX(&chip->tpm_mutex);
 	INIT_LIST_HEAD(&chip->list);
 
+	INIT_WORK(&chip->work, timeout_work, chip);
+
 	init_timer(&chip->user_read_timer);
 	chip->user_read_timer.function = user_reader_timeout;
 	chip->user_read_timer.data = (unsigned long) chip;
diff -urpN --exclude='*.o' --exclude='*.ko' --exclude='*.orig' --exclude='*mod*' --exclude='.*' --exclude='tpm_*' --exclude='*~' --exclude='*.rej' linux-2.6.14/drivers/char/tpm/tpm.h linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.h
--- linux-2.6.14/drivers/char/tpm/tpm.h	2005-11-11 16:44:23.000000000 -0600
+++ linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.h	2005-11-11 15:39:03.000000000 -0600
@@ -77,6 +77,7 @@ struct tpm_chip {
 	struct semaphore buffer_mutex;
 
 	struct timer_list user_read_timer;	/* user needs to claim result */
+	struct work_struct work;
 	struct semaphore tpm_mutex;	/* tpm is processing */
 
 	struct tpm_vendor_specific *vendor;

Re: [PATCH] TPM: cleanups

[Andrew Morton]

Kylene Jo Hall <kjhall@us.ibm.com> wrote:
>
>  +	schedule_work(&chip->work);
>  +}
>  +
>  +static void timeout_work(void * ptr)
>  +{
>  +	struct tpm_chip *chip = (struct tpm_chip*) ptr;
>  +

I cannot see where the tpm driver stops that timer which it has running on
device close or on module unload.

Wherever it is, we'll now also need a flush_scheduled_work() to avoid a race
wherein the work handler is still executing while the module gets
unloaded.

Re: [PATCH] TPM: cleanups

[Kylene Jo Hall]

Patch to add the necessary flush_schedule_work calls when canceling the
timer.

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>

On Sat, 2005-11-12 at 13:48 -0800, Andrew Morton wrote:
> Kylene Jo Hall <kjhall@us.ibm.com> wrote:
> >
> >  +	schedule_work(&chip->work);
> >  +}
> >  +
> >  +static void timeout_work(void * ptr)
> >  +{
> >  +	struct tpm_chip *chip = (struct tpm_chip*) ptr;
> >  +
> 
> I cannot see where the tpm driver stops that timer which it has running on
> device close or on module unload.
> 
> Wherever it is, we'll now also need a flush_scheduled_work() to avoid a race
> wherein the work handler is still executing while the module gets
> unloaded.
> 

---
diff -uprN --exclude='.*' --exclude='*~' --exclude='*.o' --exclude='*.ko' --exclude='*.rej' --exclude='*.orig' --exclude='*infineon*' --exclude='*nsc*' --exclude='*mod*' linux-2.6.14/drivers/char/tpm/tpm.c linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.14/drivers/char/tpm/tpm.c	2005-11-15 15:42:10.000000000 -0600
+++ linux-2.6.14-rc4-tpm/drivers/char/tpm/tpm.c	2005-11-15 14:11:20.000000000 -0600
@@ -377,6 +377,7 @@ int tpm_release(struct inode *inode, str
 	file->private_data = NULL;
 	chip->num_opens--;
 	del_singleshot_timer_sync(&chip->user_read_timer);
+	flush_scheduled_work();
 	atomic_set(&chip->data_pending, 0);
 	put_device(chip->dev);
 	kfree(chip->data_buffer);
@@ -428,6 +429,7 @@ ssize_t tpm_read(struct file * file, cha
 	int ret_size;
 
 	del_singleshot_timer_sync(&chip->user_read_timer);
+	flush_scheduled_work();
 	ret_size = atomic_read(&chip->data_pending);
 	atomic_set(&chip->data_pending, 0);
 	if (ret_size > 0) {	/* relay data */

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Andrew Morton]

Marcel Selhorst <selhorst@crypto.rub.de> wrote:
>
> @@ -489,67 +464,51 @@ static int __devinit tpm_inf_probe(struc
>  			 "product id %02x%02x"
>  			 "%s\n",
>  			 TPM_INF_ADDR,
> -			 tpm_inf.base,
> +			 TPM_INF_BASE,
>  			 version[0], version[1],
>  			 vendorid[0], vendorid[1],
>  			 productid[0], productid[1], chipname);
> 
> -		rc = tpm_register_hardware(&pci_dev->dev, &tpm_inf);
> -		if (rc < 0)
> -			goto error;
> +		rc = tpm_register_hardware(&dev->dev, &tpm_inf);
> +		if (rc < 0) {
> +			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
> +			return -ENODEV;
> +		}
>  		return 0;
>  	} else {
> -		dev_info(&pci_dev->dev, "No Infineon TPM found!\n");
> -error:
> -		pnp_unregister_driver(&tpm_inf_pnp);
> -error2:
> -		pci_disable_device(pci_dev);
> -		pnp_registered = 0;
> +		dev_info(&dev->dev, "No Infineon TPM found!\n");
>  		return -ENODEV;
>  	}
>  }

This final return will leak the I/O region from request_region().

To reduce the chance of this happening again, please send a followup patch
which prevents this function from having `return' statements sprinkled all
over it.  An example would be drivers/net/8139cp.c:cp_init_one(), thanks.

If for some reason the leak is deliberate then a comment is needed.

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Marcel Selhorst]

Hi Andrew,

> This final return will leak the I/O region from request_region().
> If for some reason the leak is deliberate then a comment is needed.

yep, you're right, I'll fix that

> To reduce the chance of this happening again, please send a followup patch
> which prevents this function from having `return' statements sprinkled all
> over it.  An example would be drivers/net/8139cp.c:cp_init_one(), thanks.

I will send a follow-up patch including the comments of Matthieu.

Thanks for reviewing!

Marcel

Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[matthieu castet]

Hi,

Marcel Selhorst wrote:
 > Dear all,
 >
 > the following patch moves the Infineon TPM driver off pci device
 > and makes it a pure pnp-driver. It was tested with IFX0101 and
 > IFX0102 and is now based on the new tpm patchset (1 to 5) from Kylene
 > Hall submitted two days ago. It now also includes pnp-port validation
 > and region requesting.
 >
 > Best regards,
 >
 > Marcel Selhorst
 >
 > Signed-off-by: Marcel Selhorst <selhorst@crypto.rub.de>
 > ---
 >

 > +	/* read IO-ports through PnP */
 > +	if (pnp_port_valid(dev, 0) && pnp_port_valid(dev, 1) &&
 > +	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
&& !(pnp_port_flags(dev, 1) & IORESOURCE_DISABLED)

 > +		TPM_INF_ADDR = pnp_port_start(dev, 0);
 > +		TPM_INF_DATA = (TPM_INF_ADDR + 1);
 > +		TPM_INF_BASE = pnp_port_start(dev, 1);
 > +		TPM_INF_PORT_LEN = pnp_port_len(dev, 1);
 > +		if (!TPM_INF_PORT_LEN)
 > +			return -EINVAL;
When I said to check the lenght I was thinking about
if (pnp_port_len(dev, 1) < 4) return -EINVAL;
According to infineon_tpm_register, the length is at least 4.

I whould also check that pnp_port_len(dev, 0) < 2.





 > +		dev_info(&dev->dev, "Found %s with ID %s\n",
 > +			 dev->name, dev_id->id);
 > +		if (!((TPM_INF_BASE >> 8) & 0xff))
 > +			return -EINVAL;
 > +		/* publish my base address and request region */
 > +		tpm_inf.base = TPM_INF_BASE;
 > +		if (request_region
 > +		    (tpm_inf.base, TPM_INF_PORT_LEN, "tpm_infineon0") == NULL) {
 > +			release_region(tpm_inf.base, TPM_INF_PORT_LEN);
if it failed, you don't need to release the region

a request_region (TPM_INF_ADDR, 2, "tpm_infineon0") could be usefull.

 > +			return -EINVAL;
 > +		}
 >  	} else {