Bug 102761

Created attachment 184761 [details]
full dmesg output

Created attachment 184771 [details]
output of: cat /proc/acpi/wakeup

Created attachment 184781 [details]
output of ls /sys/bus/acpi/drivers/*/

Created attachment 184791 [details]
grep . /sys/bus/acpi/devices/*/hid

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)

Yu,
I remembered you have a patch for surface pro3 button, is it upstreamed?

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

(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

(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

> 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.

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.

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

(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.

Created attachment 185401 [details]
cat /proc/interrupts

(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.

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.

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.

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

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.

I've just installed ubuntu on Surface 3, will start to look at this.

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.

(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.

(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).

(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?

If remember correctly, surface pro 3/4 do not use GPIOs for buttons, I'll make a double confirm on surface pro 3.

surface pro 3 uses Embedded Controller for powerbutton interrupts

And I think surface 3 is a hardware reduced platform thus it uses gpio.

(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.

OK, I'll test your patch on sp3.

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>
                                          ^

OK, I just apply one of the series.. Would you please attach the latest  patchset in the thread? thanks.

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.

Looks like I forgot to close this one.
Patch upstream as in: 1a64b719d3ae0e4fb939d9a9e31abb60b4ce4eb1 in kernel v4.10+