enable_irq_wake() can fail. Previously acpi_s2idle_prepare() silently
ignored it's return code, potentially leaving a system that never wakes
up.
Discovered when trying to go into s2idle under Xen. This leads to a
system that can't be woken, since xen-pirq currently doesn't support
setting wakeup IRQs. Real s2idle support for Xen is another topic, but
now at least the user gets an error and the system doesn't needs an hard
reset.
Signed-off-by: Simon Gaiser <[email protected]>
---
Note that I'm unfamiliar with the code so when reviewing please
carefully check if ignoring the error was indeed unintended.
If there are indeed cases where the error should be ignored I would
submit a patch that at least logs the error, although the error message
would be hard to see with broken wakeup.
drivers/acpi/sleep.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 4ca667251272..c69dd3731126 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -714,7 +714,15 @@ int acpi_s2idle_begin(void)
int acpi_s2idle_prepare(void)
{
if (acpi_sci_irq_valid()) {
- enable_irq_wake(acpi_sci_irq);
+ int error;
+
+ error = enable_irq_wake(acpi_sci_irq);
+ if (error) {
+ pr_err("Failed to enable wakeup from IRQ %d: %d\n",
+ acpi_sci_irq,
+ error);
+ return error;
+ }
acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
}
--
2.39.2
On Mon, Mar 13, 2023 at 1:54 PM Simon Gaiser
<[email protected]> wrote:
>
> enable_irq_wake() can fail. Previously acpi_s2idle_prepare() silently
> ignored it's return code, potentially leaving a system that never wakes
> up.
>
> Discovered when trying to go into s2idle under Xen. This leads to a
> system that can't be woken, since xen-pirq currently doesn't support
> setting wakeup IRQs. Real s2idle support for Xen is another topic, but
> now at least the user gets an error and the system doesn't needs an hard
> reset.
>
> Signed-off-by: Simon Gaiser <[email protected]>
> ---
>
> Note that I'm unfamiliar with the code so when reviewing please
> carefully check if ignoring the error was indeed unintended.
No, it wasn't.
First, in the majority of cases in which ACPI SCI is used, the IRQ
chip in question has IRQCHIP_SKIP_SET_WAKE set, so enable_irq_wake()
cannot fail for it.
Second, even if it could fail, it is preferred to let the system
suspend anyway, as long as there is at least one other wakeup source
in it and that is the case as a rule (for example, wakeup is enabled
for the PS/2 keyboard IRQ for all systems that have it).
> If there are indeed cases where the error should be ignored I would
> submit a patch that at least logs the error, although the error message
> would be hard to see with broken wakeup.
Logging an error would be fine, but failing suspend may not be. Of
course, suspend should be aborted if there are no other usable (and
enabled) wakeup sources, but currently there's no infrastructure for
verifying that.
> drivers/acpi/sleep.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 4ca667251272..c69dd3731126 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -714,7 +714,15 @@ int acpi_s2idle_begin(void)
> int acpi_s2idle_prepare(void)
> {
> if (acpi_sci_irq_valid()) {
> - enable_irq_wake(acpi_sci_irq);
> + int error;
> +
> + error = enable_irq_wake(acpi_sci_irq);
> + if (error) {
> + pr_err("Failed to enable wakeup from IRQ %d: %d\n",
> + acpi_sci_irq,
> + error);
> + return error;
> + }
> acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> }
>
> --
> 2.39.2
>
Rafael J. Wysocki:
> On Mon, Mar 13, 2023 at 1:54 PM Simon Gaiser
> <[email protected]> wrote:
>>
>> enable_irq_wake() can fail. Previously acpi_s2idle_prepare() silently
>> ignored it's return code, potentially leaving a system that never
>> wakes up.
>>
>> Discovered when trying to go into s2idle under Xen. This leads to a
>> system that can't be woken, since xen-pirq currently doesn't support
>> setting wakeup IRQs. Real s2idle support for Xen is another topic,
>> but now at least the user gets an error and the system doesn't needs
>> an hard reset.
>>
>> Signed-off-by: Simon Gaiser <[email protected]>
>> ---
>>
>> Note that I'm unfamiliar with the code so when reviewing please
>> carefully check if ignoring the error was indeed unintended.
>
> No, it wasn't.
>
> First, in the majority of cases in which ACPI SCI is used, the IRQ
> chip in question has IRQCHIP_SKIP_SET_WAKE set, so enable_irq_wake()
> cannot fail for it.
>
> Second, even if it could fail, it is preferred to let the system
> suspend anyway, as long as there is at least one other wakeup source
> in it and that is the case as a rule (for example, wakeup is enabled
> for the PS/2 keyboard IRQ for all systems that have it).
>
>> If there are indeed cases where the error should be ignored I would
>> submit a patch that at least logs the error, although the error
>> message would be hard to see with broken wakeup.
>
> Logging an error would be fine, but failing suspend may not be. Of
> course, suspend should be aborted if there are no other usable (and
> enabled) wakeup sources, but currently there's no infrastructure for
> verifying that.
Ah, ok, that's a bit unfortunate. Anyway sent a v2 that only logs. That
way it should at least make debugging for someone else that runs into
such a case a bit easier.
https://lore.kernel.org/linux-acpi/[email protected]/