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?

A very thoughtful workup - I'll apply these changes.  I disagree
with the need for the interrupt masking - interrupts will be masked
by the ISR/DSR process and these extra fiddles accomplish nothing
and would just slow the code down.

As for the official process for submitting patches - generate a patch
by running "cvs diff"(*) and then send the results (as an attachment to
avoid mailer problems) to ecos-patches@ecos.sourceware.org  We'll
review the changes and work with you to get them applied.  Depending
on the magnitude of the changes, we may need a copyright assignment.
Full details are on the web site (look for contributing...)

(*) Please set your CVS preferences to use "context diffs".  This can
be done by adding the line:
  diff -u5 -p -N
to your ~/.cvsrc file.

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


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss