TPM drivers support and Linux Integrity Module for 2.6.30

TPM drivers support and Linux Integrity Module for 2.6.30

[Shahbaz Khan]

Hi,

I am using Intel Q45 Express chipset with TPM version 1.2 specs of
TCG. The kernel version is 2.6.30. Problem is that the TPM drivers
cannot provide functionality to the TCG TSS giving error message:

"TCSD TDDL ERROR: Could not find a device to open!"

The device node in /dev is also not being created which should be
"/dev/tpm". If created manually then still it does not work.

What should be done?

Thanks.

--
Shaz

Re: TPM drivers support and Linux Integrity Module for 2.6.30

[Mimi Zohar]

On Fri, 2009-06-12 at 11:59 +0600, Shahbaz Khan wrote:
> Hi,
> 
> I am using Intel Q45 Express chipset with TPM version 1.2 specs of
> TCG. The kernel version is 2.6.30. Problem is that the TPM drivers
> cannot provide functionality to the TCG TSS giving error message:
> 
> "TCSD TDDL ERROR: Could not find a device to open!"
> 
> The device node in /dev is also not being created which should be
> "/dev/tpm". If created manually then still it does not work.
> 
> What should be done?
> 
> Thanks.
> 
> --
> Shaz

This is a device driver issue. Copying the TPM maintainers and the
forum.

Mimi

Re: TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

Hi Mimi, thanks for copying us.

Shaz,

If this is the same chip we find in the GM45 boards, iTPM, the upstream 
driver won't work properly with it.
Mainly because this iTPM returns the wrong status code when the driver 
didn't finish sending all bytes required for a specific command.
As suggested by Seiji Munetoh in the tpmdd-devel sf mailing list, you 
can modify line 263 of tpm_tis.c as below:

-		if ((status & TPM_STS_DATA_EXPECT) == 0) {
+		if ((status & TPM_STS_VALID) == 0) {


Then, after compiling it, since it also seems to not support PNP, load 
it with force option on:

modprobe tpm_tis force=1

If modprobe fails the first time, try the second and then it will work.

I'm going to submit a patch to make the upstream driver work with it, 
making this line depend on a module param too..

Thanks,
Rajiv

Mimi Zohar wrote:
> On Fri, 2009-06-12 at 11:59 +0600, Shahbaz Khan wrote:
>   
>> Hi,
>>
>> I am using Intel Q45 Express chipset with TPM version 1.2 specs of
>> TCG. The kernel version is 2.6.30. Problem is that the TPM drivers
>> cannot provide functionality to the TCG TSS giving error message:
>>
>> "TCSD TDDL ERROR: Could not find a device to open!"
>>
>> The device node in /dev is also not being created which should be
>> "/dev/tpm". If created manually then still it does not work.
>>
>> What should be done?
>>
>> Thanks.
>>
>> --
>> Shaz
>>     
>
> This is a device driver issue. Copying the TPM maintainers and the
> forum.
>
> Mimi
>
>   

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

Hi David,

On Sun, 2009-06-14 at 16:15 +0900, dds (☕) wrote:
> Hello, I'd been meaning to write about this.
> 
> 
> On Sun, Jun 14, 2009 at 12:55 PM, Rajiv Andrade
> <srajiv@linux.vnet.ibm.com> wrote:
>         Hi Mimi, thanks for copying us.
>         
>         Shaz,
>         
>         If this is the same chip we find in the GM45 boards, iTPM, the
>         upstream
>         driver won't work properly with it.
>         Mainly because this iTPM returns the wrong status code when
>         the driver
>         didn't finish sending all bytes required for a specific
>         command.
>         As suggested by Seiji Munetoh in the tpmdd-devel sf mailing
>         list, you
>         can modify line 263 of tpm_tis.c as below:
>         
>         -               if ((status & TPM_STS_DATA_EXPECT) == 0) {
>         +               if ((status & TPM_STS_VALID) == 0) {
>         
> 
> This isn't unreasonable. In the block that should be executing there,
> it's proper to check both, since VALID is an override for DATA_EXPECT.
> See first patch.
>  
> 
Actually, according to the TIS spec, VALID bit just ensures that the
DATA_EXPECT bit value is correct, and isn't an override for that bit (if
I got your point right).Basically you can only trust on DATA_EXPECT bit
value if VALID bit is 1. What happens with the iTPM is that it says the
DATA_EXPECT is valid but doesn't set its value to 1 when it should. What
Seiji suggested was a bypass, since wait_for_stat() right above only
returns (successfully) when the VALID bit to be set to 1. 'status &
TPM_STS_VALID == 0' will always be false there. On the other hand we can
check if it's an iTPM, and in case it's true, bypass that if statement.
This is in the patch below.

Unfortunately I don't have such TPM in hands to get its manufacturer_id
and finish the fix, can you help us here?

Copying Seiji for his awareness and since he has a board with such TPM.
> 

>         
>         Then, after compiling it, since it also seems to not support
>         PNP, load
>         it with force option on:
>         
>         modprobe tpm_tis force=1
> 
> The problem here is acpi pnp but the fix is really simple. The current
> pnpacpi/core.c routine that looks for isapnp devices enumerated in
> acpi enforces that the acpi hid be a valid isapnp id (the formats are
> slightly different). But that's broken: it shoudl be enforcing that
> either the acpi hid or any acpi cids be valid isapnp ids. It's a
> one-line change to do this, see patch 2. 
> 
>  

That's great, thanks
>         
>         If modprobe fails the first time, try the second and then it
>         will work.
> 
> This is fixed by changing the order in the code of setting default
> timeouts and requesting locality. See patch 3.

Great too
>         
Thanks,
Rajiv


>From 9516dd1867326ee52ebbad9eb8be15f4ed35f98e Mon Sep 17 00:00:00 2001
From: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Date: Sun, 14 Jun 2009 15:37:07 -0300
Subject: [PATCH] TPM: iTPM doesn't set DATA_EXPECT bit

Since this chip doesn't set it, this bit check is bypassed in case
manufacturer_id indicates that's chip we are handling with. In order
to grab manufacturer_id value, tpm_getcap() was exported from tpm.c
code.

ITPM value isn't set since I don't have the chip available to get its
manufacturer_id value..

Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm.c     |   18 +-----------------
 drivers/char/tpm/tpm.h     |   20 +++++++++++++++++++-
 drivers/char/tpm/tpm_tis.c |   12 ++++++++++--
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ccdd828..067d6a2 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -430,23 +430,6 @@ out:
 #define TPM_ERROR_SIZE 10
 #define TPM_RET_CODE_IDX 6
 
-enum tpm_capabilities {
-	TPM_CAP_FLAG = cpu_to_be32(4),
-	TPM_CAP_PROP = cpu_to_be32(5),
-	CAP_VERSION_1_1 = cpu_to_be32(0x06),
-	CAP_VERSION_1_2 = cpu_to_be32(0x1A)
-};
-
-enum tpm_sub_capabilities {
-	TPM_CAP_PROP_PCR = cpu_to_be32(0x101),
-	TPM_CAP_PROP_MANUFACTURER = cpu_to_be32(0x103),
-	TPM_CAP_FLAG_PERM = cpu_to_be32(0x108),
-	TPM_CAP_FLAG_VOL = cpu_to_be32(0x109),
-	TPM_CAP_PROP_OWNER = cpu_to_be32(0x111),
-	TPM_CAP_PROP_TIS_TIMEOUT = cpu_to_be32(0x115),
-	TPM_CAP_PROP_TIS_DURATION = cpu_to_be32(0x120),
-
-};
 
 static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
 			    int len, const char *desc)
@@ -501,6 +484,7 @@ ssize_t tpm_getcap(struct device *dev, __be32 subcap_id, cap_t *cap,
 		*cap = tpm_cmd.params.getcap_out.cap;
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_getcap);
 
 void tpm_gen_interrupt(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8e00b4d..9f609e0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -38,6 +38,23 @@ enum tpm_addr {
 	TPM_ADDR = 0x4E,
 };
 
+enum tpm_capabilities {
+	TPM_CAP_FLAG = cpu_to_be32(4),
+	TPM_CAP_PROP = cpu_to_be32(5),
+	CAP_VERSION_1_1 = cpu_to_be32(0x06),
+	CAP_VERSION_1_2 = cpu_to_be32(0x1A)
+};
+
+enum tpm_sub_capabilities {
+	TPM_CAP_PROP_PCR = cpu_to_be32(0x101),
+	TPM_CAP_PROP_MANUFACTURER = cpu_to_be32(0x103),
+	TPM_CAP_FLAG_PERM = cpu_to_be32(0x108),
+	TPM_CAP_FLAG_VOL = cpu_to_be32(0x109),
+	TPM_CAP_PROP_OWNER = cpu_to_be32(0x111),
+	TPM_CAP_PROP_TIS_TIMEOUT = cpu_to_be32(0x115),
+	TPM_CAP_PROP_TIS_DURATION = cpu_to_be32(0x120),
+
+};
 extern ssize_t tpm_show_pubek(struct device *, struct device_attribute *attr,
 				char *);
 extern ssize_t tpm_show_pcrs(struct device *, struct device_attribute *attr,
@@ -109,6 +126,7 @@ struct tpm_chip {
 
 	struct list_head list;
 	void (*release) (struct device *);
+	int manufacturer_id;
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
@@ -264,7 +282,7 @@ struct tpm_cmd_t {
 	tpm_cmd_params	params;
 }__attribute__((packed));
 
-ssize_t	tpm_getcap(struct device *, __be32, cap_t *, const char *);
+extern ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);
 
 extern void tpm_get_timeouts(struct tpm_chip *);
 extern void tpm_gen_interrupt(struct tpm_chip *);
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..6c45f8f 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
 #include "tpm.h"
 
 #define TPM_HEADER_SIZE 10
+#define ITPM_ID 0
 
 enum tis_access {
 	TPM_ACCESS_VALID = 0x80,
@@ -293,7 +294,9 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c,
 			      &chip->vendor.int_queue);
 		status = tpm_tis_status(chip);
-		if ((status & TPM_STS_DATA_EXPECT) == 0) {
+		/* iTPM never sets the DATA_EXPECT bit */
+		if (((status & TPM_STS_DATA_EXPECT) == 0) &&
+		     (chip->manufacturer_id != ITPM_ID)) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -440,6 +443,7 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	u32 vendor, intfcaps, intmask;
 	int rc, i;
 	struct tpm_chip *chip;
+	cap_t cap;
 
 	if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
 		return -ENODEV;
@@ -581,7 +585,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 
 	tpm_get_timeouts(chip);
 	tpm_continue_selftest(chip);
-
+	rc = tpm_getcap(chip->dev, TPM_CAP_PROP_MANUFACTURER, &cap,
+			"attempting to determine if it's an Intel iTPM");
+	
+	chip->manufacturer_id = (rc ? 0 : be32_to_cpu(cap.manufacturer_id));
+	
 	return 0;
 out_err:
 	if (chip->vendor.iobase)
-- 
1.6.1.2

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Eric Paris]

On Sun, 2009-06-14 at 16:20 -0300, Rajiv Andrade wrote:
> Hi David,
> 
> On Sun, 2009-06-14 at 16:15 +0900, dds (☕) wrote:
> > Hello, I'd been meaning to write about this.

I can't seem to find the mail from david in any archive, does anyone
have a pointer?

> > 
> > 
> > On Sun, Jun 14, 2009 at 12:55 PM, Rajiv Andrade
> > <srajiv@linux.vnet.ibm.com> wrote:
> >         Hi Mimi, thanks for copying us.
> >         
> >         Shaz,
> >         
> >         If this is the same chip we find in the GM45 boards, iTPM, the
> >         upstream
> >         driver won't work properly with it.
> >         Mainly because this iTPM returns the wrong status code when
> >         the driver
> >         didn't finish sending all bytes required for a specific
> >         command.
> >         As suggested by Seiji Munetoh in the tpmdd-devel sf mailing
> >         list, you
> >         can modify line 263 of tpm_tis.c as below:
> >         
> >         -               if ((status & TPM_STS_DATA_EXPECT) == 0) {
> >         +               if ((status & TPM_STS_VALID) == 0) {
> >         
> > 
> > This isn't unreasonable. In the block that should be executing there,
> > it's proper to check both, since VALID is an override for DATA_EXPECT.
> > See first patch.
> >  
> > 
> Actually, according to the TIS spec, VALID bit just ensures that the
> DATA_EXPECT bit value is correct, and isn't an override for that bit (if
> I got your point right).Basically you can only trust on DATA_EXPECT bit
> value if VALID bit is 1. What happens with the iTPM is that it says the
> DATA_EXPECT is valid but doesn't set its value to 1 when it should. What
> Seiji suggested was a bypass, since wait_for_stat() right above only
> returns (successfully) when the VALID bit to be set to 1. 'status &
> TPM_STS_VALID == 0' will always be false there. On the other hand we can
> check if it's an iTPM, and in case it's true, bypass that if statement.
> This is in the patch below.
> 
> Unfortunately I don't have such TPM in hands to get its manufacturer_id
> and finish the fix, can you help us here?

I have a Lenovo x200 with with iTPM for which I've been carrying patches
to make it work.  I added a printk (%d) in tpm_tis_init (along with
using the old TPM_STS_DATA_VALID patch instead of this one) to find out:

+#define ITPM_ID 1229870147


> @@ -581,7 +585,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>  
>  	tpm_get_timeouts(chip);
>  	tpm_continue_selftest(chip);
> -
> +	rc = tpm_getcap(chip->dev, TPM_CAP_PROP_MANUFACTURER, &cap,
> +			"attempting to determine if it's an Intel iTPM");
> +	

The line above has a tab.

> +	chip->manufacturer_id = (rc ? 0 : be32_to_cpu(cap.manufacturer_id));
> +	

The line above has a tab.

but my real problem is that the patch doesn't work!  We call tpm_getcap
to know if we should work around a tpm bug.

tpm_getcap->transmit_cmd->tpm_transmit->chip->vendor.send

which of course ends up in tpm_tis_send() but we needed that
manufacturer_id before tpm_tis_send can work!

Below is my dmesg output of a failed build.

[root@dhcp231-142 ~]# dmesg | grep -i tpm
[    0.945438] Platform driver 'tpm_tis' needs updating - please use dev_pm_ops
[    0.951161] tpm_tis tpm_tis: 1.2 TPM (device-id 0x1020, rev-id 6)
[    0.957171] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
[    0.963239] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
[    0.969190] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
[    0.975165] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
[    0.975369] tpm_tis_init: chip->manufacturer_id=0
[    1.097287] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
[    1.097445] No TPM chip found, activating TPM-bypass!
[  110.859305] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5

See all the failed tpm calls before we set the manufacturer_id at time
0.975369?  (that printk is my own addition in tpm_tis_init)

But at least you know the manucaturer_id if it helps...

-Eric

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

Eric Paris wrote:
> On Sun, 2009-06-14 at 16:20 -0300, Rajiv Andrade wrote:
>   
>> Hi David,
>>
>> On Sun, 2009-06-14 at 16:15 +0900, dds (☕) wrote:
>>     
>>> Hello, I'd been meaning to write about this.
>>>       
>
> I can't seem to find the mail from david in any archive, does anyone
> have a pointer?
>
>   
Neither do I, only have it in my inbox, seems that it got bounced by
both ML..
> but my real problem is that the patch doesn't work!  We call tpm_getcap
> to know if we should work around a tpm bug.
>
> tpm_getcap->transmit_cmd->tpm_transmit->chip->vendor.send
>
> which of course ends up in tpm_tis_send() but we needed that
> manufacturer_id before tpm_tis_send can work!
>
>   
Yep, sorry. Two options:

1- Inside that check, if got an error when in that particular
tpm_getcap() stacked call, consider that it might be this iTPM, and
bypass it just to get the value from the chip (would happen only once) -
Would work, but it's odd.

2- Forget manufacturer_id and base the decision on the PNP_ID as david
suggested. I previously considered it but since it would end up in
modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
and then wouldn't work when loading as a module with force option on, so
I moved to the manufacturer_id approach.

I'll get back to #2 meanwhile and post the patch, seems not hard to
accomplish though..
> Below is my dmesg output of a failed build.
>
> [root@dhcp231-142 ~]# dmesg | grep -i tpm
> [    0.945438] Platform driver 'tpm_tis' needs updating - please use dev_pm_ops
> [    0.951161] tpm_tis tpm_tis: 1.2 TPM (device-id 0x1020, rev-id 6)
> [    0.957171] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
> [    0.963239] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
> [    0.969190] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
> [    0.975165] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
> [    0.975369] tpm_tis_init: chip->manufacturer_id=0
> [    1.097287] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
> [    1.097445] No TPM chip found, activating TPM-bypass!
> [  110.859305] tpm_tis tpm_tis: tpm_transmit: tpm_send: error -5
>
> See all the failed tpm calls before we set the manufacturer_id at time
> 0.975369?  (that printk is my own addition in tpm_tis_init)
>
> But at least you know the manucaturer_id if it helps...
>
>   
For now it does, thanks

Rajiv

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

> 2- Forget manufacturer_id and base the decision on the PNP_ID as david
> suggested. I previously considered it but since it would end up in
> modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
> and then wouldn't work when loading as a module with force option on, so
> I moved to the manufacturer_id approach.
>
> I'll get back to #2 meanwhile and post the patch, seems not hard to
> accomplish though..
>   
Yes, it wasn't hard, at all, just get the id with to_pnp_dev(dev)->id.

However, the chip is buggy, there's no reason to make a compliant
upstream code modify its behavior just due an 'exception' for a not
compliant hardware.
No need to worry about it too though, the workaround is available as I
pointed earlier (Seiji's)...

Thanks,
Rajiv

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Eric Paris]

On Mon, 2009-06-15 at 18:42 -0300, Rajiv Andrade wrote:
> > 2- Forget manufacturer_id and base the decision on the PNP_ID as david
> > suggested. I previously considered it but since it would end up in
> > modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
> > and then wouldn't work when loading as a module with force option on, so
> > I moved to the manufacturer_id approach.
> >
> > I'll get back to #2 meanwhile and post the patch, seems not hard to
> > accomplish though..
> >   
> Yes, it wasn't hard, at all, just get the id with to_pnp_dev(dev)->id.
> 
> However, the chip is buggy, there's no reason to make a compliant
> upstream code modify its behavior just due an 'exception' for a not
> compliant hardware.
> No need to worry about it too though, the workaround is available as I
> pointed earlier (Seiji's)...

Wait what?  we refuse to work around buggy hardware that is shipping in
LOTS of hardware (all the currently shipping lenovo thinkpads) even
though the fix is easy?  This doesn't sound right.....

-Eric

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

On Tue, 2009-06-16 at 16:49 -0400, Eric Paris wrote:
> On Mon, 2009-06-15 at 18:42 -0300, Rajiv Andrade wrote:
> > > 2- Forget manufacturer_id and base the decision on the PNP_ID as david
> > > suggested. I previously considered it but since it would end up in
> > > modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
> > > and then wouldn't work when loading as a module with force option on, so
> > > I moved to the manufacturer_id approach.
> > >
> > > I'll get back to #2 meanwhile and post the patch, seems not hard to
> > > accomplish though..
> > >   
> > Yes, it wasn't hard, at all, just get the id with to_pnp_dev(dev)->id.
> > 
> > However, the chip is buggy, there's no reason to make a compliant
> > upstream code modify its behavior just due an 'exception' for a not
> > compliant hardware.
> > No need to worry about it too though, the workaround is available as I
> > pointed earlier (Seiji's)...
> 
> Wait what?  we refuse to work around buggy hardware that is shipping in
> LOTS of hardware (all the currently shipping lenovo thinkpads) even
> though the fix is easy?  This doesn't sound right.....

I didn't refuse to work on it... That depends on the meaning in this
context. My point is: not make an exception in the upstream code due a
buggy hardware (that's what that easy 'fix' does). Other than David's
patches, there is a workaround available as I said: 

http://sourceforge.net/mailarchive/message.php?msg_name=f02dbbe70812012308n32dc9fd6hd1f04d3ef6e002b7%40mail.gmail.com 

The only Lenovo thinkpad model I know that has it is the X200. If Intel
is indeed still shipping it buggy (therefore more and more unaware users
are buying it), that's another story. Can you confirm that?
The best would be to hear something from Intel, if they are planning to
fix it, discontinue it, do nothing about it or anything else.
Do you know how to do it or if that's possible?

More, that's only one bug. With the workaround in hands and being able
to load the module, did you run any regression tests? (Sorry for asking
that, but, again, I don't have this chip):

Thanks,
Rajiv

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Jonathan M. McCune]

I've seen this in Thinkpad T400 and X301 as well.

-Jon


Rajiv Andrade wrote:
> On Tue, 2009-06-16 at 16:49 -0400, Eric Paris wrote:
>   
>> On Mon, 2009-06-15 at 18:42 -0300, Rajiv Andrade wrote:
>>     
>>>> 2- Forget manufacturer_id and base the decision on the PNP_ID as david
>>>> suggested. I previously considered it but since it would end up in
>>>> modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
>>>> and then wouldn't work when loading as a module with force option on, so
>>>> I moved to the manufacturer_id approach.
>>>>
>>>> I'll get back to #2 meanwhile and post the patch, seems not hard to
>>>> accomplish though..
>>>>   
>>>>         
>>> Yes, it wasn't hard, at all, just get the id with to_pnp_dev(dev)->id.
>>>
>>> However, the chip is buggy, there's no reason to make a compliant
>>> upstream code modify its behavior just due an 'exception' for a not
>>> compliant hardware.
>>> No need to worry about it too though, the workaround is available as I
>>> pointed earlier (Seiji's)...
>>>       
>> Wait what?  we refuse to work around buggy hardware that is shipping in
>> LOTS of hardware (all the currently shipping lenovo thinkpads) even
>> though the fix is easy?  This doesn't sound right.....
>>     
>
> I didn't refuse to work on it... That depends on the meaning in this
> context. My point is: not make an exception in the upstream code due a
> buggy hardware (that's what that easy 'fix' does). Other than David's
> patches, there is a workaround available as I said: 
>
> http://sourceforge.net/mailarchive/message.php?msg_name=f02dbbe70812012308n32dc9fd6hd1f04d3ef6e002b7%40mail.gmail.com 
>
> The only Lenovo thinkpad model I know that has it is the X200. If Intel
> is indeed still shipping it buggy (therefore more and more unaware users
> are buying it), that's another story. Can you confirm that?
> The best would be to hear something from Intel, if they are planning to
> fix it, discontinue it, do nothing about it or anything else.
> Do you know how to do it or if that's possible?
>
> More, that's only one bug. With the workaround in hands and being able
> to load the module, did you run any regression tests? (Sorry for asking
> that, but, again, I don't have this chip):
>
> Thanks,
> Rajiv
>
>
>
> ------------------------------------------------------------------------------
> Are you an open source citizen? Join us for the Open Source Bridge conference!
> Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250.
> Need another reason to go? 24-hour hacker lounge. Register today!
> http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
>   

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Rajiv Andrade]

On Fri, 2009-06-19 at 18:23 -0400, Jonathan M. McCune wrote:
> I've seen this in Thinkpad T400 and X301 as well.
> 
> -Jon
> 
> 
> Rajiv Andrade wrote:
> > The only Lenovo thinkpad model I know that has it is the X200. If Intel
> > is indeed still shipping it buggy (therefore more and more unaware users
> > are buying it), that's another story. Can you confirm that?

Confirmed. Let's go to our patches so..

Thanks,
Rajiv

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[Andy Isaacson]

On Mon, Jun 15, 2009 at 01:02:27PM -0300, Rajiv Andrade wrote:
> Yep, sorry. Two options:
> 
> 1- Inside that check, if got an error when in that particular
> tpm_getcap() stacked call, consider that it might be this iTPM, and
> bypass it just to get the value from the chip (would happen only once) -
> Would work, but it's odd.
> 
> 2- Forget manufacturer_id and base the decision on the PNP_ID as david
> suggested. I previously considered it but since it would end up in
> modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
> and then wouldn't work when loading as a module with force option on, so
> I moved to the manufacturer_id approach.
> 
> I'll get back to #2 meanwhile and post the patch, seems not hard to
> accomplish though..

I've got a set of patches that seem to resolve my iTPM woes.  I've
mostly tested on T400 but I also tried X200.

I'll send the patches as a separate thread (unfortunately I can't get
git-send-email 1.6.3.1 to set a in-reply-to header on 0/5 without also
breaking threading on x/5.)

-andy

Re: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[dds (☕)]

Hi Andy, what do your patches do?

On Wed, Jul 1, 2009 at 9:40 AM, Andy Isaacson<adi@hexapodia.org> wrote:
> On Mon, Jun 15, 2009 at 01:02:27PM -0300, Rajiv Andrade wrote:
>> Yep, sorry. Two options:
>>
>> 1- Inside that check, if got an error when in that particular
>> tpm_getcap() stacked call, consider that it might be this iTPM, and
>> bypass it just to get the value from the chip (would happen only once) -
>> Would work, but it's odd.
>>
>> 2- Forget manufacturer_id and base the decision on the PNP_ID as david
>> suggested. I previously considered it but since it would end up in
>> modifying tpm_tis_init() prototype (struct device * to struct pnp_dev *)
>> and then wouldn't work when loading as a module with force option on, so
>> I moved to the manufacturer_id approach.
>>
>> I'll get back to #2 meanwhile and post the patch, seems not hard to
>> accomplish though..
>
> I've got a set of patches that seem to resolve my iTPM woes.  I've
> mostly tested on T400 but I also tried X200.
>
> I'll send the patches as a separate thread (unfortunately I can't get
> git-send-email 1.6.3.1 to set a in-reply-to header on 0/5 without also
> breaking threading on x/5.)
>
> -andy
>
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>

Fwd: [tpmdd-devel] TPM drivers support and Linux Integrity Module for 2.6.30

[dds (☕)]

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

Hi Rajiv. I'm resending this since it is not in the archives (my bad).


---------- Forwarded message ----------
From: dds (☕) <dds@google.com>
Date: Sun, Jun 14, 2009 at 4:15 PM
Subject: Re: [tpmdd-devel] TPM drivers support and Linux Integrity
Module for 2.6.30
To: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>,
tpmdd-devel@lists.sourceforge.net, linux-kernel
<linux-kernel@vger.kernel.org>, Shahbaz Khan <shaz.linux@gmail.com>


Hello, I'd been meaning to write about this.


On Sun, Jun 14, 2009 at 12:55 PM, Rajiv Andrade
<srajiv@linux.vnet.ibm.com> wrote:
>
> Hi Mimi, thanks for copying us.
>
> Shaz,
>
> If this is the same chip we find in the GM45 boards, iTPM, the upstream
> driver won't work properly with it.
> Mainly because this iTPM returns the wrong status code when the driver
> didn't finish sending all bytes required for a specific command.
> As suggested by Seiji Munetoh in the tpmdd-devel sf mailing list, you
> can modify line 263 of tpm_tis.c as below:
>
> -               if ((status & TPM_STS_DATA_EXPECT) == 0) {
> +               if ((status & TPM_STS_VALID) == 0) {
>

This isn't unreasonable. In the block that should be executing there,
it's proper to check both, since VALID is an override for DATA_EXPECT.
See first patch.

>
> Then, after compiling it, since it also seems to not support PNP, load
> it with force option on:
>
> modprobe tpm_tis force=1

The problem here is acpi pnp but the fix is really simple. The current
pnpacpi/core.c routine that looks for isapnp devices enumerated in
acpi enforces that the acpi hid be a valid isapnp id (the formats are
slightly different). But that's broken: it shoudl be enforcing that
either the acpi hid or any acpi cids be valid isapnp ids. It's a
one-line change to do this, see patch 2.


>
> If modprobe fails the first time, try the second and then it will work.

This is fixed by changing the order in the code of setting default
timeouts and requesting locality. See patch 3.

>
> I'm going to submit a patch to make the upstream driver work with it,
> making this line depend on a module param too..
>
> Thanks,
> Rajiv
>
> Mimi Zohar wrote:
> > On Fri, 2009-06-12 at 11:59 +0600, Shahbaz Khan wrote:
> >
> >> Hi,
> >>
> >> I am using Intel Q45 Express chipset with TPM version 1.2 specs of
> >> TCG. The kernel version is 2.6.30. Problem is that the TPM drivers
> >> cannot provide functionality to the TCG TSS giving error message:
> >>
> >> "TCSD TDDL ERROR: Could not find a device to open!"
> >>
> >> The device node in /dev is also not being created which should be
> >> "/dev/tpm". If created manually then still it does not work.
> >>
> >> What should be done?
> >>
> >> Thanks.
> >>
> >> --
> >> Shaz
> >>
> >
> > This is a device driver issue. Copying the TPM maintainers and the
> > forum.
> >
> > Mimi
> >
> >
>
>
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

[-- Attachment #2: 01-either_dataexpect_or_valid.patch --]
[-- Type: text/x-diff, Size: 1472 bytes --]

commit cca56d7b550bac0a00d6322b225f4d0a8d3e6b88
Author: David Smith <dds@google.com>
Date:   Tue Apr 28 18:56:39 2009 +0900

    Fix tpm_tis driver to support either DATA_EXPECT or VALID status when uploading command data.
    
    The TCG spec says that a VALID status implies that a DATA_EXPECT
    status. This occurs in the real world with the iTPM in Intel's Mobile 4
    platform which never sets DATA_EXPECT, but sets VALID when expecting more
    data.
    
    Signed-off-by: David Smith <dds@google.com>

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..be112ef 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -293,7 +293,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 		wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c,
 			      &chip->vendor.int_queue);
 		status = tpm_tis_status(chip);
-		if ((status & TPM_STS_DATA_EXPECT) == 0) {
+		if ((status & TPM_STS_DATA_EXPECT) == 0 &&
+                    (status & TPM_STS_VALID) == 0) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -306,7 +307,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c,
 		      &chip->vendor.int_queue);
 	status = tpm_tis_status(chip);
-	if ((status & TPM_STS_DATA_EXPECT) != 0) {
+	if ((status & TPM_STS_DATA_EXPECT) != 0 &&
+            (status & TPM_STS_VALID) == 1) {
 		rc = -EIO;
 		goto out_err;
 	}

[-- Attachment #3: 02-fix_acpipnp.patch --]
[-- Type: text/x-diff, Size: 926 bytes --]

commit 7a553b4e7439ad0733da7da8663d32aa4865aa9e
Author: David Smith <dds@google.com>
Date:   Tue Apr 28 18:52:02 2009 +0900

    Update ACPI PNP to support devices with EISA PNP CIDs but non-PNP HIDs
    
    Signed-off-by: David Smith <dds@google.com>

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 9496494..8bfddfb 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -159,8 +159,8 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	 * driver should not be loaded.
 	 */
 	status = acpi_get_handle(device->handle, "_CRS", &temp);
-	if (ACPI_FAILURE(status) || !ispnpidacpi(acpi_device_hid(device)) ||
-	    is_exclusive_device(device) || (!device->status.present))
+	if (ACPI_FAILURE(status) || is_exclusive_device(device) ||
+            (!device->status.present))
 		return 0;
 
 	dev = pnp_alloc_dev(&pnpacpi_protocol, num, acpi_device_hid(device));

[-- Attachment #4: 03-reorder_locality_to_after_timeouts.patch --]
[-- Type: text/x-diff, Size: 1504 bytes --]

commit 2117a060d04b1063f26bae6450bdd21be400b799
Author: David Smith <dds@google.com>
Date:   Thu Jun 11 08:34:16 2009 +0900

    Reorder setting chip timeouts to before locality is requested.
    This stops a failure to load roughly half the time of the module.
    
    Signed-off-by: David Smith <dds@google.com>

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index be112ef..eea5d4c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -452,6 +452,12 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		goto out_err;
 	}
 
+	/* Default timeouts */
+	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+	chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+
 	if (request_locality(chip, 0) != 0) {
 		rc = -ENODEV;
 		goto out_err;
@@ -459,12 +465,6 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 
 	vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
 
-	/* Default timeouts */
-	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
-	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
-	chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
-	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
-
 	dev_info(dev,
 		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));