| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <A3258B497F1C1441AD36A2E8B45FD628CB9CB2@scsmsx412.amr.corp.intel.com>
Date: Thu, 11 Oct 2007 14:08:17 -0700
From: "Agarwal, Lomesh" <lomesh.agarwal@...el.com>
To: "Randy Dunlap" <randy.dunlap@...cle.com>
Cc: <Valdis.Kletnieks@...edu>, <linux-kernel@...r.kernel.org>
Subject: RE: TPM driver changes to support multiple locality
Here is info. -
This patch adds multiple locality support in tpm_tis driver.
drivers/char/tpm/tpm_tis.c | 83 ++++++++++++++++++++++++-----------
1 file changed, 57 insertions(+), 26 deletions(-)
--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
20:42:06.000000000 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-11
13:28:38.000000000 -0700
@@ -56,29 +56,37 @@ enum tis_int_flags {
enum tis_defaults {
TIS_MEM_BASE = 0xFED40000,
- TIS_MEM_LEN = 0x5000,
+ TIS_LOCALITY_SHIFT = 12,
+ TIS_MEM_LEN = 0x1000,
TIS_SHORT_TIMEOUT = 750, /* ms */
TIS_LONG_TIMEOUT = 2000, /* 2 sec */
};
-#define TPM_ACCESS(l) (0x0000 | ((l) << 12))
-#define TPM_INT_ENABLE(l) (0x0008 | ((l) << 12))
-#define TPM_INT_VECTOR(l) (0x000C | ((l) << 12))
-#define TPM_INT_STATUS(l) (0x0010 | ((l) << 12))
-#define TPM_INTF_CAPS(l) (0x0014 | ((l) << 12))
-#define TPM_STS(l) (0x0018 | ((l) << 12))
-#define TPM_DATA_FIFO(l) (0x0024 | ((l) << 12))
+#define TPM_ACCESS(l) (0x0000)
+#define TPM_INT_ENABLE(l) (0x0008)
+#define TPM_INT_VECTOR(l) (0x000C)
+#define TPM_INT_STATUS(l) (0x0010)
+#define TPM_INTF_CAPS(l) (0x0014)
+#define TPM_STS(l) (0x0018)
+#define TPM_DATA_FIFO(l) (0x0024)
-#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
-#define TPM_RID(l) (0x0F04 | ((l) << 12))
+#define TPM_DID_VID(l) (0x0F00)
+#define TPM_RID(l) (0x0F04)
static LIST_HEAD(tis_chips);
static DEFINE_SPINLOCK(tis_lock);
static int check_locality(struct tpm_chip *chip, int l)
{
- if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
- (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
+ unsigned char tpm_access;
+
+ tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
+
+ /* check if locality is closed */
+ if(tpm_access == 0xFF)
+ return -1;
+
+ if ((tpm_access & (TPM_ACCESS_ACTIVE_LOCALITY |
TPM_ACCESS_VALID)) ==
(TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
return chip->vendor.locality = l;
@@ -251,10 +259,14 @@ static int tpm_tis_recv(struct tpm_chip
out:
tpm_tis_ready(chip);
- release_locality(chip, chip->vendor.locality, 0);
+ release_locality(chip, chip->vendor.locality, 1);
return size;
}
+static int locality = 0;
+module_param(locality, int, 0444);
+MODULE_PARM_DESC(locality, "TPM Locality To access");
+
/*
* If interrupts are used (signaled by an irq set in the vendor
structure)
* tpm.c can skip polling for the data to be available as the interrupt
is
@@ -266,7 +278,7 @@ static int tpm_tis_send(struct tpm_chip
size_t count = 0;
u32 ordinal;
- if (request_locality(chip, 0) < 0)
+ if (request_locality(chip, locality) < 0)
return -EBUSY;
status = tpm_tis_status(chip);
@@ -326,7 +338,7 @@ static int tpm_tis_send(struct tpm_chip
return len;
out_err:
tpm_tis_ready(chip);
- release_locality(chip, chip->vendor.locality, 0);
+ release_locality(chip, chip->vendor.locality, 1);
return rc;
}
@@ -401,7 +413,10 @@ static irqreturn_t tis_int_handler(int i
{
struct tpm_chip *chip = (struct tpm_chip *) dev_id;
u32 interrupt;
- int i;
+
+ /* check if interrupt is meant for this locality */
+ if(check_locality(chip, locality) < 0)
+ return IRQ_NONE;
interrupt = ioread32(chip->vendor.iobase +
TPM_INT_STATUS(chip->vendor.locality));
@@ -411,10 +426,6 @@ static irqreturn_t tis_int_handler(int i
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&chip->vendor.read_queue);
- if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
- for (i = 0; i < 5; i++)
- if (check_locality(chip, i) >= 0)
- break;
if (interrupt &
(TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
TPM_INTF_CMD_READY_INT))
@@ -440,7 +451,7 @@ static int tpm_tis_init(struct device *d
struct tpm_chip *chip;
if (!start)
- start = TIS_MEM_BASE;
+ start = TIS_MEM_BASE | (locality << TIS_LOCALITY_SHIFT);
if (!len)
len = TIS_MEM_LEN;
@@ -490,8 +501,9 @@ static int tpm_tis_init(struct device *d
if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
dev_dbg(dev, "\tData Avail Int Support\n");
- if (request_locality(chip, 0) != 0) {
- rc = -ENODEV;
+ if (request_locality(chip, locality) < 0) {
+ rc = -EBUSY;
+ printk("tpm_tis: failed request_locality %d\n",
locality);
goto out_err;
}
@@ -582,9 +594,11 @@ static int tpm_tis_init(struct device *d
tpm_get_timeouts(chip);
tpm_continue_selftest(chip);
+ release_locality(chip, chip->vendor.locality, 1);
return 0;
out_err:
+ release_locality(chip, chip->vendor.locality, 1);
if (chip->vendor.iobase)
iounmap(chip->vendor.iobase);
tpm_remove_hardware(chip->dev);
@@ -636,7 +650,7 @@ module_param_string(hid, tpm_pnp_tbl[TIS
MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to
probe");
static struct device_driver tis_drv = {
- .name = "tpm_tis",
+ .name = "",
.bus = &platform_bus_type,
.owner = THIS_MODULE,
.suspend = tpm_pm_suspend,
@@ -648,19 +662,34 @@ static struct platform_device *pdev;
static int force;
module_param(force, bool, 0444);
MODULE_PARM_DESC(force, "Force device probe rather than using ACPI
entry");
+
+static char *devname;
+
static int __init init_tis(void)
{
+#define DEVNAME_SIZE 10
+
int rc;
+ if ((locality < 0) || (locality > 4)) {
+ return PTR_ERR(pdev);
+ }
+
if (force) {
+ devname = kmalloc(DEVNAME_SIZE, GFP_KERNEL);
+ scnprintf(devname, DEVNAME_SIZE, "%s%d", "tpm_tis",
locality);
+
+ tis_drv.name = devname;
rc = driver_register(&tis_drv);
if (rc < 0)
return rc;
- if
(IS_ERR(pdev=platform_device_register_simple("tpm_tis", -1, NULL, 0)))
+
+ if (IS_ERR(pdev=platform_device_register_simple(devname,
-1, NULL, 0)))
return PTR_ERR(pdev);
if((rc=tpm_tis_init(&pdev->dev, 0, 0)) != 0) {
platform_device_unregister(pdev);
driver_unregister(&tis_drv);
+ kfree(devname);
}
return rc;
}
@@ -692,7 +721,9 @@ static void __exit cleanup_tis(void)
if (force) {
platform_device_unregister(pdev);
driver_unregister(&tis_drv);
- } else
+ kfree(devname);
+ }
+ else
pnp_unregister_driver(&tis_pnp_driver);
}
I don't have scripts/checkpatch.pl in my linux tree. Where can I get
one?
Devname is being used in 2 functions. That's why it is global. Locality
parameter is initialized with 0 just to be safe. Are all the global
variables in driver is guaranteed to be init 0? Even if it is it doesn't
hurt to init it.
-----Original Message-----
From: Randy Dunlap [mailto:randy.dunlap@...cle.com]
Sent: Thursday, October 11, 2007 11:54 AM
To: Agarwal, Lomesh
Cc: Valdis.Kletnieks@...edu; linux-kernel@...r.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 11:33:35 -0700 Agarwal, Lomesh wrote:
> Below is the patch for TPM driver.
> Comments/suggestions?
Observe/use kernel coding style.
Run the patch thru scripts/checkpatch.pl and check its suggestions.
Use "diffstat -p1 -w70" and put that summary near the top of the
patch (after the patch description).
Use -p option of diff to generate the diff so that reviewers
can see the function name that patch blocks apply to.
Use tabs instead of spaces for indenting.
More below.
> --- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
> 20:42:06.000000000 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
> 15:30:49.000000000 -0700
> @@ -56,29 +56,36 @@
>
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED40000,
> - TIS_MEM_LEN = 0x5000,
> + TIS_MEM_LEN = 0x1000,
> TIS_SHORT_TIMEOUT = 750, /* ms */
> TIS_LONG_TIMEOUT = 2000, /* 2 sec */
> };
>
> -#define TPM_ACCESS(l) (0x0000 | ((l) << 12))
> -#define TPM_INT_ENABLE(l) (0x0008 | ((l) << 12))
> -#define TPM_INT_VECTOR(l) (0x000C | ((l) << 12))
> -#define TPM_INT_STATUS(l) (0x0010 | ((l) << 12))
> -#define TPM_INTF_CAPS(l) (0x0014 | ((l) << 12))
> -#define TPM_STS(l) (0x0018 | ((l) << 12))
> -#define TPM_DATA_FIFO(l) (0x0024 | ((l) << 12))
> +#define TPM_ACCESS(l) (0x0000)
> +#define TPM_INT_ENABLE(l) (0x0008)
> +#define TPM_INT_VECTOR(l) (0x000C)
> +#define TPM_INT_STATUS(l) (0x0010)
> +#define TPM_INTF_CAPS(l) (0x0014)
> +#define TPM_STS(l) (0x0018)
> +#define TPM_DATA_FIFO(l) (0x0024)
>
> -#define TPM_DID_VID(l) (0x0F00 | ((l) << 12))
> -#define TPM_RID(l) (0x0F04 | ((l) << 12))
> +#define TPM_DID_VID(l) (0x0F00)
> +#define TPM_RID(l) (0x0F04)
>
> static LIST_HEAD(tis_chips);
> static DEFINE_SPINLOCK(tis_lock);
>
> static int check_locality(struct tpm_chip *chip, int l)
> {
> - if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
> - (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
> + unsigned char tpm_access;
> +
Indent above/below the same amount (1 tab, not spaces).
> + tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
> +
> + /* check if locality is closed */
> + if(tpm_access == 0xFF)
if (
> + return -1;
> +
> + if((tpm_access & (TPM_ACCESS_ACTIVE_LOCALITY |
if ((
> TPM_ACCESS_VALID)) ==
> (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
> return chip->vendor.locality = l;
>
> @@ -251,10 +258,14 @@
>
> out:
> tpm_tis_ready(chip);
> - release_locality(chip, chip->vendor.locality, 0);
> + release_locality(chip, chip->vendor.locality, 1);
> return size;
> }
>
> +static int locality = 0;
No need to init this to 0.
> +module_param(locality, int, 0444);
> +MODULE_PARM_DESC(locality, "TPM Locality To access");
> +
> /*
> * If interrupts are used (signaled by an irq set in the vendor
> structure)
> * tpm.c can skip polling for the data to be available as the
interrupt
> is
> @@ -266,7 +277,7 @@
> size_t count = 0;
> u32 ordinal;
>
> - if (request_locality(chip, 0) < 0)
> + if (request_locality(chip, locality) < 0)
> return -EBUSY;
>
> status = tpm_tis_status(chip);
> @@ -326,7 +337,7 @@
> return len;
> out_err:
> tpm_tis_ready(chip);
> - release_locality(chip, chip->vendor.locality, 0);
> + release_locality(chip, chip->vendor.locality, 1);
> return rc;
> }
>
> @@ -401,7 +412,10 @@
> {
> struct tpm_chip *chip = (struct tpm_chip *) dev_id;
> u32 interrupt;
> - int i;
> +
> + /* check if interrupt is meant for this locality */
> + if(check_locality(chip, locality) < 0)
> + return IRQ_NONE;
>
Use same indenting as surrounding code.
> interrupt = ioread32(chip->vendor.iobase +
> TPM_INT_STATUS(chip->vendor.locality));
> @@ -411,10 +425,6 @@
>
> if (interrupt & TPM_INTF_DATA_AVAIL_INT)
> wake_up_interruptible(&chip->vendor.read_queue);
> - if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> - for (i = 0; i < 5; i++)
> - if (check_locality(chip, i) >= 0)
> - break;
> if (interrupt &
> (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
> TPM_INTF_CMD_READY_INT))
> @@ -440,7 +450,7 @@
> struct tpm_chip *chip;
>
> if (!start)
> - start = TIS_MEM_BASE;
> + start = TIS_MEM_BASE | (locality << 12);
Looks like all of those "<< 12"s (mostly in header file) could use
some helpers.
> if (!len)
> len = TIS_MEM_LEN;
>
> @@ -490,8 +500,9 @@
> if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> dev_dbg(dev, "\tData Avail Int Support\n");
>
> - if (request_locality(chip, 0) != 0) {
> - rc = -ENODEV;
> + if (request_locality(chip, locality) < 0) {
> + rc = -EBUSY;
> + printk("failed request_locality\n");
Use some prefix identity string that tells the use what module
failed here.
> goto out_err;
> }
>
> @@ -582,9 +593,11 @@
>
> tpm_get_timeouts(chip);
> tpm_continue_selftest(chip);
> + release_locality(chip, chip->vendor.locality, 1);
>
> return 0;
> out_err:
> + release_locality(chip, chip->vendor.locality, 1);
> if (chip->vendor.iobase)
> iounmap(chip->vendor.iobase);
> tpm_remove_hardware(chip->dev);
> @@ -636,7 +649,7 @@
> MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to
> probe");
>
> static struct device_driver tis_drv = {
> - .name = "tpm_tis",
> + .name = "",
> .bus = &platform_bus_type,
> .owner = THIS_MODULE,
> .suspend = tpm_pm_suspend,
> @@ -648,19 +661,34 @@
> static int force;
> module_param(force, bool, 0444);
> MODULE_PARM_DESC(force, "Force device probe rather than using ACPI
> entry");
> +
> +static char *devname = NULL;
No need to init to 0 or NULL.
But why is devname global here?
could it be in the function below or is it used later?
A quick scan finds only local function use below...
> +
> static int __init init_tis(void)
> {
> +#define DEVNAME_SIZE 10
> +
> int rc;
>
> + if ((locality < 0) || (locality > 4)) {
> + return PTR_ERR(pdev);
> + }
> +
> if (force) {
> + devname = kmalloc(DEVNAME_SIZE, GFP_KERNEL);
> + scnprintf(devname, DEVNAME_SIZE, "%s%d", "tpm_tis",
> locality);
> +
> + tis_drv.name = devname;
> rc = driver_register(&tis_drv);
> if (rc < 0)
> return rc;
> - if
> (IS_ERR(pdev=platform_device_register_simple("tpm_tis", -1, NULL, 0)))
> +
> + if (IS_ERR(pdev=platform_device_register_simple(devname,
> -1, NULL, 0)))
> return PTR_ERR(pdev);
> if((rc=tpm_tis_init(&pdev->dev, 0, 0)) != 0) {
> platform_device_unregister(pdev);
> driver_unregister(&tis_drv);
> + kfree(devname);
> }
> return rc;
> }
> @@ -692,7 +720,9 @@
> if (force) {
> platform_device_unregister(pdev);
> driver_unregister(&tis_drv);
> - } else
> + kfree(devname);
> + }
> + else
> pnp_unregister_driver(&tis_pnp_driver);
> }
---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists