[PATCH] tpm: Autodetect itpm devices

[PATCH] tpm: Autodetect itpm devices

[Matthew Garrett]

Some Lenovos have TPMs that require a quirk to function correctly. This can
be autodetected by checking whether the device has a _HID of INTC0102. This
is an invalid PNPid, and as such is discarded by the pnp layer - however
it's still present in the ACPI code, so we can pull it out that way. This
means that the quirk won't be automatically applied on non-ACPI systems,
but without ACPI we don't have any way to identify the chip anyway so I
don't think that's a great concern.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/char/tpm/tpm_tis.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1030f84..c17a305 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/wait.h>
+#include <linux/acpi.h>
 #include "tpm.h"
 
 #define TPM_HEADER_SIZE 10
@@ -78,6 +79,26 @@ enum tis_defaults {
 static LIST_HEAD(tis_chips);
 static DEFINE_SPINLOCK(tis_lock);
 
+#ifdef CONFIG_ACPI
+static int is_itpm(struct pnp_dev *dev)
+{
+	struct acpi_device *acpi = pnp_acpi_device(dev);
+	struct acpi_hardware_id *id;
+
+	list_for_each_entry(id, &acpi->pnp.ids, list) {
+		if (!strcmp("INTC0102", id->id))
+			return 1;
+	}
+
+	return 0;
+}
+#else
+static int is_itpm(struct pnp_dev *dev)
+{
+	return 0;
+}
+#endif
+
 static int check_locality(struct tpm_chip *chip, int l)
 {
 	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
@@ -472,6 +493,9 @@ 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 (is_itpm(to_pnp_dev(dev)))
+		itpm = 1;
+
 	if (itpm)
 		dev_info(dev, "Intel iTPM workaround enabled\n");
 
-- 
1.7.3.1

Re: [PATCH] tpm: Autodetect itpm devices

[Rajiv Andrade]

On 21/10/10 19:42, Matthew Garrett wrote:
> Some Lenovos have TPMs that require a quirk to function correctly. This can
> be autodetected by checking whether the device has a _HID of INTC0102. This
> is an invalid PNPid, and as such is discarded by the pnp layer - however
> it's still present in the ACPI code, so we can pull it out that way. This
> means that the quirk won't be automatically applied on non-ACPI systems,
> but without ACPI we don't have any way to identify the chip anyway so I
> don't think that's a great concern.
>
> Signed-off-by: Matthew Garrett<mjg@redhat.com>
Thanks Matthew,

I'd be glad to remove the module option after this patch, but the 
non-ACPI case wouldn't
remain covered indeed.

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

[2.6.37-rc1 REGRESSION] suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Andy Isaacson]

On Thu, Oct 21, 2010 at 05:42:40PM -0400, Matthew Garrett wrote:
> Some Lenovos have TPMs that require a quirk to function correctly. This can
> be autodetected by checking whether the device has a _HID of INTC0102. This
> is an invalid PNPid, and as such is discarded by the pnp layer - however
> it's still present in the ACPI code, so we can pull it out that way. This
> means that the quirk won't be automatically applied on non-ACPI systems,
> but without ACPI we don't have any way to identify the chip anyway so I
> don't think that's a great concern.

Matthew,

Thanks for coming up with a clean way to integrate this iTPM support!

Unfortunately, automagically loading tpm_tis results in a suspend
regression on my X200:

(hand-transcribed, sorry for any typos)

[   16.759044] tpm_tis 00:0a: 1.2 TPM (device-id 0x1020, rev-id 6)
[ 7537.636134] PM: Entering mem sleep
[ 7537.636221] Suspending console(s) (use no_console_suspend to debug)
[ 7537.636668] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[ 7537.658111] tpm_tis 00:0a: tpm_transmit: tpm_send: error -5
[ 7537.658119] legacy_suspend(): pnp_bus_suspend+0x0/0x6a returns -5
[ 7537.658122] PM: Device 00:0a failed to suspend: error -5
[ 7537.681940] sd 0:0:0:0: [sda] Stopping disk
[ 7538.564024] PM: Some devices failed to suspend

I suspect that it would have failed previously if I'd forced the module
to load, but since tpm_tis now loads automatically as of 2.6.37-rc1, I
have to "rmmod tpm_tis" for suspend to work.

-andy

Re: [2.6.37-rc1 REGRESSION] suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Matthew Garrett]

On Mon, Nov 01, 2010 at 02:19:48PM -0700, Andy Isaacson wrote:

> I suspect that it would have failed previously if I'd forced the module
> to load, but since tpm_tis now loads automatically as of 2.6.37-rc1, I
> have to "rmmod tpm_tis" for suspend to work.

Hm. That's what this patch was intended to fix. Can you check that the 
tree you're using has it merged? The autoloading comes from the fix to 
the pnp code to use _CID even if _HID is malformed, so now we have a pnp 
device when we previously didn't.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Andy Isaacson]

On Mon, Nov 01, 2010 at 09:32:24PM +0000, Matthew Garrett wrote:
> On Mon, Nov 01, 2010 at 02:19:48PM -0700, Andy Isaacson wrote:
> > I suspect that it would have failed previously if I'd forced the module
> > to load, but since tpm_tis now loads automatically as of 2.6.37-rc1, I
> > have to "rmmod tpm_tis" for suspend to work.
> 
> Hm. That's what this patch was intended to fix. Can you check that the 
> tree you're using has it merged? The autoloading comes from the fix to 
> the pnp code to use _CID even if _HID is malformed, so now we have a pnp 
> device when we previously didn't.

Hmmm, I am testing with straight-up .37-rc1, c8ddb2713c62.  It does not
include your patch.

This time around, still running -rc1, I ran pm-suspend and it suspended
+ resumed fine.  The second time I ran pm-suspend, it switched to text
console and started flashing the moon LED, but hung there with the LCD
backlight still on and the LED moon flashing.

So, definitely something exciting going on... and not a simple
suspend regression, and definitely not caused by this patch, my
apologies.  Subject line updated accordingly.

I'll try applying "Autodetect itpm devices" and report back.

-andy

Re: suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Andy Isaacson]

On Mon, Nov 01, 2010 at 06:31:06PM -0700, Andy Isaacson wrote:
> On Mon, Nov 01, 2010 at 09:32:24PM +0000, Matthew Garrett wrote:
> > On Mon, Nov 01, 2010 at 02:19:48PM -0700, Andy Isaacson wrote:
> > > I suspect that it would have failed previously if I'd forced the module
> > > to load, but since tpm_tis now loads automatically as of 2.6.37-rc1, I
> > > have to "rmmod tpm_tis" for suspend to work.
> > 
> > Hm. That's what this patch was intended to fix. Can you check that the 
> > tree you're using has it merged? The autoloading comes from the fix to 
> > the pnp code to use _CID even if _HID is malformed, so now we have a pnp 
> > device when we previously didn't.
> 
> Hmmm, I am testing with straight-up .37-rc1, c8ddb2713c62.  It does not
> include your patch.
> 
> This time around, still running -rc1, I ran pm-suspend and it suspended
> + resumed fine.  The second time I ran pm-suspend, it switched to text
> console and started flashing the moon LED, but hung there with the LCD
> backlight still on and the LED moon flashing.
> 
> So, definitely something exciting going on... and not a simple
> suspend regression, and definitely not caused by this patch, my
> apologies.  Subject line updated accordingly.
> 
> I'll try applying "Autodetect itpm devices" and report back.

With your patch applied on top of -rc1, suspend + resume seems to work
fine, and I don't have any error messages from tpm in dmesg.

Thanks, problem solved.  (Still no idea what caused that one
hang-with-blinking-moon, but it doesn't appear to be connected.)

-andy

Re: suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Rafael J. Wysocki]

On Tuesday, November 02, 2010, Andy Isaacson wrote:
> On Mon, Nov 01, 2010 at 06:31:06PM -0700, Andy Isaacson wrote:
> > On Mon, Nov 01, 2010 at 09:32:24PM +0000, Matthew Garrett wrote:
> > > On Mon, Nov 01, 2010 at 02:19:48PM -0700, Andy Isaacson wrote:
> > > > I suspect that it would have failed previously if I'd forced the module
> > > > to load, but since tpm_tis now loads automatically as of 2.6.37-rc1, I
> > > > have to "rmmod tpm_tis" for suspend to work.
> > > 
> > > Hm. That's what this patch was intended to fix. Can you check that the 
> > > tree you're using has it merged? The autoloading comes from the fix to 
> > > the pnp code to use _CID even if _HID is malformed, so now we have a pnp 
> > > device when we previously didn't.
> > 
> > Hmmm, I am testing with straight-up .37-rc1, c8ddb2713c62.  It does not
> > include your patch.
> > 
> > This time around, still running -rc1, I ran pm-suspend and it suspended
> > + resumed fine.  The second time I ran pm-suspend, it switched to text
> > console and started flashing the moon LED, but hung there with the LCD
> > backlight still on and the LED moon flashing.
> > 
> > So, definitely something exciting going on... and not a simple
> > suspend regression, and definitely not caused by this patch, my
> > apologies.  Subject line updated accordingly.
> > 
> > I'll try applying "Autodetect itpm devices" and report back.
> 
> With your patch applied on top of -rc1, suspend + resume seems to work
> fine, and I don't have any error messages from tpm in dmesg.
> 
> Thanks, problem solved.  (Still no idea what caused that one
> hang-with-blinking-moon, but it doesn't appear to be connected.)

Please give us a pointer to the patch, there are other users experiencing this issue.

Thanks,
Rafael

Re: suspend fails on x200 (was Re: [PATCH] tpm: Autodetect itpm devices)

[Andy Isaacson]

On Tue, Nov 02, 2010 at 04:47:53AM +0100, Rafael J. Wysocki wrote:
> > > So, definitely something exciting going on... and not a simple
> > > suspend regression, and definitely not caused by this patch, my
> > > apologies.  Subject line updated accordingly.
> > > 
> > > I'll try applying "Autodetect itpm devices" and report back.
> > 
> > With your patch applied on top of -rc1, suspend + resume seems to work
> > fine, and I don't have any error messages from tpm in dmesg.
> > 
> > Thanks, problem solved.  (Still no idea what caused that one
> > hang-with-blinking-moon, but it doesn't appear to be connected.)
> 
> Please give us a pointer to the patch, there are other users
> experiencing this issue.

My iTPM-causes-suspend-to-fail is fixed by the head message in this
thread, <1287697360-31273-1-git-send-email-mjg@redhat.com>,
http://lkml.org/lkml/2010/10/21/456

-andy

Re: [PATCH] tpm: Autodetect itpm devices

[Jiri Kosina]

[ added James Morris and Rafael to CC ]

On Thu, 21 Oct 2010, Matthew Garrett wrote:

> Some Lenovos have TPMs that require a quirk to function correctly. This can
> be autodetected by checking whether the device has a _HID of INTC0102. This
> is an invalid PNPid, and as such is discarded by the pnp layer - however
> it's still present in the ACPI code, so we can pull it out that way. This
> means that the quirk won't be automatically applied on non-ACPI systems,
> but without ACPI we don't have any way to identify the chip anyway so I
> don't think that's a great concern.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

	Tested-by: Jiri Kosina <jkosina@suse.cz>

This definitely is .37 material still, as it fixes suspend regression on 
various machines, as reported here:

	http://lkml.org/lkml/2010/11/1/493
	http://lkml.org/lkml/2010/11/29/219

James, any word on this? It doesn't seem to be present either in Linus' 
tree, nor in linux-next.

Thanks.

> ---
>  drivers/char/tpm/tpm_tis.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 1030f84..c17a305 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -25,6 +25,7 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/wait.h>
> +#include <linux/acpi.h>
>  #include "tpm.h"
>  
>  #define TPM_HEADER_SIZE 10
> @@ -78,6 +79,26 @@ enum tis_defaults {
>  static LIST_HEAD(tis_chips);
>  static DEFINE_SPINLOCK(tis_lock);
>  
> +#ifdef CONFIG_ACPI
> +static int is_itpm(struct pnp_dev *dev)
> +{
> +	struct acpi_device *acpi = pnp_acpi_device(dev);
> +	struct acpi_hardware_id *id;
> +
> +	list_for_each_entry(id, &acpi->pnp.ids, list) {
> +		if (!strcmp("INTC0102", id->id))
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int is_itpm(struct pnp_dev *dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int check_locality(struct tpm_chip *chip, int l)
>  {
>  	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
> @@ -472,6 +493,9 @@ 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 (is_itpm(to_pnp_dev(dev)))
> +		itpm = 1;
> +
>  	if (itpm)
>  		dev_info(dev, "Intel iTPM workaround enabled\n");
>  
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] tpm: Autodetect itpm devices

[James Morris]

On Mon, 29 Nov 2010, Jiri Kosina wrote:

> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> 	Tested-by: Jiri Kosina <jkosina@suse.cz>
> 
> This definitely is .37 material still, as it fixes suspend regression on 
> various machines, as reported here:
> 
> 	http://lkml.org/lkml/2010/11/1/493
> 	http://lkml.org/lkml/2010/11/29/219
> 
> James, any word on this? It doesn't seem to be present either in Linus' 
> tree, nor in linux-next.

I'm pretty sure this is the first I've seen of this, which would explain 
why it's not in my tree (Rajiv: as mainatiner please collect and forward 
TPM patches to me, or at least cc me when acking them).

I can push this to Linus.


> 
> Thanks.
> 
> > ---
> >  drivers/char/tpm/tpm_tis.c |   24 ++++++++++++++++++++++++
> >  1 files changed, 24 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 1030f84..c17a305 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/wait.h>
> > +#include <linux/acpi.h>
> >  #include "tpm.h"
> >  
> >  #define TPM_HEADER_SIZE 10
> > @@ -78,6 +79,26 @@ enum tis_defaults {
> >  static LIST_HEAD(tis_chips);
> >  static DEFINE_SPINLOCK(tis_lock);
> >  
> > +#ifdef CONFIG_ACPI
> > +static int is_itpm(struct pnp_dev *dev)
> > +{
> > +	struct acpi_device *acpi = pnp_acpi_device(dev);
> > +	struct acpi_hardware_id *id;
> > +
> > +	list_for_each_entry(id, &acpi->pnp.ids, list) {
> > +		if (!strcmp("INTC0102", id->id))
> > +			return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +#else
> > +static int is_itpm(struct pnp_dev *dev)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static int check_locality(struct tpm_chip *chip, int l)
> >  {
> >  	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
> > @@ -472,6 +493,9 @@ 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 (is_itpm(to_pnp_dev(dev)))
> > +		itpm = 1;
> > +
> >  	if (itpm)
> >  		dev_info(dev, "Intel iTPM workaround enabled\n");
> >  
> > 
> 
> -- 
> Jiri Kosina
> SUSE Labs, Novell Inc.
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] tpm: Autodetect itpm devices

[Rajiv Andrade]

Em 29-11-2010 20:08, James Morris escreveu:

> On Mon, 29 Nov 2010, Jiri Kosina wrote:
>>> Signed-off-by: Matthew Garrett<mjg@redhat.com>
>> 	Tested-by: Jiri Kosina<jkosina@suse.cz>
>>
>> This definitely is .37 material still, as it fixes suspend regression on
>> various machines, as reported here:
>>
>> 	http://lkml.org/lkml/2010/11/1/493
>> 	http://lkml.org/lkml/2010/11/29/219
>>
>> James, any word on this? It doesn't seem to be present either in Linus'
>> tree, nor in linux-next.
> I'm pretty sure this is the first I've seen of this, which would explain
> why it's not in my tree (Rajiv: as mainatiner please collect and forward
> TPM patches to me, or at least cc me when acking them).
>
I normally CC you when acking, don't know what happened this time, sorry.

Rajiv