440GX interrupt

440GX interrupt

[Barbier, Renaud (GE Infrastructure)]

I have a question regarding interrupt and irq locking.
I derived  (or copied from somehwere)a library (linux 2.4.26) for the =
440GX from ppc4xx_pic.c to take care of the last interrupt register =
(UIC2).
This a newbie question regarding get_irq/spin_lock
here is get_irq:
...
     bits =3D mfdcr(DCRN_UIC_MSR(UICBASE));
     if ((bits & 0x40000000) =3D=3D 0x40000000)
     {
        bits =3D mfdcr(DCRN_UIC_MSR(UIC0));
        irq =3D ( ffs(bits));
        irq =3D 32-irq;
	}
	...

my question is what guarantee that the code is executed atomically?

The reason I asked is that we have a driver that did the following in =
the ioctl call:

	disable_irq(26);
	/* do something */
	enable_irq(26);

as you noticed there is not any spin_lock.
Sometimes, this leads  get_irq to see UICBASE indicating an irq in UIC0 =
and
UIC0_MSR to return 0. hence you get irq 32 (MAL_SERR) and an infinite =
loop.

My current fix is to use irqsave/irqrestore in the driver which I think =
is the correct way to do (but I may be wrong please help).


However, I have a colleague (here is the human problem of my questions: =
him or me is the problem) that insists that I should do something in =
get_irq to have atomic execution.

can you share your view about get_irq and spin_lock?

If it is not the correct place to ask this question, let me know where =
to send it.

Re: 440GX interrupt

[Eugene Surovegin]

On Thu, Feb 17, 2005 at 03:50:27PM -0600, Barbier, Renaud (GE Infrastructure) wrote:
> I have a question regarding interrupt and irq locking.
> I derived  (or copied from somehwere)a library (linux 2.4.26) for the 440GX from ppc4xx_pic.c to take care of the last interrupt register (UIC2).
> This a newbie question regarding get_irq/spin_lock
> here is get_irq:
> ...
>      bits = mfdcr(DCRN_UIC_MSR(UICBASE));
>      if ((bits & 0x40000000) == 0x40000000)
>      {
>         bits = mfdcr(DCRN_UIC_MSR(UIC0));
>         irq = ( ffs(bits));
>         irq = 32-irq;
> 	}
> 	...
> 
> my question is what guarantee that the code is executed atomically?

ppc_md.get_irq is called with hard irqs disabled, this makes 
its execution context atomic.

> The reason I asked is that we have a driver that did the following in the ioctl call:
> 
> 	disable_irq(26);
> 	/* do something */
> 	enable_irq(26);
> 
> as you noticed there is not any spin_lock.
> Sometimes, this leads  get_irq to see UICBASE indicating an irq in UIC0 and
> UIC0_MSR to return 0. hence you get irq 32 (MAL_SERR) and an infinite loop.

> My current fix is to use irqsave/irqrestore in the driver which I 
> think is the correct way to do (but I may be wrong please help).

Yes, this is preferable to disable/enable_irq.

> However, I have a colleague (here is the human problem of my 
> questions: him or me is the problem) that insists that I should do 
> something in get_irq to have atomic execution.

No, it's already atomic. 

Probably it's a race which cannot be avoided anyway because external 
IRQs are completely async, and your version of ppc4xx_pic.c just has a 
bug. I'll think about it a little more.

Could you try 2.6 version of ppc_4xx_pic.c? I don't think 2.4 has any 
official support for UIC2 anyway.

--
Eugene.

Re: 440GX interrupt

[Eugene Surovegin]

On Thu, Feb 17, 2005 at 02:54:14PM -0800, Eugene Surovegin wrote:
> Probably it's a race which cannot be avoided anyway because external 
> IRQs are completely async, and your version of ppc4xx_pic.c just has a 
> bug. I'll think about it a little more.

Uhh, yes, I think it's a bug in 4xx version of disable_irq.

We have to ACK parent UIC after disabling IRQ to prevent false 
triggering in case this IRQ was already pending during disable_irq 
call.

Here is a patch against current 2.6, so you can get an idea what I'm 
talking about :):

===== arch/ppc/syslib/ppc4xx_pic.c 1.13 vs edited =====
--- 1.13/arch/ppc/syslib/ppc4xx_pic.c	2005-01-03 15:49:19 -08:00
+++ edited/arch/ppc/syslib/ppc4xx_pic.c	2005-02-17 15:31:07 -08:00
@@ -48,6 +48,7 @@
 {									\
 	ppc_cached_irq_mask[n] &= ~IRQ_MASK_UIC##n(irq);		\
 	mtdcr(DCRN_UIC_ER(UIC##n), ppc_cached_irq_mask[n]);		\
+	ACK_UIC##n##_PARENT						\
 }									\
 									\
 static void ppc4xx_uic##n##_ack(unsigned int irq)			\


You can send me your version of ppc4xx_pic.c or put it somewhere on 
www and I'll make a patch for it.

--
Eugene

Re: 440GX interrupt

[Matt Porter]

On Thu, Feb 17, 2005 at 03:36:05PM -0800, Eugene Surovegin wrote:
> On Thu, Feb 17, 2005 at 02:54:14PM -0800, Eugene Surovegin wrote:
> > Probably it's a race which cannot be avoided anyway because external 
> > IRQs are completely async, and your version of ppc4xx_pic.c just has a 
> > bug. I'll think about it a little more.
> 
> Uhh, yes, I think it's a bug in 4xx version of disable_irq.
> 
> We have to ACK parent UIC after disabling IRQ to prevent false 
> triggering in case this IRQ was already pending during disable_irq 
> call.
> 
> Here is a patch against current 2.6, so you can get an idea what I'm 
> talking about :):

Gah, yes. Please add the sign off line and it can go in the mainline.

-Matt