Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 5 Jan 2004 21:53:21 +0100
From:      Maxime Henrion <mux@freebsd.org>
To:        Don Lewis <truckman@FreeBSD.org>
Cc:        current@FreeBSD.org
Subject:   Re: 5.2-RC oerrs and collisions on dc0
Message-ID:  <20040105205321.GN2060@elvis.mu.org>
In-Reply-To: <200401052007.i05K7K7E012100@gw.catspoiler.org>
References:  <20040105183445.GK2060@elvis.mu.org> <200401052007.i05K7K7E012100@gw.catspoiler.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Don Lewis wrote:
> On  5 Jan, Maxime Henrion wrote:
> > Don Lewis wrote:
> >> I just took a closer look at the busdma diff, and this change to
> >> dc_txeof() looks very suspicious:
> >> 
> >> @@ -2663,7 +2809,7 @@
> >>                 if (txstat & DC_TXSTAT_OWN)
> >>                         break;
> >>  
> >> -               if (!(cur_tx->dc_ctl & DC_TXCTL_LASTFRAG) ||
> >> +               if (!(cur_tx->dc_ctl & DC_TXCTL_FIRSTFRAG) ||
> >>                     cur_tx->dc_ctl & DC_TXCTL_SETUP) {
> >>                         if (cur_tx->dc_ctl & DC_TXCTL_SETUP) {
> >>                                 /*
> >> 
> >> The code in the "if" block ends with a "continue" which will cause the
> >> error handling code to be skipped if the "if" condition is true.  I'm
> >> pretty sure that the error status bits are only set in the last
> >> descriptor for the frame, so we want to execute the "continue" unless
> >> the DC_TXCTL_LASTFRAG bit is set.
> >> 
> >> Try reverting this part of the busdma change.
> > 
> > I think you actually found the culprit, though fixing this is not as
> > simple as reverting this line.  I changed the check to
> > DC_TXCTL_FIRSTFRAG because that's the descriptor which will have a link
> > to the mbuf, while it was previously done for the LASTFRAG descriptor.
> > However, I didn't think about the code flow enough it seems.  If you
> > don't have a fix for this soon, I should be able to do it pretty easily,
> > assuming I find the time.
> 
> How about something like the following.  I can't test it because I don't
> have the appropriate hardware, but at least it compiles ;-)
> 
> Index: sys/pci/if_dc.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/pci/if_dc.c,v
> retrieving revision 1.137
> diff -u -r1.137 if_dc.c
> --- sys/pci/if_dc.c	6 Dec 2003 02:29:31 -0000	1.137
> +++ sys/pci/if_dc.c	5 Jan 2004 20:00:04 -0000
> @@ -2859,7 +2859,7 @@
>  		if (txstat & DC_TXSTAT_OWN)
>  			break;
>  
> -		if (!(ctl & DC_TXCTL_FIRSTFRAG) || ctl & DC_TXCTL_SETUP) {
> +		if (!(ctl & DC_TXCTL_LASTFRAG) || ctl & DC_TXCTL_SETUP) {
>  			if (ctl & DC_TXCTL_SETUP) {
>  				/*
>  				 * Yes, the PNIC is so brain damaged
> @@ -3262,6 +3262,7 @@
>  	sc->dc_cdata.dc_tx_prod = frag;
>  	sc->dc_cdata.dc_tx_cnt += nseg;
>  	sc->dc_ldata->dc_tx_list[cur].dc_ctl |= htole32(DC_TXCTL_LASTFRAG);
> +	sc->dc_cdata.dc_tx_chain[cur] = sc->dc_cdata.dc_tx_mapping;
>  	if (sc->dc_flags & DC_TX_INTR_FIRSTFRAG)
>  		sc->dc_ldata->dc_tx_list[first].dc_ctl |=
>  		    htole32(DC_TXCTL_FINT);
> @@ -3311,13 +3312,13 @@
>  	 * of fragments or hit the end of the mbuf chain.
>  	 */
>  	idx = sc->dc_cdata.dc_tx_prod;
> +	sc->dc_cdata.dc_tx_mapping = *m_head;
>  	error = bus_dmamap_load_mbuf(sc->dc_mtag, sc->dc_cdata.dc_tx_map[idx],
>  	    *m_head, dc_dma_map_txbuf, sc, 0);
>  	if (error)
>  		return (error);
>  	if (sc->dc_cdata.dc_tx_err != 0)
>  		return (sc->dc_cdata.dc_tx_err); 
> -	sc->dc_cdata.dc_tx_chain[idx] = *m_head;
>  	bus_dmamap_sync(sc->dc_mtag, sc->dc_cdata.dc_tx_map[idx],
>  	    BUS_DMASYNC_PREWRITE);
>  	bus_dmamap_sync(sc->dc_ltag, sc->dc_lmap,
> Index: sys/pci/if_dcreg.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/pci/if_dcreg.h,v
> retrieving revision 1.40
> diff -u -r1.40 if_dcreg.h
> --- sys/pci/if_dcreg.h	6 Dec 2003 02:29:31 -0000	1.40
> +++ sys/pci/if_dcreg.h	5 Jan 2004 19:58:05 -0000
> @@ -486,6 +486,7 @@
>  struct dc_chain_data {
>  	struct mbuf		*dc_rx_chain[DC_RX_LIST_CNT];
>  	struct mbuf		*dc_tx_chain[DC_TX_LIST_CNT];
> +	struct mbuf		*dc_tx_mapping;
>  	bus_dmamap_t		dc_rx_map[DC_RX_LIST_CNT];
>  	bus_dmamap_t		dc_tx_map[DC_TX_LIST_CNT];
>  	u_int32_t		*dc_sbuf;

Looks mostly good, but you missed some occurences of DC_TXCTL_FIRSTFRAG
that need to be changed.

Cheers,
Maxime



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040105205321.GN2060>