Re: [ECOS] Bug (and fix) for FEC driver PowerPC FEC

[Gary Thomas <gary at mlbassoc dot com>]

On Wed, 2005-06-22 at 16:04 -0500, Fernando Flores wrote:
> Hello,
> 
>   We have a Device with a Motorola PowerPC 855 w/ a FEC.  We noticed
> that under heavy traffic loads, the network will stop receiving
> packets.  Further investigation pointed to having all the RX buffer
> descriptors being full, but the processor stop sending interrupts. 
> Upon examining the fec code under
> '/ecos/packages/devs/eth/powerpc/fec/<version>/src/if_fec.c, we
> noticed that the interrupt handler code was written as:
> 
> //
> // Interrupt processing
> //
> static void          
> fec_eth_int(struct eth_drv_sc *sc)
> {
>     struct fec_eth_info *qi = (struct fec_eth_info *)sc->driver_private;
>     unsigned long event;
> 
>     while ((event = qi->fec->iEvent) != 0) {
>         if ((event & iEvent_TFINT) != 0) {
>             fec_eth_TxEvent(sc);
>         }
>         if ((event & iEvent_RFINT) != 0) {
>             fec_eth_RxEvent(sc);
>         }
>         qi->fec->iEvent = event;  // Reset the bits we handled
>     }
> }
> 
> 
> The problem here is that we first clear out the RX buffers, then
> handle the TX buffers.  But the FEC may receive more packets and fill
> up all buffers before we reset the bits in the event register.  We
> then clear the event register but never empty the buffers when we loop
> back around.  The processor believes we have acknowledged the event
> for the newly arrived packets and never sets the interrupt.
> 
> This can be easily fixed by changing the order of the reset to the following:
> //
> // Interrupt processing
> //
> static void          
> fec_eth_int(struct eth_drv_sc *sc)
> {
>     struct fec_eth_info *qi = (struct fec_eth_info *)sc->driver_private;
>     unsigned long event;
> 
>     while ((event = qi->fec->iEvent) != 0) {
> 
>         qi->fec->iEvent = event;  // Reset the bits we will handle
> 
>         if ((event & iEvent_TFINT) != 0) {
>             fec_eth_TxEvent(sc);
>         }
>         if ((event & iEvent_RFINT) != 0) {
>             fec_eth_RxEvent(sc);
>         }
> 
>     }
> }
> 
> We have tested this with and no longer have the network hang-up
> problems after flooding it with packets.  The code is located in
> /ecos/packages/devs/eth/powerpc/fec/<version>/src/if_fec.c  I hope
> this helps anyone having similar issues.
> 
> To make this routine a bit better, it is a good idea to disable the
> FEC interrupts in the interrupt handler:
> 
> //
> // Interrupt processing
> //
> static void          
> fec_eth_int(struct eth_drv_sc *sc)
> {
>     struct fec_eth_info *qi = (struct fec_eth_info *)sc->driver_private;
>     unsigned long event;
>     unsigned long mask = qi->fec->iMask;
> 
>     // Disable all interrupts
>     qi->fec->iMask = 0x0000000;
> 
>     while ((event = qi->fec->iEvent) != 0) {
> 
>         qi->fec->iEvent = event;  // Reset the bits we will handled
> 
>         if ((event & iEvent_TFINT) != 0) {
>             fec_eth_TxEvent(sc);
>         }
>         if ((event & iEvent_RFINT) != 0) {
>             fec_eth_RxEvent(sc);
>         }
> 
>     }
>     // Re-enable Interrupts
>     qi->fec->iMask = mask;    // enable interrupts
> }
> 
> Is there a formal process to submit these changes to the official eCos
> distribution?
> 
> Thanks,
> 
> Fernando Flores

Attached patch applied, thanks.

-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------

Index: devs/eth/powerpc/fec/current/ChangeLog
===================================================================
RCS file: /misc/cvsfiles/ecos/packages/devs/eth/powerpc/fec/current/ChangeLog,v
retrieving revision 1.22
diff -u -5 -p -r1.22 ChangeLog
--- devs/eth/powerpc/fec/current/ChangeLog	13 Oct 2004 20:12:23 -0000	1.22
+++ devs/eth/powerpc/fec/current/ChangeLog	22 Jun 2005 23:23:40 -0000
@@ -1,5 +1,10 @@
+2005-06-22  Gary Thomas  <gary@mlbassoc.com>
+
+	* src/if_fec.c (fec_eth_int): Avoid race when handling interrupt
+	events - reported by Fernando Flores 
+
 2004-10-11  Harald Kuethe  <hkuethe@controlware.de>
 
        * src/if_fec.c (fec_eth_RxEvent): remove crc from packet.
 
 2003-12-21  Gary Thomas  <gary@mlbassoc.com>
Index: devs/eth/powerpc/fec/current/src/if_fec.c
===================================================================
RCS file: /misc/cvsfiles/ecos/packages/devs/eth/powerpc/fec/current/src/if_fec.c,v
retrieving revision 1.19
diff -u -5 -p -r1.19 if_fec.c
--- devs/eth/powerpc/fec/current/src/if_fec.c	13 Oct 2004 20:12:24 -0000	1.19
+++ devs/eth/powerpc/fec/current/src/if_fec.c	22 Jun 2005 23:22:16 -0000
@@ -761,17 +761,17 @@ fec_eth_int(struct eth_drv_sc *sc)
 {
     struct fec_eth_info *qi = (struct fec_eth_info *)sc->driver_private;
     unsigned long event;
 
     while ((event = qi->fec->iEvent) != 0) {
+        qi->fec->iEvent = event;  // Reset the bits we handled
         if ((event & iEvent_TFINT) != 0) {
             fec_eth_TxEvent(sc);
         }
         if ((event & iEvent_RFINT) != 0) {
             fec_eth_RxEvent(sc);
         }
-        qi->fec->iEvent = event;  // Reset the bits we handled
     }
 }
 
 //
 // Interrupt vector