* [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
@ 2020-02-05 6:09 Stephen Boyd
2020-02-05 12:27 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2020-02-05 6:09 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Marc Zyngier, Douglas Anderson, Lina Iyer, Maulik Shah
There's some confusion around if an irq that's disabled with
disable_irq() can still wake the system from sleep states such as
"suspend to RAM". Let's clarify this in the kernel documentation for
irq_set_irq_wake() so that it's clear that an irq can be disabled and
still wake the system if it has been marked for wakeup.
Cc: Marc Zyngier <maz@kernel.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Maulik Shah <mkshah@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
kernel/irq/manage.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 818b2802d3e7..fa8db98c8699 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -731,6 +731,11 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
*
* Wakeup mode lets this IRQ wake the system from sleep
* states like "suspend to RAM".
+ *
+ * Note: irq enable/disable state is completely orthogonal
+ * to the enable/disable state of irq wake. An irq can be
+ * disabled with disable_irq() and still wake the system as
+ * long as the irq has wake enabled.
*/
int irq_set_irq_wake(unsigned int irq, unsigned int on)
{
--
Sent by a computer, using git, on the internet
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
2020-02-05 6:09 [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable Stephen Boyd
@ 2020-02-05 12:27 ` Thomas Gleixner
2020-02-05 15:34 ` Lina Iyer
2020-02-05 20:37 ` Stephen Boyd
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Gleixner @ 2020-02-05 12:27 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-kernel, Marc Zyngier, Douglas Anderson, Lina Iyer, Maulik Shah
Stephen Boyd <swboyd@chromium.org> writes:
> There's some confusion around if an irq that's disabled with
> disable_irq() can still wake the system from sleep states such as
> "suspend to RAM". Let's clarify this in the kernel documentation for
> irq_set_irq_wake() so that it's clear that an irq can be disabled and
> still wake the system if it has been marked for wakeup.
>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Maulik Shah <mkshah@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> kernel/irq/manage.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 818b2802d3e7..fa8db98c8699 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -731,6 +731,11 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
> *
> * Wakeup mode lets this IRQ wake the system from sleep
> * states like "suspend to RAM".
> + *
> + * Note: irq enable/disable state is completely orthogonal
> + * to the enable/disable state of irq wake. An irq can be
> + * disabled with disable_irq() and still wake the system as
> + * long as the irq has wake enabled.
It clearly should say that this is really depending on the hardware
implementation of the particual interrupt chip whether disabled + wake
mode is supported.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
2020-02-05 12:27 ` Thomas Gleixner
@ 2020-02-05 15:34 ` Lina Iyer
2020-02-05 15:52 ` Thomas Gleixner
2020-02-05 20:37 ` Stephen Boyd
1 sibling, 1 reply; 6+ messages in thread
From: Lina Iyer @ 2020-02-05 15:34 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Stephen Boyd, linux-kernel, Marc Zyngier, Douglas Anderson, Maulik Shah
On Wed, Feb 05 2020 at 05:27 -0700, Thomas Gleixner wrote:
>Stephen Boyd <swboyd@chromium.org> writes:
>> There's some confusion around if an irq that's disabled with
>> disable_irq() can still wake the system from sleep states such as
>> "suspend to RAM". Let's clarify this in the kernel documentation for
>> irq_set_irq_wake() so that it's clear that an irq can be disabled and
>> still wake the system if it has been marked for wakeup.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Douglas Anderson <dianders@chromium.org>
>> Cc: Lina Iyer <ilina@codeaurora.org>
>> Cc: Maulik Shah <mkshah@codeaurora.org>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> ---
>> kernel/irq/manage.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 818b2802d3e7..fa8db98c8699 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -731,6 +731,11 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
>> *
>> * Wakeup mode lets this IRQ wake the system from sleep
>> * states like "suspend to RAM".
>> + *
>> + * Note: irq enable/disable state is completely orthogonal
>> + * to the enable/disable state of irq wake. An irq can be
>> + * disabled with disable_irq() and still wake the system as
>> + * long as the irq has wake enabled.
>
>It clearly should say that this is really depending on the hardware
>implementation of the particual interrupt chip whether disabled + wake
>mode is supported.
>
Could an irqchip flag be used to warn users that we may not wakeup from
suspend/resume if the interrupt if the hardware does not support wakeup
when disabled ?
--Lina
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
2020-02-05 15:34 ` Lina Iyer
@ 2020-02-05 15:52 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2020-02-05 15:52 UTC (permalink / raw)
To: Lina Iyer
Cc: Stephen Boyd, linux-kernel, Marc Zyngier, Douglas Anderson, Maulik Shah
Lina Iyer <ilina@codeaurora.org> writes:
> On Wed, Feb 05 2020 at 05:27 -0700, Thomas Gleixner wrote:
>>> @@ -731,6 +731,11 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
>>> *
>>> * Wakeup mode lets this IRQ wake the system from sleep
>>> * states like "suspend to RAM".
>>> + *
>>> + * Note: irq enable/disable state is completely orthogonal
>>> + * to the enable/disable state of irq wake. An irq can be
>>> + * disabled with disable_irq() and still wake the system as
>>> + * long as the irq has wake enabled.
>>
>>It clearly should say that this is really depending on the hardware
>>implementation of the particual interrupt chip whether disabled + wake
>>mode is supported.
>>
> Could an irqchip flag be used to warn users that we may not wakeup from
> suspend/resume if the interrupt if the hardware does not support wakeup
> when disabled ?
There are also magic ways of wakeup for irqchips which do not have wake
setup functions and still wake the system up when the interrupt line is
disabled by the kernel on suspend. :)
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
2020-02-05 12:27 ` Thomas Gleixner
2020-02-05 15:34 ` Lina Iyer
@ 2020-02-05 20:37 ` Stephen Boyd
2020-02-06 9:42 ` Thomas Gleixner
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2020-02-05 20:37 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Marc Zyngier, Douglas Anderson, Lina Iyer, Maulik Shah
Quoting Thomas Gleixner (2020-02-05 04:27:06)
> Stephen Boyd <swboyd@chromium.org> writes:
> > There's some confusion around if an irq that's disabled with
> > disable_irq() can still wake the system from sleep states such as
> > "suspend to RAM". Let's clarify this in the kernel documentation for
> > irq_set_irq_wake() so that it's clear that an irq can be disabled and
> > still wake the system if it has been marked for wakeup.
> >
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Lina Iyer <ilina@codeaurora.org>
> > Cc: Maulik Shah <mkshah@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > kernel/irq/manage.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index 818b2802d3e7..fa8db98c8699 100644
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -731,6 +731,11 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
> > *
> > * Wakeup mode lets this IRQ wake the system from sleep
> > * states like "suspend to RAM".
> > + *
> > + * Note: irq enable/disable state is completely orthogonal
> > + * to the enable/disable state of irq wake. An irq can be
> > + * disabled with disable_irq() and still wake the system as
> > + * long as the irq has wake enabled.
>
> It clearly should say that this is really depending on the hardware
> implementation of the particual interrupt chip whether disabled + wake
> mode is supported.
>
Ok. I'm having trouble parsing this. Is there a consistent wording that
can be put here?
The API seems fraught with peril if an implementation of an irqchip is
allowed to ignore wakeup on interrupts that are marked for wakeup while
the irq is disabled. Driver writers won't be able to write drivers that
work across implementations if the irq can't wake the system reliably.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable
2020-02-05 20:37 ` Stephen Boyd
@ 2020-02-06 9:42 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2020-02-06 9:42 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-kernel, Marc Zyngier, Douglas Anderson, Lina Iyer, Maulik Shah
Stephen Boyd <swboyd@chromium.org> writes:
> Quoting Thomas Gleixner (2020-02-05 04:27:06)
>> > * Wakeup mode lets this IRQ wake the system from sleep
>> > * states like "suspend to RAM".
>> > + *
>> > + * Note: irq enable/disable state is completely orthogonal
>> > + * to the enable/disable state of irq wake. An irq can be
>> > + * disabled with disable_irq() and still wake the system as
>> > + * long as the irq has wake enabled.
>>
>> It clearly should say that this is really depending on the hardware
>> implementation of the particual interrupt chip whether disabled + wake
>> mode is supported.
>>
>
> Ok. I'm having trouble parsing this. Is there a consistent wording that
> can be put here?
See below.
> The API seems fraught with peril if an implementation of an irqchip is
> allowed to ignore wakeup on interrupts that are marked for wakeup while
> the irq is disabled. Driver writers won't be able to write drivers that
> work across implementations if the irq can't wake the system reliably.
It's not really well defined but thats a result of the gazillion
variants of irq chips which all have their own quirks. The wakeup
mechansims are also widely different, some of them are built into the
SOC, others require external logic. And a large part of these things is
completely undocumented. Welcome to my wonderful world.
So versus consistent wording. I'm fine with the paragraph you suggested,
but please amend it with something like this:
If this does not hold, then either the underlying irq chip and the
related driver need to be investigated.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-06 9:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 6:09 [PATCH] genirq: Clarify that irq wake state is orthogonal to enable/disable Stephen Boyd
2020-02-05 12:27 ` Thomas Gleixner
2020-02-05 15:34 ` Lina Iyer
2020-02-05 15:52 ` Thomas Gleixner
2020-02-05 20:37 ` Stephen Boyd
2020-02-06 9:42 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).