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. Questions: - 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? Conclusions: - 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. Cheers, FJP
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) { ACPI_WARNING((AE_INFO, "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!
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( ACPI_WARNING((AE_INFO, "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.
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.
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
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.
Created attachment 22857 [details] Patch to force throttling reset
Created attachment 22858 [details] Updated patch to change warning to debug level
2a908002c7b1b666616103e9df2419b38d7c6f1f ACPI processor: force throttling state when BIOS returns incorrect value bdf57de4e6abc389cc3f3bd94ec15cce74cf6f4b acpi processor: remove superfluous warning message shipped in 2.6.31-rc7-git5 closed.