2.6.37.1 s2disk regression (TPM)
Jiri Slaby
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.
regards,
--
js
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Rafael J. Wysocki
> 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.
Yes, this has been confirmed to cause suspend regressions to happen.
Rafael
Jiri Slaby
> On Sunday, February 20, 2011, Jiri Slaby wrote:
>> 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.
>
> Yes, this has been confirmed to cause suspend regressions to happen
OK, the revert works for me too... Are there any fixes?
regards,
--
js
Rafael J. Wysocki
> On 02/20/2011 11:44 AM, Rafael J. Wysocki wrote:
> > On Sunday, February 20, 2011, Jiri Slaby wrote:
> >> 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.
> >
> > 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.
Thanks,
Rafael
Jiri Slaby
> On Sunday, February 20, 2011, Jiri Slaby wrote:
>> On 02/20/2011 11:44 AM, Rafael J. Wysocki wrote:
>>> On Sunday, February 20, 2011, Jiri Slaby wrote:
>>>> 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.
>>>
>>> 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...
thanks,
--
js
Rafael J. Wysocki
> On Sunday, February 20, 2011, Jiri Slaby wrote:
> > On 02/20/2011 11:44 AM, Rafael J. Wysocki wrote:
> > > On Sunday, February 20, 2011, Jiri Slaby wrote:
> > >> 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.
> > >
> > > 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.
BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
completely broken:
@@ -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?]
Anyway, it looks like a good revert candidate to me.
Greg KH
> On 02/20/2011 11:51 AM, Rafael J. Wysocki wrote:
> > On Sunday, February 20, 2011, Jiri Slaby wrote:
> >> On 02/20/2011 11:44 AM, Rafael J. Wysocki wrote:
> >>> On Sunday, February 20, 2011, Jiri Slaby wrote:
> >>>> 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.
> >>>
> >>> 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.
Ok, I'll do that for the next .37-stable release.
> I guess the same problem is in 2.6.36, so reverting it in 2.6.36 should
> be done too...
.36-stable is now end-of-life :(
thanks,
greg k-h
Rajiv Andrade
> On Sunday, February 20, 2011, Rafael J. Wysocki wrote:
>> 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.
> BTW, the first hunk from that commit in drivers/char/tpm/tpm.c seems to be
> completely broken:
>
> @@ -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?
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. Although it should also be
fixed, I don't think this is the cause, since in case the timeout retrieval from the TPM fail,
the device driver falls back to default values, which has been working before this commit.
> [And why not to simply use 4*sizeof(u32) FWIW?]
I can't see why, I'll update it.
The failure for this specific board then sounds to be due the TPM returning inconsistent timeout values.
Norbert, can 'cat /sys/devices/pnp0/00\:0*/timeouts' and send the output?
Rajiv
Jiri Slaby
committed that) and not even Juri.
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?
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -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;
regards,
--
js
Linus Torvalds
> Linus, please stop garbling my name. I'm not Sladby (2nd time you
> committed that) and not even Juri.
I think the Juri was just my fingers being off. The Sladby was real,
though. I don't know why I think your name has a 'd' in it. Sorry.
I tend to try to copy-paste complicated names to avoid typo's, but
yours is short enough that I think I can write it. I obviously can't.
Linus
Rajiv Andrade
Exactly, I read it too fast and inverted the logic while writing the response, missing De Morgan's.
> 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
> 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?
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -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;
>
This will work for small timeout values discrepancies, but not if, for example,
the TPM is returning the values in msecs instead of usecs, which should affect the
result when issuing commands that last longer. That's why I'd like to see
the failing timeout values first.
Rajiv
Jiri Slaby
> On 02/21/2011 01:34 PM, Jiri Slaby wrote:
>> 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
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.
regards,
--
js
Stefan Berger
> On 02/21/2011 06:12 PM, Rajiv Andrade wrote:
>> On 02/21/2011 01:34 PM, Jiri Slaby wrote:
>>> 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
>
> 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?
Thanks and regards,
Stefan
Jiri Slaby
> On 02/21/2011 03:39 PM, Jiri Slaby wrote:
>> On 02/21/2011 06:12 PM, Rajiv Andrade wrote:
>>> On 02/21/2011 01:34 PM, Jiri Slaby wrote:
>>>> 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
>>
>> 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?
> There was one person stating that this patch fixed are
> problem on his machine.
Yes, I can see that. But we have to live with our mistakes.
> Can you find the 'caps' entry in /sys
> (/sys/devices/pnp0/00:06) and let us know its content?
TPM is at 00:0c:
Manufacturer: 0x49465800
TCG version: 1.2
Firmware version: 1.0
regards,
--
js
Rajiv Andrade
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.
Rajiv
Jiri Slaby
As I wrote, you may base it on dmi data.
> 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?
regards,
--
js
Rafael J. Wysocki
In which case this report will have to be taken into account too:
http://marc.info/?l=linux-acpi&m=129796038509311&w=4
Thanks,
Rafael
Stefan Berger
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.
Using the manufacturer, firmware version etc. to distinguish would
probably open a can of worms...
Stefan
Norbert Preining
sorry for late reply, was night over here in Japan.
On Mo, 21 Feb 2011, Rajiv Andrade wrote:
> 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)
More I cannot offer ... let me know what else I can provide.
Best wishes
Norbert
------------------------------------------------------------------------
Norbert Preining preining@{jaist.ac.jp, logic.at, debian.org}
JAIST, Japan TeX Live & Debian Developer
DSA: 0x09C5B094 fp: 14DF 2E6C 0307 BE6D AD76 A9C0 D2BF 4AA3 09C5 B094
------------------------------------------------------------------------
The major difference between a thing that might go wrong
and a thing that cannot possibly go wrong is that when a
thing that cannot possibly go wrong goes wrong it usually
turns out to be impossible to get at or repair.
--- One of the laws of computers and programming revealed.
--- Douglas Adams, The Hitchhikers Guide to the Galaxy
Jiri Slaby
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.
regards,
--
js
Jiri Slaby
Ok, run some older kernel where this is not reverted (.38-rc5, 2.6.37.1)
or revert the revert :).
regards,
--
js
Norbert Preining
> Ok, run some older kernel where this is not reverted (.38-rc5, 2.6.37.1)
> or revert the revert :).
1000000 4000 152000
Best wishes
Norbert
------------------------------------------------------------------------
Norbert Preining preining@{jaist.ac.jp, logic.at, debian.org}
JAIST, Japan TeX Live & Debian Developer
DSA: 0x09C5B094 fp: 14DF 2E6C 0307 BE6D AD76 A9C0 D2BF 4AA3 09C5 B094
------------------------------------------------------------------------
HARBLEDOWN (vb.)
To manoeuvre a double mattress down a winding staircase.
--- Douglas Adams, The Meaning of Liff
Stefan Berger
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.
Stefan
Jiri Slaby
> 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.
I didn't get any. Could you add a pointer to them?
BTW. there is another guy who hit this. He has:
/sys/devices/pnp0/00:02/timeouts:1000000 2000 150000
like I do.
regards,
--
js