Re: 5.2-RC oerrs and collisions on dc0

On  3 Jan, Don Lewis wrote:

> I don't think the following code fragment in dc_txeof() is quite
> correct.
> 
>                 if (txstat & DC_TXSTAT_ERRSUM) {
>                         ifp->if_oerrors++;
>                         if (txstat & DC_TXSTAT_EXCESSCOLL)
>                                 ifp->if_collisions++;
>                         if (txstat & DC_TXSTAT_LATECOLL)
>                                 ifp->if_collisions++;
>                         if (!(txstat & DC_TXSTAT_UNDERRUN)) {  
>                                 dc_init(sc);
>                                 return;
>                         }
>                 }
>                     
>                 ifp->if_collisions += (txstat & DC_TXSTAT_COLLCNT) >> 3;
> 
> I don't happen to have a copy of the 21143 documentation, but my crufty
> old 21140A documentation says that the collision count field is valid if
> the late collision bit is set, but not if the excess collision count bit
> is set.  The documentation can't make up its mind as to whether the late
> collision bit is valid if the underrun bit is set.
> 
> It looks like the code should be:
> 
>                 if (txstat & DC_TXSTAT_ERRSUM) {
>                         ifp->if_oerrors++;
>                         if (!(txstat & DC_TXSTAT_UNDERRUN)) {  
>                                 dc_init(sc);
>                                 return;
>                         }
>                 }
> 
> 		ifp->if_collisions += (txstat & DC_TXSTAT_EXCESSCOLL) ?
>                         16 : (txstat & DC_TXSTAT_COLLCNT) >> 3);

I think this code fragment is wrong too:

                if (DC_IS_XIRCOM(sc) || DC_IS_CONEXANT(sc)) {
                        /*
                         * XXX: Why does my Xircom taunt me so?
                         * For some reason it likes setting the CARRLOST flag
                         * even when the carrier is there. wtf?!?
                         * Who knows, but Conexant chips have the
                         * same problem. Maybe they took lessons
                         * from Xircom.
                         */
                        if (/*sc->dc_type == DC_TYPE_21143 &&*/
                            sc->dc_pmode == DC_PMODE_MII &&
                            ((txstat & 0xFFFF) & ~(DC_TXSTAT_ERRSUM |
                            DC_TXSTAT_NOCARRIER)))
                                txstat &= ~DC_TXSTAT_ERRSUM;
                } else {
                        if (/*sc->dc_type == DC_TYPE_21143 &&*/
                            sc->dc_pmode == DC_PMODE_MII &&
                            ((txstat & 0xFFFF) & ~(DC_TXSTAT_ERRSUM |
                            DC_TXSTAT_NOCARRIER | DC_TXSTAT_CARRLOST)))
                                txstat &= ~DC_TXSTAT_ERRSUM;
                }

It looks like the intent is to clear the error summary bit if the only
reason for it being set is that the no carrier or carrier lost bits are
set.  The collision count bits and the deferred bit are in the lower 16
bits of txstat so the error summary bit will not be cleared if the
collision count is non-zero.  There are also some undefined bits in the
lower 16.  Maybe something like the following condition would be better:

	(txstat & (DC_TXSTAT_JABTIMEO | DC_TXSTAT_LATECOLL |
	DC_TXSTAT_EXCESSCOLL | DC_TXSTAT_UNDERRUN)) == 0

also including DC_TXSTAT_CARRLOST in the first "if" clause.

This shouldn't affect the full duplex case, since the collision counter
and defered bit should not get set in full duplex mode.