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
-

[ message continues ]

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.
[ message continues ]

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


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

[ message continues ]

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 = ...
[ message continues ]

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.






Looks like all of those "<< 12"s (mostly in header file) could use

Use some prefix identity string that tells the use what module

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?

---
~Randy
-

[ message continues ]

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

[ message continues ]

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;
 
@@ ...
[ message continues ]

whatever that means.  I guess anyone who is familiar with TPM
knows what it means,  and others can do research to find out.






Don't init statics to 0 or NULL (it's done for you).

	if (


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

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


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

		pdev = platform_device_register_simple(devname, -1,
				NULL, 0);

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.


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
-

[ message continues ]

Attached is the patch which resolves all the comments.
[ message continues ]

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
-

[ message continues ]

-----Original Message-----
From: Randy Dunlap [mailto:randy.dunlap@oracle.com]=20
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


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;
=20
+	if ((locality < 0) || (locality > 4))
+		return PTR_ERR(pdev);

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

+		pdev =3D 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
[ message continues ]

On Thu, 11 Oct 2007 11:33:35 -0700


or send word wrapped patches ....


hmmm/// why
-

[ message continues ]

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


or send word wrapped patches ....


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

[ message continues ]