[PATCH] qe_ic: Do a sync when masking interrupts.

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE7237d

8 messages, 4 authors, 2006-10-23 · open the first message on its own page

[PATCH] qe_ic: Do a sync when masking interrupts.

From: Scott Wood <hidden>
Date: 2006-10-19 18:03:12 

This patch causes a sync do be done after masking a QE interrupt, to
ensure that the masking has completed before interrupts are enabled.
This allows the masking of the cascade IRQ to be removed without causing
spurious interrupts.

The mask_and_ack function is also removed and set to the mask function,
as the two are identical.

Signed-off-by: Scott Wood <redacted>
---
 arch/powerpc/sysdev/qe_lib/qe_ic.c |   34 +++++++---------------------------
 1 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c b/arch/powerpc/sysdev/qe_lib/qe_ic.c
index 6995f51..d96e48d 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_ic.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c
@@ -222,24 +222,12 @@ static void qe_ic_mask_irq(unsigned int 
 	temp = qe_ic_read(qe_ic->regs, qe_ic_info[src].mask_reg);
 	qe_ic_write(qe_ic->regs, qe_ic_info[src].mask_reg,
 		    temp & ~qe_ic_info[src].mask);
-
-	spin_unlock_irqrestore(&qe_ic_lock, flags);
-}
-
-static void qe_ic_mask_irq_and_ack(unsigned int virq)
-{
-	struct qe_ic *qe_ic = qe_ic_from_irq(virq);
-	unsigned int src = virq_to_hw(virq);
-	unsigned long flags;
-	u32 temp;
-
-	spin_lock_irqsave(&qe_ic_lock, flags);
-
-	temp = qe_ic_read(qe_ic->regs, qe_ic_info[src].mask_reg);
-	qe_ic_write(qe_ic->regs, qe_ic_info[src].mask_reg,
-		    temp & ~qe_ic_info[src].mask);
-
-	/* There is nothing to do for ack here, ack is handled in ISR */
+	
+	/* Flush the above write before enabling interrupts;
+	 * otherwise, spurious interrupts will sometimes
+	 * happen
+	 */
+	mb();
 
 	spin_unlock_irqrestore(&qe_ic_lock, flags);
 }
@@ -248,7 +236,7 @@ static struct irq_chip qe_ic_irq_chip = 
 	.typename = " QEIC  ",
 	.unmask = qe_ic_unmask_irq,
 	.mask = qe_ic_mask_irq,
-	.mask_ack = qe_ic_mask_irq_and_ack,
+	.mask_ack = qe_ic_mask_irq,
 };
 
 static int qe_ic_host_match(struct irq_host *h, struct device_node *node)
@@ -331,8 +319,6 @@ unsigned int qe_ic_get_high_irq(struct q
 	return irq_linear_revmap(qe_ic->irqhost, irq);
 }
 
-/* FIXME: We mask all the QE Low interrupts while handling.  We should
- * let other interrupt come in, but BAD interrupts are generated */
 void fastcall qe_ic_cascade_low(unsigned int irq, struct irq_desc *desc)
 {
 	struct qe_ic *qe_ic = desc->handler_data;
@@ -340,14 +326,10 @@ void fastcall qe_ic_cascade_low(unsigned
 
 	unsigned int cascade_irq = qe_ic_get_low_irq(qe_ic);
 
-	chip->mask_ack(irq);
 	if (cascade_irq != NO_IRQ)
 		generic_handle_irq(cascade_irq);
-	chip->unmask(irq);
 }
 
-/* FIXME: We mask all the QE High interrupts while handling.  We should
- * let other interrupt come in, but BAD interrupts are generated */
 void fastcall qe_ic_cascade_high(unsigned int irq, struct irq_desc *desc)
 {
 	struct qe_ic *qe_ic = desc->handler_data;
@@ -355,10 +337,8 @@ void fastcall qe_ic_cascade_high(unsigne
 
 	unsigned int cascade_irq = qe_ic_get_high_irq(qe_ic);
 
-	chip->mask_ack(irq);
 	if (cascade_irq != NO_IRQ)
 		generic_handle_irq(cascade_irq);
-	chip->unmask(irq);
 }
 
 void __init qe_ic_init(struct device_node *node, unsigned int flags)
-- 
1.4.2.3

RE: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Li Yang-r58472 <hidden>
Date: 2006-10-20 03:02:51 

-----Original Message-----
From: linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org] On =
Behalf
Of Scott
Wood
Sent: Friday, October 20, 2006 2:03 AM
To: linuxppc-dev@ozlabs.org
Subject: [PATCH] qe_ic: Do a sync when masking interrupts.
=20
This patch causes a sync do be done after masking a QE interrupt, to
ensure that the masking has completed before interrupts are enabled.
This allows the masking of the cascade IRQ to be removed without
causing
spurious interrupts.
=20
The mask_and_ack function is also removed and set to the mask
function,
as the two are identical.
=20
Signed-off-by: Scott Wood <redacted>
Acked-by: Li Yang <redacted>
---
 arch/powerpc/sysdev/qe_lib/qe_ic.c |   34
+++++++---------------------------
quoted hunk
 1 files changed, 7 insertions(+), 27 deletions(-)
=20
diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c
b/arch/powerpc/sysdev/qe_lib/qe_ic.c
index 6995f51..d96e48d 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_ic.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c
@@ -222,24 +222,12 @@ static void qe_ic_mask_irq(unsigned int
 	temp =3D qe_ic_read(qe_ic->regs, qe_ic_info[src].mask_reg);
 	qe_ic_write(qe_ic->regs, qe_ic_info[src].mask_reg,
 		    temp & ~qe_ic_info[src].mask);
-
-	spin_unlock_irqrestore(&qe_ic_lock, flags);
-}
-
-static void qe_ic_mask_irq_and_ack(unsigned int virq)
-{
-	struct qe_ic *qe_ic =3D qe_ic_from_irq(virq);
-	unsigned int src =3D virq_to_hw(virq);
-	unsigned long flags;
-	u32 temp;
-
-	spin_lock_irqsave(&qe_ic_lock, flags);
-
-	temp =3D qe_ic_read(qe_ic->regs, qe_ic_info[src].mask_reg);
-	qe_ic_write(qe_ic->regs, qe_ic_info[src].mask_reg,
-		    temp & ~qe_ic_info[src].mask);
-
-	/* There is nothing to do for ack here, ack is handled in ISR */
+
+	/* Flush the above write before enabling interrupts;
+	 * otherwise, spurious interrupts will sometimes
+	 * happen
+	 */
+	mb();
=20
 	spin_unlock_irqrestore(&qe_ic_lock, flags);
 }
@@ -248,7 +236,7 @@ static struct irq_chip qe_ic_irq_chip =3D
 	.typename =3D " QEIC  ",
 	.unmask =3D qe_ic_unmask_irq,
 	.mask =3D qe_ic_mask_irq,
-	.mask_ack =3D qe_ic_mask_irq_and_ack,
+	.mask_ack =3D qe_ic_mask_irq,
 };
=20
 static int qe_ic_host_match(struct irq_host *h, struct device_node
*node)
quoted hunk
@@ -331,8 +319,6 @@ unsigned int qe_ic_get_high_irq(struct q
 	return irq_linear_revmap(qe_ic->irqhost, irq);
 }
=20
-/* FIXME: We mask all the QE Low interrupts while handling.  We
should
- * let other interrupt come in, but BAD interrupts are generated */
 void fastcall qe_ic_cascade_low(unsigned int irq, struct irq_desc
*desc)
quoted hunk
 {
 	struct qe_ic *qe_ic =3D desc->handler_data;
@@ -340,14 +326,10 @@ void fastcall qe_ic_cascade_low(unsigned
=20
 	unsigned int cascade_irq =3D qe_ic_get_low_irq(qe_ic);
=20
-	chip->mask_ack(irq);
 	if (cascade_irq !=3D NO_IRQ)
 		generic_handle_irq(cascade_irq);
-	chip->unmask(irq);
 }
=20
-/* FIXME: We mask all the QE High interrupts while handling.  We
should
- * let other interrupt come in, but BAD interrupts are generated */
 void fastcall qe_ic_cascade_high(unsigned int irq, struct irq_desc
*desc)
quoted hunk
 {
 	struct qe_ic *qe_ic =3D desc->handler_data;
@@ -355,10 +337,8 @@ void fastcall qe_ic_cascade_high(unsigne
=20
 	unsigned int cascade_irq =3D qe_ic_get_high_irq(qe_ic);
=20
-	chip->mask_ack(irq);
 	if (cascade_irq !=3D NO_IRQ)
 		generic_handle_irq(cascade_irq);
-	chip->unmask(irq);
 }
=20
 void __init qe_ic_init(struct device_node *node, unsigned int flags)
--
1.4.2.3
=20
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Paul Mackerras <hidden>
Date: 2006-10-23 03:59:43 

Scott Wood writes:

This patch causes a sync do be done after masking a QE interrupt, to
ensure that the masking has completed before interrupts are enabled.
This allows the masking of the cascade IRQ to be removed without causing
spurious interrupts.
Hmmm.  In general a sync having completed doesn't mean that previous
MMIO stores have actually got to the device.  Reading from a device
register generally does ensure that previous writes have actually got
to the device though - could you do that instead?  I'm concerned that
adding the sync is not a robust fix and is possibly only working due
to fortuitous timing.

Paul.

RE: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Li Yang-r58472 <hidden>
Date: 2006-10-23 09:34:57 

-----Original Message-----
From: linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org] On =
Behalf
Of Paul
Mackerras
Sent: Monday, October 23, 2006 12:00 PM
To: Wood Scott-B07421
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH] qe_ic: Do a sync when masking interrupts.
=20
Scott Wood writes:
=20
quoted
This patch causes a sync do be done after masking a QE interrupt, to
ensure that the masking has completed before interrupts are enabled.
This allows the masking of the cascade IRQ to be removed without
causing
quoted
spurious interrupts.
=20
Hmmm.  In general a sync having completed doesn't mean that previous
MMIO stores have actually got to the device.  Reading from a device
register generally does ensure that previous writes have actually got
to the device though - could you do that instead?  I'm concerned that
adding the sync is not a robust fix and is possibly only working due
to fortuitous timing.
But an i/o read will be considerably slower than a sync, and it is in
the critical path of interrupt.  I have tested the patch under
relatively heavy Ethernet load, and there is no spurious interrupt.
Maybe it is because the device is an SOC device and MMIO store completes
faster.  I'm wondering if there is a standard test method to show if the
faster approach is sufficient or not.

- Leo=20

Re: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Scott Wood <hidden>
Date: 2006-10-23 15:19:15 

Paul Mackerras wrote:
Hmmm.  In general a sync having completed doesn't mean that previous
MMIO stores have actually got to the device.  Reading from a device
register generally does ensure that previous writes have actually got
to the device though - could you do that instead?  I'm concerned that
adding the sync is not a robust fix and is possibly only working due
to fortuitous timing.
Possibly -- but the only problem on the rare occasions where the timing 
is not fortuitous is a spurious interrupt; the only reason the sync 
needs to be there at all is to avoid the overhead of the extra interrupt 
(and to avoid user complaints that they're getting "BAD" interrupts).

Note that many/most interrupt controller drivers don't even do that 
much; they just have more fortuitous timing than the QE (and/or the 
spurious interrupt is non-cascaded, and thus gets caught in 
handle_level_irq and doesn't increment the spurious interrupt count).

-Scott

Re: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Scott Wood <hidden>
Date: 2006-10-23 15:29:32 

Scott Wood wrote:
Note that many/most interrupt controller drivers don't even do that 
much; they just have more fortuitous timing than the QE (and/or the 
spurious interrupt is non-cascaded, and thus gets caught in 
handle_level_irq and doesn't increment the spurious interrupt count).
Ignore that last bit about non-cascaded interrupts; if it makes a 
difference at all, it's because of added delays (getting back to the 
fortuitous timing thing).

-Scott

Re: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Kumar Gala <hidden>
Date: 2006-10-23 17:52:28 

On Oct 23, 2006, at 10:19 AM, Scott Wood wrote:

Paul Mackerras wrote:
quoted
Hmmm.  In general a sync having completed doesn't mean that previous
MMIO stores have actually got to the device.  Reading from a device
register generally does ensure that previous writes have actually got
to the device though - could you do that instead?  I'm concerned that
adding the sync is not a robust fix and is possibly only working due
to fortuitous timing.
Possibly -- but the only problem on the rare occasions where the  
timing
is not fortuitous is a spurious interrupt; the only reason the sync
needs to be there at all is to avoid the overhead of the extra  
interrupt
(and to avoid user complaints that they're getting "BAD" interrupts).
Why wouldn't the read accomplish the same thing in a more robust way  
than the sync?

- k

Re: [PATCH] qe_ic: Do a sync when masking interrupts.

From: Scott Wood <hidden>
Date: 2006-10-23 18:23:01 

Kumar Gala wrote:
On Oct 23, 2006, at 10:19 AM, Scott Wood wrote:
quoted
Possibly -- but the only problem on the rare occasions where the  timing
is not fortuitous is a spurious interrupt; the only reason the sync
needs to be there at all is to avoid the overhead of the extra  interrupt
(and to avoid user complaints that they're getting "BAD" interrupts).

Why wouldn't the read accomplish the same thing in a more robust way  
than the sync?
It would.  However, it also adds a small amount of overhead to every QE 
interrupt, and the only thing that that overhead buys is avoiding 
possible but empirically very rare spurious interrupts; it'd cost more 
than simply accepting that a spurious interrupt might happen once in a 
great while.

Without any type of sync, spurious interrupts happen fairly regularly 
(about 5-10% of legitimate interrupts), so adding the sync should be a 
net gain over doing nothing.

If the consensus is that a read should be done anyway, I can resumbit 
the patch that way; I just think it's overkill given that a 100% 
guarantee isn't required for correctness.

-Scott
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help