TPM driver changes to support multiple locality

TPM driver changes to support multiple locality

[Agarwal, Lomesh]

Current TPM driver supports only locality 0. I am planning to add
support so that it can access any locality. Locality parameter will be
passed as parameter. Will this change be acceptable? If yes then I will
modify the driver and send the patch.

Thanks,
Lomesh

Re: TPM driver changes to support multiple locality

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said:
> Current TPM driver supports only locality 0. I am planning to add
> support so that it can access any locality. Locality parameter will be
> passed as parameter. Will this change be acceptable? If yes then I will
> modify the driver and send the patch.

Make sure you extend the API in such a way that older userspace programs
that don't pass the new locality parameter still work correctly.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

There will be no change in API. Driver will accept a parameter
"locality" and it will always operate on that locality. By default this
parameter will be 0 which is what current driver assumes.
Who is the maintainer for this driver?

-----Original Message-----
From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu] 
Sent: Wednesday, October 10, 2007 12:46 PM
To: Agarwal, Lomesh
Cc: linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality

On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said:
> Current TPM driver supports only locality 0. I am planning to add
> support so that it can access any locality. Locality parameter will be
> passed as parameter. Will this change be acceptable? If yes then I
will
> modify the driver and send the patch.

Make sure you extend the API in such a way that older userspace programs
that don't pass the new locality parameter still work correctly.

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

Below is the patch for TPM driver.
Comments/suggestions?

--- 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;
+
+	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 +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;
+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;
 
 	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);
 	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");
 		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;
+
 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);
 }


-----Original Message-----
From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu] 
Sent: Wednesday, October 10, 2007 12:46 PM
To: Agarwal, Lomesh
Cc: linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality

On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said:
> Current TPM driver supports only locality 0. I am planning to add
> support so that it can access any locality. Locality parameter will be
> passed as parameter. Will this change be acceptable? If yes then I
will
> modify the driver and send the patch.

Make sure you extend the API in such a way that older userspace programs
that don't pass the new locality parameter still work correctly.

Re: TPM driver changes to support multiple locality

[Randy Dunlap]

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

Re: TPM driver changes to support multiple locality

[Jan Engelhardt]

On Oct 11 2007 11:54, Randy Dunlap wrote:
>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).

Or, if using quilt on top of the tree, a simple `quilt diff
--diffstat --sort` will do all the nice diffing, including stat and
use diff -pu. It also preserves diffstat (if not regenerating that)
and patch description, so is suited *perfectly* for the task when you
don't want to use stgit.

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

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@oracle.com] 
Sent: Thursday, October 11, 2007 11:54 AM
To: Agarwal, Lomesh
Cc: Valdis.Kletnieks@vt.edu; linux-kernel@vger.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

Re: TPM driver changes to support multiple locality

[Randy Dunlap]

On Thu, 11 Oct 2007 14:08:17 -0700 Agarwal, Lomesh wrote:

> Here is info. -
> 
> This patch adds multiple locality support in tpm_tis driver.

whatever that means.  I guess anyone who is familiar with TPM
knows what it means,  and others can do research to find out.
Or the patch description could add another 2 lines for it (?).


> drivers/char/tpm/tpm_tis.c |   83 ++++++++++++++++++++++++-----------
>  1 file changed, 57 insertions(+), 26 deletions(-)

Thanks for that.

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

Indent above uses spaces, not tab.

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

Use tab above, not spaces.

> +
> +	tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
> +
> +    /* check if locality is closed */
> +    if(tpm_access == 0xFF)

	if (

> +        return -1;

and use tab(s) for indent, not spaces.

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

Don't init statics to 0 or NULL (it's done for you).
(same comment as first time)


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

	if (

and use tab(s) to indent, not spaces.

> +            return IRQ_NONE;
>  
>  	interrupt = ioread32(chip->vendor.iobase +
>  			     TPM_INT_STATUS(chip->vendor.locality));
> @@ -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);

printk() usually should have a facility level, like
KERN_WARNING:

		printk(KERN_WARNING "tpm_tis: failed request_locality %d\n",
			locality);

>  		goto out_err;
>  	}
>  
> @@ -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);
> +	}

No braces on single-statement "blocks."

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

Please split up the assignment to pdev and then if/return:

		pdev = platform_device_register_simple(devname, -1,
				NULL, 0);
		if (IS_ERR(pdev))
>  			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?

What kernel tree are you using?  Oh.  2.6.18.  :(
Does the patch apply to 2.6.23?  It must do that to be useful.
(Well, it does after fixing the malformed patch problems.)

Take the latest version of checkpatch.pl from
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/ .

patch complains about malformed patch:
The patch also has about 10 instances of lines being broken
(split) where they shouldn't be split, cause 'patch' not to be
able to apply it.
This is most likely your email client (MUA) doing this, although
it could be some server along the way, I suppose.

What mail client are you using to send patches?


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

All static globals are guaranteed to be init to 0.
It increases the object file size to init them again.
Kernel style is not to init these to 0 or NULL.

---
~Randy

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

[-- Attachment #1: Type: text/plain, Size: 56 bytes --]

Attached is the patch which resolves all the comments.

[-- Attachment #2: tpm_tis.c.patch --]
[-- Type: application/octet-stream, Size: 6441 bytes --]

Adding support for multiple localities in tpm driver
this driver can be instatiated with a locality parameter and that instance will operate on 
given locality.
By default it works on locality 0
Signed-off-by: Lomesh Agarwal <lomesh.agarwal@intel.com>
---
 drivers/char/tpm/tpm_tis.c |   81 ++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 25 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 15:30:53.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;
+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,10 @@ 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(KERN_WARNING "tpm_tis: failed request_locality %d\n",
+			locality);
 		goto out_err;
 	}
 
@@ -582,9 +595,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 +651,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 +663,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)))
+
+		pdev = platform_device_register_simple(devname, -1, NULL, 0);
+		if (IS_ERR(pdev))
 			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,6 +722,7 @@ static void __exit cleanup_tis(void)
 	if (force) {
 		platform_device_unregister(pdev);
 		driver_unregister(&tis_drv);
+		kfree(devname);
 	} else
 		pnp_unregister_driver(&tis_pnp_driver);
 }

Re: TPM driver changes to support multiple locality

[Randy Dunlap]

On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:

> Attached is the patch which resolves all the comments.

Inline patches are preferred so that reviewers can comment on them
more easily.

What mail client are you using?

The patch has trailing CRs on each line ("DOS mode").

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Just verifying:  this TPM device has interrupts per locality?

+	/* check if interrupt is meant for this locality */
+	if (check_locality(chip, locality) < 0)
+		return IRQ_NONE;

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

init_tis() still seems to have some problems.

 static int __init init_tis(void)
 {
+#define DEVNAME_SIZE 10
+
 	int rc;
 
+	if ((locality < 0) || (locality > 4))
+		return PTR_ERR(pdev);

pdev hasn't been set (so it's NULL).

+		pdev = platform_device_register_simple(devname, -1, NULL, 0);
+		if (IS_ERR(pdev))
 			return PTR_ERR(pdev);

Error path above needs to call driver_unregister().


---
~Randy

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]



-----Original Message-----
From: Randy Dunlap [mailto:randy.dunlap@oracle.com] 
Sent: Thursday, October 11, 2007 4:02 PM
To: Agarwal, Lomesh
Cc: Valdis.Kletnieks@vt.edu; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality

On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:

> Attached is the patch which resolves all the comments.

Inline patches are preferred so that reviewers can comment on them
more easily.

What mail client are you using?
[Agarwal, Lomesh] I am using MS Outlook. Earlier you said inline patches
have problem because of mail client. That's why I sent it as attachment.

The patch has trailing CRs on each line ("DOS mode").

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Just verifying:  this TPM device has interrupts per locality?

+	/* check if interrupt is meant for this locality */
+	if (check_locality(chip, locality) < 0)
+		return IRQ_NONE;

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[Agarwal, Lomesh] TPM device has only one interrupt. so on receiving
interrupt driver has to make sure that its meant for its locality.

init_tis() still seems to have some problems.

 static int __init init_tis(void)
 {
+#define DEVNAME_SIZE 10
+
 	int rc;
 
+	if ((locality < 0) || (locality > 4))
+		return PTR_ERR(pdev);

pdev hasn't been set (so it's NULL).

+		pdev = platform_device_register_simple(devname, -1,
NULL, 0);
+		if (IS_ERR(pdev))
 			return PTR_ERR(pdev);

Error path above needs to call driver_unregister().
[Agarwal, Lomesh] attached is the new patch.


---
~Randy

[-- Attachment #2: tpm_tis.c.patch --]
[-- Type: application/octet-stream, Size: 6264 bytes --]

Adding support for multiple localities in tpm driver
this driver can be instatiated with a locality parameter and that instance
will operate on given locality.
By default it works on locality 0
Signed-off-by: Lomesh Agarwal <lomesh.agarwal@intel.com>
---

drivers/char/tpm/tpm_tis.c |   83 ++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 25 deletions(-)

--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c	2006-09-20 11:42:06.000000000 +0800
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c	2007-10-12 07:16:42.000000000 +0800
@@ -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;
+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,10 @@ 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(KERN_WARNING "tpm_tis: failed request_locality %d\n",
+			locality);
 		goto out_err;
 	}
 
@@ -582,9 +595,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 +651,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 +663,36 @@ 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 -1;
+
 	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)))
+
+		pdev = platform_device_register_simple(devname, -1, NULL, 0);
+		if (IS_ERR(pdev)) {
+			driver_unregister(&tis_drv);
 			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,6 +724,7 @@ static void __exit cleanup_tis(void)
 	if (force) {
 		platform_device_unregister(pdev);
 		driver_unregister(&tis_drv);
+		kfree(devname);
 	} else
 		pnp_unregister_driver(&tis_pnp_driver);
 }

Re: TPM driver changes to support multiple locality

[Arjan van de Ven]

On Thu, 11 Oct 2007 11:33:35 -0700
"Agarwal, Lomesh" <lomesh.agarwal@intel.com> wrote:

> Below is the patch for TPM driver.
> Comments/suggestions?

please don't top post

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

or send word wrapped patches ....

.. or patches against 2 year old kernels for that matter ;)

>  enum tis_defaults {
>  	TIS_MEM_BASE = 0xFED40000,
> -	TIS_MEM_LEN = 0x5000,
> +	TIS_MEM_LEN = 0x1000,

hmmm/// why

RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

-----Original Message-----
From: Arjan van de Ven [mailto:arjan@linux.intel.com] 
Sent: Thursday, October 11, 2007 2:41 PM
To: Agarwal, Lomesh
Cc: Valdis.Kletnieks@vt.edu; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality

On Thu, 11 Oct 2007 11:33:35 -0700
"Agarwal, Lomesh" <lomesh.agarwal@intel.com> wrote:

> Below is the patch for TPM driver.
> Comments/suggestions?

please don't top post

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

or send word wrapped patches ....

.. or patches against 2 year old kernels for that matter ;)

>  enum tis_defaults {
>  	TIS_MEM_BASE = 0xFED40000,
> -	TIS_MEM_LEN = 0x5000,
> +	TIS_MEM_LEN = 0x1000,

hmmm/// why
[Agarwal, Lomesh] 0x5000 is the length of CSRs for all the localities.
Each locality's CSR size is 0x1000. so 0x1000 is sufficient and will
catch error if driver tries to write to wrong locality.