linux-kernel - Re: [PATCH] ACPI: s2idle: Don't ignore error when enabling wakeup IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dd9d2291-8452-0f9e-d7b1-cef14c6c667f@invisiblethingslab.com>
Date:   Mon, 13 Mar 2023 15:56:10 +0100
From:   Simon Gaiser <simon@...isiblethingslab.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: s2idle: Don't ignore error when enabling wakeup IRQ

Rafael J. Wysocki:
> On Mon, Mar 13, 2023 at 1:54 PM Simon Gaiser
> <simon@...isiblethingslab.com> 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 <simon@...isiblethingslab.com>
>> ---
>>
>> 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/20230313144710.3516-1-simon@invisiblethingslab.com/


Download attachment "OpenPGP_signature" of type "application/pgp-signature" (834 bytes)