Bug 102761

Summary: Surface 3 Hardware Buttons Not Recognized
Product: Drivers Reporter: Stephen Just (stephenjust)
Component: Input DevicesAssignee: Benjamin Tissoires (benjamin.tissoires)
Status: RESOLVED CODE_FIX    
Severity: normal CC: aaron.lu, andrea, benjamin.tissoires, bugzilla, Robert.Moore, stephenjust, szg00000
Priority: P1    
Hardware: x86-64   
OS: Linux   
Kernel Version: 4.2.0-rc6 Subsystem:
Regression: No Bisected commit-id:
Attachments: acpidump
full dmesg output
output of: cat /proc/acpi/wakeup
output of ls /sys/bus/acpi/drivers/*/
grep . /sys/bus/acpi/devices/*/hid
cat /proc/interrupts
0001-platform-Introduce-button-support-for-the-Surface-3.patch

Description Stephen Just 2015-08-12 23:10:00 UTC
Created attachment 184751 [details]
acpidump

Power button (and other hardware buttons including volume up/down and capacitive home button) on the Microsoft Surface 3 fail to function under 4.2. They do not seem to be detected at all, nor do they seem to raise any events when pressed.

I do not see any relationship between pressing the hardware keys and changes in /proc/interrupts. There is also no file at /sys/firmware/acpi/interrupts.

Running acpid and acpi_listen generates no events when pressing the power/volume/capacitive windows keys on the device.

The patch in #84651 does not change any of this behaviour.

The only item under /proc/acpi/button is lid, which always reports "closed" irrespective of the position of the attached Type Cover.

There are numerous other ACPI issues with this device, but this seems like an easy place to start.
Comment 1 Stephen Just 2015-08-12 23:10:35 UTC
Created attachment 184761 [details]
full dmesg output
Comment 2 Stephen Just 2015-08-12 23:11:09 UTC
Created attachment 184771 [details]
output of: cat /proc/acpi/wakeup
Comment 3 Stephen Just 2015-08-12 23:11:51 UTC
Created attachment 184781 [details]
output of ls /sys/bus/acpi/drivers/*/
Comment 4 Stephen Just 2015-08-12 23:12:41 UTC
Created attachment 184791 [details]
grep . /sys/bus/acpi/devices/*/hid
Comment 5 Robert Moore 2015-08-18 20:49:57 UTC
This is the result of a bug in the DSDT ASL code:

[    0.193920] ACPI Error: Needed [Integer/String/Buffer], found [Device] ffff88013f4a7e60 (20150717/exresop-422)
[    0.193960] ACPI Exception: AE_AML_OPERAND_TYPE, Could not execute arguments for [KEYS] (Region) (20150717/nsinit-355)
Comment 6 Aaron Lu 2015-08-20 03:10:30 UTC
Yu,
I remembered you have a patch for surface pro3 button, is it upstreamed?
Comment 7 Chen Yu 2015-08-20 03:14:24 UTC
Aaron,
It is not upstream yet, current status is V4 patch is waiting for reply at 
https://patchwork.kernel.org/patch/7031911/

It seems that, current thread is related to Surface 3 not Surface Pro 3,
but I think they are similar.
Yu
Comment 8 Aaron Lu 2015-08-20 03:18:21 UTC
(In reply to Chen Yu from comment #7)
> Aaron,
> It is not upstream yet, current status is V4 patch is waiting for reply at 
> https://patchwork.kernel.org/patch/7031911/

Thanks for the update.

> 
> It seems that, current thread is related to Surface 3 not Surface Pro 3,
> but I think they are similar.

Oh I didn't notice that. Can you please take a look at this model, does it need a separate driver or not?

Stephen,
Please attach acpidump:
# acpidump > acpidump.txt
Comment 9 Chen Yu 2015-08-20 03:25:20 UTC
(In reply to Aaron Lu from comment #8)
> (In reply to Chen Yu from comment #7)
> > Aaron,
> > It is not upstream yet, current status is V4 patch is waiting for reply at 
> > https://patchwork.kernel.org/patch/7031911/
> 
> Thanks for the update.
> 
> > 
> > It seems that, current thread is related to Surface 3 not Surface Pro 3,
> > but I think they are similar.
> 
> Oh I didn't notice that. Can you please take a look at this model, does it
> need a separate driver or not?
> 
Yup, I can take a look.
> Stephen,
> Please attach acpidump:
> # acpidump > acpidump.txt
Stephen, please try the latest kernel, to check if the acpi interrupt increase or not. Because there is a regression on APIC that might destroy I2C interrupt, which was fixed latestly at  http://www.gossamer-threads.com/lists/linux/kernel/2227028.
Then we might need to enable EC dynamic debug to check which device Surface 3 is using for button notification.

Yu
Comment 10 Stephen Just 2015-08-20 03:55:16 UTC
> Please attach acpidump:
acpidump is already attached.

I tried Yu's patch prior to filing this bug, but as far as I can tell, the two devices operate completely differently. (For example, this device's DSDT seems to have no Q methods analogous to the ones on the Surface Pro 3 that handled the button events.)

As Robert mentioned earlier, there is a bug in the DSDT code from Microsoft. I have already contacted MS to try to get them to address that one particular issue. I'll follow up with my internal contacts there if a fix doesn't show up in the next firmware update, unless someone else has a better idea.

If it is possible to load a custom DSDT at boot-time, I can try that and see if it helps. I've managed to manually edit the dsdt.dat that I extracted, but I'm not sure if that can be directly loaded. I am unable to recompile the ASL, so I resorted to modifying the binary file directly.

> Stephen, please try the latest kernel,...
I am currently running on a 4.2.0-rc6 kernel, with the latest from acpi, hid, and intel-dri repositories merged in (as of a few days ago). I'm still waiting for my patch for Type Cover support to make it to Linus's tree, but I can definitely move to a normal kernel release if it helps.
Comment 11 Aaron Lu 2015-08-21 03:23:28 UTC
Those ACPI error messages are due to their _STA/_SUB control method needs access some I2C/SPI operation region that we have not installed yet. Once installed(the corresponding driver is needed, like spi-pxa2xx.c/i2c-designware-platdrv.c/etc.), the _STA will be re-evaluated. _SUB will not, but I'm not sure how useful that method is.

I don't think this is a bug of the ASL code, instead, it is a gap how we deal with _DEP in Linux right now. Anyway, this should be another problem if the button event still goes the EC route as the pro version.

Yu, you said this last time:
"Then we might need to enable EC dynamic debug to check which device Surface 3 is using for button notification."
Can you please give Stephen detailed steps to do this? Thanks.
Comment 12 Chen Yu 2015-08-21 03:53:53 UTC
I search through the ACPI dump file, there is no “PNP0C09” device in it, so the ec driver might not be loaded on Surface 3.
I think we need to first confirm if the button events are expressed by SCI(by checking cat /proc/interrupt on acpi), if it is, we might need to compare the delta value before/after press button by

grep .  /sys/firmware/acpi/interrupt/gpe* 

once we find the gpe number, we can find the _L/_Exx method, and do further investigation.

Yu
Comment 13 Stephen Just 2015-08-21 05:19:56 UTC
(In reply to Chen Yu from comment #12)
> I think we need to first confirm if the button events are expressed by
> SCI(by checking cat /proc/interrupt on acpi),
The output of cat /proc/interrupt does not seem to be affected by pressing any of the buttons on the device. The interrupt values that change do so at a steady pace irrespective of button presses.

> if it is, we might need to
> compare the delta value before/after press button by
> 
> grep .  /sys/firmware/acpi/interrupt/gpe* 
/sys/firmware/acpi/interrupt does not exist at all.

I'll attach the output of cat /proc/interrupt in case there is something unusual there.
Comment 14 Stephen Just 2015-08-21 05:23:41 UTC
Created attachment 185401 [details]
cat /proc/interrupts
Comment 15 Aaron Lu 2015-08-21 06:10:21 UTC
(In reply to Stephen Just from comment #13)
> > grep .  /sys/firmware/acpi/interrupt/gpe* 
> /sys/firmware/acpi/interrupt does not exist at all.

Right, this is a hardware reduced platform(unlike the pro version), there is no support of GPE.
Comment 16 Aaron Lu 2015-08-21 06:30:35 UTC
From its ACPI table, it seems to use the 5-button-array device node for these buttons:
        Device (WBUT)
        {
            Method (_HID, 0, NotSerialized)  // _HID: Hardware ID
            {
                Return ("PNP0C40")
            }

            Name (_CID, "PNP0C40" /* Standard Button Controller */)  // _CID: Compatible ID
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0F)
            }

        }

but somehow, it doesn't have the _CRS control method to tell us which GPIO pins these buttons are connected. So the soc-button-array driver failed the probe.

I don't see how we can go further here except we can know how Microsoft is using these buttons.
Comment 17 Stephen Just 2015-08-21 23:00:54 UTC
I don't know if this is helpful at all, but from Windows, I can see that the device "Surface Home Button" (with a non-standard driver) is attached to device MSHW0028, same as the Surface Pro 3, so this is definitely a relevant device.

In _DEP, it lists PCI0.I2C2, which is the bus that contains the audio controller, and I2C7, which is the bus that contains the power-management IC, which makes sense.

I have so far discovered that /sys/class/devices/gpio/gpio405 goes low when volume down is pressed, gpio507 goes low when volume up is pressed. I have not found the other two buttons yet. I'm not sure if that is useful either, because these numbers seem meaningless compared to the dsdt.
Comment 18 Chen Yu 2015-09-27 14:47:30 UTC
Good news is that we've just received one Surface 3, however I failed to install ubuntu 15 on it so I'm downloading lastest archlinux and have a try.
Yu
Comment 19 Stephen Just 2015-09-27 16:59:21 UTC
Just a tip for getting any distro running on the S3:
* The machine thinks that the lid is always closed, so you need to disable lid switch suspend in /etc/systemd/logind.conf - the machine can't resume from suspend yet either
* You still have to use nomodeset kernel parameter
* Kernel 4.3-rc0+ has my patch to get the type cover working, otherwise you need an external keyboard
Hopefully that speeds up your setup time a bit.
Comment 20 Chen Yu 2015-11-02 08:52:42 UTC
I've just installed ubuntu on Surface 3, will start to look at this.
Comment 21 Benjamin Tissoires 2016-05-19 16:46:39 UTC
Patch series proposed on the list: http://www.spinics.net/lists/linux-input/msg44595.html

It seems to be working after a cold boot, not a hot one after switching from Win 10.
Comment 22 Chen Yu 2016-05-20 01:34:06 UTC
(In reply to Benjamin Tissoires from comment #21)
> Patch series proposed on the list:
> http://www.spinics.net/lists/linux-input/msg44595.html
> 
> It seems to be working after a cold boot, not a hot one after switching from
> Win 10.

Great, thanks for your work! 
One small question, for this one:
http://www.spinics.net/lists/linux-input/msg44598.html
I'm not sure if puting it in drivers/input/misc would confuse people with drivers/platform/x86/surfacepro3_button.c they have similar names.
Comment 23 Benjamin Tissoires 2016-05-20 07:50:38 UTC
(In reply to Chen Yu from comment #22)
> One small question, for this one:
> http://www.spinics.net/lists/linux-input/msg44598.html
> I'm not sure if puting it in drivers/input/misc would confuse people with
> drivers/platform/x86/surfacepro3_button.c they have similar names.

Yes, I choose to place the driver in drivers/input/misc because it is clearly an input driver, and it is tied to drivers/input/misc/soc_button_array.c.

But in the end, I think we should simply kill drivers/platform/x86/surfacepro3_button.c.

First, I need to find people motivated enough to check whether the driver works for the Surface Pro 3 and Pro 4. It should be a matter of adding the _HID and the object name in the list of surface3_button.c (and rename it into surface_button.c in the end for consistency).
Comment 24 Stephen Just 2016-08-11 04:56:39 UTC
(In reply to Benjamin Tissoires from comment #23)
> First, I need to find people motivated enough to check whether the driver
> works for the Surface Pro 3 and Pro 4. It should be a matter of adding the
> _HID and the object name in the list of surface3_button.c (and rename it
> into surface_button.c in the end for consistency).

I assume we are still waiting for sufficiently motivated people to test this on other hardware?
Comment 25 Chen Yu 2016-08-11 06:09:29 UTC
If remember correctly, surface pro 3/4 do not use GPIOs for buttons, I'll make a double confirm on surface pro 3.
Comment 26 Chen Yu 2016-08-11 06:11:22 UTC
surface pro 3 uses Embedded Controller for powerbutton interrupts
Comment 27 Chen Yu 2016-08-11 06:11:54 UTC
And I think surface 3 is a hardware reduced platform thus it uses gpio.
Comment 28 Benjamin Tissoires 2016-08-11 08:33:50 UTC
(In reply to Chen Yu from comment #26)
> surface pro 3 uses Embedded Controller for powerbutton interrupts

Yes, I know. The question was more: if we use the GPIOs in Surface Pro 3, does it work, so that we can have only one driver for reduced and non-reduced platforms.
Comment 29 Chen Yu 2016-08-11 09:22:18 UTC
OK, I'll test your patch on sp3.
Comment 30 Chen Yu 2016-08-11 12:37:13 UTC
humm

CC [M]  drivers/input/misc/surface3_button_array.o
drivers/input/misc/surface3_button_array.c:16:42: fatal error: linux/input/soc_button_array.h: No such file or directory
 #include <linux/input/soc_button_array.h>
                                          ^
Comment 31 Chen Yu 2016-08-11 12:40:06 UTC
OK, I just apply one of the series.. Would you please attach the latest  patchset in the thread? thanks.
Comment 32 Benjamin Tissoires 2016-12-05 15:14:59 UTC
Created attachment 246901 [details]
0001-platform-Introduce-button-support-for-the-Surface-3.patch

Sorry for the delay, this project is very low priority for me.

I finally found the time to respin the series and re-think at everything. I managed to find some acpidumps of the Surface Pro 3, and it appears that this driver will not be compatible given that the Pro version doesn't even declare the GPIOs.

Anyway, I re-work the patch and made it self-consistent (by duplicating a little bit of code), and submitted:
https://patchwork.kernel.org/patch/9461087/

I am also attaching the patch here, just in case.
Comment 33 Benjamin Tissoires 2017-06-28 12:49:12 UTC
Looks like I forgot to close this one.
Patch upstream as in: 1a64b719d3ae0e4fb939d9a9e31abb60b4ce4eb1 in kernel v4.10+