2.6.20-rc: au1x irqs broken

2.6.20-rc: au1x irqs broken

[Manuel Lauss]

Hello,

mips-git commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 broke
au1x irqs. The kernel boots; however it is not able to
mount nfsroot. Reverting the arch/mips/au1000/common/irq.c
bits of the above commit fixes it.

Thanks,

-- 
 ml.

Re: 2.6.20-rc: au1x irqs broken

[Atsushi Nemoto]

On Fri, 2 Feb 2007 07:13:56 +0100, Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> mips-git commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 broke
> au1x irqs. The kernel boots; however it is not able to
> mount nfsroot. Reverting the arch/mips/au1000/common/irq.c
> bits of the above commit fixes it.

Thank you for report.  But I can not see how that change break au1x.
You are using au1000_eth driver, and the driver use level irq, right?

Does reverting just only this part work?

 static struct irq_chip level_irq_type = {
 	.typename = "Au1000 Level",
-	.startup = startup_irq,
-	.shutdown = shutdown_irq,
-	.enable = local_enable_irq,
-	.disable = local_disable_irq,
 	.ack = mask_and_ack_level_irq,
+	.mask = local_disable_irq,
+	.mask_ack = mask_and_ack_level_irq,
+	.unmask = local_enable_irq,
 	.end = end_irq,
 };
 
And if it was work, does reverting only 4 lines removal part work?

---
Atsushi Nemoto

Re: 2.6.20-rc: au1x irqs broken

[Manuel Lauss]

On Fri, Feb 02, 2007 at 04:18:57PM +0900, Atsushi Nemoto wrote:
> On Fri, 2 Feb 2007 07:13:56 +0100, Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> > mips-git commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 broke
> > au1x irqs. The kernel boots; however it is not able to
> > mount nfsroot. Reverting the arch/mips/au1000/common/irq.c
> > bits of the above commit fixes it.
> 
> Thank you for report.  But I can not see how that change break au1x.

I'm sorry, it was my fault. For some reason I commented out all the
.ack/.end fields in the irq_chip descriptions to make an earlier
-rc boot. With these fields integrated again it works as
expected (although I'm still unsure why the .ack/.end are required
at all. Shouldn't mask/unmask/mask_ack be enough? [for non-pb1000])

Sorry for the noise!

Thanks,

-- 
 ml.

Re: 2.6.20-rc: au1x irqs broken

[Manuel Lauss]

On Fri, Feb 02, 2007 at 08:53:28AM +0100, Manuel Lauss wrote:
> On Fri, Feb 02, 2007 at 04:18:57PM +0900, Atsushi Nemoto wrote:
> > On Fri, 2 Feb 2007 07:13:56 +0100, Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> > > mips-git commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 broke
> > > au1x irqs. The kernel boots; however it is not able to
> > > mount nfsroot. Reverting the arch/mips/au1000/common/irq.c
> > > bits of the above commit fixes it.
> > 
> > Thank you for report.  But I can not see how that change break au1x.
> 
> I'm sorry, it was my fault. For some reason I commented out all the
> .ack/.end fields in the irq_chip descriptions to make an earlier

Here's what tripped me up. I switched au1x over to use the kernel
flow handlers, and forgot to undo all of it.

rediffed against 2.6.20-rc7

diff -Naurp linux-2.6.20-rc7/arch/mips/au1000/common/irq.c linux-2.6.20-rc7-work/arch/mips/au1000/common/irq.c
--- linux-2.6.20-rc7/arch/mips/au1000/common/irq.c	2007-02-01 15:04:35.601983000 +0100
+++ linux-2.6.20-rc7-work/arch/mips/au1000/common/irq.c	2007-02-02 11:23:35.251983000 +0100
@@ -70,7 +70,6 @@ extern irq_cpustat_t irq_stat [NR_CPUS];
 extern void mips_timer_interrupt(void);
 
 static void setup_local_irq(unsigned int irq, int type, int int_req);
-static void end_irq(unsigned int irq_nr);
 static inline void mask_and_ack_level_irq(unsigned int irq_nr);
 static inline void mask_and_ack_rise_edge_irq(unsigned int irq_nr);
 static inline void mask_and_ack_fall_edge_irq(unsigned int irq_nr);
@@ -111,7 +109,7 @@ inline void local_disable_irq(unsigned i
 }
 
 
-static inline void mask_and_ack_rise_edge_irq(unsigned int irq_nr)
+static void mask_and_ack_rise_edge_irq(unsigned int irq_nr)
 {
 	if (irq_nr > AU1000_LAST_INTC0_INT) {
 		au_writel(1<<(irq_nr-32), IC1_RISINGCLR);
@@ -125,7 +123,7 @@ static inline void mask_and_ack_rise_edg
 }
 
 
-static inline void mask_and_ack_fall_edge_irq(unsigned int irq_nr)
+static void mask_and_ack_fall_edge_irq(unsigned int irq_nr)
 {
 	if (irq_nr > AU1000_LAST_INTC0_INT) {
 		au_writel(1<<(irq_nr-32), IC1_FALLINGCLR);
@@ -139,7 +137,7 @@ static inline void mask_and_ack_fall_edg
 }
 
 
-static inline void mask_and_ack_either_edge_irq(unsigned int irq_nr)
+static void mask_and_ack_either_edge_irq(unsigned int irq_nr)
 {
 	/* This may assume that we don't get interrupts from
 	 * both edges at once, or if we do, that we don't care.
@@ -158,7 +156,7 @@ static inline void mask_and_ack_either_e
 }
 
 
-static inline void mask_and_ack_level_irq(unsigned int irq_nr)
+static void mask_and_ack_level_irq(unsigned int irq_nr)
 {
 
 	local_disable_irq(irq_nr);
@@ -172,20 +170,6 @@ static inline void mask_and_ack_level_ir
 	return;
 }
 
-
-static void end_irq(unsigned int irq_nr)
-{
-	if (!(irq_desc[irq_nr].status & (IRQ_DISABLED|IRQ_INPROGRESS))) {
-		local_enable_irq(irq_nr);
-	}
-#if defined(CONFIG_MIPS_PB1000)
-	if (irq_nr == AU1000_GPIO_15) {
-		au_writel(0x4000, PB1000_MDR); /* enable int */
-		au_sync();
-	}
-#endif
-}
-
 unsigned long save_local_and_disable(int controller)
 {
 	int i;
@@ -233,39 +217,31 @@ void restore_local_and_enable(int contro
 
 
 static struct irq_chip rise_edge_irq_type = {
-	.typename = "Au1000 Rise Edge",
-	.ack = mask_and_ack_rise_edge_irq,
+	.name = "Au1000",
 	.mask = local_disable_irq,
 	.mask_ack = mask_and_ack_rise_edge_irq,
 	.unmask = local_enable_irq,
-	.end = end_irq,
 };
 
 static struct irq_chip fall_edge_irq_type = {
-	.typename = "Au1000 Fall Edge",
-	.ack = mask_and_ack_fall_edge_irq,
+	.name = "Au1000",
 	.mask = local_disable_irq,
 	.mask_ack = mask_and_ack_fall_edge_irq,
 	.unmask = local_enable_irq,
-	.end = end_irq,
 };
 
 static struct irq_chip either_edge_irq_type = {
-	.typename = "Au1000 Rise or Fall Edge",
-	.ack = mask_and_ack_either_edge_irq,
+	.name = "Au1000",
 	.mask = local_disable_irq,
 	.mask_ack = mask_and_ack_either_edge_irq,
 	.unmask = local_enable_irq,
-	.end = end_irq,
 };
 
 static struct irq_chip level_irq_type = {
-	.typename = "Au1000 Level",
-	.ack = mask_and_ack_level_irq,
+	.name = "Au1000",
 	.mask = local_disable_irq,
 	.mask_ack = mask_and_ack_level_irq,
 	.unmask = local_enable_irq,
-	.end = end_irq,
 };
 
 #ifdef CONFIG_PM
@@ -309,31 +285,31 @@ static void setup_local_irq(unsigned int
 				au_writel(1<<(irq_nr-32), IC1_CFG2CLR);
 				au_writel(1<<(irq_nr-32), IC1_CFG1CLR);
 				au_writel(1<<(irq_nr-32), IC1_CFG0SET);
-				set_irq_chip(irq_nr, &rise_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &rise_edge_irq_type, handle_edge_irq, "riseedge");
 				break;
 			case INTC_INT_FALL_EDGE: /* 0:1:0 */
 				au_writel(1<<(irq_nr-32), IC1_CFG2CLR);
 				au_writel(1<<(irq_nr-32), IC1_CFG1SET);
 				au_writel(1<<(irq_nr-32), IC1_CFG0CLR);
-				set_irq_chip(irq_nr, &fall_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &fall_edge_irq_type, handle_edge_irq, "falledge");
 				break;
 			case INTC_INT_RISE_AND_FALL_EDGE: /* 0:1:1 */
 				au_writel(1<<(irq_nr-32), IC1_CFG2CLR);
 				au_writel(1<<(irq_nr-32), IC1_CFG1SET);
 				au_writel(1<<(irq_nr-32), IC1_CFG0SET);
-				set_irq_chip(irq_nr, &either_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &either_edge_irq_type, handle_edge_irq, "bothedge");
 				break;
 			case INTC_INT_HIGH_LEVEL: /* 1:0:1 */
 				au_writel(1<<(irq_nr-32), IC1_CFG2SET);
 				au_writel(1<<(irq_nr-32), IC1_CFG1CLR);
 				au_writel(1<<(irq_nr-32), IC1_CFG0SET);
-				set_irq_chip(irq_nr, &level_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &level_irq_type, handle_level_irq, "highlevel");
 				break;
 			case INTC_INT_LOW_LEVEL: /* 1:1:0 */
 				au_writel(1<<(irq_nr-32), IC1_CFG2SET);
 				au_writel(1<<(irq_nr-32), IC1_CFG1SET);
 				au_writel(1<<(irq_nr-32), IC1_CFG0CLR);
-				set_irq_chip(irq_nr, &level_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &level_irq_type, handle_level_irq, "lowlevel");
 				break;
 			case INTC_INT_DISABLED: /* 0:0:0 */
 				au_writel(1<<(irq_nr-32), IC1_CFG0CLR);
@@ -361,31 +337,31 @@ static void setup_local_irq(unsigned int
 				au_writel(1<<irq_nr, IC0_CFG2CLR);
 				au_writel(1<<irq_nr, IC0_CFG1CLR);
 				au_writel(1<<irq_nr, IC0_CFG0SET);
-				set_irq_chip(irq_nr, &rise_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &rise_edge_irq_type, handle_edge_irq, "riseedge");
 				break;
 			case INTC_INT_FALL_EDGE: /* 0:1:0 */
 				au_writel(1<<irq_nr, IC0_CFG2CLR);
 				au_writel(1<<irq_nr, IC0_CFG1SET);
 				au_writel(1<<irq_nr, IC0_CFG0CLR);
-				set_irq_chip(irq_nr, &fall_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &fall_edge_irq_type, handle_edge_irq, "falledge");
 				break;
 			case INTC_INT_RISE_AND_FALL_EDGE: /* 0:1:1 */
 				au_writel(1<<irq_nr, IC0_CFG2CLR);
 				au_writel(1<<irq_nr, IC0_CFG1SET);
 				au_writel(1<<irq_nr, IC0_CFG0SET);
-				set_irq_chip(irq_nr, &either_edge_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &either_edge_irq_type, handle_edge_irq, "bothedge");
 				break;
 			case INTC_INT_HIGH_LEVEL: /* 1:0:1 */
 				au_writel(1<<irq_nr, IC0_CFG2SET);
 				au_writel(1<<irq_nr, IC0_CFG1CLR);
 				au_writel(1<<irq_nr, IC0_CFG0SET);
-				set_irq_chip(irq_nr, &level_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &level_irq_type, handle_level_irq, "highlevel");
 				break;
 			case INTC_INT_LOW_LEVEL: /* 1:1:0 */
 				au_writel(1<<irq_nr, IC0_CFG2SET);
 				au_writel(1<<irq_nr, IC0_CFG1SET);
 				au_writel(1<<irq_nr, IC0_CFG0CLR);
-				set_irq_chip(irq_nr, &level_irq_type);
+				set_irq_chip_and_handler_name(irq_nr, &level_irq_type, handle_level_irq, "lowlevel");
 				break;
 			case INTC_INT_DISABLED: /* 0:0:0 */
 				au_writel(1<<irq_nr, IC0_CFG0CLR);
diff -Naurp linux-2.6.20-rc7/arch/mips/kernel/irq.c linux-2.6.20-rc7-work/arch/mips/kernel/irq.c
--- linux-2.6.20-rc7/arch/mips/kernel/irq.c	2007-02-01 15:04:35.831983000 +0100
+++ linux-2.6.20-rc7-work/arch/mips/kernel/irq.c	2007-02-02 11:15:34.201983000 +0100
@@ -118,6 +118,7 @@ int show_interrupts(struct seq_file *p, 
 			seq_printf(p, "%10u ", kstat_cpu(j).irqs[i]);
 #endif
 		seq_printf(p, " %14s", irq_desc[i].chip->name);
+		seq_printf(p, "-%-8s", irq_desc[i].name);
 		seq_printf(p, "  %s", action->name);
 
 		for (action=action->next; action; action = action->next)

Re: 2.6.20-rc: au1x irqs broken

[Ralf Baechle]

On Fri, Feb 02, 2007 at 07:13:56AM +0100, Manuel Lauss wrote:

> mips-git commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 broke

There is no commit 1603b5aca14f15b34848fb5594d0c7b6333b99144 ...

  Ralf

Re: 2.6.20-rc: au1x irqs broken

[Atsushi Nemoto]

On Fri, 2 Feb 2007 10:58:14 +0100, Manuel Lauss <mano@roarinelk.homelinux.net> wrote:
> Here's what tripped me up. I switched au1x over to use the kernel
> flow handlers, and forgot to undo all of it.
> 
> rediffed against 2.6.20-rc7
> 
> diff -Naurp linux-2.6.20-rc7/arch/mips/au1000/common/irq.c linux-2.6.20-rc7-work/arch/mips/au1000/common/irq.c
...
> @@ -172,20 +170,6 @@ static inline void mask_and_ack_level_ir
>  	return;
>  }
>  
> -
> -static void end_irq(unsigned int irq_nr)
> -{
> -	if (!(irq_desc[irq_nr].status & (IRQ_DISABLED|IRQ_INPROGRESS))) {
> -		local_enable_irq(irq_nr);
> -	}
> -#if defined(CONFIG_MIPS_PB1000)
> -	if (irq_nr == AU1000_GPIO_15) {
> -		au_writel(0x4000, PB1000_MDR); /* enable int */
> -		au_sync();
> -	}
> -#endif
> -}
> -
>  unsigned long save_local_and_disable(int controller)
>  {
>  	int i;

You are to drop PB1000 support?

> @@ -233,39 +217,31 @@ void restore_local_and_enable(int contro
>  
>  
>  static struct irq_chip rise_edge_irq_type = {
> -	.typename = "Au1000 Rise Edge",
> -	.ack = mask_and_ack_rise_edge_irq,
> +	.name = "Au1000",
>  	.mask = local_disable_irq,
>  	.mask_ack = mask_and_ack_rise_edge_irq,
>  	.unmask = local_enable_irq,
> -	.end = end_irq,
>  };
>  
>  static struct irq_chip fall_edge_irq_type = {
> -	.typename = "Au1000 Fall Edge",
> -	.ack = mask_and_ack_fall_edge_irq,
> +	.name = "Au1000",
>  	.mask = local_disable_irq,
>  	.mask_ack = mask_and_ack_fall_edge_irq,
>  	.unmask = local_enable_irq,
> -	.end = end_irq,
>  };
>  
>  static struct irq_chip either_edge_irq_type = {
> -	.typename = "Au1000 Rise or Fall Edge",
> -	.ack = mask_and_ack_either_edge_irq,
> +	.name = "Au1000",
>  	.mask = local_disable_irq,
>  	.mask_ack = mask_and_ack_either_edge_irq,
>  	.unmask = local_enable_irq,
> -	.end = end_irq,
>  };
>  
>  static struct irq_chip level_irq_type = {
> -	.typename = "Au1000 Level",
> -	.ack = mask_and_ack_level_irq,
> +	.name = "Au1000",
>  	.mask = local_disable_irq,
>  	.mask_ack = mask_and_ack_level_irq,
>  	.unmask = local_enable_irq,
> -	.end = end_irq,
>  };
>  
>  #ifdef CONFIG_PM

.ack entries are required for handle_edge_irq.  And if you wanted to
unregister an irq handler by set_irq_handler(irq, NULL), .ack will be
used even for level flow handler.

Also note that typename to name conversion patch is already in queue
tree.

> diff -Naurp linux-2.6.20-rc7/arch/mips/kernel/irq.c linux-2.6.20-rc7-work/arch/mips/kernel/irq.c
> --- linux-2.6.20-rc7/arch/mips/kernel/irq.c	2007-02-01 15:04:35.831983000 +0100
> +++ linux-2.6.20-rc7-work/arch/mips/kernel/irq.c	2007-02-02 11:15:34.201983000 +0100
> @@ -118,6 +118,7 @@ int show_interrupts(struct seq_file *p, 
>  			seq_printf(p, "%10u ", kstat_cpu(j).irqs[i]);
>  #endif
>  		seq_printf(p, " %14s", irq_desc[i].chip->name);
> +		seq_printf(p, "-%-8s", irq_desc[i].name);
>  		seq_printf(p, "  %s", action->name);
>  
>  		for (action=action->next; action; action = action->next)
> 

irq_desc[i].name might be NULL.

---
Atsushi Nemoto

Re: 2.6.20-rc: au1x irqs broken

[Manuel Lauss]

Atsushi Nemoto wrote:
> You are to drop PB1000 support?

I won't submit this patch for inclusion, ever. It was just a test.

> .ack entries are required for handle_edge_irq.  And if you wanted to
> unregister an irq handler by set_irq_handler(irq, NULL), .ack will be
> used even for level flow handler.

Good to know, Thanks.

-- 
  ml.