Re: [PATCH v5] ACPI: button: Report wakeup key only for power button wakeups

[Baorui . Liu]

Hi Rafael,

Thank you for pointing me at the Sashiko review. I went through the four
findings and agree with all of them. I have prepared a v6 on current
linux-pm/linux-next that addresses them as follows.

1) High: sleep-in-atomic in acpi_suspend_enter()
I agree. acpi_button_power_wakeup_pending() reached
acpi_get_first_physical_node(), which takes a mutex, while
acpi_suspend_enter() runs with IRQs disabled.

In v6, that early path no longer calls into the button driver. It only
sets plain flags in sleep.c: the existing pwr_btn_event_pending for
acpi_pm_finish()'s pm_wakeup_event(), plus pwr_btn_wakeup_pending for
KEY_WAKEUP. The button driver consumes the latter from resume via
acpi_sleep_pwr_btn_wakeup_pending().

2) Medium: IS_REACHABLE stub drops KEY_WAKEUP when CONFIG_ACPI_BUTTON=m
I agree this was a real functional hole. The v5 IS_REACHABLE() stub fixed
the LKP link error, but with a modular button driver the built-in call
site became a no-op after clearing the fixed power-button status bit.

Keeping the flag in built-in sleep.c and exporting a consumer for
button.ko fixes both the link issue and the modular KEY_WAKEUP loss. The
IS_REACHABLE stub is removed. The sleep.c flag uses WRITE_ONCE/READ_ONCE.

3) Medium: missing compiler barrier between the two stores in suspend
I agree. v6 uses WRITE_ONCE()/barrier()/READ_ONCE() around
wakeup_pending and suspended in acpi_button_suspend(). The same store
ordering is used when clearing them in resume: suspended is cleared
before wakeup_pending, with a barrier() between the two WRITE_ONCE()s.

4) Medium: race between clearing suspended and a late ACPI notify
I agree. v6 calls acpi_os_wait_events_complete() first so queued notifies
still observe suspended == true. It then samples the pending state and
reports KEY_WAKEUP before clearing suspended, so a notify that arrives
after sampling cannot set wakeup_pending only to have it dropped with
neither KEY_WAKEUP nor KEY_POWER. After KEY_WAKEUP, suspended is cleared
before wakeup_pending (with barrier()) so a new SCI takes the normal
KEY_POWER path.

I will send v6 shortly unless you prefer a different approach on any of
these points.

Thanks,
Baorui