Bug 13389

Summary: Warning 'Invalid throttling state, reset' gets displayed when it should not be
Product: ACPI Reporter: Frans Pop (elendil)
Component: BIOSAssignee: Zhang Rui (rui.zhang)
Severity: normal CC: acpi-bugzilla, james, lenb, maciej.rutecki, rjw, rui.zhang, yakui.zhao
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.30-rc6 Tree: Mainline
Regression: Yes
Bug Depends on:    
Bug Blocks: 13070    
Attachments: Dubug patch and debug output
Patch to force throttling reset
Updated patch to change warning to debug level
Patch to force throttling reset
Updated patch to change warning to debug level

Description Frans Pop 2009-05-26 15:24:09 UTC
Created attachment 21559 [details]
Dubug patch and debug output

As suggested by Rafael Wysocki I'm filing a new bug to track the possible regression introduced in 2.6.30-rc6 with commit 4973b22a: 'ACPI processor: reset the throttling state once it's invalid'.

The prior discussion about this can be found starting at comment #23 of http://bugzilla.kernel.org/show_bug.cgi?id=13259, the BR that resulted in the patch to be added. I've added the same people in CC.

Attached a debug patch and the resulting output. This shows a few things.

1) The value of pr->throttling.state after boot is 100% correct for my system and proc is able to show correct state because of that.

2) The "invalid state" returned by acpi_throttling_rdmsr() is 2, which indeed does not match any of the values of tx->control. However, it also does not seem like a completely random value.

3) The reset that is being done when the warning triggers is bogus for my case because in acpi_processor_set_throttling_ptc() it is ignored due to the following test:
	if (state == pr->throttling.state)
 		return 0;

4) After the status _is_ set by really changing the state, the value returned by acpi_throttling_rdmsr() does start to match tx->control.

- Does the ACPI spec actually demand that the value read through acpi_throttling_rdmsr() should be "correct" after system boot?
- Given that in my case all other data is consistent, is it really the right thing to do to test that value against tx->control at that point?

- Even if the value returned from acpi_throttling_rdmsr() is wrong, the inconsistency seems minor enough that the kernel should just ignore it.
- My case is fundamentally different from the original BR: while for the broken case the reset _does_ fix a problem, for my system the reset is bogus (even structurally broken?) because it does not get recognized as a status change and thus is skipped.

If additional debugging info is needed, please feel free to ask.

Comment 1 Frans Pop 2009-05-26 18:52:18 UTC
I just see that my analysis here is not complete.

> 3) The reset that is being done when the warning triggers is bogus for my
> case because in acpi_processor_set_throttling_ptc() it is ignored due to
> the following test:
>    if (state == pr->throttling.state)
>          return 0;

The problem is that the code creates a self-fulfilling prophesy.

In acpi_processor_get_throttling_ptc():
        pr->throttling.state = 0;
                state = acpi_get_throttling_state(pr, value);
                if (state == -1) {
                                "Invalid throttling state, reset"));
                        state = 0;
                        ret = acpi_processor_set_throttling(pr, state);

So acpi_processor_get_throttling_ptc() sets both pr->throttling.state _and_ state to 0 before calling acpi_processor_set_throttling(). No wonder the test there always succeeds and no change is actually made!
Comment 2 Frans Pop 2009-05-31 21:11:14 UTC
Created attachment 21671 [details]
Patch to force throttling reset

In the attachment one possible patch to fix the problem that no reset to T0 actually takes place. I've tested it on my system and with that there remains only one reset message per processor (during boot).
Because the reset now really does happen, the later warnings during
'cat /proc/acpi/processor/*/throttling' are gone.

After this bug is fixed I don't have any real objection to changing the warning to debug level, as suggested by Zhang Rui in http://bugzilla.kernel.org/show_bug.cgi?id=13259#c29.

The attached patch is straightforward, but fairly big.

One much simpler solution could be the following patch:
--- a/drivers/acpi/processor_throttling.c
+++ b/drivers/acpi/processor_throttling.c
@@ -842,6 +842,7 @@ static int acpi_processor_get_throttling_ptc(
                                "Invalid throttling state, reset\n"));
                        state = 0;
+                       pr->throttling.state = 9; /* Ensure reset happens */
                        ret = acpi_processor_set_throttling(pr, state);
                        if (ret)
                                return ret;

Yet another approach could be to always initialize the throttling state to T0 in acpi_processor_get_throttling_info(), but that would still require the change to T0 to be forced somehow.

Comments welcome.
Comment 3 Frans Pop 2009-05-31 21:16:35 UTC
Created attachment 21672 [details]
Updated patch to change warning to debug level

Based on Zhang Rui's earlier proposal, but updated because of my patch to remove the spurious newline, which is needed again now :-P

To be applied after a fix for the reset issue.
Comment 4 Rafael J. Wysocki 2009-06-01 20:40:35 UTC
Handled-By : Frans Pop <elendil@planet.nl>
Patch : http://bugzilla.kernel.org/attachment.cgi?id=21671
Patch : http://bugzilla.kernel.org/attachment.cgi?id=21672
Comment 5 Frans Pop 2009-08-17 12:19:29 UTC
Today I've been able to test that throttling works correctly with these patches when the passive trip point is reached for a thermal zone.

For details, see: http://bugzilla.kernel.org/show_bug.cgi?id=13918#c14.
Comment 6 Frans Pop 2009-08-26 09:26:41 UTC
Created attachment 22857 [details]
Patch to force throttling reset
Comment 7 Frans Pop 2009-08-26 09:27:25 UTC
Created attachment 22858 [details]
Updated patch to change warning to debug level
Comment 8 Len Brown 2009-08-27 15:02:00 UTC
ACPI processor: force throttling state when BIOS returns incorrect value

acpi processor: remove superfluous warning message

shipped in 2.6.31-rc7-git5