Bug 1349

Summary:	processor.c using byte IO even if DSDT prescribes otherwise
Product:	ACPI	Reporter:	Mario Lorenz (ml-kbug)
Component:	Power-Processor	Assignee:	Len Brown (lenb)
Status:	CLOSED CODE_FIX
Severity:	high	CC:	acpi-bugzilla
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	2.4.23-pre7 + ACPI 20031006164230	Subsystem:
Regression:	---	Bisected commit-id:
Attachments:	patch for the error patch against 2.6

Description Mario Lorenz 2003-10-12 05:00:13 UTC

Distribution: Debian
Hardware Environment: Asus M2400N Centrino
Software Environment: Bios 0206; see DSDT in Repository
Problem Description: 

ACPI Processor Performance level setting does not work on this Notebook.

The reason for this is that processor.c uses outb() byte writes to
write to the power control register, even tho the DSDT prescribes otherwise
(see PPCT Ressource template in the DSDT, the register has a length of 0x10,
and the value is a 16bit value too)

Changing the u8 typecasts to u16 and putting in an outw() - et voila: ACPI
performance level setting starts to work.

The propper fix would probably be to do away with outb() completely and use
proper ACPI generic adresses. This would also accomodate the alternate 
PCT/PSS Methods from the DSDT which use the Centrino MSR directly, and no
IO at all.

That in turn does depend uppon support of FFixedHW Address Types. Is this planned ?

I furthermore do wonder if this is somehow related to #1269, after all,
its about the same (0x82) register.

What I also noticed: switching the performance level does not update
/proc/cpuinfo (using speedstep-centrino patches, it does), and therefore
internal clock/timers could possibly be wrong after a performance level
change ?

Steps to reproduce:

load processor.o on a M2400N.
Observe processsor performance switching does not work.
Patch processor.c replacing the outb() with outw() and expanding the
value cast from u8 to u16.
recompile
insert module
observe ACPI processor performance switching does work now.

Comment 1 Shaohua 2003-10-16 19:05:16 UTC

did you mean outb in cpufreq/acpi.c(I use 2.6.0-test5). Yes, it's a bug. would 
like make a patch and attach here?

Comment 2 Mario Lorenz 2003-10-16 23:57:49 UTC

*** Bug 1348 has been marked as a duplicate of this bug. ***

Comment 3 Mario Lorenz 2003-10-17 00:07:56 UTC

No. I am talking about drivers/acpi/processor.c (in 2.4 kernels; I do not
play with 2.6 yet)
in acpi_processor_set_performance()
where "value" is first declared u8, then fetched from the ACPI information,
and finally outb() to the performance control register. And the DSDT
for the M2400N says that this register is 0x10 bits in length (ResourceTemplate
in the PCT)

Comment 4 Shaohua 2003-10-17 01:19:49 UTC

yep, we are talking about the same thing. so would you like make a patch for 
it?

Comment 5 Shaohua 2003-10-20 22:35:58 UTC

hmm, 2.6 test7 has included what you said.

Comment 6 Mario Lorenz 2003-10-21 03:45:05 UTC

*OUCH*

a) I specifically refer to 2.4, I told you I am not running 2.6 (yet)

b) Just to verify your claim I just downloaded 2.6.0-test8 to find
that indeed, instead of outb(), now outw() is used.
*THIS IS WRONG*. As wrong as outb() was. It needs to decide according
to the DSDT wheter to use outb() or outw(). I have seen both DSDTs with
8 as well as 16 bit registers.
Please note that it is not mandated that the performance control register
is in systemIO address space at all, so any out* is wrong.

c) I tried writing a proper patch, using acpi_hw_low_level_write but ran
into module symbol export problems. Now I am not that much a kernel developer
that I know this stuff. It probably would take me a couple hours to figure
out, but I do not currently have that time.

Comment 7 Shaohua 2003-10-21 19:23:23 UTC

sorry to bother you. we have noticed the same problem. I just told venki
(Venkatesh.Pallipadi@intel.com) about it. He will take a look at it. I guess 
general register will be supported.

Comment 8 Shaohua 2003-10-24 00:37:46 UTC

Created attachment 1164 [details]
patch for the error

venki said we don't need the complete GAS details in acpi processor performance
struct, because the complete GAS structure will not be useful in MSR case
anyway. so I just proposed a patch to support byte, word, or dword IO.

Comment 9 Mario Lorenz 2003-10-24 05:36:31 UTC

Yes, while not yet having tested this code, it definitly looks better.
However, please take another look at the DSDT for the M2400N, 0206 (out of
the repository on acpi.sourceforge.net) into the definition of _PCT.
As you can see, it is (albeit here its optional) fully legal, and (obviously, as
this is living proof) it happens in the wild, that the Performance Control
Register does not even live in the SystemIO space (FFixedHW here). In which case
the code will break again. I thus assume General ACPI register access is the way
to go here. (Yes, I know there is no FFixedHW support there yet, anyway) Could
you thus reconsider, or consider evaluating _PCT from the ACPI Tables at
 runtime ? (I honestly do not know how much overhead that incurs) The tradeof
 of that vs. a few bytes in the processor performance struct should be considered.

Comment 10 Shaohua 2003-10-26 19:19:30 UTC

FFixedHW means the register is dependent on CPU. In pentium-M, there are MSRs 
for it. So, we don't think it's necessary to use GAS. The separate patch for 
this feature is on it's way.

Comment 11 Shaohua 2003-10-29 21:03:33 UTC

Created attachment 1273 [details]
patch against 2.6

Comment 12 Len Brown 2004-01-30 20:22:20 UTC

shipped in 2.6.2-rc3 -- closing.