From owner-freebsd-bugs@FreeBSD.ORG Wed Jul 25 22:00:09 2007 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2685B16A41B for ; Wed, 25 Jul 2007 22:00:09 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 0237513C46E for ; Wed, 25 Jul 2007 22:00:09 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.1/8.14.1) with ESMTP id l6PM081A034184 for ; Wed, 25 Jul 2007 22:00:08 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.1/8.14.1/Submit) id l6PM08V4034183; Wed, 25 Jul 2007 22:00:08 GMT (envelope-from gnats) Resent-Date: Wed, 25 Jul 2007 22:00:08 GMT Resent-Message-Id: <200707252200.l6PM08V4034183@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Till Straumann Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3FA6216A418 for ; Wed, 25 Jul 2007 21:53:05 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (unknown [IPv6:2001:4f8:fff6::21]) by mx1.freebsd.org (Postfix) with ESMTP id 2996813C442 for ; Wed, 25 Jul 2007 21:53:05 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.14.1/8.14.1) with ESMTP id l6PLr4mJ079115 for ; Wed, 25 Jul 2007 21:53:04 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.14.1/8.14.1/Submit) id l6PLr4J1079114; Wed, 25 Jul 2007 21:53:04 GMT (envelope-from nobody) Message-Id: <200707252153.l6PLr4J1079114@www.freebsd.org> Date: Wed, 25 Jul 2007 21:53:04 GMT From: Till Straumann To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-3.0 Cc: Subject: kern/114915: pcn (sys/pci/if_pcn.c) ethernet driver fixes (including endianness) X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Jul 2007 22:00:09 -0000 >Number: 114915 >Category: kern >Synopsis: pcn (sys/pci/if_pcn.c) ethernet driver fixes (including endianness) >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Jul 25 22:00:08 GMT 2007 >Closed-Date: >Last-Modified: >Originator: Till Straumann >Release: cvs head (as of 20070717) >Organization: SLAC >Environment: I am not running FreeBSD but found the problems described below when porting the pcn driver to RTEMS. I believe the issues are relevant to FreeBSD, too, so I report them here. >Description: a) The if_pcn driver is not endian-safe. It won't work on a big-endian platform. b) The driver accesses a field in a RX descriptor after handing the descriptor over to the pcn chip which may alter the value before the CPU access happens. Unless the ring is completely filled this is unlikely to cause problems but IMO it should nevertheless be fixed. Also, IMO, descriptor fields should be 'volatile'. c) No attempts are made to synchronize access to descriptors in memory -- reordering of accesses by the compiler and/or the CPU itself may cause problems (e.g., if on a platform that does out-of-order stores the OWN bit is effectively written before other fields in the descriptor are). The attached patch addresses a-c. I don't know how to properly address c] in FreeBSD (probably, descriptors should be allocated using bus_space_alloc() rather than contigmalloc() and synchronized with bus_space_barrier() ?). Anyhow, the places where synchronization would be prudent can be located in the patch by looking for #ifdef __rtems__ membarrier_xy(); #endif constructs. In addition to using synchronization, relevant fields in the descriptors have been declared 'volatile'. >How-To-Repeat: >Fix: See attached patch. Patch attached with submission follows: *** if_pcn.c.orig 2007-07-17 16:55:23.000000000 -0700 --- if_pcn.c 2007-07-25 14:21:18.000000000 -0700 *************** *** 607,612 **** --- 607,618 ---- eaddr[0] = CSR_READ_4(sc, PCN_IO32_APROM00); eaddr[1] = CSR_READ_4(sc, PCN_IO32_APROM01); + /* if we are on a big endian host, read did swap the byte order + * and we need to swap back. On a LE host, the following is a noop + */ + eaddr[0] = htole32(eaddr[0]); + eaddr[1] = htole32(eaddr[1]); + callout_init_mtx(&sc->pcn_stat_callout, &sc->pcn_mtx, 0); sc->pcn_ldata = contigmalloc(sizeof(struct pcn_list_data), M_DEVBUF, *************** *** 753,761 **** for (i = 0; i < PCN_TX_LIST_CNT; i++) { cd->pcn_tx_chain[i] = NULL; ! ld->pcn_tx_list[i].pcn_tbaddr = 0; ! ld->pcn_tx_list[i].pcn_txctl = 0; ! ld->pcn_tx_list[i].pcn_txstat = 0; } cd->pcn_tx_prod = cd->pcn_tx_cons = cd->pcn_tx_cnt = 0; --- 759,767 ---- for (i = 0; i < PCN_TX_LIST_CNT; i++) { cd->pcn_tx_chain[i] = NULL; ! ld->pcn_tx_list[i].pcn_tbaddr = htole32(0); ! ld->pcn_tx_list[i].pcn_txctl = htole32(0); ! ld->pcn_tx_list[i].pcn_txstat = htole32(0); } cd->pcn_tx_prod = cd->pcn_tx_cons = cd->pcn_tx_cnt = 0; *************** *** 820,829 **** m_adj(m_new, ETHER_ALIGN); sc->pcn_cdata.pcn_rx_chain[idx] = m_new; ! c->pcn_rbaddr = vtophys(mtod(m_new, caddr_t)); ! c->pcn_bufsz = (~(PCN_RXLEN) + 1) & PCN_RXLEN_BUFSZ; ! c->pcn_bufsz |= PCN_RXLEN_MBO; ! c->pcn_rxstat = PCN_RXSTAT_STP|PCN_RXSTAT_ENP|PCN_RXSTAT_OWN; return(0); } --- 826,838 ---- m_adj(m_new, ETHER_ALIGN); sc->pcn_cdata.pcn_rx_chain[idx] = m_new; ! c->pcn_rbaddr = htole32(vtophys(mtod(m_new, caddr_t))); ! c->pcn_bufsz = htole16((~(PCN_RXLEN) + 1) & PCN_RXLEN_BUFSZ); ! c->pcn_bufsz |= htole16(PCN_RXLEN_MBO); ! #ifdef __rtems__ ! membarrier_w(); ! #endif ! c->pcn_rxstat = htole16(PCN_RXSTAT_STP|PCN_RXSTAT_ENP|PCN_RXSTAT_OWN); return(0); } *************** *** 839,845 **** struct mbuf *m; struct ifnet *ifp; struct pcn_rx_desc *cur_rx; ! int i; PCN_LOCK_ASSERT(sc); --- 848,854 ---- struct mbuf *m; struct ifnet *ifp; struct pcn_rx_desc *cur_rx; ! int i,len; PCN_LOCK_ASSERT(sc); *************** *** 847,855 **** --- 856,868 ---- i = sc->pcn_cdata.pcn_rx_prod; while(PCN_OWN_RXDESC(&sc->pcn_ldata->pcn_rx_list[i])) { + #ifdef __rtems__ + membarrier_rw(); + #endif cur_rx = &sc->pcn_ldata->pcn_rx_list[i]; m = sc->pcn_cdata.pcn_rx_chain[i]; sc->pcn_cdata.pcn_rx_chain[i] = NULL; + len = le16toh(cur_rx->pcn_rxlen); /* * If an error occurs, update stats, clear the *************** *** 857,869 **** * it should simply get re-used next time this descriptor * comes up in the ring. */ ! if (cur_rx->pcn_rxstat & PCN_RXSTAT_ERR) { ifp->if_ierrors++; pcn_newbuf(sc, i, m); PCN_INC(i, PCN_RX_LIST_CNT); continue; } if (pcn_newbuf(sc, i, NULL)) { /* Ran out of mbufs; recycle this one. */ pcn_newbuf(sc, i, m); --- 870,885 ---- * it should simply get re-used next time this descriptor * comes up in the ring. */ ! if (le16toh(cur_rx->pcn_rxstat) & PCN_RXSTAT_ERR) { ifp->if_ierrors++; pcn_newbuf(sc, i, m); PCN_INC(i, PCN_RX_LIST_CNT); continue; } + /* MUST NOT use cur_rx descriptor beyond this point; + * pcn_newbuf() writes to it and hands it over to the chip. + */ if (pcn_newbuf(sc, i, NULL)) { /* Ran out of mbufs; recycle this one. */ pcn_newbuf(sc, i, m); *************** *** 877,883 **** /* No errors; receive the packet. */ ifp->if_ipackets++; m->m_len = m->m_pkthdr.len = ! cur_rx->pcn_rxlen - ETHER_CRC_LEN; m->m_pkthdr.rcvif = ifp; PCN_UNLOCK(sc); --- 893,899 ---- /* No errors; receive the packet. */ ifp->if_ipackets++; m->m_len = m->m_pkthdr.len = ! len - ETHER_CRC_LEN; m->m_pkthdr.rcvif = ifp; PCN_UNLOCK(sc); *************** *** 915,937 **** if (!PCN_OWN_TXDESC(cur_tx)) break; ! ! if (!(cur_tx->pcn_txctl & PCN_TXCTL_ENP)) { sc->pcn_cdata.pcn_tx_cnt--; PCN_INC(idx, PCN_TX_LIST_CNT); continue; } ! if (cur_tx->pcn_txctl & PCN_TXCTL_ERR) { ifp->if_oerrors++; ! if (cur_tx->pcn_txstat & PCN_TXSTAT_EXDEF) ifp->if_collisions++; ! if (cur_tx->pcn_txstat & PCN_TXSTAT_RTRY) ifp->if_collisions++; } ifp->if_collisions += ! cur_tx->pcn_txstat & PCN_TXSTAT_TRC; ifp->if_opackets++; if (sc->pcn_cdata.pcn_tx_chain[idx] != NULL) { --- 931,955 ---- if (!PCN_OWN_TXDESC(cur_tx)) break; ! #ifdef __rtems__ ! membarrier_rw(); ! #endif ! if (!(le32toh(cur_tx->pcn_txctl) & PCN_TXCTL_ENP)) { sc->pcn_cdata.pcn_tx_cnt--; PCN_INC(idx, PCN_TX_LIST_CNT); continue; } ! if (le32toh(cur_tx->pcn_txctl) & PCN_TXCTL_ERR) { ifp->if_oerrors++; ! if (le32toh(cur_tx->pcn_txstat) & PCN_TXSTAT_EXDEF) ifp->if_collisions++; ! if (le32toh(cur_tx->pcn_txstat) & PCN_TXSTAT_RTRY) ifp->if_collisions++; } ifp->if_collisions += ! le32toh(cur_tx->pcn_txstat) & PCN_TXSTAT_TRC; ifp->if_opackets++; if (sc->pcn_cdata.pcn_tx_chain[idx] != NULL) { *************** *** 1058,1070 **** if ((PCN_TX_LIST_CNT - (sc->pcn_cdata.pcn_tx_cnt + cnt)) < 2) return(ENOBUFS); f = &sc->pcn_ldata->pcn_tx_list[frag]; ! f->pcn_txctl = (~(m->m_len) + 1) & PCN_TXCTL_BUFSZ; ! f->pcn_txctl |= PCN_TXCTL_MBO; ! f->pcn_tbaddr = vtophys(mtod(m, vm_offset_t)); if (cnt == 0) ! f->pcn_txctl |= PCN_TXCTL_STP; else ! f->pcn_txctl |= PCN_TXCTL_OWN; cur = frag; PCN_INC(frag, PCN_TX_LIST_CNT); cnt++; --- 1076,1091 ---- if ((PCN_TX_LIST_CNT - (sc->pcn_cdata.pcn_tx_cnt + cnt)) < 2) return(ENOBUFS); f = &sc->pcn_ldata->pcn_tx_list[frag]; ! f->pcn_txctl = htole32((~(m->m_len) + 1) & PCN_TXCTL_BUFSZ); ! f->pcn_txctl |= htole32(PCN_TXCTL_MBO); ! f->pcn_tbaddr = htole32(vtophys(mtod(m, vm_offset_t))); ! #ifdef __rtems__ ! membarrier_w(); ! #endif if (cnt == 0) ! f->pcn_txctl |= htole32(PCN_TXCTL_STP); else ! f->pcn_txctl |= htole32(PCN_TXCTL_OWN); cur = frag; PCN_INC(frag, PCN_TX_LIST_CNT); cnt++; *************** *** 1075,1082 **** sc->pcn_cdata.pcn_tx_chain[cur] = m_head; sc->pcn_ldata->pcn_tx_list[cur].pcn_txctl |= ! PCN_TXCTL_ENP|PCN_TXCTL_ADD_FCS|PCN_TXCTL_MORE_LTINT; ! sc->pcn_ldata->pcn_tx_list[*txidx].pcn_txctl |= PCN_TXCTL_OWN; sc->pcn_cdata.pcn_tx_cnt += cnt; *txidx = frag; --- 1096,1106 ---- sc->pcn_cdata.pcn_tx_chain[cur] = m_head; sc->pcn_ldata->pcn_tx_list[cur].pcn_txctl |= ! htole32(PCN_TXCTL_ENP|PCN_TXCTL_ADD_FCS|PCN_TXCTL_MORE_LTINT); ! #ifdef __rtems__ ! membarrier_w(); ! #endif ! sc->pcn_ldata->pcn_tx_list[*txidx].pcn_txctl |= htole32(PCN_TXCTL_OWN); sc->pcn_cdata.pcn_tx_cnt += cnt; *txidx = frag; *************** *** 1208,1219 **** ife = mii->mii_media.ifm_cur; /* Set MAC address */ ! pcn_csr_write(sc, PCN_CSR_PAR0, ! ((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[0]); ! pcn_csr_write(sc, PCN_CSR_PAR1, ! ((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[1]); ! pcn_csr_write(sc, PCN_CSR_PAR2, ! ((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[2]); /* Init circular RX list. */ if (pcn_list_rx_init(sc) == ENOBUFS) { --- 1232,1246 ---- ife = mii->mii_media.ifm_cur; /* Set MAC address */ ! { unsigned tmp; ! /* fix endinanness; LLADDR gets swapped on a BE machine */ ! tmp = htole16(((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[0]); ! pcn_csr_write(sc, PCN_CSR_PAR0, tmp); ! tmp = htole16(((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[1]); ! pcn_csr_write(sc, PCN_CSR_PAR1, tmp); ! tmp = htole16(((u_int16_t *)IF_LLADDR(sc->pcn_ifp))[2]); ! pcn_csr_write(sc, PCN_CSR_PAR2, tmp); ! } /* Init circular RX list. */ if (pcn_list_rx_init(sc) == ENOBUFS) { Index: if_pcnreg.h =================================================================== RCS file: /afs/slac/g/spear/cvsrep/drivers/if_pcn-new-rtems/if_pcnreg.h,v retrieving revision 1.1.1.1 diff -c -r1.1.1.1 if_pcnreg.h *** if_pcnreg.h 17 Jul 2007 23:55:23 -0000 1.1.1.1 --- if_pcnreg.h 25 Jul 2007 21:15:33 -0000 *************** *** 362,371 **** #define PCN_PHY_EXTERNAL 0x0002 struct pcn_rx_desc { ! u_int16_t pcn_rxlen; ! u_int16_t pcn_rsvd0; u_int16_t pcn_bufsz; ! u_int16_t pcn_rxstat; u_int32_t pcn_rbaddr; u_int32_t pcn_uspace; }; --- 362,371 ---- #define PCN_PHY_EXTERNAL 0x0002 struct pcn_rx_desc { ! volatile u_int16_t pcn_rxlen; ! volatile u_int16_t pcn_rsvd0; u_int16_t pcn_bufsz; ! volatile u_int16_t pcn_rxstat; u_int32_t pcn_rbaddr; u_int32_t pcn_uspace; }; *************** *** 383,393 **** #define PCN_RXLEN_MBO 0xF000 #define PCN_RXLEN_BUFSZ 0x0FFF ! #define PCN_OWN_RXDESC(x) (((x)->pcn_rxstat & PCN_RXSTAT_OWN) == 0) struct pcn_tx_desc { ! u_int32_t pcn_txstat; ! u_int32_t pcn_txctl; u_int32_t pcn_tbaddr; u_int32_t pcn_uspace; }; --- 383,393 ---- #define PCN_RXLEN_MBO 0xF000 #define PCN_RXLEN_BUFSZ 0x0FFF ! #define PCN_OWN_RXDESC(x) ((le16toh((x)->pcn_rxstat) & PCN_RXSTAT_OWN) == 0) struct pcn_tx_desc { ! volatile u_int32_t pcn_txstat; ! volatile u_int32_t pcn_txctl; u_int32_t pcn_tbaddr; u_int32_t pcn_uspace; }; *************** *** 412,418 **** #define PCN_TXCTL_MBO 0x0000F000 #define PCN_TXCTL_BUFSZ 0x00000FFF ! #define PCN_OWN_TXDESC(x) (((x)->pcn_txctl & PCN_TXCTL_OWN) == 0) #define PCN_RX_LIST_CNT 64 #define PCN_TX_LIST_CNT 256 --- 412,418 ---- #define PCN_TXCTL_MBO 0x0000F000 #define PCN_TXCTL_BUFSZ 0x00000FFF ! #define PCN_OWN_TXDESC(x) ((le32toh((x)->pcn_txctl) & PCN_TXCTL_OWN) == 0) #define PCN_RX_LIST_CNT 64 #define PCN_TX_LIST_CNT 256 >Release-Note: >Audit-Trail: >Unformatted: