Bug 1920

Summary: Power Button: FF fails, CM works.
Product: ACPI Reporter: Greg Sarjeant (greg)
Component: Power-OffAssignee: Alexey Starikovskiy (astarikovskiy)
Status: CLOSED PATCH_ALREADY_AVAILABLE    
Severity: normal CC: acpi-bugzilla, jmranger
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.1 Subsystem:
Regression: --- Bisected commit-id:
Attachments: Patch for boot parameter to ignore FF buttons
Output of acpidmp
Outout od dmesg -s40000
/proc/interrupts
Patch to choose CM button over FF if both are found.
Patch to allow all buttons to be used
acpidmp from my toshiba laptop
expanded patch v2.6.10

Description Greg Sarjeant 2004-01-20 16:44:03 UTC
Distribution: Gentoo
Hardware Environment: Gateway 200X
Software Environment: Kernel 2.6.1 with EC._INI patch from bug 1690 (BIOS does
not provide an ECDT)
Problem Description: When ACPI starts, if both a FF and a CM button are present,
the CM button is ignored in favor of the FF. As a result, the button does not
send ACPI events

Steps to reproduce: Include button functionality on a system with both an FF and
a CM button defined (for instance, the power button on the Gateway 200X).

On my machine, dmesg reports:
ACPI: Power Button (FF) [PWRF]
ACPI: Lid Switch [LID0]
ACPI: Sleep Button (CM) [SLPB]

I get no events from the FF Power Button. After commenting out the following
lines in scan.c acpi_bus_scan_fixed():

if (acpi_fadt.pwr_button == 0)
    result = acpi_bus_add(&device, acpi_root,
             NULL, ACPI_BUS_TYPE_POWER_BUTTON);

I get the following from dmesg:
ACPI: Lid Switch [LID0]
ACPI: Sleep Button (CM) [SLPB]
ACPI: Power Button (CM) [PWRB]

Once the CM Power Button is detected, I do get the acpi events from it. 

It would be nice if the CM button could take precedence over the FF button by
default.
Comment 1 Greg Sarjeant 2004-01-21 07:33:04 UTC
OK, My reading comprehension was a bit off last night. I thought that this was 
the behavior defined by the spec, but it's not. This contradicts the spec. So, 
it would be great for me if this were the default behavior, but if it would 
break other systems, then we wouldn't want that. I'll see if I can modify the 
summary to reflect the questionable nature of this change.

If the FF button is supposed to take precedence, though, is it supposed to 
generate ACPI events?

Thanks,
Greg
Comment 2 Greg Sarjeant 2004-01-24 11:47:38 UTC
Created attachment 1944 [details]
Patch for boot parameter to ignore FF buttons

Hi,

I'm not sure whether it would be proper to change this behavior. So, at the
risk of being presumptuous, I've decided to give a patch a try.

I have attached a patch that allows you to pass a boot parameter to tell the
kernel to ignore the FF buttons. The parameter is called ignore_ff_buttons.
Valid values are "PWRF", "SLPF" or "BOTH". They basically do what you would
expect.

For example, in grub.conf, you would add one of the following to the kernel
line:

ignore_ff_buttons=PWRF	 (ignore only the FF power button)
ignore_ff_buttons=SLPF	 (ignore only the FF sleep button)
ignore_ff_buttons=BOTH	 (ignore both the FF power and sleep buttons)

I've never done this before, so if anyone decides to use the patch, please do
so with care. I have tried it on my system with all three settings, and
unspecified, and everything seems to work. I don't have an FF sleep button,
though, so I can't verify that it really does ignore that successfully.

Also, if anyone would like to tell me if something about this is horribly
wrong, I'd appreciate that, too.

Thanks,
Greg
Comment 3 Len Brown 2004-02-04 23:14:10 UTC
Does the box have a single physical power button and a single physical sleep button? 
It seems odd that the system defines both FF and CM access to the same power button. 
 
Please attach the output from acpidmp, available in /usr/sbin/, or in pmtools: 
http://ftp.kernel.org/pub/linux/kernel/people/lenb/acpi/utils/ 
plus the output of dmesg -s40000 and /proc/interrupts for the unpatched kernel. 
 
re: patch in comment #2 -- boot flags are typically string-compared at boot-time, setting an 
integer flag for run-time comparison -- rather than copying the actual string for run-time 
comparison.  see examples throughout the kernel. 
 
thanks, 
-Len 
 
Comment 4 Greg Sarjeant 2004-02-05 06:56:18 UTC
Thanks, Len.

The machine has a single physical power button and uses a function key for the
sleep button (Fn+F2 as I recall, but I don't have it with me here at work, so
I'm not 100% sure of the exact key).

I'll attach the output that you requested tonight when I get home. I do have a
couple of questions about that, since I am using three patches on this kernel:

1. initrd DSDT override patch
2. Embedded Control patch from bug 1690
3. power button patch above

Should I use my overridden DSDT for this purpose, or would you rather the
original DSDT? My DSDT had an error in the Embedded Control's ECR region method;
it is addressed at bug 1744. Also, should I use a completely clean kernel for
the output, or should I just remove the power button patch? I assume the latter,
since we're just looking into the power button here.

Thanks for the guidance on the patch. I'll modify it appropriately.

Thanks,
Greg
Comment 5 Greg Sarjeant 2004-02-05 18:59:56 UTC
Created attachment 2025 [details]
Output of acpidmp
Comment 6 Greg Sarjeant 2004-02-05 19:07:59 UTC
Created attachment 2026 [details]
Outout od dmesg -s40000
Comment 7 Greg Sarjeant 2004-02-05 19:13:03 UTC
Created attachment 2027 [details]
/proc/interrupts

For acpidmp, dmesg and /proc/interrupts, I used an unpatched vanilla 2.6.2
kernel. The errors in dmesg go away if I use my fake DSDT and the Embedded
Control patch from bug 1690.

Thanks,
 Greg
Comment 8 Len Brown 2004-05-12 18:46:11 UTC
upcoming GPE re-write should address this
Comment 9 Robert Moore 2004-05-28 14:38:54 UTC
GPE update completed in 20040514
Comment 10 Len Brown 2004-07-11 23:35:28 UTC
While the FADT.PWR_BUTTON definition in the ACPI spec
seems to say that 1 means CM, 0 means FF -- the definitions
of PM1 Status SLPBTN/PWRBTN_EN/STS say that if the CM device
is defined, the OS should ignore the FF bits; no matter
what the setting of FADT.SLP/PWR_BUTTON.

So perhaps we can fix this w/o any manual workarounds...
Comment 11 Shaohua 2004-11-23 00:30:14 UTC
Does recent kernel work for you? We now always enable button GPE.
Comment 12 Alexey Starikovskiy 2004-11-23 03:26:06 UTC
Created attachment 4123 [details]
Patch to choose CM button over FF if both are found.

Could you please try this patch?
Comment 13 Shaohua 2004-11-29 17:54:49 UTC
This patch makes my button in Toshiba laptop failed. I can't receive any 
button interrupts more.
Here is the definition of the power button in my laptop:
Device(PBTN)
{
    Name(_HID, EidsID("PNP0C0C"))
}
No other methods here, and no 'notify(PBTN, 0x80)' in the dmesg. That means 
PBTN can't act as a power button. 
From the dmesg, I got:
>ACPI:Power Button (FF) [PWRF]
>ACPI:Power Button (CM) [PBTN]
But the /proc/acpi/ is correct, only has PBTN. But anyway, the dmesg will 
mislead users.
Is this the BIOS error or did you test the patch in other systems?
If not, I suggest use a module paramter to override FF button with CM button 
and doesn't break other systems.
Comment 14 Alexey Starikovskiy 2004-12-15 02:26:51 UTC
Created attachment 4268 [details]
Patch to allow all buttons to be used

David, could you try this patch?
Comment 15 Len Brown 2004-12-17 23:17:44 UTC
Toshiba seems to be the strange case here. 
On my satellite pro: 
 
FADT.Flags.POWER_BUTTON = 0 -- suggesting FF, 
but the existence of a PNP0C0C device in the DSDT sugges CM: 
 
        Device (PWRB) 
        { 
            Name (_HID, EisaId ("PNP0C0C")) 
            Name (_PRW, Package (0x02) 
            { 
                0x08, 
                0x04 
            }) 
            Name (_STA, 0x0B) 
        } 
 
The GPE handler for 0x08 is in the SSDT: 
    Scope (\_GPE) 
    { 
        Method (_L08, 0, Serialized) 
        { 
         <many other devices sharing this GPE> 
        If (\_SB.MEM.GP91) 
        { 
            Store (0x00, \_SB.MEM.GP91) 
            If (LEqual (Arg0, 0x04)) 
            { 
                Notify (\_SB.PWRB, 0x02) 
            } 
        } 
 
So it appears that the CM capability is only used for the system wake notification (0x2), 
it is not used for the system power-down button-pressed notification (0x80). 
This is why trying to use this CM button for poweroff fails. 
 
Looks like Shaohua's Toshiba has an even more sparse definition of PNP0C0C, 
and without any GPE, it is a mystery to me what that device is supposed to do. 
Shaohua, can you attach the acpidmp for your toshiba? 
Comment 16 Len Brown 2004-12-17 23:30:18 UTC
For the record, Greg's system has 
FADT.Flags:            0x00000225 
PWR_BUTTON is bit 4, so it is 0 here, suggesting FF. 
But PNP0C0C is defined in the DSDT like so: 
 
        Device (PWRB) 
        { 
            Name (_HID, EisaId ("PNP0C0C")) 
            Name (_PRW, Package (0x02) 
            { 
                0x1D, 
                0x04 
            }) 
        } 
 
There is a GPE notify handler for wakup on 1D: 
    Scope (\_GPE) 
    { 
... 
        Method (_L1D, 0, NotSerialized) 
        { 
            Store (\_SB.PHS1 (0x8E, 0x00), Local0) 
            If (LEqual (Local0, 0x01)) 
            { 
                Notify (\_SB.PWRB, 0x02) 
            } 
 
            If (LEqual (Local0, 0x02)) 
            { 
                Notify (\_SB.PWRB, 0x02) 
            } 
        } 
 
And there is also a button-pressed notify method defined in the embedded controller device: 
 
                    Method (_Q50, 0, NotSerialized) 
                    { 
                        Notify (\_SB.PWRB, 0x80) 
                    } 
So it looks like SMBUS device 50 kicks this one off. 
 
Comment 17 Shaohua 2004-12-19 17:37:01 UTC
Created attachment 4285 [details]
acpidmp from my toshiba laptop

This is the acpidmp output from my toshiba laptop.
Comment 18 Len Brown 2005-01-03 22:47:38 UTC
no response from submitter since 2/2004 -- 
we don't have a failing system to test fixes on.

closing as UNREPRODUCIBLE -- re-open if you find
a system that fails.
Comment 19 Jean-Marc Ranger 2005-03-14 15:19:37 UTC
Hi,

Just to let you know that problem is still present, and Greg's original solution
still working (which means power button does nothing with stock kernel, and
triggers shutdown when acpi_bus_scan_fixed() is modified).

Hardware:  Gateway 200x
Bios:  newer version than Greg's, could find exact versions if relevant
Kernel:
- 2.6.11.2
- custom DSDT (home-made, not Greg's)
- patch for synaptics wheel
- (optional) Greg's "three lines commented out" power button patch

Just drop me a line if I can help (more info, try patches, ...)

Jean-Marc Ranger


Comment 20 Alexey Starikovskiy 2005-03-15 01:28:52 UTC
It will help a lot if you try patch http://bugme.osdl.org/attachment.cgi?id=4268
Comment 21 Jean-Marc Ranger 2005-03-15 15:30:51 UTC
Output of dmesg|grep Button on stock 2.6.11 kernel:
   ACPI: Power Button (FF) [PWRF]
   ACPI: Sleep Button (CM) [SLPB]
Power button doing nothing.

Output of dmesg|grep Button on 2.6.11 + patch 4268
   ACPI: Power Button (FF) [PWRF]
   ACPI: Sleep Button (CM) [SLPB]
   ACPI: Power Button (CM) [PWRB]
Power button triggers shutdown.

Looks perfect to me.  Do you wish any specific log ?

JMR
Comment 22 Jean-Marc Ranger 2005-03-15 15:37:14 UTC
woops...

Add "custom DSDT" to both kernel descriptions in previous post.

JMR
Comment 23 Alexey Starikovskiy 2005-03-16 06:22:47 UTC
Thanks for verification of the patch
Comment 24 Len Brown 2005-03-18 11:06:09 UTC
applying the patch makes the additional CM button appear on my Toshiba Satellite Pro, 
and it doesn't hurt the (already functioning) button. 
 
However, this patch fails the "rmmod button" test: 
 
Badness in remove_proc_entry at fs/proc/generic.c:688 
 [<c0187fdb>] remove_proc_entry+0x11b/0x130 
 [<e081e8a8>] acpi_button_exit+0x0/0xa7 [button] 
 [<e081e93f>] acpi_button_exit+0x97/0xa7 [button] 
 [<c013459e>] sys_delete_module+0x16e/0x180 
 [<c014d753>] do_munmap+0x113/0x170 
 [<c014d7e4>] sys_munmap+0x34/0x50 
 [<c0102d99>] sysenter_past_esp+0x52/0x79 
 
--- 
Looking over the driver, I'm leaning towards deleting the bloated /proc code 
instead of fixing it.  The existence of the buttons is given in dmesg, 
and the LID state is the only real run-time object -- and that is 
already handled by events...  We'll get it back when we add 
the acpi devices to sysfs. 
Comment 25 Len Brown 2005-03-18 12:39:44 UTC
Created attachment 4756 [details]
expanded patch v2.6.10

Took Alexey's patch and then
deleted /proc/acpi/button and all the code supporting for it (40% of the driver
source).
Comment 26 Len Brown 2005-07-27 16:57:55 UTC
shipped in 2.6.13-rc3 - closing
(though apparently we'll need to restore the /proc stuff...:-(