2.6.37.1 s2disk regression (TPM)

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 8 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

Quoted 2 lines

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Yes, this has been confirmed to cause suspend regressions to happen

Quoted 8 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

Quoted 2 lines

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Yes, this has been confirmed to cause suspend regressions to happen

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

Quoted 4 lines

Quoted 2 lines

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

Quoted 4 lines

Quoted 2 lines

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Quoted 14 lines · Rafael J. Wysocki

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Ok then, stable team, please revert that from 2.6.37 stable.

I guess the same problem is in 2.6.36, so reverting it in 2.6.36 should be done too...

Quoted 14 lines · Rafael J. Wysocki

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Ok then, stable team, please revert that from 2.6.37 stable.

I guess the same problem is in 2.6.36, so reverting it in 2.6.36 should be done too...

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Quoted 13 lines · Jiri Slaby

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Jiri Slaby

Hi,

I'm unable to hibernate 2.6.37.1 unless I rmmod tpm_tis:

[10974.074587] Suspending console(s) (use no_console_suspend to debug)
[10974.103073] tpm_tis 00:0c: Operation Timed out
[10974.103089] legacy_suspend(): pnp_bus_suspend+0x0/0xa0 returns -62
[10974.103095] PM: Device 00:0c failed to freeze: error -62

2.6.37 worked fine. Going to revert 9b29050f8f7 (tpm_tis: Use timeouts returned from TPM) for testing.

Quoted 3 lines

Quoted 1 line

Yes, this has been confirmed to cause suspend regressions to happen

OK, the revert works for me too... Are there any fixes?

No, and the author and maintainer have not been responding. If that contiunes, I'll simply ask Linus to revert it.

Quoted 1 line · Rafael J. Wysocki

No, and the author and maintainer have not been responding. If that contiunes,I'll simply ask Linus to revert it.

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
         if (rc)
                 return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
                 return;
+
         duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
         chip->vendor.duration[TPM_SHORT] =
             usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Namely, either the old code always returned as a result of the conditional being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

[And why not to simply use 4*sizeof(u32) FWIW?]

Quoted 1 line · Rafael J. Wysocki

No, and the author and maintainer have not been responding. If that contiunes,I'll simply ask Linus to revert it.

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
         if (rc)
                 return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32))
                 return;
+
         duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
         chip->vendor.duration[TPM_SHORT] =
             usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Namely, either the old code always returned as a result of the conditional being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

[And why not to simply use 4*sizeof(u32) FWIW?]

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

No, and the author and maintainer have not been responding. If that contiunes,I'll simply ask Linus to revert it.

Sorry, but you sent the email this Friday, I didn't catch it in time and I wasn't working during the weekend.

Quoted 19 lines

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
         if (rc)
                 return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 *
 sizeof(u32))
                 return;
+
         duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
         chip->vendor.duration[TPM_SHORT] =

             usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Quoted 5 lines

Namely, either the old code always returned as a result of the conditional
being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

The previous code was checking the wrong field of the TPM returned buffer, probably
due an old commit that incorporated the tpm_cmd strucuture, it should check if the return code
is != 0, which if true, means that the command didn't succeed. The output length check should be
just a sanity check, so indeed the logical operator should be&& instead.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

No, and the author and maintainer have not been responding. If that contiunes,I'll simply ask Linus to revert it.

Sorry, but you sent the email this Friday, I didn't catch it in time and I wasn't working during the weekend.

Quoted 19 lines

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
         if (rc)
                 return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 *
 sizeof(u32))
                 return;
+
         duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
         chip->vendor.duration[TPM_SHORT] =

             usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Quoted 5 lines

Namely, either the old code always returned as a result of the conditional
being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

The previous code was checking the wrong field of the TPM returned buffer, probably
due an old commit that incorporated the tpm_cmd strucuture, it should check if the return code
is != 0, which if true, means that the command didn't succeed. The output length check should be
just a sanity check, so indeed the logical operator should be&& instead.

Linus, please stop garbling my name. I'm not Sladby (2nd time you committed that) and not even Juri.

Linus, please stop garbling my name. I'm not Sladby (2nd time you committed that) and not even Juri.

Quoted 19 lines

Quoted 19 lines

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
          if (rc)
                  return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 *
 sizeof(u32))
                  return;
+
          duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
          chip->vendor.duration[TPM_SHORT] =

              usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Quoted 13 lines

Quoted 5 lines

Namely, either the old code always returned as a result of the conditional
being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

The previous code was checking the wrong field of the TPM returned buffer, probably
due an old commit that incorporated the tpm_cmd strucuture, it should check if the return code
is != 0, which if true, means that the command didn't succeed. The output length check should be
just a sanity check, so indeed the logical operator should be&& instead.

No it should not. You want 'if (wrong_retcode OR wrong_len) die;'. IOW I don't see what exactly is wrong on the 'if'. I think it's correct. (Given the old test was incorrect.)

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

I'm not at that machine now, what are the usual timeouts in usecs? The use of conversed jiffies seem bogus. If the usecs are so low (or HZ so high on some configs) that the conversion returns 1 jiffie, wait_event_interruptible_timeout in wait_for_stat will return 0 when:
* 1 jiffie passes without change in status (proper timeout)
* status changed, but also the timer ticked once meanwhile, i.e. we scheduled a moment before timer tick

But this is only a theory so far. What about this (wrapped, just dropped by mouse), I may try it when I'm back to the machine?

drivers/char/tpm/tpm_tis.c +1 -1

@@ -201,7 +201,7 @@ static int wait_for_stat(struct tpm_chip *chip, u8
 mask, unsigned long timeout,
                                                       ((tpm_tis_status
                                                         (chip)&  mask) ==
                                                        mask), timeout);
-               if (rc>  0)
+               if (rc>  0 || (tpm_tis_status(chip)&  mask) == mask)
                         return 0;
         } else {

                 stop = jiffies + timeout;

Quoted 19 lines

Quoted 19 lines

BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
completely broken:

(quoted diff) +4 -2

@@ -577,9 +577,11 @@ duration:
          if (rc)
                  return;
 
-       if (be32_to_cpu(tpm_cmd.header.out.return_code)
-           != 3 * sizeof(u32))
+       if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
+           be32_to_cpu(tpm_cmd.header.out.length)
+           != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 *
 sizeof(u32))
                  return;
+
          duration_cap =&tpm_cmd.params.getcap_out.cap.duration;
          chip->vendor.duration[TPM_SHORT] =

              usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));

Quoted 13 lines

Quoted 5 lines

Namely, either the old code always returned as a result of the conditional
being removed, or the new code will always return as a result of the (... != 0) check. I wonder if there's supposed to be (... == 0) instead?

The previous code was checking the wrong field of the TPM returned buffer, probably
due an old commit that incorporated the tpm_cmd strucuture, it should check if the return code
is != 0, which if true, means that the command didn't succeed. The output length check should be
just a sanity check, so indeed the logical operator should be&& instead.

No it should not. You want 'if (wrong_retcode OR wrong_len) die;'. IOW I don't see what exactly is wrong on the 'if'. I think it's correct. (Given the old test was incorrect.)

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

I'm not at that machine now, what are the usual timeouts in usecs? The use of conversed jiffies seem bogus. If the usecs are so low (or HZ so high on some configs) that the conversion returns 1 jiffie, wait_event_interruptible_timeout in wait_for_stat will return 0 when:
* 1 jiffie passes without change in status (proper timeout)
* status changed, but also the timer ticked once meanwhile, i.e. we scheduled a moment before timer tick

But this is only a theory so far. What about this (wrapped, just dropped by mouse), I may try it when I'm back to the machine?

drivers/char/tpm/tpm_tis.c +1 -1

@@ -201,7 +201,7 @@ static int wait_for_stat(struct tpm_chip *chip, u8
 mask, unsigned long timeout,
                                                       ((tpm_tis_status
                                                         (chip)&  mask) ==
                                                        mask), timeout);
-               if (rc>  0)
+               if (rc>  0 || (tpm_tis_status(chip)&  mask) == mask)
                         return 0;
         } else {

                 stop = jiffies + timeout;

Quoted 3 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 3 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 4 lines · Rajiv Andrade

Quoted 3 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Quoted 3 lines

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Quoted 4 lines · Rajiv Andrade

Quoted 3 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Quoted 3 lines

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Quoted 6 lines · Jiri Slaby

Quoted 5 lines · Rajiv Andrade

Quoted 4 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Quoted 7 lines

Quoted 3 lines

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

There was one person stating that this patch fixed are problem on his machine.

Can you find the 'caps' entry in /sys (/sys/devices/pnp0/00:06) and let us know its content?

Quoted 6 lines · Jiri Slaby

Quoted 5 lines · Rajiv Andrade

Quoted 4 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Quoted 7 lines

Quoted 3 lines

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

There was one person stating that this patch fixed are problem on his machine.

Can you find the 'caps' entry in /sys (/sys/devices/pnp0/00:06) and let us know its content?

Quoted 12 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 8 lines · Rajiv Andrade

Quoted 4 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 12 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 8 lines · Rajiv Andrade

Quoted 4 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 26 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

Quoted 4 lines

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

Quoted 26 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

Quoted 4 lines

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

Quoted 31 lines · Rajiv Andrade

Quoted 30 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 31 lines · Rajiv Andrade

Quoted 30 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 31 lines · Rajiv Andrade

Quoted 30 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

Quoted 31 lines · Rajiv Andrade

Quoted 30 lines · Jiri Slaby

Quoted 14 lines · Stefan Berger

Quoted 13 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

1000000 2000 150000

Quoted 8 lines

Quoted 2 lines

Actually the first one from HW is 1. This is one is HZ after correction in get_timeout. So perhaps it is in ms, yes.

Following the specs, the timeouts are supposed to be in microseconds and ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that. Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

Quoted 36 lines · Jiri Slaby

Quoted 35 lines · Rajiv Andrade

Quoted 34 lines · Jiri Slaby

Quoted 17 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 5 lines

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction
in get_timeout. So perhaps it is in ms, yes.

Quoted 8 lines

Following the specs, the timeouts are supposed to be in microseconds and
ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices
like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that.
Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 8 lines

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

One way of doing it would be to at least make sure that the timeouts are

short < medium < long

and if that's not true, as in the case of your TPM, set the timeouts to 0 and have Rajiv's work-around kick in OR we assign the same high values to the timeouts explicily that Rajiv's work-around is using right now. Of course there could be another type of bad TPM firmware out there where all values are in ascending order but given in ms and cause time-outs -- but I would wait for someone to point that out since I am not aware of such a device.

Quoted 36 lines · Jiri Slaby

Quoted 35 lines · Rajiv Andrade

Quoted 34 lines · Jiri Slaby

Quoted 17 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 5 lines

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction
in get_timeout. So perhaps it is in ms, yes.

Quoted 8 lines

Following the specs, the timeouts are supposed to be in microseconds and
ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices
like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that.
Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 8 lines

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

One way of doing it would be to at least make sure that the timeouts are

short < medium < long

and if that's not true, as in the case of your TPM, set the timeouts to 0 and have Rajiv's work-around kick in OR we assign the same high values to the timeouts explicily that Rajiv's work-around is using right now. Of course there could be another type of bad TPM firmware out there where all values are in ascending order but given in ms and cause time-outs -- but I would wait for someone to point that out since I am not aware of such a device.

Quoted 49 lines · Stefan Berger

Quoted 36 lines · Jiri Slaby

Quoted 35 lines · Rajiv Andrade

Quoted 34 lines · Jiri Slaby

Quoted 17 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 5 lines

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction
in get_timeout. So perhaps it is in ms, yes.

Quoted 8 lines

Following the specs, the timeouts are supposed to be in microseconds and
ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices
like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that.
Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 8 lines

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

One way of doing it would be to at least make sure that the timeouts are

short< medium< long

Quoted 7 lines

and if that's not true, as in the case of your TPM, set the timeouts to 0 and have Rajiv's work-around kick in OR we assign the same high values to the timeouts explicily that Rajiv's work-around is using right now. Of course there could be another type of bad TPM firmware out there where all values are in ascending order but given in ms and cause time-outs -- but I would wait for someone to point that out since I am not aware of such a device.

Note that it is in ascending order (1 2000 150000). As I wrote the first timeout (1) is replaced by one HZ in get_timeouts.

Quoted 49 lines · Stefan Berger

Quoted 36 lines · Jiri Slaby

Quoted 35 lines · Rajiv Andrade

Quoted 34 lines · Jiri Slaby

Quoted 17 lines · Stefan Berger

Quoted 11 lines · Jiri Slaby

Quoted 10 lines · Rajiv Andrade

Quoted 5 lines · Jiri Slaby

There has to be another problem which caused my regression. And since it
reports "Operation Timed out", the former default timeout values worked
for me, the ones read from TPM do not.

Yes, it's highly due inconsistent timeout values reported by the TPM as
I mentioned, my working timeouts are:
3020000 4510000 181000000

Quoted 5 lines

1000000 2000 150000

Actually the first one from HW is 1. This is one is HZ after correction
in get_timeout. So perhaps it is in ms, yes.

Quoted 8 lines

Following the specs, the timeouts are supposed to be in microseconds and
ascending order for short, medium and long duration. Of course, if the device returns wrong timeouts, the command isn't going to succeed, failing the suspend in this case. Nevertheless, I think we need the patch I put in but at the same time we'll need a work-around for devices
like this.

Yes, the patch is correct per se. But as it breaks bunch of machines it cannot go in now. The rule is no regressions.

After you have the workaround it should go into the next rc1 after that.
Do you plan to add a dmi-based quirk? Or, IOW do you want me to attach dmidecode output? Or are you going to base it solely on TPM manufacturer/version

Quoted 8 lines

Quoted 3 lines

It's more reliable to base the workaround on the values themselves, instead of the TPM's ID, since
we don't know whether other models will behave similarly.

As I wrote, you may base it on dmi data.

Quoted 2 lines

It should be fine then to extend the existing workaround for short timeouts to the medium and long ones.

OK, but how will you guess the values?

One way of doing it would be to at least make sure that the timeouts are

short< medium< long

Quoted 7 lines

and if that's not true, as in the case of your TPM, set the timeouts to 0 and have Rajiv's work-around kick in OR we assign the same high values to the timeouts explicily that Rajiv's work-around is using right now. Of course there could be another type of bad TPM firmware out there where all values are in ascending order but given in ms and cause time-outs -- but I would wait for someone to point that out since I am not aware of such a device.

Note that it is in ascending order (1 2000 150000). As I wrote the first timeout (1) is replaced by one HZ in get_timeouts.

The forthcoming patch will simply also adapt the other 2 values and multiply them by 1000. The reason for the suspend failure is the 2nd timeout with TPM_SaveState command being of medium duration.

There will be a 2nd patch for re-enabling the TPM's interrupts that the BIOS may (this may be BIOS-dependent) have disabled while sending a command (TPM_Startup) to the TPM upon resume and having used polling mode and leaving it with the interrupts disabled.

I'd appreciate it if you tested both of them.

The forthcoming patch will simply also adapt the other 2 values and multiply them by 1000. The reason for the suspend failure is the 2nd timeout with TPM_SaveState command being of medium duration.

There will be a 2nd patch for re-enabling the TPM's interrupts that the BIOS may (this may be BIOS-dependent) have disabled while sending a command (TPM_Startup) to the TPM upon resume and having used polling mode and leaving it with the interrupts disabled.

I'd appreciate it if you tested both of them.

Norbert, can 'cat /sys/devices/pnp0/00\:0*/timeouts' and send the output?

Norbert, can 'cat /sys/devices/pnp0/00\:0*/timeouts' and send the output?

Hi everyone,

sorry for late reply, was night over here in Japan.

Quoted 1 line · Rajiv Andrade

Norbert, can 'cat /sys/devices/pnp0/00\:0*/timeouts' and send the output?

I don't have any of these files:

$ ls /sys/devices/pnp0/*
/sys/devices/pnp0/uevent

/sys/devices/pnp0/00:00:
id options power resources subsystem uevent

/sys/devices/pnp0/00:01:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:02:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:03:
driver id options resources subsystem
firmware_node nvram power rtc uevent

/sys/devices/pnp0/00:04:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:05:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:06:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:07:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:08:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:09:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:0a:
active driver id owned pubek temp_deactivated
cancel enabled misc pcrs resources uevent
caps firmware_node options power subsystem

/sys/devices/pnp0/power:
async runtime_status wakeup_count
autosuspend_delay_ms runtime_suspended_time wakeup_hit_count
control runtime_usage wakeup_last_time_ms

runtime_active_kids   wakeup		      wakeup_max_time_ms
runtime_active_time   wakeup_active	      wakeup_total_time_ms
runtime_enabled       wakeup_active_count

$

running git kernel 6f576d57f1 (were the commit is reverted)

Hi everyone,

sorry for late reply, was night over here in Japan.

Quoted 1 line · Rajiv Andrade

Norbert, can 'cat /sys/devices/pnp0/00\:0*/timeouts' and send the output?

I don't have any of these files:

$ ls /sys/devices/pnp0/*
/sys/devices/pnp0/uevent

/sys/devices/pnp0/00:00:
id options power resources subsystem uevent

/sys/devices/pnp0/00:01:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:02:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:03:
driver id options resources subsystem
firmware_node nvram power rtc uevent

/sys/devices/pnp0/00:04:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:05:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:06:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:07:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:08:
driver firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:09:
firmware_node id options power resources subsystem uevent

/sys/devices/pnp0/00:0a:
active driver id owned pubek temp_deactivated
cancel enabled misc pcrs resources uevent
caps firmware_node options power subsystem

/sys/devices/pnp0/power:
async runtime_status wakeup_count
autosuspend_delay_ms runtime_suspended_time wakeup_hit_count
control runtime_usage wakeup_last_time_ms

runtime_active_kids   wakeup		      wakeup_max_time_ms
runtime_active_time   wakeup_active	      wakeup_total_time_ms
runtime_enabled       wakeup_active_count

$

running git kernel 6f576d57f1 (were the commit is reverted)

Ok, run some older kernel where this is not reverted (.38-rc5, 2.6.37.1) or revert the revert :).

Ok, run some older kernel where this is not reverted (.38-rc5, 2.6.37.1) or revert the revert :).