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 <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Lina Iyer <[email protected]>
Cc: Maulik Shah <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Changes from v1:
* Added the last sentence from tglx
kernel/irq/manage.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 818b2802d3e7..e1e217d7778c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -731,6 +731,13 @@ 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. If this does not hold,
+ * then either the underlying irq chip and the related driver
+ * need to be investigated.
*/
int irq_set_irq_wake(unsigned int irq, unsigned int on)
{
--
Sent by a computer, using git, on the internet
Hi,
On Thu, Feb 6, 2020 at 11:15 AM Stephen Boyd <[email protected]> wrote:
>
> 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 <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Lina Iyer <[email protected]>
> Cc: Maulik Shah <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
>
> Changes from v1:
> * Added the last sentence from tglx
>
> kernel/irq/manage.c | 7 +++++++
> 1 file changed, 7 insertions(+)
FWIW this mathes my understanding and in the past I've done work to
'drivers/pinctrl/pinctrl-rockchip.c' making it support this exact
thing. Thanks for documenting.
Reviewed-by: Douglas Anderson <[email protected]>
-Doug
On Thu, Feb 06 2020 at 12:15 -0700, Stephen Boyd wrote:
>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.
>
Thomas also mentioned that hardware could work either way and probably
should not be assumed to work one way or the other.
>Cc: Marc Zyngier <[email protected]>
>Cc: Douglas Anderson <[email protected]>
>Cc: Lina Iyer <[email protected]>
>Cc: Maulik Shah <[email protected]>
>Signed-off-by: Stephen Boyd <[email protected]>
>---
>
>Changes from v1:
> * Added the last sentence from tglx
>
> kernel/irq/manage.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>index 818b2802d3e7..e1e217d7778c 100644
>--- a/kernel/irq/manage.c
>+++ b/kernel/irq/manage.c
>@@ -731,6 +731,13 @@ 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. If this does not hold,
>+ * then either the underlying irq chip and the related driver
>+ * need to be investigated.
> */
> int irq_set_irq_wake(unsigned int irq, unsigned int on)
> {
>--
>Sent by a computer, using git, on the internet
>
Hi,
On Thu, Feb 6, 2020 at 11:57 AM Lina Iyer <[email protected]> wrote:
>
> On Thu, Feb 06 2020 at 12:15 -0700, Stephen Boyd wrote:
> >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.
> >
> Thomas also mentioned that hardware could work either way and probably
> should not be assumed to work one way or the other.
Right...
...and then (paraphrasing) Stephen pointed out that policy makes it
really hard for clients of the API to work properly.
...and then (paraphrasing) Thomas said "Good point. As long as you
document that not all drivers _actually_ behave the way you describe,
it's fine to add a comment saying that drivers _should_ behave the way
you describe".
Or, said another way: if a driver doesn't behave the way Stephen
describes then it should be fixed unless there is some reason why
there is no possible way to make it happen.
(Thomas / Stephen: please correct if I got my paraphrasing incorrect).
-Doug
Doug Anderson <[email protected]> writes:
>> Thomas also mentioned that hardware could work either way and probably
>> should not be assumed to work one way or the other.
>
> Right...
>
> ...and then (paraphrasing) Stephen pointed out that policy makes it
> really hard for clients of the API to work properly.
>
> ...and then (paraphrasing) Thomas said "Good point. As long as you
> document that not all drivers _actually_ behave the way you describe,
> it's fine to add a comment saying that drivers _should_ behave the way
> you describe".
>
> Or, said another way: if a driver doesn't behave the way Stephen
> describes then it should be fixed unless there is some reason why
> there is no possible way to make it happen.
Yes, that's right.
Thanks,
tglx
The following commit has been merged into the irq/urgent branch of tip:
Commit-ID: f9f21cea311340f38074ff93a8d89b4a9cae6bcc
Gitweb: https://git.kernel.org/tip/f9f21cea311340f38074ff93a8d89b4a9cae6bcc
Author: Stephen Boyd <[email protected]>
AuthorDate: Thu, 06 Feb 2020 11:15:21 -08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 07 Feb 2020 21:37:08 +01:00
genirq: Clarify that irq wake state is orthogonal to enable/disable
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".
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.
Signed-off-by: Stephen Boyd <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/irq/manage.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 818b280..3089a60 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -731,6 +731,13 @@ 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. If this does not hold,
+ * then the underlying irq chip and the related driver need
+ * to be investigated.
*/
int irq_set_irq_wake(unsigned int irq, unsigned int on)
{