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

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

[Fernando Flores]

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

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

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

[Gary Thomas]

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

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

[Fernando Flores]

Gary,

  Thanks for applying the patch and for your explaination of why we
don't need the FEC masking.  I concur that the ISR/DSR will handle it
and FEC masking is not needed.

  Fernando Flores

On 6/22/05, Gary Thomas <gary@mlbassoc.com> wrote:
> 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