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
@@ -5,6 +5,7 @@
* Specifications at http://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");
Signed-off-by: Kylene Hall <[email protected]>
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 <[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
> @@ -5,6 +5,7 @@
> * Specifications at http://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");
>
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 <[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
> @@ -5,6 +5,7 @@
> * Specifications at http://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 */
> + /* 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
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
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
@@ -5,6 +5,7 @@
* Specifications at http://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");
Signed-off-by: Kylene Hall <[email protected]>
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 <[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
> @@ -5,6 +5,7 @@
> * Specifications at http://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");
>
Marcel Selhorst <[email protected]> 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.
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 <[email protected]>
> ---
>
> + /* 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 {
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 ([email protected])");
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 {
_
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.
Roland Dreier <[email protected]> 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 ([email protected])");
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 {
_
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
Acked-by: Kylene Hall <[email protected]>
On Thu, 2005-10-27 at 15:26 -0700, Andrew Morton wrote:
> Roland Dreier <[email protected]> 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 ([email protected])");
> 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 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 <[email protected]>
---
--- 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);
> >
> > 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 <[email protected]>
---
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;
Kylene Jo Hall <[email protected]> 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.
Patch to add the necessary flush_schedule_work calls when canceling the
timer.
Signed-off-by: Kylene Hall <[email protected]>
On Sat, 2005-11-12 at 13:48 -0800, Andrew Morton wrote:
> Kylene Jo Hall <[email protected]> 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 */