iwm(4) currently sets the SINGLE_FRAME_MSK bit of the Rx config register.
This bit tells the firmware that the driver wants one interrupt per frame,
i.e. it prevents the firmware from placing more than one frame into the 4k RX
buffer. Frames can contain firmware command responses and actual 802.11 frames.

With more than one frame in the RX buffer we get less interrupts but the
driver has to carefully split smaller frames out of the 4k buffer.
I used m_copym() for this purpose. Making sure that mbufs are freed becomes
a bit more complicated. If you review this diff please pay close attention
to this.

I worry about future firmware versions dropping support for single-frame
RX altogether. Linux has stopped setting this bit years ago and I have
been told that Intel's firmware engineers do not test other configurations.

I found what looks like evidence for this lack of testing while working on
monitor mode support. It seems the firmware does not respect the single-frame
bit in monitor mode and ends up sending junk. Combined with the diff below
I have not been able to reproduce this issue.

Dragonfly flipped this bit in commit b5eb43f0280bbcfd26af51cf5a4b8e8ff3590b67.

I have tested my diff on 7260, 7265, and 8260 devices.
Does anyone want to test other devices before I commit this?

Any concerns?

Index: if_iwm.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.169
diff -u -p -r1.169 if_iwm.c
--- if_iwm.c    21 Apr 2017 16:40:11 -0000      1.169
+++ if_iwm.c    22 Apr 2017 17:11:31 -0000
@@ -362,8 +362,8 @@ int iwm_get_signal_strength(struct iwm_s
 void   iwm_rx_rx_phy_cmd(struct iwm_softc *, struct iwm_rx_packet *,
            struct iwm_rx_data *);
 int    iwm_get_noise(const struct iwm_statistics_rx_non_phy *);
-void   iwm_rx_rx_mpdu(struct iwm_softc *, struct iwm_rx_packet *,
-           struct iwm_rx_data *);
+void   iwm_rx_mpdu(struct iwm_softc *, struct mbuf *);
+void   iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *);
 void   iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
            struct iwm_node *);
 void   iwm_rx_tx_cmd(struct iwm_softc *, struct iwm_rx_packet *,
@@ -449,6 +449,7 @@ const char *iwm_desc_lookup(uint32_t);
 void   iwm_nic_error(struct iwm_softc *);
 void   iwm_nic_umac_error(struct iwm_softc *);
 #endif
+int    iwm_rx_frame(struct iwm_softc *, struct mbuf *);
 void   iwm_notif_intr(struct iwm_softc *);
 int    iwm_intr(void *);
 int    iwm_match(struct device *, void *, void *);
@@ -1672,7 +1673,6 @@ iwm_nic_rx_init(struct iwm_softc *sc)
            IWM_FH_RCSR_RX_CONFIG_CHNL_EN_ENABLE_VAL            |
            IWM_FH_RCSR_CHNL0_RX_IGNORE_RXF_EMPTY               |  /* HW bug */
            IWM_FH_RCSR_CHNL0_RX_CONFIG_IRQ_DEST_INT_HOST_VAL   |
-           IWM_FH_RCSR_CHNL0_RX_CONFIG_SINGLE_FRAME_MSK        |
            (IWM_RX_RB_TIMEOUT << IWM_FH_RCSR_RX_CONFIG_REG_IRQ_RBTH_POS) |
            IWM_FH_RCSR_RX_CONFIG_REG_VAL_RB_SIZE_4K            |
            IWM_RX_QUEUE_SIZE_LOG << IWM_FH_RCSR_RX_CONFIG_RBDCB_SIZE_POS);
@@ -3296,43 +3296,23 @@ iwm_get_noise(const struct iwm_statistic
        return (nbant == 0) ? -127 : (total / nbant) - 107;
 }
 
-void
-iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_packet *pkt,
-    struct iwm_rx_data *data)
+int
+iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m)
 {
        struct ieee80211com *ic = &sc->sc_ic;
        struct ieee80211_frame *wh;
        struct ieee80211_node *ni;
        struct ieee80211_channel *c = NULL;
        struct ieee80211_rxinfo rxi;
-       struct mbuf *m;
        struct iwm_rx_phy_info *phy_info;
-       struct iwm_rx_mpdu_res_start *rx_res;
        int device_timestamp;
-       uint32_t len;
-       uint32_t rx_pkt_status;
        int rssi;
 
-       bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
-           BUS_DMASYNC_POSTREAD);
-
        phy_info = &sc->sc_last_phy_info;
-       rx_res = (struct iwm_rx_mpdu_res_start *)pkt->data;
-       wh = (struct ieee80211_frame *)(pkt->data + sizeof(*rx_res));
-       len = le16toh(rx_res->byte_count);
-       rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
-           sizeof(*rx_res) + len));
-
-       m = data->m;
-       m->m_data = pkt->data + sizeof(*rx_res);
-       m->m_pkthdr.len = m->m_len = len;
+       wh = mtod(m, struct ieee80211_frame *);
 
        if (__predict_false(phy_info->cfg_phy_cnt > 20))
-               return;
-
-       if (!(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) ||
-           !(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK))
-               return; /* drop */
+               return EINVAL;
 
        device_timestamp = le32toh(phy_info->system_timestamp);
 
@@ -3344,9 +3324,6 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, str
        rssi = (0 - IWM_MIN_DBM) + rssi;        /* normalize */
        rssi = MIN(rssi, ic->ic_max_rssi);      /* clip to max. 100% */
 
-       if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur) != 0)
-               return;
-
        if (le32toh(phy_info->channel) < nitems(ic->ic_channels))
                c = &ic->ic_channels[le32toh(phy_info->channel)];
 
@@ -3415,6 +3392,8 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, str
 #endif
        ieee80211_input(IC2IFP(ic), m, ni, &rxi);
        ieee80211_release_node(ic, ni);
+
+       return 0;
 }
 
 void
@@ -6472,47 +6451,97 @@ do {                                                    
                \
 #define ADVANCE_RXQ(sc) (sc->rxq.cur = (sc->rxq.cur + 1) % IWM_RX_RING_COUNT);
 
 void
-iwm_notif_intr(struct iwm_softc *sc)
+iwm_rx_mpdu(struct iwm_softc *sc, struct mbuf *m)
 {
-       uint16_t hw;
+       struct ifnet *ifp = IC2IFP(&sc->sc_ic);
+       struct iwm_rx_packet *pkt;
+       struct iwm_rx_mpdu_res_start *rx_res;
+       uint32_t len;
+       uint32_t rx_pkt_status;
+       int rxfail;
 
-       bus_dmamap_sync(sc->sc_dmat, sc->rxq.stat_dma.map,
-           0, sc->rxq.stat_dma.size, BUS_DMASYNC_POSTREAD);
+       pkt = mtod(m, struct iwm_rx_packet *);
+       rx_res = (struct iwm_rx_mpdu_res_start *)pkt->data;
+       len = le16toh(rx_res->byte_count);
+       rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
+           sizeof(*rx_res) + len));
 
-       hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
-       while (sc->rxq.cur != hw) {
-               struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
-               struct iwm_rx_packet *pkt;
-               struct iwm_cmd_response *cresp;
-               int qid, idx, code;
-
-               bus_dmamap_sync(sc->sc_dmat, data->map, 0, sizeof(*pkt),
-                   BUS_DMASYNC_POSTREAD);
-               pkt = mtod(data->m, struct iwm_rx_packet *);
+       rxfail = ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) == 0 ||
+           (rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK) == 0);
+       if (rxfail) {
+               ifp->if_ierrors++;
+               m_freem(m);
+               return;
+       }
 
+       /* Extract the 802.11 frame. */
+       m->m_data = (caddr_t)pkt->data + sizeof(*rx_res);
+       m->m_pkthdr.len = m->m_len = len;
+       if (iwm_rx_frame(sc, m) != 0) {
+               ifp->if_ierrors++;
+               m_freem(m);
+       }
+}
+
+void
+iwm_rx_pkt(struct iwm_softc *sc, struct iwm_rx_data *data)
+{
+       struct ifnet *ifp = IC2IFP(&sc->sc_ic);
+       struct iwm_rx_packet *pkt;
+       struct iwm_cmd_response *cresp;
+       uint32_t offset = 0, nmpdu = 0, len;
+       struct mbuf *m0;
+       const uint32_t minsz = sizeof(uint32_t) + sizeof(struct iwm_cmd_header);
+       int qid, idx, code;
+
+       bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
+           BUS_DMASYNC_POSTREAD);
+
+       m0 = data->m;
+       while (offset + minsz < IWM_RBUF_SIZE) {
+               pkt = (struct iwm_rx_packet *)(m0->m_data + offset);
                qid = pkt->hdr.qid & ~0x80;
                idx = pkt->hdr.idx;
-
                code = IWM_WIDE_ID(pkt->hdr.flags, pkt->hdr.code);
 
-               /*
-                * randomly get these from the firmware, no idea why.
-                * they at least seem harmless, so just ignore them for now
-                */
-               if (__predict_false((pkt->hdr.code == 0 && qid == 0 && idx == 0)
-                   || pkt->len_n_flags == htole32(0x55550000))) {
-                       ADVANCE_RXQ(sc);
-                       continue;
+               if ((code == 0 && qid == 0 && idx == 0) ||
+                   pkt->len_n_flags == htole32(IWM_FH_RSCSR_FRAME_INVALID)) {
+                       break;
                }
 
+               if (code == IWM_REPLY_RX_MPDU_CMD && ++nmpdu == 1) {
+                       /* Take mbuf m0 off the RX ring. */
+                       if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur)) {
+                               ifp->if_ierrors++;
+                               break;
+                       }
+                       KASSERT(data->m != m0);
+               }
+
+               len = le32toh(pkt->len_n_flags) & IWM_FH_RSCSR_FRAME_SIZE_MSK;
+               len += sizeof(uint32_t); /* account for status word */
+
                switch (code) {
                case IWM_REPLY_RX_PHY_CMD:
                        iwm_rx_rx_phy_cmd(sc, pkt, data);
                        break;
 
-               case IWM_REPLY_RX_MPDU_CMD:
-                       iwm_rx_rx_mpdu(sc, pkt, data);
+               case IWM_REPLY_RX_MPDU_CMD: {
+                       struct mbuf *m = m_copym(m0, offset, len, M_DONTWAIT);
+                       if (m == NULL) {
+                               ifp->if_ierrors++;
+                               break;
+                       }
+                       if (offset != 0) {
+                               if (m_dup_pkthdr(m, m0, M_DONTWAIT) != 0)  {
+                                       m_freem(m);
+                                       ifp->if_ierrors++;
+                                       break;
+                               }
+                       }
+                       iwm_rx_mpdu(sc, m);
                        break;
+               }
 
                case IWM_TX_CMD:
                        iwm_rx_tx_cmd(sc, pkt, data);
@@ -6731,6 +6760,26 @@ iwm_notif_intr(struct iwm_softc *sc)
                        iwm_cmd_done(sc, pkt);
                }
 
+               offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
+       }
+
+       if (m0 != data->m)
+               m_freem(m0);
+}
+
+void
+iwm_notif_intr(struct iwm_softc *sc)
+{
+       uint16_t hw;
+
+       bus_dmamap_sync(sc->sc_dmat, sc->rxq.stat_dma.map,
+           0, sc->rxq.stat_dma.size, BUS_DMASYNC_POSTREAD);
+
+       hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
+       while (sc->rxq.cur != hw) {
+               struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
+
+               iwm_rx_pkt(sc, data);
                ADVANCE_RXQ(sc);
        }
 
Index: if_iwmreg.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_iwmreg.h,v
retrieving revision 1.25
diff -u -p -r1.25 if_iwmreg.h
--- if_iwmreg.h 4 Apr 2017 00:40:52 -0000       1.25
+++ if_iwmreg.h 22 Apr 2017 16:33:40 -0000
@@ -5876,7 +5876,13 @@ struct iwm_rx_packet {
         * 31:    flag flush RB request
         * 30:    flag ignore TC (terminal counter) request
         * 29:    flag fast IRQ request
-        * 28-14: Reserved
+        * 28-26: Reserved
+        * 25:    Offload enabled
+        * 24:    RPF enabled
+        * 23:    RSS enabled
+        * 22:    Checksum enabled
+        * 21-16: RX queue
+        * 15-14: Reserved
         * 13-00: RX frame size
         */
        uint32_t len_n_flags;
@@ -5885,6 +5891,11 @@ struct iwm_rx_packet {
 } __packed;
 
 #define        IWM_FH_RSCSR_FRAME_SIZE_MSK     0x00003fff
+#define        IWM_FH_RSCSR_FRAME_INVALID      0x55550000
+#define        IWM_FH_RSCSR_FRAME_ALIGN        0x40
+#define        IWM_FH_RSCSR_RPA_EN             (1 << 25)
+#define        IWM_FH_RSCSR_RXQ_POS            16
+#define        IWM_FH_RSCSR_RXQ_MASK           0x3F0000
 
 static uint32_t
 iwm_rx_packet_len(const struct iwm_rx_packet *pkt)

Reply via email to