Meet some issues when operating one ioapic chip irq which
is set with _NO_SUSPEND flag and calling irq_set_irq_wake().
Written the below patches, thanks your time to review.
[PATCH 1/3] x86, io_apic: Adding the flag IRQCHIP_SKIP_SET_WAKE
[PATCH 2/3] genirq: Do not consider the irqs with disabling and IRQF_NO_SUSPEND
[PATCH 3/3] genirq: Give warning in case calling irq_set_irq_wake with _NO_SUSPEND flag
Currently for an ioapic chip irq, if we call irq_set_irq_wake()
we will get the ENXIO returning error, but some drivers need the
wake-up interrupts pending mechanism.
Here adding the flag IRQCHIP_SKIP_SET_WAKE instead of emtpy callback.
Signed-off-by: liu chuansheng <[email protected]>
---
arch/x86/kernel/apic/io_apic.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 9ed796c..7d0baf7 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2520,6 +2520,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
.irq_eoi = ack_apic_level,
.irq_set_affinity = native_ioapic_set_affinity,
.irq_retrigger = ioapic_retrigger_irq,
+ .flags = IRQCHIP_SKIP_SET_WAKE,
};
static inline void init_IO_APIC_traps(void)
--
1.7.0.4
According to commit 9c6079aa1bf(genirq: Do not consider disabled
wakeup irqs), we should not break the suspend when one irq is pending
but has been disabled before suspending.
But there is another case missed, that one irq with flag IRQF_NO_SUSPEND,
which has been disabled before suspending, and irq pending there,
in this case, we still should not break the suspending, otherwise,
the suspend abort over and over.
Here also checking if the desc->istate & IRQS_SUSPENDED is true.
Signed-off-by: liu chuansheng <[email protected]>
---
kernel/irq/pm.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index cb228bf..1470c1b 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -109,7 +109,8 @@ int check_wakeup_irqs(void)
* can abort suspend.
*/
if (irqd_is_wakeup_set(&desc->irq_data)) {
- if (desc->depth == 1 && desc->istate & IRQS_PENDING)
+ if (desc->depth == 1 && (desc->istate & IRQS_PENDING)
+ && (desc->istate & IRQS_SUSPENDED))
return -EBUSY;
continue;
}
--
1.7.0.4
When one irq is setup with flag IRQF_NO_SUSPEND, it is pointless
to call irq_set_irq_wake().
Because check_wakeup_irqs() is just checking the irq which has pending
but is in IRQS_SUSPENDED state when do syscore_suspend().
Signed-off-by: liu chuansheng <[email protected]>
---
kernel/irq/manage.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..94c5af9 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -513,6 +513,14 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
if (!desc)
return -EINVAL;
+ /* Checking if the flag IRQF_NO_SUSPEND is set,
+ * if set, drivers do not need to call irq_set_irq_wake(),
+ * it is pointless.
+ */
+ if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
+ WARN(1, "IRQ %d with flag IRQF_NO_SUSPEND, no need to call here\n",
+ irq);
+
/* wakeup-capable irqs can be shared between drivers that
* don't need to have the same sleep mode behaviors.
*/
--
1.7.0.4
On Tue, 12 Mar 2013, Chuansheng Liu wrote:
> According to commit 9c6079aa1bf(genirq: Do not consider disabled
> wakeup irqs), we should not break the suspend when one irq is pending
> but has been disabled before suspending.
>
> But there is another case missed, that one irq with flag IRQF_NO_SUSPEND,
> which has been disabled before suspending, and irq pending there,
> in this case, we still should not break the suspending, otherwise,
> the suspend abort over and over.
>
> Here also checking if the desc->istate & IRQS_SUSPENDED is true.
So what you are saying is:
If an interrupt which is marked IRQF_NO_SUSPEND has been disabled
before suspend invocation then desc->depth is 1 and therefor it should
not be checked for IRQS_PENDING in check_wakeup_irqs().
Makes sense, though the changelog needs to be more clear and the code
wants to have a proper comment.
Thanks,
tglx
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> kernel/irq/pm.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index cb228bf..1470c1b 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -109,7 +109,8 @@ int check_wakeup_irqs(void)
> * can abort suspend.
> */
> if (irqd_is_wakeup_set(&desc->irq_data)) {
> - if (desc->depth == 1 && desc->istate & IRQS_PENDING)
> + if (desc->depth == 1 && (desc->istate & IRQS_PENDING)
> + && (desc->istate & IRQS_SUSPENDED))
> return -EBUSY;
> continue;
> }
> --
> 1.7.0.4
>
>
>
>
> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: Tuesday, March 12, 2013 5:24 AM
> To: Liu, Chuansheng
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] genirq: Do not consider the irqs with disabling and
> IRQF_NO_SUSPEND
>
> On Tue, 12 Mar 2013, Chuansheng Liu wrote:
> > According to commit 9c6079aa1bf(genirq: Do not consider disabled
> > wakeup irqs), we should not break the suspend when one irq is pending
> > but has been disabled before suspending.
> >
> > But there is another case missed, that one irq with flag IRQF_NO_SUSPEND,
> > which has been disabled before suspending, and irq pending there,
> > in this case, we still should not break the suspending, otherwise,
> > the suspend abort over and over.
> >
> > Here also checking if the desc->istate & IRQS_SUSPENDED is true.
>
> So what you are saying is:
>
> If an interrupt which is marked IRQF_NO_SUSPEND has been disabled
> before suspend invocation then desc->depth is 1 and therefor it should
> not be checked for IRQS_PENDING in check_wakeup_irqs().
>
> Makes sense, though the changelog needs to be more clear and the code
> wants to have a proper comment.
Thanks, will copy your saying into comments, and send patch V2.
>
> Thanks,
>
> tglx
>
> > Signed-off-by: liu chuansheng <[email protected]>
> > ---
> > kernel/irq/pm.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index cb228bf..1470c1b 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -109,7 +109,8 @@ int check_wakeup_irqs(void)
> > * can abort suspend.
> > */
> > if (irqd_is_wakeup_set(&desc->irq_data)) {
> > - if (desc->depth == 1 && desc->istate & IRQS_PENDING)
> > + if (desc->depth == 1 && (desc->istate & IRQS_PENDING)
> > + && (desc->istate & IRQS_SUSPENDED))
> > return -EBUSY;
> > continue;
> > }
> > --
> > 1.7.0.4
> >
> >
> >
> >
According to commit 9c6079aa1bf(genirq: Do not consider disabled
wakeup irqs), we should not break the suspend when one interrupt has
been disabled before suspending and is pending there.
But there is another case missed:
If an interrupt which is marked IRQF_NO_SUSPEND has been disabled
before suspend invocation then desc->depth is 1 and therefor it should
not be checked for IRQS_PENDING in check_wakeup_irqs().
Here also checking if the desc->istate & IRQS_SUSPENDED is true to avoid
this case.
Signed-off-by: liu chuansheng <[email protected]>
---
kernel/irq/pm.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index cb228bf..f02a03d 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -107,9 +107,16 @@ int check_wakeup_irqs(void)
* Only interrupts which are marked as wakeup source
* and have not been disabled before the suspend check
* can abort suspend.
+ *
+ * Meanwhile, if an interrupt which is marked IRQF_NO_SUSPEND
+ * has been disabled before suspend invocation then
+ * desc->depth is 1 and therefor it should not be checked
+ * for IRQS_PENDING, so also adding the checking of
+ * desc->istate & IRQS_SUSPENDED for this case.
*/
if (irqd_is_wakeup_set(&desc->irq_data)) {
- if (desc->depth == 1 && desc->istate & IRQS_PENDING)
+ if (desc->depth == 1 && (desc->istate & IRQS_PENDING)
+ && (desc->istate & IRQS_SUSPENDED))
return -EBUSY;
continue;
}
--
1.7.0.4
Hello Thomas,
Sorry to miss the V2 in the subject.
I have updated the comments in this new patch, could you consider to take it into upstream?
Thanks.
Best Regards
Liu chuansheng
> -----Original Message-----
> From: Liu, Chuansheng
> Sent: Tuesday, March 12, 2013 5:58 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; Liu, Chuansheng
> Subject: [PATCH 2/3] genirq: Do not consider the irqs with IRQF_NO_SUSPEND
> in check_wakeup_irqs()
>
>
> According to commit 9c6079aa1bf(genirq: Do not consider disabled
> wakeup irqs), we should not break the suspend when one interrupt has
> been disabled before suspending and is pending there.
>
> But there is another case missed:
> If an interrupt which is marked IRQF_NO_SUSPEND has been disabled
> before suspend invocation then desc->depth is 1 and therefor it should
> not be checked for IRQS_PENDING in check_wakeup_irqs().
>
> Here also checking if the desc->istate & IRQS_SUSPENDED is true to avoid
> this case.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> kernel/irq/pm.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index cb228bf..f02a03d 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -107,9 +107,16 @@ int check_wakeup_irqs(void)
> * Only interrupts which are marked as wakeup source
> * and have not been disabled before the suspend check
> * can abort suspend.
> + *
> + * Meanwhile, if an interrupt which is marked IRQF_NO_SUSPEND
> + * has been disabled before suspend invocation then
> + * desc->depth is 1 and therefor it should not be checked
> + * for IRQS_PENDING, so also adding the checking of
> + * desc->istate & IRQS_SUSPENDED for this case.
> */
> if (irqd_is_wakeup_set(&desc->irq_data)) {
> - if (desc->depth == 1 && desc->istate & IRQS_PENDING)
> + if (desc->depth == 1 && (desc->istate & IRQS_PENDING)
> + && (desc->istate & IRQS_SUSPENDED))
> return -EBUSY;
> continue;
> }
> --
> 1.7.0.4
>
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?