Skip to contentSkip to conversation

New TPM driver, hwrng driver and fixes (2)

PATCH In review James Morris 3 participants 9 messages Aug 3, 2012 – Aug 7, 2012 1 message not in the archive TPM device driver

lore
Share & tools
lore
#2 lore raw
Quoted 8 lines · James Morris
Quoted 3 lines · Kent Yoder

Hi James,

The following changes since commit 663728418e3494f8e4a82f5d1b2f23c22d11be35:

Pulled to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next

Reverted:
CC [M] drivers/char/tpm/tpm_i2c_infineon.o
drivers/char/tpm/tpm_i2c_infineon.c:740: error: lvalue required as unary & operand make[1]: *** [drivers/char/tpm/tpm_i2c_infineon.o] Error 1

Was this code tested?

Signature · 2 lines
James Morris
<[email protected]>
#3 lore raw

Hi James,

Quoted 6 lines

Reverted:
CC [M] drivers/char/tpm/tpm_i2c_infineon.o
drivers/char/tpm/tpm_i2c_infineon.c:740: error: lvalue required as unary & operand make[1]: *** [drivers/char/tpm/tpm_i2c_infineon.o] Error 1

Was this code tested?

Yes - quite extensively and also reviewed quite extensively.

I'll have a look at this - probably the case if CONFIG_PM is not set is causing this. Sorry for the inconvenience, I'll create a patch.

Peter

#4 lore raw

PATCH 1/1 char/tpm: Fix compile error if CONFIG_PM is not set in tpm_i2c_infineon

In reply to James Morris

If CONFIG_PM was not set the compiler aborted with the following message:
tpm_i2c_infineon.c:740:12: error: lvalue required as unary '&' operand

This patch fixes this error by not defining tpm_tis_i2c_ops as NULL if CONFIG_PM is not set, but only setting the suspend and resume function pointer as NULL

---
Sorry for the inconvenience - now tested with and without CONFIG_PM. Sorry.

 drivers/char/tpm/tpm_i2c_infineon.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/char/tpm/tpm_i2c_infineon.c +4 -5
index 1794a09..114e1a1 100644
@@ -674,16 +674,15 @@ static int tpm_tis_i2c_resume(struct device *dev)
 {
 	return tpm_pm_resume(dev);
 }
+#else
+#define tpm_tis_i2c_suspend NULL
+#define tpm_tis_i2c_resume NULL
+#endif
 
 static const struct dev_pm_ops tpm_tis_i2c_ops = {
 	.suspend = tpm_tis_i2c_suspend,
 	.resume = tpm_tis_i2c_resume,
 };
-#else
-#define tpm_tis_i2c_suspend NULL
-#define tpm_tis_i2c_resume NULL
-#define tpm_tis_i2c_ops NULL
-#endif
 
 static int __devinit tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
Signature · 1 line
1.7.6.msysgit.0
#5 lore raw

Re: [PATCH] char/tpm: Fix compile error if CONFIG_PM is not set in tpm_i2c_infineon

Hi Peter,

Quoted 6 lines · Peter Huewe

If CONFIG_PM was not set the compiler aborted with the following message:
tpm_i2c_infineon.c:740:12: error: lvalue required as unary '&' operand

This patch fixes this error by not defining tpm_tis_i2c_ops as NULL if CONFIG_PM is not set, but only setting the suspend and resume function pointer as NULL

Lets try to sync up with the work Rafael Wysocki is doing for PM.
Please take a look at these commits:

035e2ce8eb7412dbcb8522c19676a1dd52f3c024
8324be05380be044df8b70cb4bfb0c0b50eec3e5

Thanks,
Kent

Quoted 35 lines

---
Sorry for the inconvenience - now tested with and without CONFIG_PM. Sorry.

 drivers/char/tpm/tpm_i2c_infineon.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/char/tpm/tpm_i2c_infineon.c +4 -5
index 1794a09..114e1a1 100644
@@ -674,16 +674,15 @@ static int tpm_tis_i2c_resume(struct device *dev)
 {
 	return tpm_pm_resume(dev);
 }
+#else
+#define tpm_tis_i2c_suspend NULL
+#define tpm_tis_i2c_resume NULL
+#endif
 
 static const struct dev_pm_ops tpm_tis_i2c_ops = {
 	.suspend = tpm_tis_i2c_suspend,
 	.resume = tpm_tis_i2c_resume,
 };
-#else
-#define tpm_tis_i2c_suspend NULL
-#define tpm_tis_i2c_resume NULL
-#define tpm_tis_i2c_ops NULL
-#endif
 
 static int __devinit tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
Signature · 1 line
1.7.6.msysgit.0
#6 lore raw

PATCH 1/1 char/tpm: Use struct dev_pm_ops for power management.

Make the tpm_i2c_infineon driver define its PM callbacks trough a struct dev_pm_ops by using SIMPLE_DEV_PM_OPS instead of coding it explicitly.

This simplifies the code and allows the driver to use tpm_pm_suspend() and tpm_pm_resume() as its PM callbacks directly, without defining its own PM callback routines.

---
Thanks Kent and Rafael - much better this way ;)

 drivers/char/tpm/tpm_i2c_infineon.c |   30 +-----------------------------
 1 files changed, 1 insertions(+), 29 deletions(-)
drivers/char/tpm/tpm_i2c_infineon.c +1 -29
index 1794a09..65761b6 100644
@@ -655,35 +655,7 @@ static const struct i2c_device_id tpm_tis_i2c_table[] = {
 };
 
 MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
-
-#ifdef CONFIG_PM
-/* NOTE:
- * Suspend does currently not work Nvidias Tegra2 Platform
- * but works fine on Beagleboard (arm omap).
- *
- * This function will block System Suspend if TPM is not initialized,
- * however the TPM is usually initialized by BIOS/u-boot or by sending
- * a TPM_Startup command.
- */
-static int tpm_tis_i2c_suspend(struct device *dev)
-{
-	return tpm_pm_suspend(dev, dev->power.power_state);
-}
-
-static int tpm_tis_i2c_resume(struct device *dev)
-{
-	return tpm_pm_resume(dev);
-}
-
-static const struct dev_pm_ops tpm_tis_i2c_ops = {
-	.suspend = tpm_tis_i2c_suspend,
-	.resume = tpm_tis_i2c_resume,
-};
-#else
-#define tpm_tis_i2c_suspend NULL
-#define tpm_tis_i2c_resume NULL
-#define tpm_tis_i2c_ops NULL
-#endif
+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend, tpm_pm_resume);
 
 static int __devinit tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
Signature · 1 line
1.7.6.msysgit.0
#7 lore raw

Re: [PATCH] char/tpm: Use struct dev_pm_ops for power management.

Quoted 7 lines · Peter Huewe

Make the tpm_i2c_infineon driver define its PM callbacks trough a struct dev_pm_ops by using SIMPLE_DEV_PM_OPS instead of coding it explicitly.

This simplifies the code and allows the driver to use tpm_pm_suspend() and tpm_pm_resume() as its PM callbacks directly, without defining its own PM callback routines.

Thanks Peter. One more request - can you roll this fix into the driver
patch itself and just add a note in the change log? Sorry I didn't mention this before.

Thanks,
Kent

Quoted 51 lines

---
Thanks Kent and Rafael - much better this way ;)

 drivers/char/tpm/tpm_i2c_infineon.c |   30 +-----------------------------
 1 files changed, 1 insertions(+), 29 deletions(-)
drivers/char/tpm/tpm_i2c_infineon.c +1 -29
index 1794a09..65761b6 100644
@@ -655,35 +655,7 @@ static const struct i2c_device_id tpm_tis_i2c_table[] = {
 };
 
 MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
-
-#ifdef CONFIG_PM
-/* NOTE:
- * Suspend does currently not work Nvidias Tegra2 Platform
- * but works fine on Beagleboard (arm omap).
- *
- * This function will block System Suspend if TPM is not initialized,
- * however the TPM is usually initialized by BIOS/u-boot or by sending
- * a TPM_Startup command.
- */
-static int tpm_tis_i2c_suspend(struct device *dev)
-{
-	return tpm_pm_suspend(dev, dev->power.power_state);
-}
-
-static int tpm_tis_i2c_resume(struct device *dev)
-{
-	return tpm_pm_resume(dev);
-}
-
-static const struct dev_pm_ops tpm_tis_i2c_ops = {
-	.suspend = tpm_tis_i2c_suspend,
-	.resume = tpm_tis_i2c_resume,
-};
-#else
-#define tpm_tis_i2c_suspend NULL
-#define tpm_tis_i2c_resume NULL
-#define tpm_tis_i2c_ops NULL
-#endif
+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend, tpm_pm_resume);
 
 static int __devinit tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
Signature · 1 line
1.7.6.msysgit.0
#8 lore raw

RE: [PATCH] char/tpm: Use struct dev_pm_ops for power management.

Hi Kent,

Quoted 3 lines

Thanks Peter. One more request - can you roll this fix into the driver patch itself and just add a note in the change log? Sorry I didn't mention this before.

Yes I'll do that.
And while at it I'll also replace our i2c_transfer_nolock with the new (in 3.6rc-1) __i2c_transfer function and integrate Bryans Patch directly into the driver.

Peter

#9 lore raw

PATCH 1/1 char/tpm: Add new driver for Infineon I2C TIS TPM

In reply to Kent Yoder

This patch adds a driver to support Infineon's SLB 9635 TT 1.2 Soft I2C TPMs which follow the TGC TIS 1.2 TPM specification[1] and Infineon's I2C Protocol Stack Specification 0.20.
The I2C Protocol Stack Specification is a simple adaption of the LPC TIS Protocol to the I2C Bus.
The I2C TPMs can be used when LPC Bus is not available (i.e. non x86 architectures like ARM).

The driver is based on the tpm_tis.c driver by Leendert van Dorn and Kyleen Hall and has quite similar functionality.

Tested on Nvidia ARM Tegra2 Development Platform and Beagleboard (ARM OMAP) Tested with the Trousers[2] TSS API Testsuite v 0.3 [3] Compile-tested on x86 (32/64-bit)

Updates since version 2.1.4:
- included "Lock the I2C adapter for a sequence of requests", by Bryan Freed
- use __i2c_transfer instead of own implementation of unlocked i2c_transfer
- use struct dev_pm_ops for power management via SIMPLE_DEV_PM_OPS

Updates since version 2.1.3:
- use proper probing mechanism
* either add the tpm using I2C_BOARD_INFO to your board file or probe it
* during runtime e.g on BeagleBoard using :
* "echo tpm_i2c_infineon 0x20 > /sys/bus/i2c/devices/i2c-2/new_device"
- fix possible endless loop if hardware misbehaves
- improved return codes
- consistent spelling i2c/tpm -> I2C/TPM
- remove hardcoded sleep values and msleep usage
- removed debug statements
- added check for I2C functionality
- renaming to tpm_i2c_infineon

Updates since version 2.1.2:
- added sysfs entries for duration and timeouts
- updated to new tpm_do_selftest

Updates since version 2.1.0:
- improved error handling
- implemented workarounds needed by the tpm
- fixed typos

References:
[1]
http://www.trustedcomputinggroup.org/resources/pc_client_work_group_pc_client_
specific_tpm_interface_specification_tis_version_12/
[2] http://trousers.sourceforge.net/
[3]
http://sourceforge.net/projects/trousers/files/TSS%20API%20test%20suite/0.3/

---
Full patch, against 3.6-rc1
Sorry for the inconvenience.
I did remove the signed-off-by, acked-by and reviewed-by tags as this is a new version.

 drivers/char/tpm/Kconfig            |  11 +
 drivers/char/tpm/Makefile           |   1 +
 drivers/char/tpm/tpm_i2c_infineon.c | 695 ++++++++++++++++++++++++++++++++++++
 3 files changed, 707 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_i2c_infineon.c
drivers/char/tpm/Kconfig +11 -0
index a048199..c4aac48 100644
@@ -33,6 +33,17 @@ config TCG_TIS
 	  from within Linux.  To compile this driver as a module, choose
 	  M here; the module will be called tpm_tis.
 
+config TCG_TIS_I2C_INFINEON
+	tristate "TPM Interface Specification 1.2 Interface (I2C - Infineon)"
+	depends on I2C
+	---help---
+	  If you have a TPM security chip that is compliant with the
+	  TCG TIS 1.2 TPM specification and Infineon's I2C Protocol Stack
+	  Specification 0.20 say Yes and it will be accessible from within
+	  Linux.
+	  To compile this driver as a module, choose M here; the module
+	  will be called tpm_tis_i2c_infineon.
+
 config TCG_NSC
 	tristate "National Semiconductor TPM Interface"
 	depends on X86
drivers/char/tpm/Makefile +1 -0
index ea3a1e0..a9c3afc 100644
@@ -6,6 +6,7 @@ ifdef CONFIG_ACPI
 	obj-$(CONFIG_TCG_TPM) += tpm_bios.o
 endif
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
+obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
 obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
 obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
 obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
drivers/char/tpm/tpm_i2c_infineon.c new file +695 -0

695 lines not on this page · raw message

Signature · 1 line
1.7.11.msysgit.0