Module Name: src Committed By: skrll Date: Fri Sep 16 03:55:53 UTC 2022
Modified Files: src/sys/dev/pci: if_aq.c Log Message: Some MP improvements - Remove use of IFF_OACTIVE - Remove use of if_timer and provide an MP safe multiqueue watchdog - Sprinkle some lock assertions. Tested by ryo@. Thanks. To generate a diff of this commit: cvs rdiff -u -r1.32 -r1.33 src/sys/dev/pci/if_aq.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Modified files: Index: src/sys/dev/pci/if_aq.c diff -u src/sys/dev/pci/if_aq.c:1.32 src/sys/dev/pci/if_aq.c:1.33 --- src/sys/dev/pci/if_aq.c:1.32 Thu Sep 8 07:05:42 2022 +++ src/sys/dev/pci/if_aq.c Fri Sep 16 03:55:53 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: if_aq.c,v 1.32 2022/09/08 07:05:42 skrll Exp $ */ +/* $NetBSD: if_aq.c,v 1.33 2022/09/16 03:55:53 skrll Exp $ */ /** * aQuantia Corporation Network Driver @@ -62,7 +62,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: if_aq.c,v 1.32 2022/09/08 07:05:42 skrll Exp $"); +__KERNEL_RCSID(0, "$NetBSD: if_aq.c,v 1.33 2022/09/16 03:55:53 skrll Exp $"); #ifdef _KERNEL_OPT #include "opt_if_aq.h" @@ -854,6 +854,9 @@ struct aq_txring { int txr_index; kmutex_t txr_mutex; bool txr_active; + bool txr_stopping; + bool txr_sending; + time_t txr_lastsent; pcq_t *txr_pcq; void *txr_softint; @@ -878,6 +881,7 @@ struct aq_rxring { kmutex_t rxr_mutex; bool rxr_active; bool rxr_discarding; + bool rxr_stopping; struct mbuf *rxr_receiving_m; /* receiving jumboframe */ struct mbuf *rxr_receiving_m_last; /* last mbuf of jumboframe */ @@ -934,10 +938,12 @@ struct aq_firmware_ops { #define AQ_LOCK(sc) mutex_enter(&(sc)->sc_mutex); #define AQ_UNLOCK(sc) mutex_exit(&(sc)->sc_mutex); +#define AQ_LOCKED(sc) KASSERT(mutex_owned(&(sc)->sc_mutex)); /* lock for FW2X_MPI_{CONTROL,STATE]_REG read-modify-write */ #define AQ_MPI_LOCK(sc) mutex_enter(&(sc)->sc_mpi_mutex); #define AQ_MPI_UNLOCK(sc) mutex_exit(&(sc)->sc_mpi_mutex); +#define AQ_MPI_LOCKED(sc) KASSERT(mutex_owned(&(sc)->sc_mpi_mutex)); struct aq_softc { @@ -1018,6 +1024,15 @@ struct aq_softc { int sc_ec_capenable; /* last ec_capenable */ unsigned short sc_if_flags; /* last if_flags */ + bool sc_tx_sending; + bool sc_stopping; + + struct workqueue *sc_reset_wq; + struct work sc_reset_work; + volatile unsigned sc_reset_pending; + + bool sc_trigger_reset; + #ifdef AQ_EVENT_COUNTERS aq_hw_stats_s_t sc_statistics[2]; int sc_statistics_idx; @@ -1059,13 +1074,14 @@ static void aq_ifmedia_status(struct ifn static int aq_vlan_cb(struct ethercom *ec, uint16_t vid, bool set); static int aq_ifflags_cb(struct ethercom *); static int aq_init(struct ifnet *); +static int aq_init_locked(struct ifnet *); static void aq_send_common_locked(struct ifnet *, struct aq_softc *, struct aq_txring *, bool); static int aq_transmit(struct ifnet *, struct mbuf *); static void aq_deferred_transmit(void *); static void aq_start(struct ifnet *); static void aq_stop(struct ifnet *, int); -static void aq_watchdog(struct ifnet *); +static void aq_stop_locked(struct ifnet *, bool); static int aq_ioctl(struct ifnet *, unsigned long, void *); static int aq_txrx_rings_alloc(struct aq_softc *); @@ -1076,6 +1092,10 @@ static void aq_tx_pcq_free(struct aq_sof static void aq_initmedia(struct aq_softc *); static void aq_enable_intr(struct aq_softc *, bool, bool); +static void aq_handle_reset_work(struct work *, void *); +static void aq_unset_stopping_flags(struct aq_softc *); +static void aq_set_stopping_flags(struct aq_softc *); + #if NSYSMON_ENVSYS > 0 static void aq_temp_refresh(struct sysmon_envsys *, envsys_data_t *); #endif @@ -1119,6 +1139,12 @@ static int fw2x_get_stats(struct aq_soft static int fw2x_get_temperature(struct aq_softc *, uint32_t *); #endif +#ifndef AQ_WATCHDOG_TIMEOUT +#define AQ_WATCHDOG_TIMEOUT 5 +#endif +static int aq_watchdog_timeout = AQ_WATCHDOG_TIMEOUT; + + static const struct aq_firmware_ops aq_fw1x_ops = { .reset = fw1x_reset, .set_mode = fw1x_set_mode, @@ -1370,9 +1396,20 @@ aq_attach(device_t parent, device_t self if (error != 0) goto attach_failure; - callout_init(&sc->sc_tick_ch, 0); + callout_init(&sc->sc_tick_ch, CALLOUT_MPSAFE); callout_setfunc(&sc->sc_tick_ch, aq_tick, sc); + char wqname[MAXCOMLEN]; + snprintf(wqname, sizeof(wqname), "%sReset", device_xname(sc->sc_dev)); + error = workqueue_create(&sc->sc_reset_wq, wqname, + aq_handle_reset_work, sc, PRI_SOFTNET, IPL_SOFTCLOCK, + WQ_MPSAFE); + if (error) { + aprint_error_dev(sc->sc_dev, + "unable to create reset workqueue\n"); + goto attach_failure; + } + sc->sc_intr_moderation_enable = CONFIG_INTR_MODERATION_ENABLE; if (sc->sc_msix && (sc->sc_nqueues > 1)) @@ -1423,7 +1460,7 @@ aq_attach(device_t parent, device_t self ifp->if_transmit = aq_transmit; ifp->if_start = aq_start; ifp->if_stop = aq_stop; - ifp->if_watchdog = aq_watchdog; + ifp->if_watchdog = NULL; IFQ_SET_READY(&ifp->if_snd); /* initialize capabilities */ @@ -1551,9 +1588,7 @@ aq_detach(device_t self, int flags __unu if (sc->sc_iosize != 0) { if (ifp->if_softc != NULL) { - const int s = splnet(); aq_stop(ifp, 0); - splx(s); } for (i = 0; i < AQ_NINTR_MAX; i++) { @@ -2753,6 +2788,7 @@ static int aq_ifmedia_change(struct ifnet * const ifp) { struct aq_softc * const sc = ifp->if_softc; + aq_link_speed_t rate = AQ_LINK_NONE; aq_link_fc_t fc = AQ_FC_NONE; aq_link_eee_t eee = AQ_EEE_DISABLE; @@ -3828,10 +3864,67 @@ aq_temp_refresh(struct sysmon_envsys *sm } #endif + + +static bool +aq_watchdog_check(struct aq_softc * const sc) +{ + + AQ_LOCKED(sc); + + bool ok = true; + for (u_int n = 0; n < sc->sc_nqueues; n++) { + struct aq_txring *txring = &sc->sc_queue[n].txring; + + mutex_enter(&txring->txr_mutex); + if (txring->txr_sending && + time_uptime - txring->txr_lastsent > aq_watchdog_timeout) + ok = false; + + mutex_exit(&txring->txr_mutex); + + if (!ok) + return false; + } + + if (sc->sc_trigger_reset) { + /* debug operation, no need for atomicity or reliability */ + sc->sc_trigger_reset = 0; + return false; + } + + return true; +} + + + +static bool +aq_watchdog_tick(struct ifnet *ifp) +{ + struct aq_softc * const sc = ifp->if_softc; + + AQ_LOCKED(sc); + + if (!sc->sc_trigger_reset && aq_watchdog_check(sc)) + return true; + + if (atomic_swap_uint(&sc->sc_reset_pending, 1) == 0) { + workqueue_enqueue(sc->sc_reset_wq, &sc->sc_reset_work, NULL); + } + + return false; +} + static void aq_tick(void *arg) { - struct aq_softc *sc = arg; + struct aq_softc * const sc = arg; + + AQ_LOCK(sc); + if (sc->sc_stopping) { + AQ_UNLOCK(sc); + return; + } if (sc->sc_poll_linkstat || sc->sc_detect_linkstat) { sc->sc_detect_linkstat = false; @@ -3843,13 +3936,12 @@ aq_tick(void *arg) aq_update_statistics(sc); #endif - if (sc->sc_poll_linkstat -#ifdef AQ_EVENT_COUNTERS - || sc->sc_poll_statistics -#endif - ) { + struct ifnet * const ifp = &sc->sc_ethercom.ec_if; + const bool ok = aq_watchdog_tick(ifp); + if (ok) callout_schedule(&sc->sc_tick_ch, hz); - } + + AQ_UNLOCK(sc); } /* interrupt enable/disable */ @@ -3931,7 +4023,7 @@ aq_txrx_intr(void *arg) static int aq_link_intr(void *arg) { - struct aq_softc *sc = arg; + struct aq_softc * const sc = arg; uint32_t status; int nintr = 0; @@ -4229,6 +4321,7 @@ aq_tx_intr(void *arg) hw_head = AQ_READ_REG_BIT(sc, TX_DMA_DESC_HEAD_PTR_REG(ringidx), TX_DMA_DESC_HEAD_PTR); if (hw_head == txring->txr_considx) { + txring->txr_sending = false; goto tx_intr_done; } @@ -4256,12 +4349,9 @@ aq_tx_intr(void *arg) IF_STAT_PUTREF(ifp); - if (ringidx == 0 && txring->txr_nfree >= AQ_TXD_MIN) - ifp->if_flags &= ~IFF_OACTIVE; - /* no more pending TX packet, cancel watchdog */ if (txring->txr_nfree >= AQ_TXD_NUM) - ifp->if_timer = 0; + txring->txr_sending = false; tx_intr_done: mutex_exit(&txring->txr_mutex); @@ -4536,15 +4626,31 @@ aq_ifflags_cb(struct ethercom *ec) return error; } + static int aq_init(struct ifnet *ifp) { struct aq_softc * const sc = ifp->if_softc; + + AQ_LOCK(sc); + + int ret = aq_init_locked(ifp); + + AQ_UNLOCK(sc); + + return ret; +} + +static int +aq_init_locked(struct ifnet *ifp) +{ + struct aq_softc * const sc = ifp->if_softc; int i, error = 0; - aq_stop(ifp, false); + KASSERT(IFNET_LOCKED(ifp)); + AQ_LOCKED(sc); - AQ_LOCK(sc); + aq_stop_locked(ifp, false); aq_set_vlan_filters(sc); aq_set_capability(sc); @@ -4570,18 +4676,13 @@ aq_init(struct ifnet *ifp) aq_init_rss(sc); aq_hw_l3_filter_set(sc); - /* need to start callout? */ - if (sc->sc_poll_linkstat -#ifdef AQ_EVENT_COUNTERS - || sc->sc_poll_statistics -#endif - ) { - callout_schedule(&sc->sc_tick_ch, hz); - } + /* ring reset? */ + aq_unset_stopping_flags(sc); + + callout_schedule(&sc->sc_tick_ch, hz); /* ready */ ifp->if_flags |= IFF_RUNNING; - ifp->if_flags &= ~IFF_OACTIVE; /* start TX and RX */ aq_enable_intr(sc, true, true); @@ -4591,8 +4692,6 @@ aq_init(struct ifnet *ifp) aq_init_failure: sc->sc_if_flags = ifp->if_flags; - AQ_UNLOCK(sc); - return error; } @@ -4603,7 +4702,7 @@ aq_send_common_locked(struct ifnet *ifp, struct mbuf *m; int npkt, error; - if ((ifp->if_flags & IFF_RUNNING) == 0) + if (txring->txr_nfree < AQ_TXD_MIN) return; for (npkt = 0; ; npkt++) { @@ -4615,9 +4714,6 @@ aq_send_common_locked(struct ifnet *ifp, if (m == NULL) break; - if (txring->txr_nfree < AQ_TXD_MIN) - break; - if (is_transmit) pcq_get(txring->txr_pcq); else @@ -4628,8 +4724,6 @@ aq_send_common_locked(struct ifnet *ifp, /* too many mbuf chains? or not enough descriptors? */ m_freem(m); if_statinc(ifp, if_oerrors); - if (txring->txr_index == 0 && error == ENOBUFS) - ifp->if_flags |= IFF_OACTIVE; break; } @@ -4641,11 +4735,11 @@ aq_send_common_locked(struct ifnet *ifp, bpf_mtap(ifp, m, BPF_D_OUT); } - if (txring->txr_index == 0 && txring->txr_nfree < AQ_TXD_MIN) - ifp->if_flags |= IFF_OACTIVE; - - if (npkt) - ifp->if_timer = 5; + if (npkt) { + /* Set a watchdog timer in case the chip flakes out. */ + txring->txr_lastsent = time_uptime; + txring->txr_sending = true; + } } static void @@ -4656,7 +4750,7 @@ aq_start(struct ifnet *ifp) struct aq_txring * const txring = &sc->sc_queue[0].txring; mutex_enter(&txring->txr_mutex); - if (txring->txr_active && !ISSET(ifp->if_flags, IFF_OACTIVE)) + if (txring->txr_active && !txring->txr_stopping) aq_send_common_locked(ifp, sc, txring, false); mutex_exit(&txring->txr_mutex); } @@ -4701,15 +4795,80 @@ aq_deferred_transmit(void *arg) mutex_exit(&txring->txr_mutex); } + +static void +aq_unset_stopping_flags(struct aq_softc *sc) +{ + + AQ_LOCKED(sc); + + /* Must unset stopping flags in ascending order. */ + for (unsigned i = 0; i < sc->sc_nqueues; i++) { + struct aq_txring *txr = &sc->sc_queue[i].txring; + struct aq_rxring *rxr = &sc->sc_queue[i].rxring; + + mutex_enter(&txr->txr_mutex); + txr->txr_stopping = false; + mutex_exit(&txr->txr_mutex); + + mutex_enter(&rxr->rxr_mutex); + rxr->rxr_stopping = false; + mutex_exit(&rxr->rxr_mutex); + } + + sc->sc_stopping = false; +} + +static void +aq_set_stopping_flags(struct aq_softc *sc) +{ + + AQ_LOCKED(sc); + + /* Must unset stopping flags in ascending order. */ + for (unsigned i = 0; i < sc->sc_nqueues; i++) { + struct aq_txring *txr = &sc->sc_queue[i].txring; + struct aq_rxring *rxr = &sc->sc_queue[i].rxring; + + mutex_enter(&txr->txr_mutex); + txr->txr_stopping = true; + mutex_exit(&txr->txr_mutex); + + mutex_enter(&rxr->rxr_mutex); + rxr->rxr_stopping = true; + mutex_exit(&rxr->rxr_mutex); + } + + sc->sc_stopping = true; +} + + static void aq_stop(struct ifnet *ifp, int disable) { struct aq_softc * const sc = ifp->if_softc; - int i; + + ASSERT_SLEEPABLE(); + KASSERT(IFNET_LOCKED(ifp)); AQ_LOCK(sc); + aq_stop_locked(ifp, disable ? true : false); + AQ_UNLOCK(sc); +} + + + +static void +aq_stop_locked(struct ifnet *ifp, bool disable) +{ + struct aq_softc * const sc = ifp->if_softc; + int i; + + ASSERT_SLEEPABLE(); + KASSERT(IFNET_LOCKED(ifp)); + AQ_LOCKED(sc); - ifp->if_timer = 0; + aq_set_stopping_flags(sc); if ((ifp->if_flags & IFF_RUNNING) == 0) goto already_stopped; @@ -4732,25 +4891,25 @@ aq_stop(struct ifnet *ifp, int disable) AQ_READ_REG_BIT(sc, RX_DMA_DESC_CACHE_INIT_REG, RX_DMA_DESC_CACHE_INIT) ^ 1); - ifp->if_timer = 0; - already_stopped: if (!disable) { /* when pmf stop, disable link status intr and callout */ aq_enable_intr(sc, false, false); - callout_stop(&sc->sc_tick_ch); + callout_halt(&sc->sc_tick_ch, &sc->sc_mutex); } - ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE); - - AQ_UNLOCK(sc); + ifp->if_flags &= ~IFF_RUNNING; + sc->sc_if_flags = ifp->if_flags; } + static void -aq_watchdog(struct ifnet *ifp) +aq_handle_reset_work(struct work *work, void *arg) { - struct aq_softc * const sc = ifp->if_softc; - int n, head, tail; + struct aq_softc * const sc = arg; + struct ifnet * const ifp = &sc->sc_ethercom.ec_if; + + printf("%s: watchdog timeout -- resetting\n", ifp->if_xname); AQ_LOCK(sc); @@ -4758,15 +4917,15 @@ aq_watchdog(struct ifnet *ifp) __func__, AQ_READ_REG(sc, AQ_INTR_MASK_REG), AQ_READ_REG(sc, AQ_INTR_STATUS_REG)); - for (n = 0; n < sc->sc_nqueues; n++) { - struct aq_txring * const txring = &sc->sc_queue[n].txring; - head = AQ_READ_REG_BIT(sc, + for (u_int n = 0; n < sc->sc_nqueues; n++) { + struct aq_txring *txring = &sc->sc_queue[n].txring; + u_int head = AQ_READ_REG_BIT(sc, TX_DMA_DESC_HEAD_PTR_REG(txring->txr_index), - TX_DMA_DESC_HEAD_PTR), - tail = AQ_READ_REG(sc, + TX_DMA_DESC_HEAD_PTR); + u_int tail = AQ_READ_REG(sc, TX_DMA_DESC_TAIL_PTR_REG(txring->txr_index)); - device_printf(sc->sc_dev, "%s: TXring[%d] HEAD/TAIL=%d/%d\n", + device_printf(sc->sc_dev, "%s: TXring[%u] HEAD/TAIL=%u/%u\n", __func__, txring->txr_index, head, tail); aq_tx_intr(txring); @@ -4774,7 +4933,15 @@ aq_watchdog(struct ifnet *ifp) AQ_UNLOCK(sc); + /* Don't want ioctl operations to happen */ + IFNET_LOCK(ifp); + + /* reset the interface. */ aq_init(ifp); + + IFNET_UNLOCK(ifp); + + atomic_store_relaxed(&sc->sc_reset_pending, 0); } static int @@ -4784,6 +4951,14 @@ aq_ioctl(struct ifnet *ifp, unsigned lon struct ifreq * const ifr = data; int error = 0; + switch (cmd) { + case SIOCADDMULTI: + case SIOCDELMULTI: + break; + default: + KASSERT(IFNET_LOCKED(ifp)); + } + const int s = splnet(); switch (cmd) { case SIOCSIFMTU: @@ -4809,14 +4984,15 @@ aq_ioctl(struct ifnet *ifp, unsigned lon break; case SIOCADDMULTI: case SIOCDELMULTI: - if ((ifp->if_flags & IFF_RUNNING) == 0) - break; - - /* - * Multicast list has changed; set the hardware filter - * accordingly. - */ - error = aq_set_filter(sc); + AQ_LOCK(sc); + if ((sc->sc_if_flags & IFF_RUNNING) != 0) { + /* + * Multicast list has changed; set the hardware filter + * accordingly. + */ + error = aq_set_filter(sc); + } + AQ_UNLOCK(sc); break; }