[PATCH 0/6] tpm_tis: various cleanups, and support for Intel iTPM

[PATCH 0/6] tpm_tis: various cleanups, and support for Intel iTPM

[Andy Isaacson]

Several patches to improve drivers/char/tpm/tpm_tis.c:

 - autoload tpm_tis.ko based on acpi / pnp
 - fix driver/hardware bugs observed on iTPM
 - improve module probing printks
 - add workaround code for iTPM

With these patches, the T500's iTPM probes ok and has reasonable
contents in /sys/kernel/security/tpm0/ascii_bios_measurements:

% dmesg|grep tpm
[    8.599680] tpm_tis INTC0102:00: found 0xfed40000(0x5000)
[    8.599769] tpm_tis INTC0102:00: no IRQ found in _CRS, polling mode
[    8.608607] tpm_tis INTC0102:00: 1.2 TPM (8086:1020 rev 6)
[    8.608665] tpm_tis INTC0102:00: Intel iTPM workaround enabled
% sudo head -3 /sys/kernel/security/tpm0/ascii_bios_measurements
 0 d6184a2d2e3a8bfae13a1ebb37d62e222bfab57d 08 [S-CRTM Version]
 1 0f18524a0ed83806de3bab13c05e2dcc7a8e9c09 09 [CPU Microcode]
 0 d9f7d65755c884f6c25680d577f348523006eed3 01 [POST CODE]

Also, I've tested that the driver still works on Dell E4300.

I'm not sure that switching to ACPI probing is either necessary or a
good idea, but I wasn't able to get the pnp_driver system to load the
driver on the Lenovo.

 drivers/char/tpm/Kconfig   |    2 +-
 drivers/char/tpm/tpm_tis.c |  202 +++++++++++++++++------
 2 files changed, 150 insertions(+), 54 deletions(-)

-andy

[PATCH 1/6] tpm_tis: various cleanups

[Andy Isaacson]

Avoid tabs in printk output.
Use parentheses and ARRAY_SIZE() in macro definition.

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/tpm_tis.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 717af7a..1b84441 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -474,23 +474,23 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
 		intfcaps);
 	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
-		dev_dbg(dev, "\tBurst Count Static\n");
+		dev_dbg(dev, "    Burst Count Static\n");
 	if (intfcaps & TPM_INTF_CMD_READY_INT)
-		dev_dbg(dev, "\tCommand Ready Int Support\n");
+		dev_dbg(dev, "    Command Ready Int Support\n");
 	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
-		dev_dbg(dev, "\tInterrupt Edge Falling\n");
+		dev_dbg(dev, "    Interrupt Edge Falling\n");
 	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
-		dev_dbg(dev, "\tInterrupt Edge Rising\n");
+		dev_dbg(dev, "    Interrupt Edge Rising\n");
 	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
-		dev_dbg(dev, "\tInterrupt Level Low\n");
+		dev_dbg(dev, "    Interrupt Level Low\n");
 	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
-		dev_dbg(dev, "\tInterrupt Level High\n");
+		dev_dbg(dev, "    Interrupt Level High\n");
 	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
-		dev_dbg(dev, "\tLocality Change Int Support\n");
+		dev_dbg(dev, "    Locality Change Int Support\n");
 	if (intfcaps & TPM_INTF_STS_VALID_INT)
-		dev_dbg(dev, "\tSts Valid Int Support\n");
+		dev_dbg(dev, "    Sts Valid Int Support\n");
 	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
-		dev_dbg(dev, "\tData Avail Int Support\n");
+		dev_dbg(dev, "    Data Avail Int Support\n");
 
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&chip->vendor.read_queue);
@@ -649,7 +649,7 @@ static struct pnp_driver tis_pnp_driver = {
 	.remove = tpm_tis_pnp_remove,
 };
 
-#define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
+#define TIS_HID_USR_IDX (ARRAY_SIZE(tpm_pnp_tbl) - 2)
 module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
 		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
 MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
-- 
1.6.3.1

[PATCH 2/6] tpm_tis: add MODULE_DEVICE_TABLE to enable autoload

[Andy Isaacson]

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/tpm_tis.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1b84441..e859e0e 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -629,6 +629,7 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
 	{"", 0},		/* User Specified */
 	{"", 0}			/* Terminator */
 };
+MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
 
 static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
 {
-- 
1.6.3.1

[PATCH 3/6] tpm_tis: set timeouts before calling request_locality

[Andy Isaacson]

On Intel iTPM implementations, the timeouts must be set before
calling request_locality.

Based on a patch from Colin Didier and the tpmdd-devel mailing list:
http://cybione.org/~cdidier/log/data/200812020841/itpm.diff

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/tpm_tis.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e859e0e..e0bda02 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -450,6 +450,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;
@@ -457,12 +463,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)));
-- 
1.6.3.1

[PATCH 4/6] tpm_tis: print complete vendor information

[Andy Isaacson]

The TPM_VID field contains a vendor-id as well as a device-id,
but the code only prints the device-id.  Fix it.

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/tpm_tis.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e0bda02..50c2a8a 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -464,7 +464,7 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
 
 	dev_info(dev,
-		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
+		 "1.2 TPM (%04X:%04X rev %d)\n", vendor & 0xffff,
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
 	/* Figure out the capabilities */
-- 
1.6.3.1

[PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Andy Isaacson]

Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
and the TPM spec says that the _CID method should be used to enumerate
the TPM chip.

Convert to acpi_driver so that we can access the _CID entry.

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/Kconfig   |    2 +-
 drivers/char/tpm/tpm_tis.c |  157 +++++++++++++++++++++++++++++++++----------
 2 files changed, 121 insertions(+), 38 deletions(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index f5fc64f..204b69f 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -24,7 +24,7 @@ if TCG_TPM
 
 config TCG_TIS
 	tristate "TPM Interface Specification 1.2 Interface"
-	depends on PNP
+	depends on ACPI
 	---help---
 	  If you have a TPM security chip that is compliant with the
 	  TCG TIS 1.2 TPM specification say Yes and it will be accessible
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 50c2a8a..7f0cb6c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -21,9 +21,12 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
-#include <linux/pnp.h>
 #include <linux/interrupt.h>
 #include <linux/wait.h>
+
+#include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
+
 #include "tpm.h"
 
 #define TPM_HEADER_SIZE 10
@@ -77,6 +80,13 @@ enum tis_defaults {
 static LIST_HEAD(tis_chips);
 static DEFINE_SPINLOCK(tis_lock);
 
+struct tpm_data {
+	unsigned long tpm_phys_address;
+	void __iomem *tpm_address;
+	int tpm_size;
+	int tpm_irq;
+};
+
 static int check_locality(struct tpm_chip *chip, int l)
 {
 	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
@@ -590,34 +600,114 @@ out_err:
 	return rc;
 }
 
-static int __devinit tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
-				      const struct pnp_device_id *pnp_id)
+static acpi_status tpm_resources(struct acpi_resource *res, void *data)
+{
+	struct acpi_device *device = data;
+	struct device *dev = &device->dev;
+	struct tpm_data *tpm = device->driver_data;
+	acpi_status status;
+	struct acpi_resource_address64 addr;
+
+	status = acpi_resource_to_address64(res, &addr);
+
+	if (ACPI_SUCCESS(status)) {
+		dev_info(dev, "found 0x%llx(0x%llx)\n",
+				(long long)addr.minimum,
+				(long long)addr.address_length);
+		tpm->tpm_phys_address = addr.minimum;
+		tpm->tpm_address = ioremap(addr.minimum, addr.address_length);
+		tpm->tpm_size = addr.address_length;
+	} else if (res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
+		struct acpi_resource_fixed_memory32 *fixmem32;
+
+		fixmem32 = &res->data.fixed_memory32;
+		if (!fixmem32)
+			return AE_NO_MEMORY;
+
+		dev_info(dev, "found 0x%llx(0x%llx)\n",
+				(long long)fixmem32->address,
+				(long long)TIS_MEM_LEN);
+		tpm->tpm_phys_address = fixmem32->address;
+		tpm->tpm_address = ioremap(fixmem32->address, TIS_MEM_LEN);
+		tpm->tpm_size = fixmem32->address_length;
+	} else if (res->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
+		struct acpi_resource_extended_irq *irq;
+
+		irq = &res->data.extended_irq;
+		if (irq->interrupt_count > 0 && irq->interrupts[0] > 0) {
+			dev_info(dev, "IRQ %d (%d, %d)\n",
+					irq->interrupts[0],
+					irq->triggering, irq->polarity);
+			tpm->tpm_irq = irq->interrupts[0];
+		}
+	}
+
+	return AE_OK;
+}
+
+static int tpm_tis_acpi_add(struct acpi_device *device)
 {
-	resource_size_t start, len;
-	unsigned int irq = 0;
+	acpi_status result;
+	struct tpm_data *tpm = kzalloc(sizeof(*tpm), GFP_KERNEL);
+	int rc = -ENOMEM;
+
+	if (!tpm)
+		goto out;
 
-	start = pnp_mem_start(pnp_dev, 0);
-	len = pnp_mem_len(pnp_dev, 0);
+	device->driver_data = tpm;
+
+	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+			tpm_resources, device);
+
+	rc = -ENODEV;
+	if (ACPI_FAILURE(result))
+		goto out_free;
+
+	if (!tpm->tpm_address) {
+		dev_err(&device->dev, "no address found in _CRS\n");
+		goto out_free;
+	}
 
-	if (pnp_irq_valid(pnp_dev, 0))
-		irq = pnp_irq(pnp_dev, 0);
-	else
+	if (!tpm->tpm_irq) {
+		dev_err(&device->dev, "no IRQ found in _CRS, polling mode\n");
 		interrupts = 0;
+	}
 
-	return tpm_tis_init(&pnp_dev->dev, start, len, irq);
+	return tpm_tis_init(&device->dev, tpm->tpm_phys_address, tpm->tpm_size,
+			tpm->tpm_irq);
+out_free:
+	kfree(tpm);
+out:
+	return rc;
 }
 
-static int tpm_tis_pnp_suspend(struct pnp_dev *dev, pm_message_t msg)
+static int tpm_tis_acpi_remove(struct acpi_device *device, int type)
 {
-	return tpm_pm_suspend(&dev->dev, msg);
+	struct tpm_chip *chip = dev_get_drvdata(&device->dev);
+	tpm_remove_hardware(&device->dev);
+	iowrite32(~TPM_GLOBAL_INT_ENABLE & ioread32(chip->vendor.iobase +
+				TPM_INT_ENABLE(chip->vendor.locality)),
+		  chip->vendor.iobase +
+		  TPM_INT_ENABLE(chip->vendor.locality));
+	release_locality(chip, chip->vendor.locality, 1);
+	if (chip->vendor.irq)
+		free_irq(chip->vendor.irq, chip);
+	iounmap(chip->vendor.iobase);
+	kfree(device->driver_data);
+	return 0;
 }
 
-static int tpm_tis_pnp_resume(struct pnp_dev *dev)
+static int tpm_tis_acpi_suspend(struct acpi_device *dev, pm_message_t state)
+{
+	return tpm_pm_suspend(&dev->dev, state);
+}
+
+static int tpm_tis_acpi_resume(struct acpi_device *dev)
 {
 	return tpm_pm_resume(&dev->dev);
 }
 
-static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
+static struct acpi_device_id tpm_tis_acpi_tbl[] __devinitdata = {
 	{"PNP0C31", 0},		/* TPM */
 	{"ATM1200", 0},		/* Atmel */
 	{"IFX0102", 0},		/* Infineon */
@@ -625,34 +715,27 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
 	{"BCM0102", 0},		/* Broadcom */
 	{"NSC1200", 0},		/* National */
 	{"ICO0102", 0},		/* Intel */
+	{"INTC0102", 0},	/* TPM spec says to look for this in _CID */
 	/* Add new here */
 	{"", 0},		/* User Specified */
 	{"", 0}			/* Terminator */
 };
-MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
-
-static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
-{
-	struct tpm_chip *chip = pnp_get_drvdata(dev);
-
-	tpm_dev_vendor_release(chip);
-
-	kfree(chip);
-}
-
+MODULE_DEVICE_TABLE(acpi, tpm_tis_acpi_tbl);
 
-static struct pnp_driver tis_pnp_driver = {
+static struct acpi_driver tis_acpi_driver = {
 	.name = "tpm_tis",
-	.id_table = tpm_pnp_tbl,
-	.probe = tpm_tis_pnp_init,
-	.suspend = tpm_tis_pnp_suspend,
-	.resume = tpm_tis_pnp_resume,
-	.remove = tpm_tis_pnp_remove,
+	.ids = tpm_tis_acpi_tbl,
+	.ops = {
+		.add = tpm_tis_acpi_add,
+		.remove = tpm_tis_acpi_remove,
+		.suspend = tpm_tis_acpi_suspend,
+		.resume = tpm_tis_acpi_resume,
+	},
 };
 
-#define TIS_HID_USR_IDX (ARRAY_SIZE(tpm_pnp_tbl) - 2)
-module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
-		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
+#define TIS_HID_USR_IDX (ARRAY_SIZE(tpm_tis_acpi_tbl) - 2)
+module_param_string(hid, tpm_tis_acpi_tbl[TIS_HID_USR_IDX].id,
+		    sizeof(tpm_tis_acpi_tbl[TIS_HID_USR_IDX].id), 0444);
 MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
 
 static struct device_driver tis_drv = {
@@ -685,7 +768,7 @@ static int __init init_tis(void)
 		return rc;
 	}
 
-	return pnp_register_driver(&tis_pnp_driver);
+	return acpi_bus_register_driver(&tis_acpi_driver);
 }
 
 static void __exit cleanup_tis(void)
@@ -714,7 +797,7 @@ static void __exit cleanup_tis(void)
 		platform_device_unregister(pdev);
 		driver_unregister(&tis_drv);
 	} else
-		pnp_unregister_driver(&tis_pnp_driver);
+		acpi_bus_unregister_driver(&tis_acpi_driver);
 }
 
 module_init(init_tis);
-- 
1.6.3.1

[PATCH 6/6] tpm_tis: add workarounds for iTPM

[Andy Isaacson]

Some Lenovo platforms (X200 and T400, maybe others) have an "iTPM" which
does not quite conform to the TPM spec.  With one small hack the iTPM
seems to work OK.

iTPM does not set TPM_STS_DATA_EXPECT reliably during multi-burst
transfers.  (STS_DATA_EXPECT appears to be set after the final burst,
not after each burst.)  So we give up and don't attempt to report
errors during write.

Based on a patch from Colin Didier and the tpmdd-devel mailing list:
http://cybione.org/~cdidier/log/data/200812020841/itpm.diff

Signed-off-by: Andy Isaacson <adi@vmware.com>
---
 drivers/char/tpm/tpm_tis.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 7f0cb6c..07ac7e0 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -85,6 +85,7 @@ struct tpm_data {
 	void __iomem *tpm_address;
 	int tpm_size;
 	int tpm_irq;
+	int itpm;
 };
 
 static int check_locality(struct tpm_chip *chip, int l)
@@ -277,6 +278,7 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	int rc, status, burstcnt;
 	size_t count = 0;
 	u32 ordinal;
+	struct tpm_data *tpm = to_acpi_device(chip->dev)->driver_data;
 
 	if (request_locality(chip, 0) < 0)
 		return -EBUSY;
@@ -303,7 +305,7 @@ 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 (!tpm->itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -444,12 +446,17 @@ static int interrupts = 1;
 module_param(interrupts, bool, 0444);
 MODULE_PARM_DESC(interrupts, "Enable interrupts");
 
+static int itpm;
+module_param(itpm, bool, 0444);
+MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
+
 static int tpm_tis_init(struct device *dev, resource_size_t start,
 			resource_size_t len, unsigned int irq)
 {
 	u32 vendor, intfcaps, intmask;
 	int rc, i;
 	struct tpm_chip *chip;
+	struct tpm_data *tpm = to_acpi_device(dev)->driver_data;
 
 	if (!(chip = tpm_register_hardware(dev, &tpm_tis)))
 		return -ENODEV;
@@ -477,6 +484,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		 "1.2 TPM (%04X:%04X rev %d)\n", vendor & 0xffff,
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
+	if (itpm || vendor == 0x10208086) {
+		dev_info(dev, "Intel iTPM workaround enabled\n");
+		tpm->itpm = 1;
+	}
+
 	/* Figure out the capabilities */
 	intfcaps =
 	    ioread32(chip->vendor.iobase +
-- 
1.6.3.1

Re: [PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Alan Cox]

On Tue, 30 Jun 2009 18:04:14 -0700
Andy Isaacson <adi@vmware.com> wrote:

> Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
> and the TPM spec says that the _CID method should be used to enumerate
> the TPM chip.

There are a number of systems with TPMs (older laptops) that don't work
very well if you enable ACPI.

This is therefore a regression - NAK

Probably the best thing to do is to provide both ACPI and PnP
registration according to what is configured into the kernel. (And I
guess spot duplicates although the resource should be busy anyway)

Re: [PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Rajiv Andrade]

On Wed, 2009-07-01 at 11:01 +0100, Alan Cox wrote:
> On Tue, 30 Jun 2009 18:04:14 -0700
> Andy Isaacson <adi@vmware.com> wrote:
> 
> > Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
> > and the TPM spec says that the _CID method should be used to enumerate
> > the TPM chip.
> 
> There are a number of systems with TPMs (older laptops) that don't work
> very well if you enable ACPI.
> 
> This is therefore a regression - NAK
> 
> Probably the best thing to do is to provide both ACPI and PnP
> registration according to what is configured into the kernel. (And I
> guess spot duplicates although the resource should be busy anyway)
> --
David sent this earlier when I said that PNP didn't work with this chip:
        
<quote>
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. 

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

</quote>

If so, we can just base our DATA_EXPECT bypass on the EISA PNP CID:

>From 47516ff6b63b81d1e806148dc5e3052a001e45d0 Mon Sep 17 00:00:00 2001
From: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Date: Wed, 1 Jul 2009 09:59:55 -0300
Subject: [PATCH] TPM: DATA_EXPECT bit check bypass

Since the iTPM doesn't set the DATA_EXPECT bit when it should, we bypass
this bit check in case we're running the code over this specific TPM.

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

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8e00b4d..ed4ecf0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -109,6 +109,7 @@ struct tpm_chip {
 
 	struct list_head list;
 	void (*release) (struct device *);
+	bool is_itpm;
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..74a60d7 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 "INTC0102"
 
 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->is_itpm)) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 	tpm_get_timeouts(chip);
 	tpm_continue_selftest(chip);
 
+	for (i=0; i < 8; i++)
+		if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
+			break;
+	if (i == 8)
+		chip->is_itpm = 1;
+
 	return 0;
 out_err:
 	if (chip->vendor.iobase)
-- 
1.6.3.1

Re: [tpmdd-devel] [PATCH 6/6] tpm_tis: add workarounds for iTPM

[Marcin Obara]

2009/7/1 Andy Isaacson <adi@vmware.com>:
> Some Lenovo platforms (X200 and T400, maybe others) have an "iTPM" which
> does not quite conform to the TPM spec.  With one small hack the iTPM
> seems to work OK.

Small comment:

Intel iTPM on mobile platforms require this patch.
Intel iTPM on desktop platforms works well without this patch.

--
Marcin

Re: [tpmdd-devel] [PATCH 6/6] tpm_tis: add workarounds for iTPM

[Andy Isaacson]

On Fri, Jul 03, 2009 at 08:18:18PM +0200, Marcin Obara wrote:
> 2009/7/1 Andy Isaacson <adi@vmware.com>:
> > Some Lenovo platforms (X200 and T400, maybe others) have an "iTPM" which
> > does not quite conform to the TPM spec. ?With one small hack the iTPM
> > seems to work OK.
> 
> Small comment:
> 
> Intel iTPM on mobile platforms require this patch.
> Intel iTPM on desktop platforms works well without this patch.

Please do expand on this -- I don't have access to any iTPM desktop
platforms to verify.  Can you provide information on what desktop
platforms behave differently so that I can adjust the patch?

If you could simply boot a kernel with my patch series on a desktop iTPM
and provide the dmesg output, that would be great!

Thanks,
-andy

Re: [tpmdd-devel] [PATCH 6/6] tpm_tis: add workarounds for iTPM

[Marcin Obara]

2009/7/3 Andy Isaacson <adi@hexapodia.org>:
> On Fri, Jul 03, 2009 at 08:18:18PM +0200, Marcin Obara wrote:
>> Intel iTPM on mobile platforms require this patch.
>> Intel iTPM on desktop platforms works well without this patch.
>
> Please do expand on this -- I don't have access to any iTPM desktop
> platforms to verify.  Can you provide information on what desktop
> platforms behave differently so that I can adjust the patch?
>
> If you could simply boot a kernel with my patch series on a desktop iTPM
> and provide the dmesg output, that would be great!

Intel iTPM with HID:  ICO0102 does not require this patch.
 (http://lkml.org/lkml/2008/6/28/143)

Currently, I don't have Q45 platform tested few months ago.

--
Marcin

Re: [tpmdd-devel] [PATCH 6/6] tpm_tis: add workarounds for iTPM

[Andy Isaacson]

On Fri, Jul 03, 2009 at 10:10:30PM +0200, Marcin Obara wrote:
> 2009/7/3 Andy Isaacson <adi@hexapodia.org>:
> > On Fri, Jul 03, 2009 at 08:18:18PM +0200, Marcin Obara wrote:
> >> Intel iTPM on mobile platforms require this patch.
> >> Intel iTPM on desktop platforms works well without this patch.
> >
> > Please do expand on this -- I don't have access to any iTPM desktop
> > platforms to verify.  Can you provide information on what desktop
> > platforms behave differently so that I can adjust the patch?
> >
> > If you could simply boot a kernel with my patch series on a desktop iTPM
> > and provide the dmesg output, that would be great!
> 
> Intel iTPM with HID:  ICO0102 does not require this patch.
>  (http://lkml.org/lkml/2008/6/28/143)
> 
> Currently, I don't have Q45 platform tested few months ago.

Ah, thanks for the reference.  I may be able to find a Q45+iTPM to test
on.  It would still be very useful to get a dmesg (specifically with
"[PATCH 4/6] tpm_tis: print complete vendor information") to verify the
behavior on the Q45.

-andy

Re: [PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Rajiv Andrade]

Realized just now that this message was stuck in my outbox and was never
sent.. Anyway, here it is. The ones who own the chip, please let me know
if the patches here work OK.

Rajiv

On Wed, 2009-07-01 at 10:46 -0300, Rajiv Andrade wrote:
> On Wed, 2009-07-01 at 11:01 +0100, Alan Cox wrote:
> > On Tue, 30 Jun 2009 18:04:14 -0700
> > Andy Isaacson <adi@vmware.com> wrote:
> > 
> > > Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
> > > and the TPM spec says that the _CID method should be used to enumerate
> > > the TPM chip.
> > 
> > There are a number of systems with TPMs (older laptops) that don't work
> > very well if you enable ACPI.
> > 
> > This is therefore a regression - NAK
> > 
> > Probably the best thing to do is to provide both ACPI and PnP
> > registration according to what is configured into the kernel. (And I
> > guess spot duplicates although the resource should be busy anyway)
> > --
> David sent this earlier when I said that PNP didn't work with this chip:
>         
> <quote>
> 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. 
> 
> 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));
> 
> </quote>
> 
> If so, we can just base our DATA_EXPECT bypass on the EISA PNP CID:
> 
> >From 47516ff6b63b81d1e806148dc5e3052a001e45d0 Mon Sep 17 00:00:00 2001
> From: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> Date: Wed, 1 Jul 2009 09:59:55 -0300
> Subject: [PATCH] TPM: DATA_EXPECT bit check bypass
> 
> Since the iTPM doesn't set the DATA_EXPECT bit when it should, we bypass
> this bit check in case we're running the code over this specific TPM.
> 
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm.h     |    1 +
>  drivers/char/tpm/tpm_tis.c |   11 ++++++++++-
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 8e00b4d..ed4ecf0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -109,6 +109,7 @@ struct tpm_chip {
>  
>  	struct list_head list;
>  	void (*release) (struct device *);
> +	bool is_itpm;
>  };
>  
>  #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index aec1931..74a60d7 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 "INTC0102"
>  
>  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->is_itpm)) {
>  			rc = -EIO;
>  			goto out_err;
>  		}
> @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>  	tpm_get_timeouts(chip);
>  	tpm_continue_selftest(chip);
>  
> +	for (i=0; i < 8; i++)
> +		if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> +			break;
> +	if (i == 8)
> +		chip->is_itpm = 1;
> +
>  	return 0;
>  out_err:
>  	if (chip->vendor.iobase)

[PATCH] TPM: DATA_EXPECT bit check bypass

[Rajiv Andrade]

Sending now inline in order to work ok with git-am, sorry for the flood.

This patch depends on patch http://patchwork.kernel.org/patch/33486/

Since the iTPM doesn't set the DATA_EXPECT bit when it should, we bypass
this bit check in case we're running the code over this specific TPM.

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

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8e00b4d..ed4ecf0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -109,6 +109,7 @@ struct tpm_chip {
 
        struct list_head list;
        void (*release) (struct device *);
+       bool is_itpm;
 };
 
 #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..74a60d7 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 "INTC0102"
 
 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->is_itpm)) {
                        rc = -EIO;
                        goto out_err;
                }
@@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
        tpm_get_timeouts(chip);
        tpm_continue_selftest(chip);
 
+       for (i=0; i < 8; i++)
+               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
+                       break;
+       if (i == 8)
+               chip->is_itpm = 1;
+
        return 0;
 out_err:
        if (chip->vendor.iobase)
-- 
1.6.3.1

Re: [PATCH] TPM: DATA_EXPECT bit check bypass

[Valdis.Kletnieks]

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

On Thu, 16 Jul 2009 14:43:32 -0300, Rajiv Andrade said:

> @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_siz
e_t start,
>         tpm_get_timeouts(chip);
>         tpm_continue_selftest(chip);
>  
> +       for (i=0; i < 8; i++)
> +               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> +                       break;
> +       if (i == 8)
> +               chip->is_itpm = 1;
> +

strcmp() variant of some sort instead?

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

Re: [PATCH] TPM: DATA_EXPECT bit check bypass

[Rajiv Andrade]

On Thu, 2009-07-16 at 16:08 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 16 Jul 2009 14:43:32 -0300, Rajiv Andrade said:
> 
> > @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_siz
> e_t start,
> >         tpm_get_timeouts(chip);
> >         tpm_continue_selftest(chip);
> >  
> > +       for (i=0; i < 8; i++)
> > +               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> > +                       break;
> > +       if (i == 8)
> > +               chip->is_itpm = 1;
> > +
> 
> strcmp() variant of some sort instead?

Much better, thanks. Forgot that lib/string.c can be handy sometimes :-)

Rajiv

Re: [PATCH] TPM: DATA_EXPECT bit check bypass

[Rajiv Andrade]

On Thu, 2009-07-16 at 16:08 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 16 Jul 2009 14:43:32 -0300, Rajiv Andrade said:
> 
> > @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_siz
> e_t start,
> >         tpm_get_timeouts(chip);
> >         tpm_continue_selftest(chip);
> >  
> > +       for (i=0; i < 8; i++)
> > +               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> > +                       break;
> > +       if (i == 8)
> > +               chip->is_itpm = 1;
> > +
> 
> strcmp() variant of some sort instead?

Wait, is to_pnp_dev(dev)->id->id[i] null terminated? Maybe memcmp() fits
better here..

Rajiv

Re: [PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Andy Isaacson]

On Wed, Jul 01, 2009 at 10:45:19AM -0300, Rajiv Andrade wrote:
> On Wed, 2009-07-01 at 11:01 +0100, Alan Cox wrote:
> > On Tue, 30 Jun 2009 18:04:14 -0700
> > Andy Isaacson <adi@vmware.com> wrote:
> > 
> > > Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
> > > and the TPM spec says that the _CID method should be used to enumerate
> > > the TPM chip.
> > 
> > There are a number of systems with TPMs (older laptops) that don't work
> > very well if you enable ACPI.
> > 
> > This is therefore a regression - NAK
> > 
> > Probably the best thing to do is to provide both ACPI and PnP
> > registration according to what is configured into the kernel. (And I
> > guess spot duplicates although the resource should be busy anyway)
> > --
> David sent this earlier when I said that PNP didn't work with this chip:
>         
> <quote>
> 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. 
> 
> 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));
> 
> </quote>

Len,

Is this an acceptable change to pnpacpi?  It resolves an issue with
tpm_tis but I'm concerned that it might have far-reaching impact.

I've pasted in the problematic DSDT (manually fixing up whitespace to
make it more readable), and then a normal TPM simply has a _HID which
is matched by a pnp_device_id table in the driver
(drivers/char/tpm/tpm_tis.c).

T400:
        Device (TPM)
        {
            Method (_HID, 0, NotSerialized)
            {
                TPHY (0x00)
                If (LEqual (TPMV, 0x01)) { Return (0x0201D824) } 
                If (LEqual (TPMV, 0x02)) { Return (0x0435CF4D) } 
                If (LEqual (TPMV, 0x03)) { Return (0x02016D08) } 
                If (LEqual (TPMV, 0x04)) { Return (0x01016D08) } 
                If (LOr (LEqual (TPMV, 0x05), LEqual (TPMV, 0x06))) {
			Return (0x0010A35C)
		} 
                If (LEqual (TPMV, 0x08)) { Return (0x00128D06) } 
                If (LEqual (TPMV, 0x09)) { Return ("INTC0102") } 
                Return (0x310CD041)
            }

            Name (_CID, EisaId ("PNP0C31"))

standard TPM:
        Device (TPM)
        {
            Name (_HID, EisaId ("BCM0102"))
            Name (_CID, EisaId ("PNP0C31"))

The full thread is at
http://lkml.org/lkml/2009/7/1/265

Thanks for any insight.

-andy

Re: [PATCH] TPM: DATA_EXPECT bit check bypass

[Andy Isaacson]

On Thu, Jul 16, 2009 at 06:20:26PM -0300, Rajiv Andrade wrote:
> On Thu, 2009-07-16 at 16:08 -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Thu, 16 Jul 2009 14:43:32 -0300, Rajiv Andrade said:
> > 
> > > @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_siz
> > e_t start,
> > >         tpm_get_timeouts(chip);
> > >         tpm_continue_selftest(chip);
> > >  
> > > +       for (i=0; i < 8; i++)
> > > +               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> > > +                       break;
> > > +       if (i == 8)
> > > +               chip->is_itpm = 1;
> > > +
> > 
> > strcmp() variant of some sort instead?
> 
> Wait, is to_pnp_dev(dev)->id->id[i] null terminated? Maybe memcmp() fits
> better here..

Rather than checking the PNP ID at this point, I suggest something like:

(the context here depends on my earlier series, but it's fairly
obvious.)

@@ -467,6 +481,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		 "1.2 TPM (%04X:%04X rev %d)\n", vendor & 0xffff,
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
+	if (vendor == 0x10208086) {
+		dev_info(dev, "Intel iTPM workaround enabled\n");
+		chip->itpm = 1;
+	}
+
 	/* Figure out the capabilities */
 	intfcaps =
 	    ioread32(chip->vendor.iobase +

(I suppose there should be a #define of 0x10208086 somewhere.)

I'll cook up a refreshed patch series.

-andy

Re: [PATCH] TPM: DATA_EXPECT bit check bypass

[Rajiv Andrade]

On Mon, 2009-07-20 at 16:28 -0700, Andy Isaacson wrote:
> On Thu, Jul 16, 2009 at 06:20:26PM -0300, Rajiv Andrade wrote:
> > On Thu, 2009-07-16 at 16:08 -0400, Valdis.Kletnieks@vt.edu wrote:
> > > On Thu, 16 Jul 2009 14:43:32 -0300, Rajiv Andrade said:
> > > 
> > > > @@ -582,6 +585,12 @@ static int tpm_tis_init(struct device *dev, resource_siz
> > > e_t start,
> > > >         tpm_get_timeouts(chip);
> > > >         tpm_continue_selftest(chip);
> > > >  
> > > > +       for (i=0; i < 8; i++)
> > > > +               if (ITPM_ID[i] != to_pnp_dev(dev)->id->id[i])
> > > > +                       break;
> > > > +       if (i == 8)
> > > > +               chip->is_itpm = 1;
> > > > +
> > > 
> > > strcmp() variant of some sort instead?
> > 
> > Wait, is to_pnp_dev(dev)->id->id[i] null terminated? Maybe memcmp() fits
> > better here..
> 
> Rather than checking the PNP ID at this point, I suggest something like:
> 
> (the context here depends on my earlier series, but it's fairly
> obvious.)
> 
> @@ -467,6 +481,11 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>  		 "1.2 TPM (%04X:%04X rev %d)\n", vendor & 0xffff,
>  		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
> 
> +	if (vendor == 0x10208086) {
> +		dev_info(dev, "Intel iTPM workaround enabled\n");
> +		chip->itpm = 1;
> +	}
> +
>  	/* Figure out the capabilities */
>  	intfcaps =
>  	    ioread32(chip->vendor.iobase +
> 
> (I suppose there should be a #define of 0x10208086 somewhere.)
> 

Much better, my patch would break everything in case force option was
set.

> I'll cook up a refreshed patch series.

Great, I'll ack this one when I get it, thanks.

Rajiv
+++

Re: [PATCH 5/6] tpm_tis: convert from pnp_driver to acpi_driver

[Rajiv Andrade]

On Mon, 2009-07-20 at 11:27 -0700, Andy Isaacson wrote:
> On Wed, Jul 01, 2009 at 10:45:19AM -0300, Rajiv Andrade wrote:
> > On Wed, 2009-07-01 at 11:01 +0100, Alan Cox wrote:
> > > On Tue, 30 Jun 2009 18:04:14 -0700
> > > Andy Isaacson <adi@vmware.com> wrote:
> > > 
> > > > Not all TIS-compatible TPM chips have a _HID method in their ACPI entry,
> > > > and the TPM spec says that the _CID method should be used to enumerate
> > > > the TPM chip.
> > > 
> > > There are a number of systems with TPMs (older laptops) that don't work
> > > very well if you enable ACPI.
> > > 
> > > This is therefore a regression - NAK
> > > 
> > > Probably the best thing to do is to provide both ACPI and PnP
> > > registration according to what is configured into the kernel. (And I
> > > guess spot duplicates although the resource should be busy anyway)
> > > --
> > David sent this earlier when I said that PNP didn't work with this chip:
> >         
> > <quote>
> > 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. 
> > 
> > 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));
> > 
> > </quote>
> 
> Len,
> 
> Is this an acceptable change to pnpacpi?  It resolves an issue with
> tpm_tis but I'm concerned that it might have far-reaching impact.
> 
> I've pasted in the problematic DSDT (manually fixing up whitespace to
> make it more readable), and then a normal TPM simply has a _HID which
> is matched by a pnp_device_id table in the driver
> (drivers/char/tpm/tpm_tis.c).
> 
> T400:
>         Device (TPM)
>         {
>             Method (_HID, 0, NotSerialized)
>             {
>                 TPHY (0x00)
>                 If (LEqual (TPMV, 0x01)) { Return (0x0201D824) } 
>                 If (LEqual (TPMV, 0x02)) { Return (0x0435CF4D) } 
>                 If (LEqual (TPMV, 0x03)) { Return (0x02016D08) } 
>                 If (LEqual (TPMV, 0x04)) { Return (0x01016D08) } 
>                 If (LOr (LEqual (TPMV, 0x05), LEqual (TPMV, 0x06))) {
> 			Return (0x0010A35C)
> 		} 
>                 If (LEqual (TPMV, 0x08)) { Return (0x00128D06) } 
>                 If (LEqual (TPMV, 0x09)) { Return ("INTC0102") } 
>                 Return (0x310CD041)
>             }
> 
>             Name (_CID, EisaId ("PNP0C31"))
> 
> standard TPM:
>         Device (TPM)
>         {
>             Name (_HID, EisaId ("BCM0102"))
>             Name (_CID, EisaId ("PNP0C31"))
> 
> The full thread is at
> http://lkml.org/lkml/2009/7/1/265
> 
> Thanks for any insight.
> 

We've already waited too much on this, is it acceptable to make the
workaround depend on (and only on) the module parameter you've set in
patch 6/6? Therefore no need to check the vendor ID.

<snip>
+MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)"); 
</snip>

It already mentions _Force_, which in many cases maps to "it's all your
responsibility"...

And yes, still without PNP, but at least, working.

Rajiv

[PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Rajiv Andrade]

Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.

This patch is based on Andy Isaacson's one:

http://marc.info/?l=linux-kernel&m=124650185023495&w=2

It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
patch to do so was NACK'd:

http://marc.info/?l=linux-kernel&m=124650186423711&w=2

This way we let the user choose using this workaround or not based on his
observations on this code behavior when trying to use the TPM.

Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..c9990db 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -257,6 +257,10 @@ out:
 	return size;
 }
 
+static int itpm = 0;
+module_param(itpm, bool, 0444);
+MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
+
 /*
  * 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
@@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
+	if (itpm) 
+		dev_info(dev, "Intel iTPM workaround enabled\n");
+
+
 	/* Figure out the capabilities */
 	intfcaps =
 	    ioread32(chip->vendor.iobase +

Re: [PATCH 1/6] tpm_tis: various cleanups

[Rajiv Andrade]

Acked-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

On Tue, 2009-06-30 at 18:04 -0700, Andy Isaacson wrote:
> Avoid tabs in printk output.
> Use parentheses and ARRAY_SIZE() in macro definition.
> 
> Signed-off-by: Andy Isaacson <adi@vmware.com>
> ---
>  drivers/char/tpm/tpm_tis.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 717af7a..1b84441 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -474,23 +474,23 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>  	dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>  		intfcaps);
>  	if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> -		dev_dbg(dev, "\tBurst Count Static\n");
> +		dev_dbg(dev, "    Burst Count Static\n");
>  	if (intfcaps & TPM_INTF_CMD_READY_INT)
> -		dev_dbg(dev, "\tCommand Ready Int Support\n");
> +		dev_dbg(dev, "    Command Ready Int Support\n");
>  	if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> -		dev_dbg(dev, "\tInterrupt Edge Falling\n");
> +		dev_dbg(dev, "    Interrupt Edge Falling\n");
>  	if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> -		dev_dbg(dev, "\tInterrupt Edge Rising\n");
> +		dev_dbg(dev, "    Interrupt Edge Rising\n");
>  	if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> -		dev_dbg(dev, "\tInterrupt Level Low\n");
> +		dev_dbg(dev, "    Interrupt Level Low\n");
>  	if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> -		dev_dbg(dev, "\tInterrupt Level High\n");
> +		dev_dbg(dev, "    Interrupt Level High\n");
>  	if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> -		dev_dbg(dev, "\tLocality Change Int Support\n");
> +		dev_dbg(dev, "    Locality Change Int Support\n");
>  	if (intfcaps & TPM_INTF_STS_VALID_INT)
> -		dev_dbg(dev, "\tSts Valid Int Support\n");
> +		dev_dbg(dev, "    Sts Valid Int Support\n");
>  	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> -		dev_dbg(dev, "\tData Avail Int Support\n");
> +		dev_dbg(dev, "    Data Avail Int Support\n");
> 
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&chip->vendor.read_queue);
> @@ -649,7 +649,7 @@ static struct pnp_driver tis_pnp_driver = {
>  	.remove = tpm_tis_pnp_remove,
>  };
> 
> -#define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
> +#define TIS_HID_USR_IDX (ARRAY_SIZE(tpm_pnp_tbl) - 2)
>  module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
>  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");

Re: [PATCH 2/6] tpm_tis: add MODULE_DEVICE_TABLE to enable autoload

[Rajiv Andrade]

Acked-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

On Tue, 2009-06-30 at 18:04 -0700, Andy Isaacson wrote:
> Signed-off-by: Andy Isaacson <adi@vmware.com>
> ---
>  drivers/char/tpm/tpm_tis.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 1b84441..e859e0e 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -629,6 +629,7 @@ static struct pnp_device_id tpm_pnp_tbl[] __devinitdata = {
>  	{"", 0},		/* User Specified */
>  	{"", 0}			/* Terminator */
>  };
> +MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
> 
>  static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
>  {

Re: [PATCH 4/6] tpm_tis: print complete vendor information

[Rajiv Andrade]

Acked-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

On Tue, 2009-06-30 at 18:04 -0700, Andy Isaacson wrote:
> The TPM_VID field contains a vendor-id as well as a device-id,
> but the code only prints the device-id.  Fix it.
> 
> Signed-off-by: Andy Isaacson <adi@vmware.com>
> ---
>  drivers/char/tpm/tpm_tis.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index e0bda02..50c2a8a 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -464,7 +464,7 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>  	vendor = ioread32(chip->vendor.iobase + TPM_DID_VID(0));
> 
>  	dev_info(dev,
> -		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
> +		 "1.2 TPM (%04X:%04X rev %d)\n", vendor & 0xffff,
>  		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
> 
>  	/* Figure out the capabilities */

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Daniel Walker]

On Thu, 2009-09-10 at 16:54 -0300, Rajiv Andrade wrote:

> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> ---
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index aec1931..c9990db 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -257,6 +257,10 @@ out:
>  	return size;
>  }
>  
> +static int itpm = 0;

This patch has some checkpatch issues .. Could you run it
through ./script/checkpatch.pl and fix the issues that get reported?

Daniel

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Rajiv Andrade]

On Thu, 2009-09-10 at 12:58 -0700, Daniel Walker wrote:
> On Thu, 2009-09-10 at 16:54 -0300, Rajiv Andrade wrote:
> 
> > Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> > ---
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index aec1931..c9990db 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -257,6 +257,10 @@ out:
> >  	return size;
> >  }
> >  
> > +static int itpm = 0;
> 
> This patch has some checkpatch issues .. Could you run it
> through ./script/checkpatch.pl and fix the issues that get reported?
> 

Thanks for reminding me Daniel, it indeed complained about the itpm
initialization to 0 and about a whitespace. Will post the fixed patch in
reply to this note (avoid the quotes)

Rajiv

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Rajiv Andrade]

Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.

This patch is based on Andy Isaacson's one:

http://marc.info/?l=linux-kernel&m=124650185023495&w=2

It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
patch to do so was NACK'd:

http://marc.info/?l=linux-kernel&m=124650186423711&w=2

This way we let the user choose using this workaround or not based on his
observations on this code behavior when trying to use the TPM.

Fixed a checkpatch issue present on the previous patch, thanks to Daniel Walker.

Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
---
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index aec1931..c9990db 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -257,6 +257,10 @@ out:
 	return size;
 }
 
+static int itpm;
+module_param(itpm, bool, 0444);
+MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
+
 /*
  * 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
@@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
 			rc = -EIO;
 			goto out_err;
 		}
@@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
 		 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
 		 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
 
+	if (itpm)
+		dev_info(dev, "Intel iTPM workaround enabled\n");
+
+
 	/* Figure out the capabilities */
 	intfcaps =
 	    ioread32(chip->vendor.iobase +

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Andy Isaacson]

On Thu, Sep 10, 2009 at 12:58:29PM -0700, Daniel Walker wrote:
> > +static int itpm = 0;
> 
> This patch has some checkpatch issues .. Could you run it
> through ./script/checkpatch.pl and fix the issues that get reported?

Thanks for the reminder, will do.

-andy

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Seiji Munetoh]

On Fri, Sep 11, 2009 at 5:09 AM, Rajiv Andrade
<srajiv@linux.vnet.ibm.com> wrote:
> Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
> when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
> the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.
>
> This patch is based on Andy Isaacson's one:
>
> http://marc.info/?l=linux-kernel&m=124650185023495&w=2
>
> It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
> patch to do so was NACK'd:
>
> http://marc.info/?l=linux-kernel&m=124650186423711&w=2
>
> This way we let the user choose using this workaround or not based on his
> observations on this code behavior when trying to use the TPM.
>
> Fixed a checkpatch issue present on the previous patch, thanks to Daniel Walker.
>
> Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

As far as I know, only the intel tpm has this PNP issue, so I'm fine with it.

Tested-by: Seiji Munetoh <seiji.munetoh@gmail.com>

> ---
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index aec1931..c9990db 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -257,6 +257,10 @@ out:
>        return size;
>  }
>
> +static int itpm;
> +module_param(itpm, bool, 0444);
> +MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
> +
>  /*
>  * 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
> @@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>                        rc = -EIO;
>                        goto out_err;
>                }
> @@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>                 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
>                 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
>
> +       if (itpm)
> +               dev_info(dev, "Intel iTPM workaround enabled\n");
> +
> +
>        /* Figure out the capabilities */
>        intfcaps =
>            ioread32(chip->vendor.iobase +
>
>
>

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Rajiv Andrade]

This was already tested and, given no more comments on it, finally
reviewed. Can it already be merged?

Thanks,
Rajiv

On Sat, 2009-09-12 at 08:34 +0900, Seiji Munetoh wrote:
> On Fri, Sep 11, 2009 at 5:09 AM, Rajiv Andrade
> <srajiv@linux.vnet.ibm.com> wrote:
> > Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
> > when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
> > the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.
> >
> > This patch is based on Andy Isaacson's one:
> >
> > http://marc.info/?l=linux-kernel&m=124650185023495&w=2
> >
> > It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
> > patch to do so was NACK'd:
> >
> > http://marc.info/?l=linux-kernel&m=124650186423711&w=2
> >
> > This way we let the user choose using this workaround or not based on his
> > observations on this code behavior when trying to use the TPM.
> >
> > Fixed a checkpatch issue present on the previous patch, thanks to Daniel Walker.
> >
> > Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> 
> As far as I know, only the intel tpm has this PNP issue, so I'm fine with it.
> 
> Tested-by: Seiji Munetoh <seiji.munetoh@gmail.com>
> 
> > ---
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index aec1931..c9990db 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -257,6 +257,10 @@ out:
> >        return size;
> >  }
> >
> > +static int itpm;
> > +module_param(itpm, bool, 0444);
> > +MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
> > +
> >  /*
> >  * 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
> > @@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
> >                        rc = -EIO;
> >                        goto out_err;
> >                }
> > @@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
> >                 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
> >                 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
> >
> > +       if (itpm)
> > +               dev_info(dev, "Intel iTPM workaround enabled\n");
> > +
> > +
> >        /* Figure out the capabilities */
> >        intfcaps =
> >            ioread32(chip->vendor.iobase +
> >
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[David Smith]

Hi, can this be merged, please? Using the module parameter is not
optimal but it's better than the complete lack of support today.

On Fri, Sep 25, 2009 at 3:43 AM, Rajiv Andrade
<srajiv@linux.vnet.ibm.com> wrote:
> This was already tested and, given no more comments on it, finally
> reviewed. Can it already be merged?
>
> Thanks,
> Rajiv
>
> On Sat, 2009-09-12 at 08:34 +0900, Seiji Munetoh wrote:
>> On Fri, Sep 11, 2009 at 5:09 AM, Rajiv Andrade
>> <srajiv@linux.vnet.ibm.com> wrote:
>> > Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
>> > when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
>> > the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.
>> >
>> > This patch is based on Andy Isaacson's one:
>> >
>> > http://marc.info/?l=linux-kernel&m=124650185023495&w=2
>> >
>> > It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
>> > patch to do so was NACK'd:
>> >
>> > http://marc.info/?l=linux-kernel&m=124650186423711&w=2
>> >
>> > This way we let the user choose using this workaround or not based on his
>> > observations on this code behavior when trying to use the TPM.
>> >
>> > Fixed a checkpatch issue present on the previous patch, thanks to Daniel Walker.
>> >
>> > Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
>>
>> As far as I know, only the intel tpm has this PNP issue, so I'm fine with it.
>>
>> Tested-by: Seiji Munetoh <seiji.munetoh@gmail.com>
>>
>> > ---
>> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
>> > index aec1931..c9990db 100644
>> > --- a/drivers/char/tpm/tpm_tis.c
>> > +++ b/drivers/char/tpm/tpm_tis.c
>> > @@ -257,6 +257,10 @@ out:
>> >        return size;
>> >  }
>> >
>> > +static int itpm;
>> > +module_param(itpm, bool, 0444);
>> > +MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
>> > +
>> >  /*
>> >  * 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
>> > @@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>> >                        rc = -EIO;
>> >                        goto out_err;
>> >                }
>> > @@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>> >                 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
>> >                 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
>> >
>> > +       if (itpm)
>> > +               dev_info(dev, "Intel iTPM workaround enabled\n");
>> > +
>> > +
>> >        /* Figure out the capabilities */
>> >        intfcaps =
>> >            ioread32(chip->vendor.iobase +
>> >
>> >
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[Eric Paris]

On Tue, Oct 27, 2009 at 10:45 PM, David Smith
<david.daniel.smith@gmail.com> wrote:
> Hi, can this be merged, please? Using the module parameter is not
> optimal but it's better than the complete lack of support today.
>
> On Fri, Sep 25, 2009 at 3:43 AM, Rajiv Andrade
> <srajiv@linux.vnet.ibm.com> wrote:
>> This was already tested and, given no more comments on it, finally
>> reviewed. Can it already be merged?

James can we get this merged?

Acked-by: Eric Paris <eparis@redhat.com>

>>
>> Thanks,
>> Rajiv
>>
>> On Sat, 2009-09-12 at 08:34 +0900, Seiji Munetoh wrote:
>>> On Fri, Sep 11, 2009 at 5:09 AM, Rajiv Andrade
>>> <srajiv@linux.vnet.ibm.com> wrote:
>>> > Some newer Lenovo models are shipped with a TPM that doesn't seem to set the TPM_STS_DATA_EXPECT status bit
>>> > when sending it a burst of data, so the code understands it as a failure and doesn't proceed sending the chip
>>> > the intended data. In this patch we bypass this bit check in case the itpm module parameter was set.
>>> >
>>> > This patch is based on Andy Isaacson's one:
>>> >
>>> > http://marc.info/?l=linux-kernel&m=124650185023495&w=2
>>> >
>>> > It was heavily discussed how should we deal with identifying the chip in kernel space, but the required
>>> > patch to do so was NACK'd:
>>> >
>>> > http://marc.info/?l=linux-kernel&m=124650186423711&w=2
>>> >
>>> > This way we let the user choose using this workaround or not based on his
>>> > observations on this code behavior when trying to use the TPM.
>>> >
>>> > Fixed a checkpatch issue present on the previous patch, thanks to Daniel Walker.
>>> >
>>> > Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
>>>
>>> As far as I know, only the intel tpm has this PNP issue, so I'm fine with it.
>>>
>>> Tested-by: Seiji Munetoh <seiji.munetoh@gmail.com>
>>>
>>> > ---
>>> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
>>> > index aec1931..c9990db 100644
>>> > --- a/drivers/char/tpm/tpm_tis.c
>>> > +++ b/drivers/char/tpm/tpm_tis.c
>>> > @@ -257,6 +257,10 @@ out:
>>> >        return size;
>>> >  }
>>> >
>>> > +static int itpm;
>>> > +module_param(itpm, bool, 0444);
>>> > +MODULE_PARM_DESC(itpm, "Force iTPM workarounds (found on some Lenovo laptops)");
>>> > +
>>> >  /*
>>> >  * 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
>>> > @@ -293,7 +297,7 @@ 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 (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>>> >                        rc = -EIO;
>>> >                        goto out_err;
>>> >                }
>>> > @@ -467,6 +471,10 @@ static int tpm_tis_init(struct device *dev, resource_size_t start,
>>> >                 "1.2 TPM (device-id 0x%X, rev-id %d)\n",
>>> >                 vendor >> 16, ioread8(chip->vendor.iobase + TPM_RID(0)));
>>> >
>>> > +       if (itpm)
>>> > +               dev_info(dev, "Intel iTPM workaround enabled\n");
>>> > +
>>> > +
>>> >        /* Figure out the capabilities */
>>> >        intfcaps =
>>> >            ioread32(chip->vendor.iobase +
>>> >
>>> >
>>> >
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] tpm_tis: TPM_STS_DATA_EXPECT workaround

[James Morris]

On Sat, 31 Oct 2009, Eric Paris wrote:

> On Tue, Oct 27, 2009 at 10:45 PM, David Smith
> <david.daniel.smith@gmail.com> wrote:
> > Hi, can this be merged, please? Using the module parameter is not
> > optimal but it's better than the complete lack of support today.
> >
> > On Fri, Sep 25, 2009 at 3:43 AM, Rajiv Andrade
> > <srajiv@linux.vnet.ibm.com> wrote:
> >> This was already tested and, given no more comments on it, finally
> >> reviewed. Can it already be merged?
> 
> James can we get this merged?
> 
> Acked-by: Eric Paris <eparis@redhat.com>


Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next


-- 
James Morris
<jmorris@namei.org>