From 02ee7d07da1857d98989596627bda3894c3c7b1d Mon Sep 17 00:00:00 2001 From: patrick Date: Thu, 8 Feb 2018 05:00:38 +0000 Subject: [PATCH] Move bwfm(4) from ifq begin/commit/rollback semantics to the newer ifq dequeue semantics. This basically means we need to check for available space before dequeuing a packet. As soon as we dequeue a packet we commit to it. On the PCIe backend this check can not be done easily since the flowring depends on the packet contents and we cannot take a peek. When there is no flowring we cache the mbuf and send it out as soon as the flowring opened up. Then the ifq can be restarted and traffic can flow. Typically we usually run out of packet ids, which can be checked without consulting the packet. The flowring probably never becomes full as the bwfm(4) firmware takes the packets off the ring without actually sending them out. Discussed with dlg@ --- sys/dev/ic/bwfm.c | 25 ++++----- sys/dev/ic/bwfmvar.h | 6 +- sys/dev/pci/if_bwfm_pci.c | 115 +++++++++++++++++++++++++++++++------- sys/dev/usb/if_bwfm_usb.c | 18 +++++- 4 files changed, 124 insertions(+), 40 deletions(-) diff --git a/sys/dev/ic/bwfm.c b/sys/dev/ic/bwfm.c index eaf1aece02e..68bf91b8f7b 100644 --- a/sys/dev/ic/bwfm.c +++ b/sys/dev/ic/bwfm.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bwfm.c,v 1.38 2018/02/07 22:01:04 patrick Exp $ */ +/* $OpenBSD: bwfm.c,v 1.39 2018/02/08 05:00:38 patrick Exp $ */ /* * Copyright (c) 2010-2016 Broadcom Corporation * Copyright (c) 2016,2017 Patrick Wildt @@ -293,7 +293,6 @@ bwfm_start(struct ifnet *ifp) { struct bwfm_softc *sc = ifp->if_softc; struct mbuf *m; - int error; if (!(ifp->if_flags & IFF_RUNNING)) return; @@ -304,30 +303,26 @@ bwfm_start(struct ifnet *ifp) /* TODO: return if no link? */ - m = ifq_deq_begin(&ifp->if_snd); - while (m != NULL) { - error = sc->sc_bus_ops->bs_txdata(sc, m); - if (error == ENOBUFS) { - ifq_deq_rollback(&ifp->if_snd, m); + for (;;) { + if (sc->sc_bus_ops->bs_txcheck(sc)) { ifq_set_oactive(&ifp->if_snd); break; } - if (error == EFBIG) { - ifq_deq_commit(&ifp->if_snd, m); - m_freem(m); /* give up: drop it */ + + m = ifq_dequeue(&ifp->if_snd); + if (m == NULL) + break; + + if (sc->sc_bus_ops->bs_txdata(sc, m) != 0) { ifp->if_oerrors++; + m_freem(m); continue; } - /* Now we are committed to transmit the packet. */ - ifq_deq_commit(&ifp->if_snd, m); - #if NBPFILTER > 0 if (ifp->if_bpf) bpf_mtap(ifp->if_bpf, m, BPF_DIRECTION_OUT); #endif - - m = ifq_deq_begin(&ifp->if_snd); } } diff --git a/sys/dev/ic/bwfmvar.h b/sys/dev/ic/bwfmvar.h index cd74e40b720..20a652d6e32 100644 --- a/sys/dev/ic/bwfmvar.h +++ b/sys/dev/ic/bwfmvar.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bwfmvar.h,v 1.11 2018/02/07 22:01:04 patrick Exp $ */ +/* $OpenBSD: bwfmvar.h,v 1.12 2018/02/08 05:00:38 patrick Exp $ */ /* * Copyright (c) 2010-2016 Broadcom Corporation * Copyright (c) 2016,2017 Patrick Wildt @@ -87,6 +87,7 @@ struct bwfm_chip { struct bwfm_bus_ops { void (*bs_init)(struct bwfm_softc *); void (*bs_stop)(struct bwfm_softc *); + int (*bs_txcheck)(struct bwfm_softc *); int (*bs_txdata)(struct bwfm_softc *, struct mbuf *); int (*bs_txctl)(struct bwfm_softc *, char *, size_t); int (*bs_rxctl)(struct bwfm_softc *, char *, size_t *); @@ -125,8 +126,7 @@ struct bwfm_cmd_mbuf { }; struct bwfm_cmd_flowring_create { - uint8_t da[ETHER_ADDR_LEN]; - uint8_t sa[ETHER_ADDR_LEN]; + struct mbuf *m; int flowid; int prio; }; diff --git a/sys/dev/pci/if_bwfm_pci.c b/sys/dev/pci/if_bwfm_pci.c index 5dab9290431..c8a1ddced1e 100644 --- a/sys/dev/pci/if_bwfm_pci.c +++ b/sys/dev/pci/if_bwfm_pci.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_bwfm_pci.c,v 1.17 2018/02/07 22:02:48 patrick Exp $ */ +/* $OpenBSD: if_bwfm_pci.c,v 1.18 2018/02/08 05:00:38 patrick Exp $ */ /* * Copyright (c) 2010-2016 Broadcom Corporation * Copyright (c) 2017 Patrick Wildt @@ -89,6 +89,7 @@ struct bwfm_pci_msgring { int itemsz; enum ring_status status; struct bwfm_pci_dmamem *ring; + struct mbuf *m; int fifo; uint8_t mac[ETHER_ADDR_LEN]; @@ -199,6 +200,8 @@ struct bwfm_pci_dmamem * bwfm_pci_dmamem_alloc(struct bwfm_pci_softc *, bus_size_t, bus_size_t); void bwfm_pci_dmamem_free(struct bwfm_pci_softc *, struct bwfm_pci_dmamem *); +int bwfm_pci_pktid_avail(struct bwfm_pci_softc *, + struct bwfm_pci_pkts *); int bwfm_pci_pktid_new(struct bwfm_pci_softc *, struct bwfm_pci_pkts *, struct mbuf *, uint32_t *, paddr_t *); @@ -255,6 +258,7 @@ void bwfm_pci_flowring_create_cb(struct bwfm_softc *, void *); void bwfm_pci_flowring_delete(struct bwfm_pci_softc *, int); void bwfm_pci_stop(struct bwfm_softc *); +int bwfm_pci_txcheck(struct bwfm_softc *); int bwfm_pci_txdata(struct bwfm_softc *, struct mbuf *); #ifdef BWFM_DEBUG @@ -278,6 +282,7 @@ struct bwfm_buscore_ops bwfm_pci_buscore_ops = { struct bwfm_bus_ops bwfm_pci_bus_ops = { .bs_init = NULL, .bs_stop = bwfm_pci_stop, + .bs_txcheck = bwfm_pci_txcheck, .bs_txdata = bwfm_pci_txdata, .bs_txctl = NULL, .bs_rxctl = NULL, @@ -806,6 +811,22 @@ bwfm_pci_dmamem_free(struct bwfm_pci_softc *sc, struct bwfm_pci_dmamem *bdm) * a transfer completed, we only know the ID so we have to look up * the memory for the ID. This simply looks for an empty slot. */ +int +bwfm_pci_pktid_avail(struct bwfm_pci_softc *sc, struct bwfm_pci_pkts *pkts) +{ + int i, idx; + + idx = pkts->last + 1; + for (i = 0; i < pkts->npkt; i++) { + if (idx == pkts->npkt) + idx = 0; + if (pkts->pkts[idx].bb_m == NULL) + return 0; + idx++; + } + return ENOBUFS; +} + int bwfm_pci_pktid_new(struct bwfm_pci_softc *sc, struct bwfm_pci_pkts *pkts, struct mbuf *m, uint32_t *pktid, paddr_t *paddr) @@ -872,6 +893,8 @@ bwfm_pci_fill_rx_ioctl_ring(struct bwfm_pci_softc *sc, struct if_rxring *rxring, s = splnet(); for (slots = if_rxr_get(rxring, 8); slots > 0; slots--) { + if (bwfm_pci_pktid_avail(sc, &sc->sc_rx_pkts)) + break; req = bwfm_pci_ring_write_reserve(sc, &sc->sc_ctrl_submit); if (req == NULL) break; @@ -910,6 +933,8 @@ bwfm_pci_fill_rx_buf_ring(struct bwfm_pci_softc *sc) s = splnet(); for (slots = if_rxr_get(&sc->sc_rxbuf_ring, sc->sc_max_rxbufpost); slots > 0; slots--) { + if (bwfm_pci_pktid_avail(sc, &sc->sc_rx_pkts)) + break; req = bwfm_pci_ring_write_reserve(sc, &sc->sc_rxpost_submit); if (req == NULL) break; @@ -1222,10 +1247,20 @@ bwfm_pci_msg_rx(struct bwfm_pci_softc *sc, void *buf) printf("%s: failed to open flowring %d\n", DEVNAME(sc), flowid); ring->status = RING_CLOSED; + if (ring->m) { + m_freem(ring->m); + ring->m = NULL; + } ifq_restart(&ifp->if_snd); break; } ring->status = RING_OPEN; + if (ring->m != NULL) { + m = ring->m; + ring->m = NULL; + if (bwfm_pci_txdata(&sc->sc_sc, m)) + m_freem(ring->m); + } ifq_restart(&ifp->if_snd); break; case MSGBUF_TYPE_FLOW_RING_DELETE_CMPLT: @@ -1493,6 +1528,7 @@ bwfm_pci_flowring_create(struct bwfm_pci_softc *sc, struct mbuf *m) struct ieee80211com *ic = &sc->sc_sc.sc_ic; struct bwfm_cmd_flowring_create cmd; uint8_t *da = mtod(m, uint8_t *); + struct bwfm_pci_msgring *ring; int flowid, prio, fifo; int i, found; @@ -1519,22 +1555,27 @@ bwfm_pci_flowring_create(struct bwfm_pci_softc *sc, struct mbuf *m) found = 0; flowid = flowid % sc->sc_max_flowrings; for (i = 0; i < sc->sc_max_flowrings; i++) { - if (sc->sc_flowrings[flowid].status == RING_CLOSED) { + ring = &sc->sc_flowrings[flowid]; + if (ring->status == RING_CLOSED) { + ring->status = RING_OPENING; found = 1; break; } flowid = (flowid + 1) % sc->sc_max_flowrings; } + /* + * We cannot recover from that so far. Only a stop/init + * cycle can revive this if it ever happens at all. + */ if (!found) { printf("%s: no flowring available\n", DEVNAME(sc)); return; } + cmd.m = m; cmd.prio = prio; cmd.flowid = flowid; - memcpy(cmd.da, mtod(m, char *) + 0 * ETHER_ADDR_LEN, ETHER_ADDR_LEN); - memcpy(cmd.sa, mtod(m, char *) + 1 * ETHER_ADDR_LEN, ETHER_ADDR_LEN); bwfm_do_async(&sc->sc_sc, bwfm_pci_flowring_create_cb, &cmd, sizeof(cmd)); } @@ -1546,14 +1587,14 @@ bwfm_pci_flowring_create_cb(struct bwfm_softc *bwfm, void *arg) struct bwfm_cmd_flowring_create *cmd = arg; struct msgbuf_tx_flowring_create_req *req; struct bwfm_pci_msgring *ring; - int flowid, prio; + uint8_t *da, *sa; - flowid = cmd->flowid; - prio = cmd->prio; + da = mtod(cmd->m, char *) + 0 * ETHER_ADDR_LEN; + sa = mtod(cmd->m, char *) + 1 * ETHER_ADDR_LEN; - ring = &sc->sc_flowrings[flowid]; - if (ring->status != RING_CLOSED) { - printf("%s: flowring not closed\n", DEVNAME(sc)); + ring = &sc->sc_flowrings[cmd->flowid]; + if (ring->status != RING_OPENING) { + printf("%s: flowring not opening\n", DEVNAME(sc)); return; } @@ -1569,20 +1610,21 @@ bwfm_pci_flowring_create_cb(struct bwfm_softc *bwfm, void *arg) } ring->status = RING_OPENING; - ring->fifo = bwfm_pci_prio2fifo[prio]; - memcpy(ring->mac, cmd->da, ETHER_ADDR_LEN); + ring->fifo = bwfm_pci_prio2fifo[cmd->prio]; + ring->m = cmd->m; + memcpy(ring->mac, da, ETHER_ADDR_LEN); #ifndef IEEE80211_STA_ONLY - if (ic->ic_opmode == IEEE80211_M_HOSTAP && ETHER_IS_MULTICAST(cmd->da)) + if (ic->ic_opmode == IEEE80211_M_HOSTAP && ETHER_IS_MULTICAST(da)) memcpy(ring->mac, etherbroadcastaddr, ETHER_ADDR_LEN); #endif req->msg.msgtype = MSGBUF_TYPE_FLOW_RING_CREATE; req->msg.ifidx = 0; req->msg.request_id = 0; - req->tid = bwfm_pci_prio2fifo[prio]; - req->flow_ring_id = letoh16(flowid + 2); - memcpy(req->da, cmd->da, ETHER_ADDR_LEN); - memcpy(req->sa, cmd->sa, ETHER_ADDR_LEN); + req->tid = bwfm_pci_prio2fifo[cmd->prio]; + req->flow_ring_id = letoh16(cmd->flowid + 2); + memcpy(req->da, da, ETHER_ADDR_LEN); + memcpy(req->sa, sa, ETHER_ADDR_LEN); req->flow_ring_addr.high_addr = letoh32(BWFM_PCI_DMA_DVA(ring->ring) >> 32); req->flow_ring_addr.low_addr = @@ -1636,6 +1678,28 @@ bwfm_pci_stop(struct bwfm_softc *bwfm) } } +int +bwfm_pci_txcheck(struct bwfm_softc *bwfm) +{ + struct bwfm_pci_softc *sc = (void *)bwfm; + struct bwfm_pci_msgring *ring; + int i; + + /* If we are transitioning, we cannot send. */ + for (i = 0; i < sc->sc_max_flowrings; i++) { + ring = &sc->sc_flowrings[i]; + if (ring->status == RING_OPENING) + return ENOBUFS; + } + + if (bwfm_pci_pktid_avail(sc, &sc->sc_tx_pkts)) { + sc->sc_tx_pkts_full = 1; + return ENOBUFS; + } + + return 0; +} + int bwfm_pci_txdata(struct bwfm_softc *bwfm, struct mbuf *m) { @@ -1648,8 +1712,17 @@ bwfm_pci_txdata(struct bwfm_softc *bwfm, struct mbuf *m) flowid = bwfm_pci_flowring_lookup(sc, m); if (flowid < 0) { + /* + * We cannot send the packet right now as there is + * no flowring yet. The flowring will be created + * asynchronously. While the ring is transitioning + * the TX check will tell the upper layers that we + * cannot send packets right now. When the flowring + * is created the queue will be restarted and this + * mbuf will be transmitted. + */ bwfm_pci_flowring_create(sc, m); - return ENOBUFS; + return 0; } ring = &sc->sc_flowrings[flowid]; @@ -1676,7 +1749,11 @@ bwfm_pci_txdata(struct bwfm_softc *bwfm, struct mbuf *m) ret = bwfm_pci_pktid_new(sc, &sc->sc_tx_pkts, m, &pktid, &paddr); if (ret) { - sc->sc_tx_pkts_full = 1; + if (ret == ENOBUFS) { + printf("%s: no pktid available for TX\n", + DEVNAME(sc)); + sc->sc_tx_pkts_full = 1; + } bwfm_pci_ring_write_cancel(sc, ring, 1); return ret; } diff --git a/sys/dev/usb/if_bwfm_usb.c b/sys/dev/usb/if_bwfm_usb.c index e4e3061151b..dbc47311407 100644 --- a/sys/dev/usb/if_bwfm_usb.c +++ b/sys/dev/usb/if_bwfm_usb.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_bwfm_usb.c,v 1.9 2018/02/07 22:01:04 patrick Exp $ */ +/* $OpenBSD: if_bwfm_usb.c,v 1.10 2018/02/08 05:00:38 patrick Exp $ */ /* * Copyright (c) 2010-2016 Broadcom Corporation * Copyright (c) 2016,2017 Patrick Wildt @@ -196,6 +196,7 @@ void bwfm_usb_free_rx_list(struct bwfm_usb_softc *); int bwfm_usb_alloc_tx_list(struct bwfm_usb_softc *); void bwfm_usb_free_tx_list(struct bwfm_usb_softc *); +int bwfm_usb_txcheck(struct bwfm_softc *); int bwfm_usb_txdata(struct bwfm_softc *, struct mbuf *); int bwfm_usb_txctl(struct bwfm_softc *, char *, size_t); int bwfm_usb_rxctl(struct bwfm_softc *, char *, size_t *); @@ -207,6 +208,7 @@ void bwfm_usb_txeof(struct usbd_xfer *, void *, usbd_status); struct bwfm_bus_ops bwfm_usb_bus_ops = { .bs_init = NULL, .bs_stop = NULL, + .bs_txcheck = bwfm_usb_txcheck, .bs_txdata = bwfm_usb_txdata, .bs_txctl = bwfm_usb_txctl, .bs_rxctl = bwfm_usb_rxctl, @@ -588,8 +590,7 @@ bwfm_usb_txeof(struct usbd_xfer *xfer, void *priv, usbd_status status) /* We just released a Tx buffer, notify Tx. */ if (ifq_is_oactive(&ifp->if_snd)) { - ifq_clr_oactive(&ifp->if_snd); - ifp->if_start(ifp); + ifq_restart(&ifp->if_snd); } splx(s); } @@ -727,6 +728,17 @@ err: return 1; } +int +bwfm_usb_txcheck(struct bwfm_softc *bwfm) +{ + struct bwfm_usb_softc *sc = (void *)bwfm; + + if (TAILQ_EMPTY(&sc->sc_tx_free_list)) + return ENOBUFS; + + return 0; +} + int bwfm_usb_txdata(struct bwfm_softc *bwfm, struct mbuf *m) { -- 2.20.1