Date: Tue, 16 Sep 2014 15:45:53 +0000 (UTC) From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r271679 - head/sys/dev/altera/atse Message-ID: <201409161545.s8GFjreh035440@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bz Date: Tue Sep 16 15:45:53 2014 New Revision: 271679 URL: http://svnweb.freebsd.org/changeset/base/271679 Log: Merge atse(4) interrupt handling and race condition fixes from cheribsd: commit 8bd88585ed8e3f7def0d780a1bc30d96fe642b9c Rework atse_rx_cycles handling: count packets instead of fills, and use the limit only when polling, not when in interrupt mode. Otherwise, we may stop reading the FIFO midpacket and clear the event mask even though the FIFO still has data to read, which could stall receive when a large packet arrives. Add a comment about races in the Altera FIFO interface: we may need to do a little more work to handle races than we are. commit 20b39086cc612f8874dc9e6ef4c0c2eb777ba92a Use 'sizeof(data)' rather than '4' when checking an mbuf bound, as is the case for adjusting length/etc. commit e18953174a265f40e9ba60d76af7d288927f5382 Break out atse_intr() into two separate routines, one for each of the two interrupt sources: receive and transmit. commit 6deedb43246ab3f9f597918361831fbab7fac4ce For the RX interrupt, take interest only in ALMOSTEMPTY and OVERFLOW. For the TX interrupt, take interest only in ALMOSTFULL and UNDERFLOW. Perform TX atse_start_locked() once rather than twice in TX interrupt handling -- and only if !FULL, rather than unconditionally. commit 12601972ba08d4380201a74f5b967bdaeb23092c Experimentation suggests that the Altera Triple-Speed Ethernet documentation is incorrect and bits in the event and interrupt-enable registers are not irrationally rearranged relative to the status register. commit 3cff2ffad769289fce3a728152e7be09405385d8 Substantially rework interrupt handling in the atse(4) driver: - Introduce a new macro ATSE_TX_PENDING() which checks whether there is any pending data to transmit, either in an in-progress packet or in the TX queue. - Introduce new ATSE_RX_STATUS_READ() and ATSE_TX_STAUTS_WRITE() macros that query the FIFO status registers rather than event registers, offering level- rather than edge-triggered FIFO conditions. - For RX, interrupt only on full/overflow/underflow; for TX, interrupt only on empty/overflow/underflow. - Add new ATSE_RX_INTR_READ() and ATSE_RX_INTR_WRITE() macros useful for debugging interrupt behaviour. - Add a debug.atse_intr_debug_enable sysctl that causes various pieces of FIFO state to be printed out on each RX or TX interrupt. This is disabled by default but good to turn on if the interface appears to wedge. Also print debugging information when polling. - In the watchdog handler, do receive, not just transmit, processing, to ensure that the rx, not just tx, queue is being handled -- and, in particular, will be drained such that interrupts can resume. - Rework both atse_rx_intr() and atse_tx_intr() to eliminate many race conditions, and add comments on why various things are in various orders. Interactions between modifications to the event and interrupt masks are quite subtle indeed, and we must actively check for a number of races (e.g., event mask cleared; packet arrives; interrupts enabled). We also now use the status registers rather than event registers for FIFO status checks to avoid other races; we continue to use event registers for underflow/overflow. With this change, interrupt-driven operation of atse appears (for the time being) robust. commit 3393bbff5c68a4e61699f9b4a62af5d2a5f918f8 atse: Fix build after 3cff2ffa Obtained from: cheribsd Submitted by: rwatson, emaste Sponsored by: DARPA/AFRL MFC after: 3 days Modified: head/sys/dev/altera/atse/a_api.h head/sys/dev/altera/atse/if_atse.c Modified: head/sys/dev/altera/atse/a_api.h ============================================================================== --- head/sys/dev/altera/atse/a_api.h Tue Sep 16 15:35:13 2014 (r271678) +++ head/sys/dev/altera/atse/a_api.h Tue Sep 16 15:45:53 2014 (r271679) @@ -69,20 +69,20 @@ #define A_ONCHIP_FIFO_MEM_CORE_STATUS_UNDERFLOW (1<<5) /* Table 16-6. Event Bit Field Descriptions. */ -/* XXX Datasheet has weird bit fields. Validate. */ -#define A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY (1<<0) -#define A_ONCHIP_FIFO_MEM_CORE_EVENT_FULL (1<<1) -#define A_ONCHIP_FIFO_MEM_CORE_EVENT_ALMOSTEMPTY (1<<2) -#define A_ONCHIP_FIFO_MEM_CORE_EVENT_ALMOSTFULL (1<<3) +/* XXX Datasheet has incorrect bit fields. Validate. */ +#define A_ONCHIP_FIFO_MEM_CORE_EVENT_FULL (1<<0) +#define A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY (1<<1) +#define A_ONCHIP_FIFO_MEM_CORE_EVENT_ALMOSTFULL (1<<2) +#define A_ONCHIP_FIFO_MEM_CORE_EVENT_ALMOSTEMPTY (1<<3) #define A_ONCHIP_FIFO_MEM_CORE_EVENT_OVERFLOW (1<<4) #define A_ONCHIP_FIFO_MEM_CORE_EVENT_UNDERFLOW (1<<5) /* Table 16-7. InterruptEnable Bit Field Descriptions. */ -/* XXX Datasheet has weird bit fields. Validate. */ -#define A_ONCHIP_FIFO_MEM_CORE_INTR_EMPTY (1<<0) -#define A_ONCHIP_FIFO_MEM_CORE_INTR_FULL (1<<1) -#define A_ONCHIP_FIFO_MEM_CORE_INTR_ALMOSTEMPTY (1<<2) -#define A_ONCHIP_FIFO_MEM_CORE_INTR_ALMOSTFULL (1<<3) +/* XXX Datasheet has incorrect bit fields. Validate. */ +#define A_ONCHIP_FIFO_MEM_CORE_INTR_FULL (1<<0) +#define A_ONCHIP_FIFO_MEM_CORE_INTR_EMPTY (1<<1) +#define A_ONCHIP_FIFO_MEM_CORE_INTR_ALMOSTFULL (1<<2) +#define A_ONCHIP_FIFO_MEM_CORE_INTR_ALMOSTEMPTY (1<<3) #define A_ONCHIP_FIFO_MEM_CORE_INTR_OVERFLOW (1<<4) #define A_ONCHIP_FIFO_MEM_CORE_INTR_UNDERFLOW (1<<5) #define A_ONCHIP_FIFO_MEM_CORE_INTR_ALL \ Modified: head/sys/dev/altera/atse/if_atse.c ============================================================================== --- head/sys/dev/altera/atse/if_atse.c Tue Sep 16 15:35:13 2014 (r271678) +++ head/sys/dev/altera/atse/if_atse.c Tue Sep 16 15:45:53 2014 (r271679) @@ -1,5 +1,6 @@ /*- * Copyright (c) 2012,2013 Bjoern A. Zeeb + * Copyright (c) 2014 Robert N. M. Watson * All rights reserved. * * This software was developed by SRI International and the University of @@ -103,6 +104,11 @@ static poll_handler_t atse_poll; static int atse_ethernet_option_bits_flag = ATSE_ETHERNET_OPTION_BITS_UNDEF; static uint8_t atse_ethernet_option_bits[ALTERA_ETHERNET_OPTION_BITS_LEN]; +static int atse_intr_debug_enable = 0; +SYSCTL_INT(_debug, OID_AUTO, atse_intr_debug_enable, CTLFLAG_RW, + &atse_intr_debug_enable, 0, + "Extra debugging output for atse interrupts"); + /* * Softc and critical resource locking. */ @@ -110,6 +116,9 @@ static uint8_t atse_ethernet_option_bits #define ATSE_UNLOCK(_sc) mtx_unlock(&(_sc)->atse_mtx) #define ATSE_LOCK_ASSERT(_sc) mtx_assert(&(_sc)->atse_mtx, MA_OWNED) +#define ATSE_TX_PENDING(sc) (sc->atse_tx_m != NULL || \ + !IFQ_DRV_IS_EMPTY(&ifp->if_snd)) + #ifdef DEBUG #define DPRINTF(format, ...) printf(format, __VA_ARGS__) #else @@ -169,6 +178,16 @@ a_onchip_fifo_mem_core_read(struct resou A_ONCHIP_FIFO_MEM_CORE_METADATA, \ "RXM", __func__, __LINE__) +#define ATSE_RX_STATUS_READ(sc) \ + a_onchip_fifo_mem_core_read((sc)->atse_rxc_mem_res, \ + A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_I_STATUS, \ + "RX_EVENT", __func__, __LINE__) + +#define ATSE_TX_STATUS_READ(sc) \ + a_onchip_fifo_mem_core_read((sc)->atse_txc_mem_res, \ + A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_I_STATUS, \ + "TX_EVENT", __func__, __LINE__) + #define ATSE_RX_EVENT_READ(sc) \ a_onchip_fifo_mem_core_read((sc)->atse_rxc_mem_res, \ A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_EVENT, \ @@ -208,24 +227,41 @@ a_onchip_fifo_mem_core_read(struct resou val4, "TX_EVENT", __func__, __LINE__); \ } while(0) +#define ATSE_RX_EVENTS (A_ONCHIP_FIFO_MEM_CORE_INTR_FULL | \ + A_ONCHIP_FIFO_MEM_CORE_INTR_OVERFLOW | \ + A_ONCHIP_FIFO_MEM_CORE_INTR_UNDERFLOW) #define ATSE_RX_INTR_ENABLE(sc) \ a_onchip_fifo_mem_core_write((sc)->atse_rxc_mem_res, \ A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, \ - A_ONCHIP_FIFO_MEM_CORE_INTR_ALL, \ + ATSE_RX_EVENTS, \ "RX_INTR", __func__, __LINE__) /* XXX-BZ review later. */ #define ATSE_RX_INTR_DISABLE(sc) \ a_onchip_fifo_mem_core_write((sc)->atse_rxc_mem_res, \ A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, 0, \ "RX_INTR", __func__, __LINE__) +#define ATSE_RX_INTR_READ(sc) \ + a_onchip_fifo_mem_core_read((sc)->atse_rxc_mem_res, \ + A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, \ + "RX_INTR", __func__, __LINE__) + +#define ATSE_TX_EVENTS (A_ONCHIP_FIFO_MEM_CORE_INTR_EMPTY | \ + A_ONCHIP_FIFO_MEM_CORE_INTR_OVERFLOW | \ + A_ONCHIP_FIFO_MEM_CORE_INTR_UNDERFLOW) #define ATSE_TX_INTR_ENABLE(sc) \ a_onchip_fifo_mem_core_write((sc)->atse_txc_mem_res, \ A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, \ - A_ONCHIP_FIFO_MEM_CORE_INTR_ALL, \ + ATSE_TX_EVENTS, \ "TX_INTR", __func__, __LINE__) /* XXX-BZ review later. */ #define ATSE_TX_INTR_DISABLE(sc) \ a_onchip_fifo_mem_core_write((sc)->atse_txc_mem_res, \ A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, 0, \ "TX_INTR", __func__, __LINE__) +#define ATSE_TX_INTR_READ(sc) \ + a_onchip_fifo_mem_core_read((sc)->atse_txc_mem_res, \ + A_ONCHIP_FIFO_MEM_CORE_STATUS_REG_INT_ENABLE, \ + "TX_INTR", __func__, __LINE__) + +static int atse_rx_locked(struct atse_softc *sc); /* * Register space access macros. @@ -985,6 +1021,11 @@ atse_init(void *xsc) { struct atse_softc *sc; + /* + * XXXRW: There is some argument that we should immediately do RX + * processing after enabling interrupts, or one may not fire if there + * are buffered packets. + */ sc = (struct atse_softc *)xsc; ATSE_LOCK(sc); atse_init_locked(sc); @@ -1082,6 +1123,33 @@ atse_ioctl(struct ifnet *ifp, u_long com } static void +atse_intr_debug(struct atse_softc *sc, const char *intrname) +{ + uint32_t rxs, rxe, rxi, rxf, txs, txe, txi, txf; + + if (!atse_intr_debug_enable) + return; + + rxs = ATSE_RX_STATUS_READ(sc); + rxe = ATSE_RX_EVENT_READ(sc); + rxi = ATSE_RX_INTR_READ(sc); + rxf = ATSE_RX_READ_FILL_LEVEL(sc); + + txs = ATSE_TX_STATUS_READ(sc); + txe = ATSE_TX_EVENT_READ(sc); + txi = ATSE_TX_INTR_READ(sc); + txf = ATSE_TX_READ_FILL_LEVEL(sc); + + printf( + "%s - %s: " + "rxs 0x%x rxe 0x%x rxi 0x%x rxf 0x%x " + "txs 0x%x txe 0x%x txi 0x%x txf 0x%x\n", + __func__, intrname, + rxs, rxe, rxi, rxf, + txs, txe, txi, txf); +} + +static void atse_watchdog(struct atse_softc *sc) { @@ -1093,9 +1161,12 @@ atse_watchdog(struct atse_softc *sc) device_printf(sc->atse_dev, "watchdog timeout\n"); sc->atse_ifp->if_oerrors++; + atse_intr_debug(sc, "poll"); + sc->atse_ifp->if_drv_flags &= ~IFF_DRV_RUNNING; atse_init_locked(sc); + atse_rx_locked(sc); if (!IFQ_DRV_IS_EMPTY(&sc->atse_ifp->if_snd)) atse_start_locked(sc->atse_ifp); } @@ -1169,10 +1240,6 @@ atse_rx_locked(struct atse_softc *sc) meta = 0; do { outer: - if (sc->atse_rx_cycles <= 0) - return (rx_npkts); - sc->atse_rx_cycles--; - if (sc->atse_rx_m == NULL) { m = m_getcl(M_NOWAIT, MT_DATA, M_PKTHDR); if (m == NULL) @@ -1238,7 +1305,8 @@ outer: data = ATSE_RX_DATA_READ(sc); #endif /* Make sure to not overflow the mbuf data size. */ - if (sc->atse_rx_buf_len >= sc->atse_rx_m->m_len - 4) { + if (sc->atse_rx_buf_len >= sc->atse_rx_m->m_len - + sizeof(data)) { /* * XXX-BZ Error. We need more mbufs and are * not setup for this yet. @@ -1275,15 +1343,19 @@ outer: if (sc->atse_flags & ATSE_FLAGS_ERROR) { sc->atse_flags &= ~ATSE_FLAGS_ERROR; m_freem(m); - /* Need to start with a new packet. */ - goto outer; + } else { + m->m_pkthdr.rcvif = ifp; + ATSE_UNLOCK(sc); + (*ifp->if_input)(ifp, m); + ATSE_LOCK(sc); } - - m->m_pkthdr.rcvif = ifp; - - ATSE_UNLOCK(sc); - (*ifp->if_input)(ifp, m); - ATSE_LOCK(sc); +#ifdef DEVICE_POLLING + if (ifp->if_capenable & IFCAP_POLLING) { + if (sc->atse_rx_cycles <= 0) + return (rx_npkts); + sc->atse_rx_cycles--; + } +#endif goto outer; /* Need a new mbuf. */ } else { sc->atse_rx_buf_len += sizeof(data); @@ -1291,7 +1363,7 @@ outer: } /* for */ /* XXX-BZ could optimize in case of another packet waiting. */ - } while ((meta & A_ONCHIP_FIFO_MEM_CORE_EOP) == 0 || fill > 0); + } while (fill > 0); return (rx_npkts); } @@ -1317,11 +1389,11 @@ atse_ifmedia_sts(struct ifnet *ifp, stru } static void -atse_intr(void *arg) +atse_rx_intr(void *arg) { struct atse_softc *sc; struct ifnet *ifp; - uint32_t rx, tx; + uint32_t rxe; sc = (struct atse_softc *)arg; ifp = sc->atse_ifp; @@ -1334,54 +1406,94 @@ atse_intr(void *arg) } #endif - ATSE_RX_INTR_DISABLE(sc); - ATSE_TX_INTR_DISABLE(sc); - - rx = ATSE_RX_EVENT_READ(sc); - tx = ATSE_TX_EVENT_READ(sc); - if (rx != 0) { - if (rx & (A_ONCHIP_FIFO_MEM_CORE_EVENT_OVERFLOW| - A_ONCHIP_FIFO_MEM_CORE_EVENT_UNDERFLOW)) { - /* XXX-BZ ERROR HANDLING. */ - atse_update_rx_err(sc, ((rx & - A_ONCHIP_FIFO_MEM_CORE_ERROR_MASK) >> - A_ONCHIP_FIFO_MEM_CORE_ERROR_SHIFT) & 0xff); - ifp->if_ierrors++; - } - if ((rx & A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY) != 0) { - sc->atse_rx_cycles = RX_CYCLES_IN_INTR; - atse_rx_locked(sc); - } + atse_intr_debug(sc, "rx"); + rxe = ATSE_RX_EVENT_READ(sc); + if (rxe & (A_ONCHIP_FIFO_MEM_CORE_EVENT_OVERFLOW| + A_ONCHIP_FIFO_MEM_CORE_EVENT_UNDERFLOW)) { + /* XXX-BZ ERROR HANDLING. */ + atse_update_rx_err(sc, ((rxe & + A_ONCHIP_FIFO_MEM_CORE_ERROR_MASK) >> + A_ONCHIP_FIFO_MEM_CORE_ERROR_SHIFT) & 0xff); + ifp->if_ierrors++; } - if (tx != 0) { - /* XXX-BZ build histogram. */ - if (tx & (A_ONCHIP_FIFO_MEM_CORE_EVENT_OVERFLOW| - A_ONCHIP_FIFO_MEM_CORE_EVENT_UNDERFLOW)) { - /* XXX-BZ ERROR HANDLING. */ - ifp->if_oerrors++; - } - if (tx & A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY) - sc->atse_watchdog_timer = 0; + + /* + * There is considerable subtlety in the race-free handling of rx + * interrupts: we must disable interrupts whenever we manipulate the + * FIFO to prevent further interrupts from firing before we are done; + * we must clear the event after processing to prevent the event from + * being immediately reposted due to data remaining; we must clear the + * event mask before reenabling interrupts or risk missing a positive + * edge; and we must recheck everything after completing in case the + * event posted between clearing events and reenabling interrupts. If + * a race is experienced, we must restart the whole mechanism. + */ + do { + ATSE_RX_INTR_DISABLE(sc); #if 0 - if (tx & (A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY| - A_ONCHIP_FIFO_MEM_CORE_EVENT_ALMOSTEMPTY)) - atse_start_locked(ifp); + sc->atse_rx_cycles = RX_CYCLES_IN_INTR; #endif - } + atse_rx_locked(sc); + ATSE_RX_EVENT_CLEAR(sc); - /* Clear events before re-enabling intrs. */ - ATSE_TX_EVENT_CLEAR(sc); - ATSE_RX_EVENT_CLEAR(sc); + /* Disable interrupts if interface is down. */ + if (ifp->if_drv_flags & IFF_DRV_RUNNING) + ATSE_RX_INTR_ENABLE(sc); + } while (!(ATSE_RX_STATUS_READ(sc) & + A_ONCHIP_FIFO_MEM_CORE_STATUS_EMPTY)); + ATSE_UNLOCK(sc); - if (ifp->if_drv_flags & IFF_DRV_RUNNING) { - /* Re-enable interrupts. */ - ATSE_RX_INTR_ENABLE(sc); - ATSE_TX_INTR_ENABLE(sc); +} - if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) - atse_start_locked(ifp); +static void +atse_tx_intr(void *arg) +{ + struct atse_softc *sc; + struct ifnet *ifp; + uint32_t txe; + + sc = (struct atse_softc *)arg; + ifp = sc->atse_ifp; + + ATSE_LOCK(sc); +#ifdef DEVICE_POLLING + if (ifp->if_capenable & IFCAP_POLLING) { + ATSE_UNLOCK(sc); + return; + } +#endif + + /* XXX-BZ build histogram. */ + atse_intr_debug(sc, "tx"); + txe = ATSE_TX_EVENT_READ(sc); + if (txe & (A_ONCHIP_FIFO_MEM_CORE_EVENT_OVERFLOW| + A_ONCHIP_FIFO_MEM_CORE_EVENT_UNDERFLOW)) { + /* XXX-BZ ERROR HANDLING. */ + ifp->if_oerrors++; } + /* + * There is also considerable subtlety in the race-free handling of + * tx interrupts: all processing occurs with interrupts disabled to + * prevent spurious refiring while transmit is in progress (which + * could occur if the FIFO drains while sending -- quite likely); we + * must not clear the event mask until after we've sent, also to + * prevent spurious refiring; once we've cleared the event mask we can + * reenable interrupts, but there is a possible race between clear and + * enable, so we must recheck and potentially repeat the whole process + * if it is detected. + */ + do { + ATSE_TX_INTR_DISABLE(sc); + sc->atse_watchdog_timer = 0; + atse_start_locked(ifp); + ATSE_TX_EVENT_CLEAR(sc); + + /* Disable interrupts if interface is down. */ + if (ifp->if_drv_flags & IFF_DRV_RUNNING) + ATSE_TX_INTR_ENABLE(sc); + } while (ATSE_TX_PENDING(sc) && + !(ATSE_TX_STATUS_READ(sc) & A_ONCHIP_FIFO_MEM_CORE_STATUS_FULL)); ATSE_UNLOCK(sc); } @@ -1422,7 +1534,7 @@ atse_poll(struct ifnet *ifp, enum poll_c /* XXX-BZ ERROR HANDLING. */ ifp->if_oerrors++; } - if (tx & A_ONCHIP_FIFO_MEM_CORE_EVENT_EMPTY) + if (ATSE_TX_READ_FILL_LEVEL(sc) == 0) sc->atse_watchdog_timer = 0; #if 0 @@ -1719,7 +1831,7 @@ atse_attach(device_t dev) /* Hook up interrupts. */ if (sc->atse_rx_irq_res != NULL) { error = bus_setup_intr(dev, sc->atse_rx_irq_res, INTR_TYPE_NET | - INTR_MPSAFE, NULL, atse_intr, sc, &sc->atse_rx_intrhand); + INTR_MPSAFE, NULL, atse_rx_intr, sc, &sc->atse_rx_intrhand); if (error != 0) { device_printf(dev, "enabling RX IRQ failed\n"); ether_ifdetach(ifp); @@ -1729,7 +1841,7 @@ atse_attach(device_t dev) if (sc->atse_tx_irq_res != NULL) { error = bus_setup_intr(dev, sc->atse_tx_irq_res, INTR_TYPE_NET | - INTR_MPSAFE, NULL, atse_intr, sc, &sc->atse_tx_intrhand); + INTR_MPSAFE, NULL, atse_tx_intr, sc, &sc->atse_tx_intrhand); if (error != 0) { bus_teardown_intr(dev, sc->atse_rx_irq_res, sc->atse_rx_intrhand);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201409161545.s8GFjreh035440>