irqchip: sun4i: Fix irq 0 not working

PATCH New Hans de Goede 2 participants 3 messages Mar 11, 2014 – Mar 12, 2014

Subsystems (2)

Arm/Allwinner sunXi SoC support irqchip drivers

lore

Share & tools

lore

Hans de Goede

author

2014-03-11 15:51 UTC #0 lore raw

PATCH 1/1 irqchip: sun4i: Fix irq 0 not working

SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things:
1) irq 0 pending
2) no more irqs pending

So we must loop always atleast once to make irq 0 work, otherwise irq 0 will never get serviced and we end up with a hard hang because sun4i_handle_irq gets re-entered constantly.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/irqchip/irq-sun4i.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sun4i.c +8 -2

index a5438d8..3761bf1 100644
@@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re
 {
 	u32 irq, hwirq;
 
+	/*
+	 * hwirq == 0 can mean one of 2 things:
+	 * 1) irq 0 pending
+	 * 2) no more irqs pending
+	 * So loop always atleast once to make irq 0 work.
+	 */
 	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	while (hwirq != 0) {
+	do {
 		irq = irq_find_mapping(sun4i_irq_domain, hwirq);
 		handle_IRQ(irq, regs);
 		hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	}
+	} while (hwirq != 0);
 }

Signature · 1 line

1.9.0

Maxime Ripard

2014-03-12 10:09 UTC #1 lore raw

PATCH 1/1 irqchip: sun4i: Fix irq 0 not working

Hi,

Quoted 18 lines · Hans de Goede

SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things:
1) irq 0 pending
2) no more irqs pending
So we must loop always atleast once to make irq 0 work, otherwise irq 0 will never get serviced and we end up with a hard hang because sun4i_handle_irq gets re-entered constantly.
Signed-off-by Hans de Goede <[email protected]>
---
 drivers/irqchip/irq-sun4i.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sun4i.c +0 -0
index a5438d8..3761bf1 100644

drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 146

Quoted 7 lines

drivers/irqchip/irq-sun4i.c +4 -0

@@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re
 {
 	u32 irq, hwirq;
 
+	/*
+	 * hwirq == 0 can mean one of 2 things:
+	 * 1) irq 0 pending
+	 * 2) no more irqs pending

3) spurious interrupt.

drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 150

Quoted 5 lines

drivers/irqchip/irq-sun4i.c +3 -1

+	 * So loop always atleast once to make irq 0 work.
+	 */
 	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	while (hwirq != 0) {
+	do {

I'd at least lookup in the pending register to see if the interrupt 0 was actually triggered. Otherwise, you could end up with spurious handler calls on the interrupt 0.

drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 153

Quoted 3 lines

drivers/irqchip/irq-sun4i.c +0 -0

 		irq = irq_find_mapping(sun4i_irq_domain, hwirq);
 		handle_IRQ(irq, regs);
 		hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;

And you end up with the same issue if there's a first != 0 interrupt, and then the interrupt 0.

What about something like:

while (1) {
	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
	if (!hwirq)
		if (!(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0)))
			break;

	irq = irq_find_mapping(sun4i_irq_domain, hwirq);
	handle_IRQ(irq, regs);
}

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140312/7d937b9c/attachment.sig>

Hans de Goede

author

2014-03-12 13:45 UTC #2 lore raw

PATCH 1/1 irqchip: sun4i: Fix irq 0 not working

Hi,

Quoted 31 lines · Maxime Ripard

Hi,
Quoted 18 lines · Hans de Goede
SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 2 things:
1) irq 0 pending
2) no more irqs pending
So we must loop always atleast once to make irq 0 work, otherwise irq 0 will never get serviced and we end up with a hard hang because sun4i_handle_irq gets re-entered constantly.
Signed-off-by Hans de Goede <[email protected]>
---
 drivers/irqchip/irq-sun4i.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sun4i.c +0 -0
index a5438d8..3761bf1 100644
drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 146
Quoted 7 lines
drivers/irqchip/irq-sun4i.c +4 -0
@@ -140,10 +140,16 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re
 {
 	u32 irq, hwirq;
 
+	/*
+	 * hwirq == 0 can mean one of 2 things:
+	 * 1) irq 0 pending
+	 * 2) no more irqs pending
3) spurious interrupt.

Quoted 9 lines

drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 150
Quoted 5 lines
drivers/irqchip/irq-sun4i.c +3 -1
+	 * So loop always atleast once to make irq 0 work.
+	 */
 	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	while (hwirq != 0) {
+	do {
I'd at least lookup in the pending register to see if the interrupt 0 was actually triggered. Otherwise, you could end up with spurious handler calls on the interrupt 0.

Yes, I was already worrying about this myself after sending the patch, and considered reading pending too.

Quoted 7 lines

drivers/irqchip/irq-sun4i.c static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re · line 153
Quoted 4 lines
drivers/irqchip/irq-sun4i.c +0 -0
 		irq = irq_find_mapping(sun4i_irq_domain, hwirq);
 		handle_IRQ(irq, regs);
 		hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
And you end up with the same issue if there's a first != 0 interrupt, and then the interrupt 0.

No, before my fix sun4i_handle_irq would be called continuously since we were never handling irq 0, so if this happens we will simply drop out of sun4i_handle_irq only to immediately get recalled, this does make this scenario more expensive, but things will still work, while it saves an also not cheap read from SUN4I_IRQ_PENDING_REG(0) for each regular interrupt.

Note I agree the spurious irq case is an issue, as said that has me worried too.

Quoted 11 lines

What about something like:

while (1) {
	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
	if (!hwirq)
		if (!(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0)))
			break;

	irq = irq_find_mapping(sun4i_irq_domain, hwirq);
	handle_IRQ(irq, regs);
}

Yes that should work nicely, but for the straight path it means reading pending once for each interrupt.

I agree we need to read pending before calling handle_IRQ for irq 0, but we only need to do so if the first read from SUN4I_IRQ_VECTOR_REG == 0, on any subsequent reads from SUN4I_IRQ_VECTOR_REG returning 0 we can exit immediately, in the worst case we'll get called again, and then do the right thing.

IE something like this:

	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
	/* Ensure hwirq == 0 is because of irq 0 pending */
	if (hwirq == 0 && !(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0)))
		return;
	do {
		irq = irq_find_mapping(sun4i_irq_domain, hwirq);
		handle_IRQ(irq, regs);
		hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
	} while (hwirq);

Note untested, and this might be unnecessary optimization. So let me know which version you prefer and I'll give it a test run.

Regards,

Hans