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
> <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
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
> > <[email protected]> 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
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
> 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
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
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
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
Hi Andy, what do your patches do?
On Wed, Jul 1, 2009 at 9:40 AM, Andy Isaacson<[email protected]> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>