Date: Wed, 25 Jul 2007 21:53:04 GMT From: Till Straumann <strauman@slac.stanford.edu> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/114915: pcn (sys/pci/if_pcn.c) ethernet driver fixes (including endianness) Message-ID: <200707252153.l6PLr4J1079114@www.freebsd.org> Resent-Message-ID: <200707252200.l6PM08V4034183@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200707252153.l6PLr4J1079114>