Infineon TPM: move infineon driver off pci_dev

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 <[email protected]>

---

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

(quoted diff) +50 -97

@@ -5,6 +5,7 @@
  * Specifications at www.trustedcomputinggroup.org
  *
  * Copyright (C) 2005, Marcel Selhorst <[email protected]>
+ * 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 <[email protected]>");
 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");

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 <[email protected]>

---

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

(quoted diff) +1 -0

@@ -5,6 +5,7 @@
  * Specifications at www.trustedcomputinggroup.org
  *
  * Copyright (C) 2005, Marcel Selhorst <[email protected]>
+ * 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
  *

Review

Quoted 6 lines

(quoted diff) +6 -0

+    /* 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.

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 <[email protected]>

---

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

(quoted diff) +58 -99

@@ -5,6 +5,7 @@
  * Specifications at www.trustedcomputinggroup.org
  *
  * Copyright (C) 2005, Marcel Selhorst <[email protected]>
+ * 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 <[email protected]>");
 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");

(quoted diff) +6 -4

@@ -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 {

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 <[email protected]>

---

(quoted diff) +2 -0

+			return -EINVAL;
+		}
 	} else {

Andrew, a few comments on your trivial comments:

Review

Quoted 2 lines

(quoted diff) +1 -1

-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.

Review

Quoted 2 lines

(quoted diff) +1 -1

-	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?

Review

Quoted 2 lines

(quoted diff) +1 -1

-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?

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.

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.

Quoted 7 lines · Roland Dreier

Andrew, a few comments on your trivial comments:

drivers/char/tpm/tpm_atmel.c static int tpm_atml_recv(struct tpm_chip · line 103

Quoted 2 lines

drivers/char/tpm/tpm_atmel.c renamed from drivers/char/tpm/tpm_atmel.c~tpm-tidies +1 -1

-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.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-	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.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-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

drivers/char/tpm/tpm_atmel.c renamed from drivers/char/tpm/tpm_atmel.c~tpm-tidies +8 -9

@@ -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

drivers/char/tpm/tpm.c renamed from drivers/char/tpm/tpm.c~tpm-tidies +11 -24

@@ -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 ([email protected])");

diff -puN drivers/char/tpm/tpm_infineon.c~tpm-tidies drivers/char/tpm/tpm_infineon.c

drivers/char/tpm/tpm_infineon.c renamed from drivers/char/tpm/tpm_infineon.c~tpm-tidies +4 -4

@@ -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 {

_

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?

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

Quoted 6 lines

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

Quoted 7 lines

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.

+ schedule_work(&chip->work);
+}
+
+static void timeout_work(void * ptr)
+{
+ struct tpm_chip *chip = (struct tpm_chip*) ptr;
+

Quoted 8 lines · Kylene Jo Hall

+ 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.