Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-05-08 Thread NeilBrown 
On Fri, 4 May 2012 18:01:22 +0200 (CEST) Thomas Gleixner t...@linutronix.de
wrote:

 Neil,
 
 On Fri, 4 May 2012, NeilBrown wrote:
 
  On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
  t...@linutronix.de wrote:
  
   Why not simply managing the pending bit for level irqs ?
   
  
  Hi Thomas,
   thanks again for the patch.  I finally made time to test it and it works as
  expected.  I've included it below with a change-log entry and tested-by:
  in case that helps.
 
 thanks for testing. The changelog is great. You know how to make the
 live of lazy buggers easier :)

Just buttering you up so any future patches slip past easily :-)

I think I'll need to ask for IRQS_PENDING to be set for nested interrupts too
but I'll be a little while before  I an look at that issue properly and
propose a patch.

Thanks for your help,
NeilBrown



   
  for_each_irq_desc(irq, desc) {
  -   if (irqd_is_wakeup_set(desc-irq_data)) {
  +   if (desc-depth == 1 
  +   irqd_is_wakeup_set(desc-irq_data)) {
  if (desc-istate  IRQS_PENDING)
  return -EBUSY;
  continue;
 
 I split that part into a separate patch, as it's really a different
 issue.
 
 Thanks,
 
   tglx



signature.asc
Description: PGP signature

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-05-04 Thread Thomas Gleixner 
Neil,

On Fri, 4 May 2012, NeilBrown wrote:

 On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
 t...@linutronix.de wrote:
 
  Why not simply managing the pending bit for level irqs ?
  
 
 Hi Thomas,
  thanks again for the patch.  I finally made time to test it and it works as
 expected.  I've included it below with a change-log entry and tested-by:
 in case that helps.

thanks for testing. The changelog is great. You know how to make the
live of lazy buggers easier :)
  
   for_each_irq_desc(irq, desc) {
 - if (irqd_is_wakeup_set(desc-irq_data)) {
 + if (desc-depth == 1 
 + irqd_is_wakeup_set(desc-irq_data)) {
   if (desc-istate  IRQS_PENDING)
   return -EBUSY;
   continue;

I split that part into a separate patch, as it's really a different
issue.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-05-03 Thread NeilBrown 
On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
t...@linutronix.de wrote:

 Why not simply managing the pending bit for level irqs ?
 

Hi Thomas,
 thanks again for the patch.  I finally made time to test it and it works as
expected.  I've included it below with a change-log entry and tested-by:
in case that helps.

Thanks,
NeilBrown


From: Thomas Gleixner t...@linutronix.de
Date: Wed, 25 Apr 2012 12:54:54 +0200
Subject: [PATCH] IRQ: allow check_wakeup_irqs to notice level-triggered 
interrupts.

Level triggered interrupts do not cause IRQS_PENDING to be set when
they fire-while-disabled as the 'pending' state is always present in
the level - they automatically refire where re-enabled.

However the IRQS_PENDING flag is also used to abort a suspend cycle -
if any 'is_wakeup_set' interrupt is PENDING, check_wakeup_irqs() will
cause suspend to abort. Without IRQS_PENDING, suspend won't abort.

Consequently, level-triggered interrupts that fire during the 'noirq'
phase of suspend do not currently abort suspend.

So set IRQS_PENDING even for level triggered interrupts, and make sure
to clear the flag in check_irq_resend.

Also: in check_wakeup_irqs, ignore 'is_wakeup' interrupts that were
already disabled before suspend_device_irqs() disabled them further.

Tested-by: NeilBrown ne...@suse.de

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6080f6b..741f836 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
 * If its disabled or no action available
 * keep it masked and get out of here
 */
-   if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data)))
+   if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data))) {
+   desc-istate |= IRQS_PENDING;
goto out_unlock;
+   }
 
handle_irq_event(desc);
 
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 15e53b1..b858ce3 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
int irq;
 
for_each_irq_desc(irq, desc) {
-   if (irqd_is_wakeup_set(desc-irq_data)) {
+   if (desc-depth == 1 
+   irqd_is_wakeup_set(desc-irq_data)) {
if (desc-istate  IRQS_PENDING)
return -EBUSY;
continue;
diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c
index 14dd576..6454db7 100644
--- a/kernel/irq/resend.c
+++ b/kernel/irq/resend.c
@@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *desc, unsigned int 
irq)
/*
 * We do not resend level type interrupts. Level type
 * interrupts are resent by hardware when they are still
-* active.
+* active. Clear the pending bit so suspend/resume does not
+* get confused.
 */
-   if (irq_settings_is_level(desc))
+   if (irq_settings_is_level(desc)) {
+   desc-istate = ~IRQS_PENDING;
return;
+   }
if (desc-istate  IRQS_REPLAY)
return;
if (desc-istate  IRQS_PENDING) {


signature.asc
Description: PGP signature

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-04-25 Thread Thomas Gleixner 
On Wed, 25 Apr 2012, NeilBrown wrote:

 Level triggered interrupts do not cause IRQS_PENDING to be set, so
 check_wakeup_irqs ignores them.
 They don't need to set IRQS_PENDING as the level stays high which
 shows that they must be pending.  However if such an interrupt fired
 during late suspend, it will have been masked so the fact that it
 is still asserted will not cause the suspend to abort.
 
 So if any wakeup interrupt is masked, unmask it when checking wakeup
 irqs.  If the interrupt is asserted, suspend will abort.
 
 Signed-off-by: NeilBrown ne...@suse.de
 ---
 
  kernel/irq/pm.c |6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
 index 15e53b1..0d26206 100644
 --- a/kernel/irq/pm.c
 +++ b/kernel/irq/pm.c
 @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
   if (irqd_is_wakeup_set(desc-irq_data)) {
   if (desc-istate  IRQS_PENDING)
   return -EBUSY;
 + if (irqd_irq_masked(desc-irq_data))
 + /* Probably a level interrupt
 +  * which fired recently and was
 +  * masked
 +  */
 + unmask_irq(desc);

Oh no. We don't unmask unconditionally. What about an interrupt which
is disabled, has no handler . ? That needs more thought.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-04-25 Thread NeilBrown 
On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
t...@linutronix.de wrote:

 On Wed, 25 Apr 2012, NeilBrown wrote:
 
  Level triggered interrupts do not cause IRQS_PENDING to be set, so
  check_wakeup_irqs ignores them.
  They don't need to set IRQS_PENDING as the level stays high which
  shows that they must be pending.  However if such an interrupt fired
  during late suspend, it will have been masked so the fact that it
  is still asserted will not cause the suspend to abort.
  
  So if any wakeup interrupt is masked, unmask it when checking wakeup
  irqs.  If the interrupt is asserted, suspend will abort.
  
  Signed-off-by: NeilBrown ne...@suse.de
  ---
  
   kernel/irq/pm.c |6 ++
   1 file changed, 6 insertions(+)
  
  diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
  index 15e53b1..0d26206 100644
  --- a/kernel/irq/pm.c
  +++ b/kernel/irq/pm.c
  @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
  if (irqd_is_wakeup_set(desc-irq_data)) {
  if (desc-istate  IRQS_PENDING)
  return -EBUSY;
  +   if (irqd_irq_masked(desc-irq_data))
  +   /* Probably a level interrupt
  +* which fired recently and was
  +* masked
  +*/
  +   unmask_irq(desc);
 
 Oh no. We don't unmask unconditionally. What about an interrupt which
 is disabled, has no handler . ? That needs more thought.

If there is no handler, then irqd_is_wakeup_set() should fail should it not?

For disabled: would it be OK to check desc-depth?
Something like:
 if (desc-depth == 1  (desc-state  IRQS_SUSPENDED) 
 irqd_irq_masked(desc-irq_data))
  unmask_irq(desc);

??

Thanks,
NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-04-25 Thread Thomas Gleixner 
On Wed, 25 Apr 2012, NeilBrown wrote:
 On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
 t...@linutronix.de wrote:
 
  On Wed, 25 Apr 2012, NeilBrown wrote:
  
   Level triggered interrupts do not cause IRQS_PENDING to be set, so
   check_wakeup_irqs ignores them.
   They don't need to set IRQS_PENDING as the level stays high which
   shows that they must be pending.  However if such an interrupt fired
   during late suspend, it will have been masked so the fact that it
   is still asserted will not cause the suspend to abort.
   
   So if any wakeup interrupt is masked, unmask it when checking wakeup
   irqs.  If the interrupt is asserted, suspend will abort.
   
   Signed-off-by: NeilBrown ne...@suse.de
   ---
   
kernel/irq/pm.c |6 ++
1 file changed, 6 insertions(+)
   
   diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
   index 15e53b1..0d26206 100644
   --- a/kernel/irq/pm.c
   +++ b/kernel/irq/pm.c
   @@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
 if (irqd_is_wakeup_set(desc-irq_data)) {
 if (desc-istate  IRQS_PENDING)
 return -EBUSY;
   + if (irqd_irq_masked(desc-irq_data))
   + /* Probably a level interrupt
   +  * which fired recently and was
   +  * masked
   +  */
   + unmask_irq(desc);
  
  Oh no. We don't unmask unconditionally. What about an interrupt which
  is disabled, has no handler . ? That needs more thought.
 
 If there is no handler, then irqd_is_wakeup_set() should fail should it not?

Well, it does not. The wakeup state is a permanent flag on the irq
line. Though we don't have IRQS_SUSPENDED on that descriptor.
 
 For disabled: would it be OK to check desc-depth?
 Something like:
  if (desc-depth == 1  (desc-state  IRQS_SUSPENDED) 
  irqd_irq_masked(desc-irq_data))
   unmask_irq(desc);
 

Why not simply managing the pending bit for level irqs ?

Index: tip/kernel/irq/chip.c
===
--- tip.orig/kernel/irq/chip.c
+++ tip/kernel/irq/chip.c
@@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struc
 * If its disabled or no action available
 * keep it masked and get out of here
 */
-   if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data)))
+   if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data))) {
+   desc-istate |= IRQS_PENDING;
goto out_unlock;
+   }
 
handle_irq_event(desc);
 
Index: tip/kernel/irq/resend.c
===
--- tip.orig/kernel/irq/resend.c
+++ tip/kernel/irq/resend.c
@@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *d
/*
 * We do not resend level type interrupts. Level type
 * interrupts are resent by hardware when they are still
-* active.
+* active. Clear the pending bit so suspend/resume does not
+* get confused.
 */
-   if (irq_settings_is_level(desc))
+   if (irq_settings_is_level(desc)) {
+   desc-istate = ~IRQS_PENDING;
return;
+   }
if (desc-istate  IRQS_REPLAY)
return;
if (desc-istate  IRQS_PENDING) {

And to handle interrupts which have been disabled before suspend add:

Index: tip/kernel/irq/pm.c
===
--- tip.orig/kernel/irq/pm.c
+++ tip/kernel/irq/pm.c
@@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
int irq;
 
for_each_irq_desc(irq, desc) {
-   if (irqd_is_wakeup_set(desc-irq_data)) {
+   if (desc-depth == 1 
+   irqd_is_wakeup_set(desc-irq_data)) {
if (desc-istate  IRQS_PENDING)
return -EBUSY;
continue;
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-04-25 Thread NeilBrown 
On Wed, 25 Apr 2012 14:54:54 +0200 (CEST) Thomas Gleixner
t...@linutronix.de wrote:

 On Wed, 25 Apr 2012, NeilBrown wrote:
  On Wed, 25 Apr 2012 10:50:15 +0200 (CEST) Thomas Gleixner
  t...@linutronix.de wrote:
  
   On Wed, 25 Apr 2012, NeilBrown wrote:
   
Level triggered interrupts do not cause IRQS_PENDING to be set, so
check_wakeup_irqs ignores them.
They don't need to set IRQS_PENDING as the level stays high which
shows that they must be pending.  However if such an interrupt fired
during late suspend, it will have been masked so the fact that it
is still asserted will not cause the suspend to abort.

So if any wakeup interrupt is masked, unmask it when checking wakeup
irqs.  If the interrupt is asserted, suspend will abort.

Signed-off-by: NeilBrown ne...@suse.de
---

 kernel/irq/pm.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 15e53b1..0d26206 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
if (irqd_is_wakeup_set(desc-irq_data)) {
if (desc-istate  IRQS_PENDING)
return -EBUSY;
+   if (irqd_irq_masked(desc-irq_data))
+   /* Probably a level interrupt
+* which fired recently and was
+* masked
+*/
+   unmask_irq(desc);
   
   Oh no. We don't unmask unconditionally. What about an interrupt which
   is disabled, has no handler . ? That needs more thought.
  
  If there is no handler, then irqd_is_wakeup_set() should fail should it not?
 
 Well, it does not. The wakeup state is a permanent flag on the irq
 line. Though we don't have IRQS_SUSPENDED on that descriptor.
  
  For disabled: would it be OK to check desc-depth?
  Something like:
   if (desc-depth == 1  (desc-state  IRQS_SUSPENDED) 
   irqd_irq_masked(desc-irq_data))
unmask_irq(desc);
  
 
 Why not simply managing the pending bit for level irqs ?

Primarily because I was aiming for the simplest patch that would have the
desired effect.  Also because 'pending' is implicit in the level so it seems
superfluous to store the bit separately.  And understanding all the
consequences of that change is not something I felt up to.

However: thanks for the patch. I'll try to explore it tomorrow and see if
seems to be behaving correctly.

Thanks,
NeilBrown

 
 Index: tip/kernel/irq/chip.c
 ===
 --- tip.orig/kernel/irq/chip.c
 +++ tip/kernel/irq/chip.c
 @@ -379,8 +379,10 @@ handle_level_irq(unsigned int irq, struc
* If its disabled or no action available
* keep it masked and get out of here
*/
 - if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data)))
 + if (unlikely(!desc-action || irqd_irq_disabled(desc-irq_data))) {
 + desc-istate |= IRQS_PENDING;
   goto out_unlock;
 + }
  
   handle_irq_event(desc);
  
 Index: tip/kernel/irq/resend.c
 ===
 --- tip.orig/kernel/irq/resend.c
 +++ tip/kernel/irq/resend.c
 @@ -58,10 +58,13 @@ void check_irq_resend(struct irq_desc *d
   /*
* We do not resend level type interrupts. Level type
* interrupts are resent by hardware when they are still
 -  * active.
 +  * active. Clear the pending bit so suspend/resume does not
 +  * get confused.
*/
 - if (irq_settings_is_level(desc))
 + if (irq_settings_is_level(desc)) {
 + desc-istate = ~IRQS_PENDING;
   return;
 + }
   if (desc-istate  IRQS_REPLAY)
   return;
   if (desc-istate  IRQS_PENDING) {
 
 And to handle interrupts which have been disabled before suspend add:
 
 Index: tip/kernel/irq/pm.c
 ===
 --- tip.orig/kernel/irq/pm.c
 +++ tip/kernel/irq/pm.c
 @@ -103,7 +103,8 @@ int check_wakeup_irqs(void)
   int irq;
  
   for_each_irq_desc(irq, desc) {
 - if (irqd_is_wakeup_set(desc-irq_data)) {
 + if (desc-depth == 1 
 + irqd_is_wakeup_set(desc-irq_data)) {
   if (desc-istate  IRQS_PENDING)
   return -EBUSY;
   continue;



signature.asc
Description: PGP signature

[PATCH 2/3] IRQ: allow check_wakeup_irqs to notice level-triggered interrupts.

2012-04-24 Thread NeilBrown 
Level triggered interrupts do not cause IRQS_PENDING to be set, so
check_wakeup_irqs ignores them.
They don't need to set IRQS_PENDING as the level stays high which
shows that they must be pending.  However if such an interrupt fired
during late suspend, it will have been masked so the fact that it
is still asserted will not cause the suspend to abort.

So if any wakeup interrupt is masked, unmask it when checking wakeup
irqs.  If the interrupt is asserted, suspend will abort.

Signed-off-by: NeilBrown ne...@suse.de
---

 kernel/irq/pm.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 15e53b1..0d26206 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -106,6 +106,12 @@ int check_wakeup_irqs(void)
if (irqd_is_wakeup_set(desc-irq_data)) {
if (desc-istate  IRQS_PENDING)
return -EBUSY;
+   if (irqd_irq_masked(desc-irq_data))
+   /* Probably a level interrupt
+* which fired recently and was
+* masked
+*/
+   unmask_irq(desc);
continue;
}
/*


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html