linux-surface / kernel

Linux kernel with modifications for Microsoft Surface devices.
Other
118 stars 33 forks source link

pinctrl: amd: disable and mask interrupts on probe #107

Closed nakato closed 2 years ago

nakato commented 2 years ago

Some systems such as the Microsoft Surface Laptop 4 leave interrupts enabled and configured for use in sleep states on boot, which cause unexpected behaviour such as spurious wakes and failed resumes in s2idle states.

As interrupts should not be enabled until they are claimed and explicitly enabled, disabling any interrupts mistakenly left enabled by firmware should be safe.

I've proposed this upstream at: https://lore.kernel.org/linux-gpio/20211001161714.2053597-1-nakato@nakato.io/T/#t

nakato commented 2 years ago

I marked it as draft to take a conservative approach, as I'm not sure if it will get in upstream, but I'm happy for it to go in here. It's made the ssh interrupts not be problematic anymore.

The main thing this patch fixes, in a Surface Laptop context, is spurious wakeups from the SSH interrupt, and a few other unknown interrupts that were enabled on boot but not enabled by Linux, and lacking any reference in the acpidump. There is however an odd interaction between pinctrl-amd and the ACPI GPE's that means the device wakes up for things like un/plugging power, and closing the lid. (Are laptops supposed to wake up when you close the lid? It goes back to sleep after about 30 seconds which I think is systemd's doing.)

qzed commented 2 years ago

I marked it as draft to take a conservative approach, as I'm not sure if it will get in upstream, but I'm happy for it to go in here. It's made the ssh interrupts not be problematic anymore.

Sounds good to me. I'll merge this. We can always revert and apply the updated version later on once there are some comments on it.

Are laptops supposed to wake up when you close the lid?

Partially, yes, but not completely. As far as I know, there's no way to differentiate if an interrupt is for close or open, so both should in some way wake the device. At some point there's supposed to be a check though that reads back the state of the lid. In the ACPI driver that's done in acpi_lid_update_state(), which is called when the lid gets notified for a status change (e.g. here, which triggers a call to acpi_button_notify() with event == ACPI_BUTTON_NOTIFY_STATUS == 0x80). Note the if (state && signal_wakeup) check in acpi_lid_update_state(), which should ensure that the wakeup event only gets sent when the lid is being opened.

So from what I can tell there are two things that might go wrong:

I'm not an expert on this so though. Also not sure how the interaction between AC state and lid state should work, since that'd need to be done at higher level (like systemd maybe) as far as I can tell.