iwm: support more than one frame per interrupt

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

iwm: support more than one frame per interrupt

Stefan Sperling-5
This adds support for receiving more than one frame per interrupt to iwm(4).
There have been several past attempts at this already, all of which failed
due to stability issues found during testing.

Previous versions of this diff included monitor mode support. I have stripped
this out for now and I am looking for stability tests in client mode only.
If this diff passes through testing this time around, adding monitor mode
on top will be easy.

Please update your tree before applying this and make sure your iwm device
is using either -17 or -34 firmware.
 
diff aa91687117b06dc9f357920a56ef1a22c0de6a7e 662ed8a1ce51441cd90dfc355f27a6814968d0b9
blob - f7449f560623517dfa1b3fbe6d55b10a5494e26c
blob + 3fef13e8baf3c1564493717b3cf7750447faba4e
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -366,8 +366,10 @@ int iwm_get_signal_strength(struct iwm_softc *, struct
 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 *, struct mbuf_list *);
+int iwm_rx_frame(struct iwm_softc *, struct mbuf *, uint32_t,
+    struct mbuf_list *);
+int iwm_ccmp_decap(struct iwm_softc *, struct mbuf *,
+    struct ieee80211_node *);
 void iwm_enable_ht_cck_fallback(struct iwm_softc *, struct iwm_node *);
 void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
     struct iwm_node *);
@@ -428,7 +430,7 @@ uint8_t iwm_ridx2rate(struct ieee80211_rateset *, int)
 int iwm_rval2ridx(int);
 void iwm_ack_rates(struct iwm_softc *, struct iwm_node *, int *, int *);
 void iwm_mac_ctxt_cmd_common(struct iwm_softc *, struct iwm_node *,
-    struct iwm_mac_ctx_cmd *, uint32_t, int);
+    struct iwm_mac_ctx_cmd *, uint32_t);
 void iwm_mac_ctxt_cmd_fill_sta(struct iwm_softc *, struct iwm_node *,
     struct iwm_mac_data_sta *, int);
 int iwm_mac_ctxt_cmd(struct iwm_softc *, struct iwm_node *, uint32_t, int);
@@ -472,6 +474,11 @@ const char *iwm_desc_lookup(uint32_t);
 void iwm_nic_error(struct iwm_softc *);
 void iwm_nic_umac_error(struct iwm_softc *);
 #endif
+void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, size_t,
+    struct mbuf_list *);
+int iwm_rx_pkt_valid(struct iwm_rx_packet *);
+void iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *,
+    struct mbuf_list *);
 void iwm_notif_intr(struct iwm_softc *);
 int iwm_intr(void *);
 int iwm_match(struct device *, void *, void *);
@@ -1807,7 +1814,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);
@@ -3539,56 +3545,24 @@ iwm_get_noise(const struct iwm_statistics_rx_non_phy *
  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, struct mbuf_list *ml)
+int
+iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m, uint32_t rx_pkt_status,
+    struct mbuf_list *ml)
 {
  struct ieee80211com *ic = &sc->sc_ic;
  struct ieee80211_frame *wh;
  struct ieee80211_node *ni;
  struct ieee80211_rxinfo rxi;
  struct ieee80211_channel *bss_chan;
- 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, chanidx;
  uint8_t saved_bssid[IEEE80211_ADDR_LEN] = { 0 };
 
- 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);
- if (len < IEEE80211_MIN_LEN) {
- ic->ic_stats.is_rx_tooshort++;
- IC2IFP(ic)->if_ierrors++;
- return;
- }
- if (len > IWM_RBUF_SIZE - sizeof(*rx_res)) {
- IC2IFP(ic)->if_ierrors++;
- return;
- }
- rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
-    sizeof(*rx_res) + len));
-
  if (__predict_false(phy_info->cfg_phy_cnt > 20))
- return;
+ return EINVAL;
 
- if (!(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) ||
-    !(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK))
- return; /* drop */
-
- m = data->m;
- if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur) != 0)
- return;
- m->m_data = pkt->data + sizeof(*rx_res);
- m->m_pkthdr.len = m->m_len = len;
-
  device_timestamp = le32toh(phy_info->system_timestamp);
 
  rssi = iwm_get_signal_strength(sc, phy_info);
@@ -3599,6 +3573,7 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
  if (chanidx < 0 || chanidx >= nitems(ic->ic_channels))
  chanidx = ieee80211_chan2ieee(ic, ic->ic_ibss_chan);
 
+ wh = mtod(m, struct ieee80211_frame *);
  ni = ieee80211_find_rxnode(ic, wh);
  if (ni == ic->ic_bss) {
  /*
@@ -3672,6 +3647,8 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
  if (ni == ic->ic_bss && IEEE80211_ADDR_EQ(saved_bssid, ni->ni_macaddr))
  ni->ni_chan = bss_chan;
  ieee80211_release_node(ic, ni);
+
+ return 0;
 }
 
 void
@@ -7525,38 +7502,103 @@ 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, size_t maxlen,
+    struct mbuf_list *ml)
 {
- struct mbuf_list ml = MBUF_LIST_INITIALIZER();
- uint16_t hw;
+ struct ieee80211com *ic = &sc->sc_ic;
+ struct ifnet *ifp = IC2IFP(ic);
+ struct iwm_rx_packet *pkt;
+ struct iwm_rx_mpdu_res_start *rx_res;
+ uint16_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);
+ if (len < IEEE80211_MIN_LEN) {
+ ic->ic_stats.is_rx_tooshort++;
+ IC2IFP(ic)->if_ierrors++;
+ m_freem(m);
+ return;
+ }
+ if (len + sizeof(*rx_res) + sizeof(rx_pkt_status) > maxlen ||
+    len > IEEE80211_MAX_LEN) {
+ IC2IFP(ic)->if_ierrors++;
+ m_freem(m);
+ return;
+ }
 
- hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
- hw &= (IWM_RX_RING_COUNT - 1);
- while (sc->rxq.cur != hw) {
- struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
- struct iwm_rx_packet *pkt;
- int qid, idx, code, handled = 1;
+ memcpy(&rx_pkt_status, pkt->data + sizeof(*rx_res) + len,
+    sizeof(rx_pkt_status));
+ rx_pkt_status = le32toh(rx_pkt_status);
+ 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;
+ }
 
- bus_dmamap_sync(sc->sc_dmat, data->map, 0, sizeof(*pkt),
-    BUS_DMASYNC_POSTREAD);
- pkt = mtod(data->m, struct iwm_rx_packet *);
+ /* 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, rx_pkt_status, ml) != 0) {
+ ifp->if_ierrors++;
+ m_freem(m);
+ }
+}
 
+int
+iwm_rx_pkt_valid(struct iwm_rx_packet *pkt)
+{
+ int qid, idx, code;
+
+ qid = pkt->hdr.qid & ~0x80;
+ idx = pkt->hdr.idx;
+ code = IWM_WIDE_ID(pkt->hdr.flags, pkt->hdr.code);
+
+ return (!(qid == 0 && idx == 0 && code == 0) &&
+    pkt->len_n_flags != htole32(IWM_FH_RSCSR_FRAME_INVALID));
+}
+
+void
+iwm_rx_pkt(struct iwm_softc *sc, struct iwm_rx_data *data, struct mbuf_list *ml)
+{
+ struct ifnet *ifp = IC2IFP(&sc->sc_ic);
+ struct iwm_rx_packet *pkt, *nextpkt;
+ uint32_t offset = 0, nextoff = 0, nmpdu = 0, len;
+ struct mbuf *m0, *m;
+ const size_t minsz = sizeof(pkt->len_n_flags) + sizeof(pkt->hdr);
+ size_t remain = IWM_RBUF_SIZE;
+ int qid, idx, code, handled = 1;
+
+ bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
+    BUS_DMASYNC_POSTREAD);
+
+ m0 = data->m;
+ while (m0 && offset + minsz < IWM_RBUF_SIZE) {
+ pkt = (struct iwm_rx_packet *)(m0->m_data + offset);
  qid = pkt->hdr.qid;
  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 & ~0x80) == 0
-    && idx == 0) || pkt->len_n_flags == htole32(0x55550000))) {
- ADVANCE_RXQ(sc);
- continue;
+ if (!iwm_rx_pkt_valid(pkt))
+ break;
+
+ len = sizeof(pkt->len_n_flags) + iwm_rx_packet_len(pkt);
+ if (len < sizeof(pkt->hdr) ||
+    len > (IWM_RBUF_SIZE - offset - minsz))
+ 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);
  }
 
  switch (code) {
@@ -7564,10 +7606,40 @@ iwm_notif_intr(struct iwm_softc *sc)
  iwm_rx_rx_phy_cmd(sc, pkt, data);
  break;
 
- case IWM_REPLY_RX_MPDU_CMD:
- iwm_rx_rx_mpdu(sc, pkt, data, &ml);
- break;
+ case IWM_REPLY_RX_MPDU_CMD: {
+ nextoff = offset +
+    roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
+ nextpkt = (struct iwm_rx_packet *)
+    (m0->m_data + nextoff);
+ if (nextoff + minsz >= IWM_RBUF_SIZE ||
+    !iwm_rx_pkt_valid(nextpkt)) {
+ /* No need to copy last frame in buffer. */
+ if (offset > 0)
+ m_adj(m0, offset);
+ iwm_rx_mpdu(sc, m0, remain - minsz, ml);
+ m0 = NULL; /* stack owns m0 now; abort loop */
+ } else {
+ /*
+ * Create an mbuf which points to the current
+ * packet. Always copy from offset zero to
+ * preserve m_pkthdr.
+ */
+ m = m_copym(m0, 0, M_COPYALL, M_DONTWAIT);
+ if (m == NULL) {
+ ifp->if_ierrors++;
+ break;
+ }
+ m_adj(m, offset);
+ iwm_rx_mpdu(sc, m, remain - minsz, ml);
+ }
 
+ if (offset + minsz < remain)
+ remain -= offset;
+ else
+ remain = minsz;
+ break;
+ }
+
  case IWM_TX_CMD:
  iwm_rx_tx_cmd(sc, pkt, data);
  break;
@@ -7818,6 +7890,27 @@ iwm_notif_intr(struct iwm_softc *sc)
  iwm_cmd_done(sc, qid, idx, code);
  }
 
+ offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
+ }
+
+ if (m0 && m0 != data->m)
+ m_freem(m0);
+}
+
+void
+iwm_notif_intr(struct iwm_softc *sc)
+{
+ struct mbuf_list ml = MBUF_LIST_INITIALIZER();
+ 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;
+ hw &= (IWM_RX_RING_COUNT - 1);
+ while (sc->rxq.cur != hw) {
+ struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
+ iwm_rx_pkt(sc, data, &ml);
  ADVANCE_RXQ(sc);
  }
  if_input(&sc->sc_ic.ic_if, &ml);

Reply | Threaded
Open this post in threaded view
|

Re: iwm: support more than one frame per interrupt

Stefan Sperling-5
On Fri, Nov 08, 2019 at 07:13:17PM +0200, Stefan Sperling wrote:
> Previous versions of this diff included monitor mode support. I have stripped
> this out for now and I am looking for stability tests in client mode only.
> If this diff passes through testing this time around, adding monitor mode
> on top will be easy.

Sorry, I made a mistake when splitting the diff. The diff I sent doesn't build.
I will post a new one shortly.

Reply | Threaded
Open this post in threaded view
|

Re: iwm: support more than one frame per interrupt

Stefan Sperling-5
On Fri, Nov 08, 2019 at 07:15:18PM +0200, Stefan Sperling wrote:
> On Fri, Nov 08, 2019 at 07:13:17PM +0200, Stefan Sperling wrote:
> > Previous versions of this diff included monitor mode support. I have stripped
> > this out for now and I am looking for stability tests in client mode only.
> > If this diff passes through testing this time around, adding monitor mode
> > on top will be easy.
>
> Sorry, I made a mistake when splitting the diff. The diff I sent doesn't build.
> I will post a new one shortly.

Fixed diff.

diff aa91687117b06dc9f357920a56ef1a22c0de6a7e 16cec9db4484c7e9e6fa4838707a4c0c7ba2f314
blob - f7449f560623517dfa1b3fbe6d55b10a5494e26c
blob + 0d5f766801f75e839476ebc2393568ed31494e62
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -366,8 +366,10 @@ int iwm_get_signal_strength(struct iwm_softc *, struct
 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 *, struct mbuf_list *);
+int iwm_rx_frame(struct iwm_softc *, struct mbuf *, uint32_t,
+    struct mbuf_list *);
+int iwm_ccmp_decap(struct iwm_softc *, struct mbuf *,
+    struct ieee80211_node *);
 void iwm_enable_ht_cck_fallback(struct iwm_softc *, struct iwm_node *);
 void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
     struct iwm_node *);
@@ -472,6 +474,11 @@ const char *iwm_desc_lookup(uint32_t);
 void iwm_nic_error(struct iwm_softc *);
 void iwm_nic_umac_error(struct iwm_softc *);
 #endif
+void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, size_t,
+    struct mbuf_list *);
+int iwm_rx_pkt_valid(struct iwm_rx_packet *);
+void iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *,
+    struct mbuf_list *);
 void iwm_notif_intr(struct iwm_softc *);
 int iwm_intr(void *);
 int iwm_match(struct device *, void *, void *);
@@ -1807,7 +1814,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);
@@ -3539,56 +3545,24 @@ iwm_get_noise(const struct iwm_statistics_rx_non_phy *
  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, struct mbuf_list *ml)
+int
+iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m, uint32_t rx_pkt_status,
+    struct mbuf_list *ml)
 {
  struct ieee80211com *ic = &sc->sc_ic;
  struct ieee80211_frame *wh;
  struct ieee80211_node *ni;
  struct ieee80211_rxinfo rxi;
  struct ieee80211_channel *bss_chan;
- 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, chanidx;
  uint8_t saved_bssid[IEEE80211_ADDR_LEN] = { 0 };
 
- 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);
- if (len < IEEE80211_MIN_LEN) {
- ic->ic_stats.is_rx_tooshort++;
- IC2IFP(ic)->if_ierrors++;
- return;
- }
- if (len > IWM_RBUF_SIZE - sizeof(*rx_res)) {
- IC2IFP(ic)->if_ierrors++;
- return;
- }
- rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
-    sizeof(*rx_res) + len));
-
  if (__predict_false(phy_info->cfg_phy_cnt > 20))
- return;
+ return EINVAL;
 
- if (!(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) ||
-    !(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK))
- return; /* drop */
-
- m = data->m;
- if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur) != 0)
- return;
- m->m_data = pkt->data + sizeof(*rx_res);
- m->m_pkthdr.len = m->m_len = len;
-
  device_timestamp = le32toh(phy_info->system_timestamp);
 
  rssi = iwm_get_signal_strength(sc, phy_info);
@@ -3599,6 +3573,7 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
  if (chanidx < 0 || chanidx >= nitems(ic->ic_channels))
  chanidx = ieee80211_chan2ieee(ic, ic->ic_ibss_chan);
 
+ wh = mtod(m, struct ieee80211_frame *);
  ni = ieee80211_find_rxnode(ic, wh);
  if (ni == ic->ic_bss) {
  /*
@@ -3672,6 +3647,8 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
  if (ni == ic->ic_bss && IEEE80211_ADDR_EQ(saved_bssid, ni->ni_macaddr))
  ni->ni_chan = bss_chan;
  ieee80211_release_node(ic, ni);
+
+ return 0;
 }
 
 void
@@ -7525,38 +7502,103 @@ 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, size_t maxlen,
+    struct mbuf_list *ml)
 {
- struct mbuf_list ml = MBUF_LIST_INITIALIZER();
- uint16_t hw;
+ struct ieee80211com *ic = &sc->sc_ic;
+ struct ifnet *ifp = IC2IFP(ic);
+ struct iwm_rx_packet *pkt;
+ struct iwm_rx_mpdu_res_start *rx_res;
+ uint16_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);
+ if (len < IEEE80211_MIN_LEN) {
+ ic->ic_stats.is_rx_tooshort++;
+ IC2IFP(ic)->if_ierrors++;
+ m_freem(m);
+ return;
+ }
+ if (len + sizeof(*rx_res) + sizeof(rx_pkt_status) > maxlen ||
+    len > IEEE80211_MAX_LEN) {
+ IC2IFP(ic)->if_ierrors++;
+ m_freem(m);
+ return;
+ }
 
- hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
- hw &= (IWM_RX_RING_COUNT - 1);
- while (sc->rxq.cur != hw) {
- struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
- struct iwm_rx_packet *pkt;
- int qid, idx, code, handled = 1;
+ memcpy(&rx_pkt_status, pkt->data + sizeof(*rx_res) + len,
+    sizeof(rx_pkt_status));
+ rx_pkt_status = le32toh(rx_pkt_status);
+ 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;
+ }
 
- bus_dmamap_sync(sc->sc_dmat, data->map, 0, sizeof(*pkt),
-    BUS_DMASYNC_POSTREAD);
- pkt = mtod(data->m, struct iwm_rx_packet *);
+ /* 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, rx_pkt_status, ml) != 0) {
+ ifp->if_ierrors++;
+ m_freem(m);
+ }
+}
 
+int
+iwm_rx_pkt_valid(struct iwm_rx_packet *pkt)
+{
+ int qid, idx, code;
+
+ qid = pkt->hdr.qid & ~0x80;
+ idx = pkt->hdr.idx;
+ code = IWM_WIDE_ID(pkt->hdr.flags, pkt->hdr.code);
+
+ return (!(qid == 0 && idx == 0 && code == 0) &&
+    pkt->len_n_flags != htole32(IWM_FH_RSCSR_FRAME_INVALID));
+}
+
+void
+iwm_rx_pkt(struct iwm_softc *sc, struct iwm_rx_data *data, struct mbuf_list *ml)
+{
+ struct ifnet *ifp = IC2IFP(&sc->sc_ic);
+ struct iwm_rx_packet *pkt, *nextpkt;
+ uint32_t offset = 0, nextoff = 0, nmpdu = 0, len;
+ struct mbuf *m0, *m;
+ const size_t minsz = sizeof(pkt->len_n_flags) + sizeof(pkt->hdr);
+ size_t remain = IWM_RBUF_SIZE;
+ int qid, idx, code, handled = 1;
+
+ bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
+    BUS_DMASYNC_POSTREAD);
+
+ m0 = data->m;
+ while (m0 && offset + minsz < IWM_RBUF_SIZE) {
+ pkt = (struct iwm_rx_packet *)(m0->m_data + offset);
  qid = pkt->hdr.qid;
  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 & ~0x80) == 0
-    && idx == 0) || pkt->len_n_flags == htole32(0x55550000))) {
- ADVANCE_RXQ(sc);
- continue;
+ if (!iwm_rx_pkt_valid(pkt))
+ break;
+
+ len = sizeof(pkt->len_n_flags) + iwm_rx_packet_len(pkt);
+ if (len < sizeof(pkt->hdr) ||
+    len > (IWM_RBUF_SIZE - offset - minsz))
+ 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);
  }
 
  switch (code) {
@@ -7564,10 +7606,40 @@ iwm_notif_intr(struct iwm_softc *sc)
  iwm_rx_rx_phy_cmd(sc, pkt, data);
  break;
 
- case IWM_REPLY_RX_MPDU_CMD:
- iwm_rx_rx_mpdu(sc, pkt, data, &ml);
- break;
+ case IWM_REPLY_RX_MPDU_CMD: {
+ nextoff = offset +
+    roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
+ nextpkt = (struct iwm_rx_packet *)
+    (m0->m_data + nextoff);
+ if (nextoff + minsz >= IWM_RBUF_SIZE ||
+    !iwm_rx_pkt_valid(nextpkt)) {
+ /* No need to copy last frame in buffer. */
+ if (offset > 0)
+ m_adj(m0, offset);
+ iwm_rx_mpdu(sc, m0, remain - minsz, ml);
+ m0 = NULL; /* stack owns m0 now; abort loop */
+ } else {
+ /*
+ * Create an mbuf which points to the current
+ * packet. Always copy from offset zero to
+ * preserve m_pkthdr.
+ */
+ m = m_copym(m0, 0, M_COPYALL, M_DONTWAIT);
+ if (m == NULL) {
+ ifp->if_ierrors++;
+ break;
+ }
+ m_adj(m, offset);
+ iwm_rx_mpdu(sc, m, remain - minsz, ml);
+ }
 
+ if (offset + minsz < remain)
+ remain -= offset;
+ else
+ remain = minsz;
+ break;
+ }
+
  case IWM_TX_CMD:
  iwm_rx_tx_cmd(sc, pkt, data);
  break;
@@ -7818,6 +7890,27 @@ iwm_notif_intr(struct iwm_softc *sc)
  iwm_cmd_done(sc, qid, idx, code);
  }
 
+ offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
+ }
+
+ if (m0 && m0 != data->m)
+ m_freem(m0);
+}
+
+void
+iwm_notif_intr(struct iwm_softc *sc)
+{
+ struct mbuf_list ml = MBUF_LIST_INITIALIZER();
+ 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;
+ hw &= (IWM_RX_RING_COUNT - 1);
+ while (sc->rxq.cur != hw) {
+ struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
+ iwm_rx_pkt(sc, data, &ml);
  ADVANCE_RXQ(sc);
  }
  if_input(&sc->sc_ic.ic_if, &ml);

Reply | Threaded
Open this post in threaded view
|

Re: iwm: support more than one frame per interrupt

Florian Obser-2
My 7260 doesn't like it. Same problem as last time.
These counters go through the roof and network stalls.
Also when packets are flowing throughput varies a lot.
        30131 input block ack window slides
        30171 input frames above block ack window end
        827766 expected input block ack frames never arrived

On Fri, Nov 08, 2019 at 07:23:36PM +0200, Stefan Sperling wrote:

> On Fri, Nov 08, 2019 at 07:15:18PM +0200, Stefan Sperling wrote:
> > On Fri, Nov 08, 2019 at 07:13:17PM +0200, Stefan Sperling wrote:
> > > Previous versions of this diff included monitor mode support. I have stripped
> > > this out for now and I am looking for stability tests in client mode only.
> > > If this diff passes through testing this time around, adding monitor mode
> > > on top will be easy.
> >
> > Sorry, I made a mistake when splitting the diff. The diff I sent doesn't build.
> > I will post a new one shortly.
>
> Fixed diff.
>
> diff aa91687117b06dc9f357920a56ef1a22c0de6a7e 16cec9db4484c7e9e6fa4838707a4c0c7ba2f314
> blob - f7449f560623517dfa1b3fbe6d55b10a5494e26c
> blob + 0d5f766801f75e839476ebc2393568ed31494e62
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -366,8 +366,10 @@ int iwm_get_signal_strength(struct iwm_softc *, struct
>  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 *, struct mbuf_list *);
> +int iwm_rx_frame(struct iwm_softc *, struct mbuf *, uint32_t,
> +    struct mbuf_list *);
> +int iwm_ccmp_decap(struct iwm_softc *, struct mbuf *,
> +    struct ieee80211_node *);
>  void iwm_enable_ht_cck_fallback(struct iwm_softc *, struct iwm_node *);
>  void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
>      struct iwm_node *);
> @@ -472,6 +474,11 @@ const char *iwm_desc_lookup(uint32_t);
>  void iwm_nic_error(struct iwm_softc *);
>  void iwm_nic_umac_error(struct iwm_softc *);
>  #endif
> +void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, size_t,
> +    struct mbuf_list *);
> +int iwm_rx_pkt_valid(struct iwm_rx_packet *);
> +void iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *,
> +    struct mbuf_list *);
>  void iwm_notif_intr(struct iwm_softc *);
>  int iwm_intr(void *);
>  int iwm_match(struct device *, void *, void *);
> @@ -1807,7 +1814,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);
> @@ -3539,56 +3545,24 @@ iwm_get_noise(const struct iwm_statistics_rx_non_phy *
>   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, struct mbuf_list *ml)
> +int
> +iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m, uint32_t rx_pkt_status,
> +    struct mbuf_list *ml)
>  {
>   struct ieee80211com *ic = &sc->sc_ic;
>   struct ieee80211_frame *wh;
>   struct ieee80211_node *ni;
>   struct ieee80211_rxinfo rxi;
>   struct ieee80211_channel *bss_chan;
> - 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, chanidx;
>   uint8_t saved_bssid[IEEE80211_ADDR_LEN] = { 0 };
>  
> - 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);
> - if (len < IEEE80211_MIN_LEN) {
> - ic->ic_stats.is_rx_tooshort++;
> - IC2IFP(ic)->if_ierrors++;
> - return;
> - }
> - if (len > IWM_RBUF_SIZE - sizeof(*rx_res)) {
> - IC2IFP(ic)->if_ierrors++;
> - return;
> - }
> - rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
> -    sizeof(*rx_res) + len));
> -
>   if (__predict_false(phy_info->cfg_phy_cnt > 20))
> - return;
> + return EINVAL;
>  
> - if (!(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) ||
> -    !(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK))
> - return; /* drop */
> -
> - m = data->m;
> - if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur) != 0)
> - return;
> - m->m_data = pkt->data + sizeof(*rx_res);
> - m->m_pkthdr.len = m->m_len = len;
> -
>   device_timestamp = le32toh(phy_info->system_timestamp);
>  
>   rssi = iwm_get_signal_strength(sc, phy_info);
> @@ -3599,6 +3573,7 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
>   if (chanidx < 0 || chanidx >= nitems(ic->ic_channels))
>   chanidx = ieee80211_chan2ieee(ic, ic->ic_ibss_chan);
>  
> + wh = mtod(m, struct ieee80211_frame *);
>   ni = ieee80211_find_rxnode(ic, wh);
>   if (ni == ic->ic_bss) {
>   /*
> @@ -3672,6 +3647,8 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
>   if (ni == ic->ic_bss && IEEE80211_ADDR_EQ(saved_bssid, ni->ni_macaddr))
>   ni->ni_chan = bss_chan;
>   ieee80211_release_node(ic, ni);
> +
> + return 0;
>  }
>  
>  void
> @@ -7525,38 +7502,103 @@ 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, size_t maxlen,
> +    struct mbuf_list *ml)
>  {
> - struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> - uint16_t hw;
> + struct ieee80211com *ic = &sc->sc_ic;
> + struct ifnet *ifp = IC2IFP(ic);
> + struct iwm_rx_packet *pkt;
> + struct iwm_rx_mpdu_res_start *rx_res;
> + uint16_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);
> + if (len < IEEE80211_MIN_LEN) {
> + ic->ic_stats.is_rx_tooshort++;
> + IC2IFP(ic)->if_ierrors++;
> + m_freem(m);
> + return;
> + }
> + if (len + sizeof(*rx_res) + sizeof(rx_pkt_status) > maxlen ||
> +    len > IEEE80211_MAX_LEN) {
> + IC2IFP(ic)->if_ierrors++;
> + m_freem(m);
> + return;
> + }
>  
> - hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
> - hw &= (IWM_RX_RING_COUNT - 1);
> - while (sc->rxq.cur != hw) {
> - struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
> - struct iwm_rx_packet *pkt;
> - int qid, idx, code, handled = 1;
> + memcpy(&rx_pkt_status, pkt->data + sizeof(*rx_res) + len,
> +    sizeof(rx_pkt_status));
> + rx_pkt_status = le32toh(rx_pkt_status);
> + 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;
> + }
>  
> - bus_dmamap_sync(sc->sc_dmat, data->map, 0, sizeof(*pkt),
> -    BUS_DMASYNC_POSTREAD);
> - pkt = mtod(data->m, struct iwm_rx_packet *);
> + /* 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, rx_pkt_status, ml) != 0) {
> + ifp->if_ierrors++;
> + m_freem(m);
> + }
> +}
>  
> +int
> +iwm_rx_pkt_valid(struct iwm_rx_packet *pkt)
> +{
> + int qid, idx, code;
> +
> + qid = pkt->hdr.qid & ~0x80;
> + idx = pkt->hdr.idx;
> + code = IWM_WIDE_ID(pkt->hdr.flags, pkt->hdr.code);
> +
> + return (!(qid == 0 && idx == 0 && code == 0) &&
> +    pkt->len_n_flags != htole32(IWM_FH_RSCSR_FRAME_INVALID));
> +}
> +
> +void
> +iwm_rx_pkt(struct iwm_softc *sc, struct iwm_rx_data *data, struct mbuf_list *ml)
> +{
> + struct ifnet *ifp = IC2IFP(&sc->sc_ic);
> + struct iwm_rx_packet *pkt, *nextpkt;
> + uint32_t offset = 0, nextoff = 0, nmpdu = 0, len;
> + struct mbuf *m0, *m;
> + const size_t minsz = sizeof(pkt->len_n_flags) + sizeof(pkt->hdr);
> + size_t remain = IWM_RBUF_SIZE;
> + int qid, idx, code, handled = 1;
> +
> + bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
> +    BUS_DMASYNC_POSTREAD);
> +
> + m0 = data->m;
> + while (m0 && offset + minsz < IWM_RBUF_SIZE) {
> + pkt = (struct iwm_rx_packet *)(m0->m_data + offset);
>   qid = pkt->hdr.qid;
>   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 & ~0x80) == 0
> -    && idx == 0) || pkt->len_n_flags == htole32(0x55550000))) {
> - ADVANCE_RXQ(sc);
> - continue;
> + if (!iwm_rx_pkt_valid(pkt))
> + break;
> +
> + len = sizeof(pkt->len_n_flags) + iwm_rx_packet_len(pkt);
> + if (len < sizeof(pkt->hdr) ||
> +    len > (IWM_RBUF_SIZE - offset - minsz))
> + 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);
>   }
>  
>   switch (code) {
> @@ -7564,10 +7606,40 @@ iwm_notif_intr(struct iwm_softc *sc)
>   iwm_rx_rx_phy_cmd(sc, pkt, data);
>   break;
>  
> - case IWM_REPLY_RX_MPDU_CMD:
> - iwm_rx_rx_mpdu(sc, pkt, data, &ml);
> - break;
> + case IWM_REPLY_RX_MPDU_CMD: {
> + nextoff = offset +
> +    roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
> + nextpkt = (struct iwm_rx_packet *)
> +    (m0->m_data + nextoff);
> + if (nextoff + minsz >= IWM_RBUF_SIZE ||
> +    !iwm_rx_pkt_valid(nextpkt)) {
> + /* No need to copy last frame in buffer. */
> + if (offset > 0)
> + m_adj(m0, offset);
> + iwm_rx_mpdu(sc, m0, remain - minsz, ml);
> + m0 = NULL; /* stack owns m0 now; abort loop */
> + } else {
> + /*
> + * Create an mbuf which points to the current
> + * packet. Always copy from offset zero to
> + * preserve m_pkthdr.
> + */
> + m = m_copym(m0, 0, M_COPYALL, M_DONTWAIT);
> + if (m == NULL) {
> + ifp->if_ierrors++;
> + break;
> + }
> + m_adj(m, offset);
> + iwm_rx_mpdu(sc, m, remain - minsz, ml);
> + }
>  
> + if (offset + minsz < remain)
> + remain -= offset;
> + else
> + remain = minsz;
> + break;
> + }
> +
>   case IWM_TX_CMD:
>   iwm_rx_tx_cmd(sc, pkt, data);
>   break;
> @@ -7818,6 +7890,27 @@ iwm_notif_intr(struct iwm_softc *sc)
>   iwm_cmd_done(sc, qid, idx, code);
>   }
>  
> + offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
> + }
> +
> + if (m0 && m0 != data->m)
> + m_freem(m0);
> +}
> +
> +void
> +iwm_notif_intr(struct iwm_softc *sc)
> +{
> + struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> + 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;
> + hw &= (IWM_RX_RING_COUNT - 1);
> + while (sc->rxq.cur != hw) {
> + struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
> + iwm_rx_pkt(sc, data, &ml);
>   ADVANCE_RXQ(sc);
>   }
>   if_input(&sc->sc_ic.ic_if, &ml);
>

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: iwm: support more than one frame per interrupt

Claudio Jeker
On Sat, Nov 09, 2019 at 03:20:10PM +0100, Florian Obser wrote:
> My 7260 doesn't like it. Same problem as last time.
> These counters go through the roof and network stalls.
> Also when packets are flowing throughput varies a lot.
>         30131 input block ack window slides
>         30171 input frames above block ack window end
>         827766 expected input block ack frames never arrived

I have similar issues with a 8265:
        65731 input frames above block ack window end
        65711 input block ack window slides
        150884 expected input block ack frames never arrived

It works but feels slower than before.
 

> On Fri, Nov 08, 2019 at 07:23:36PM +0200, Stefan Sperling wrote:
> > On Fri, Nov 08, 2019 at 07:15:18PM +0200, Stefan Sperling wrote:
> > > On Fri, Nov 08, 2019 at 07:13:17PM +0200, Stefan Sperling wrote:
> > > > Previous versions of this diff included monitor mode support. I have stripped
> > > > this out for now and I am looking for stability tests in client mode only.
> > > > If this diff passes through testing this time around, adding monitor mode
> > > > on top will be easy.
> > >
> > > Sorry, I made a mistake when splitting the diff. The diff I sent doesn't build.
> > > I will post a new one shortly.
> >
> > Fixed diff.
> >
> > diff aa91687117b06dc9f357920a56ef1a22c0de6a7e 16cec9db4484c7e9e6fa4838707a4c0c7ba2f314
> > blob - f7449f560623517dfa1b3fbe6d55b10a5494e26c
> > blob + 0d5f766801f75e839476ebc2393568ed31494e62
> > --- sys/dev/pci/if_iwm.c
> > +++ sys/dev/pci/if_iwm.c
> > @@ -366,8 +366,10 @@ int iwm_get_signal_strength(struct iwm_softc *, struct
> >  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 *, struct mbuf_list *);
> > +int iwm_rx_frame(struct iwm_softc *, struct mbuf *, uint32_t,
> > +    struct mbuf_list *);
> > +int iwm_ccmp_decap(struct iwm_softc *, struct mbuf *,
> > +    struct ieee80211_node *);
> >  void iwm_enable_ht_cck_fallback(struct iwm_softc *, struct iwm_node *);
> >  void iwm_rx_tx_cmd_single(struct iwm_softc *, struct iwm_rx_packet *,
> >      struct iwm_node *);
> > @@ -472,6 +474,11 @@ const char *iwm_desc_lookup(uint32_t);
> >  void iwm_nic_error(struct iwm_softc *);
> >  void iwm_nic_umac_error(struct iwm_softc *);
> >  #endif
> > +void iwm_rx_mpdu(struct iwm_softc *, struct mbuf *, size_t,
> > +    struct mbuf_list *);
> > +int iwm_rx_pkt_valid(struct iwm_rx_packet *);
> > +void iwm_rx_pkt(struct iwm_softc *, struct iwm_rx_data *,
> > +    struct mbuf_list *);
> >  void iwm_notif_intr(struct iwm_softc *);
> >  int iwm_intr(void *);
> >  int iwm_match(struct device *, void *, void *);
> > @@ -1807,7 +1814,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);
> > @@ -3539,56 +3545,24 @@ iwm_get_noise(const struct iwm_statistics_rx_non_phy *
> >   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, struct mbuf_list *ml)
> > +int
> > +iwm_rx_frame(struct iwm_softc *sc, struct mbuf *m, uint32_t rx_pkt_status,
> > +    struct mbuf_list *ml)
> >  {
> >   struct ieee80211com *ic = &sc->sc_ic;
> >   struct ieee80211_frame *wh;
> >   struct ieee80211_node *ni;
> >   struct ieee80211_rxinfo rxi;
> >   struct ieee80211_channel *bss_chan;
> > - 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, chanidx;
> >   uint8_t saved_bssid[IEEE80211_ADDR_LEN] = { 0 };
> >  
> > - 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);
> > - if (len < IEEE80211_MIN_LEN) {
> > - ic->ic_stats.is_rx_tooshort++;
> > - IC2IFP(ic)->if_ierrors++;
> > - return;
> > - }
> > - if (len > IWM_RBUF_SIZE - sizeof(*rx_res)) {
> > - IC2IFP(ic)->if_ierrors++;
> > - return;
> > - }
> > - rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
> > -    sizeof(*rx_res) + len));
> > -
> >   if (__predict_false(phy_info->cfg_phy_cnt > 20))
> > - return;
> > + return EINVAL;
> >  
> > - if (!(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) ||
> > -    !(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK))
> > - return; /* drop */
> > -
> > - m = data->m;
> > - if (iwm_rx_addbuf(sc, IWM_RBUF_SIZE, sc->rxq.cur) != 0)
> > - return;
> > - m->m_data = pkt->data + sizeof(*rx_res);
> > - m->m_pkthdr.len = m->m_len = len;
> > -
> >   device_timestamp = le32toh(phy_info->system_timestamp);
> >  
> >   rssi = iwm_get_signal_strength(sc, phy_info);
> > @@ -3599,6 +3573,7 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> >   if (chanidx < 0 || chanidx >= nitems(ic->ic_channels))
> >   chanidx = ieee80211_chan2ieee(ic, ic->ic_ibss_chan);
> >  
> > + wh = mtod(m, struct ieee80211_frame *);
> >   ni = ieee80211_find_rxnode(ic, wh);
> >   if (ni == ic->ic_bss) {
> >   /*
> > @@ -3672,6 +3647,8 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> >   if (ni == ic->ic_bss && IEEE80211_ADDR_EQ(saved_bssid, ni->ni_macaddr))
> >   ni->ni_chan = bss_chan;
> >   ieee80211_release_node(ic, ni);
> > +
> > + return 0;
> >  }
> >  
> >  void
> > @@ -7525,38 +7502,103 @@ 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, size_t maxlen,
> > +    struct mbuf_list *ml)
> >  {
> > - struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> > - uint16_t hw;
> > + struct ieee80211com *ic = &sc->sc_ic;
> > + struct ifnet *ifp = IC2IFP(ic);
> > + struct iwm_rx_packet *pkt;
> > + struct iwm_rx_mpdu_res_start *rx_res;
> > + uint16_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);
> > + if (len < IEEE80211_MIN_LEN) {
> > + ic->ic_stats.is_rx_tooshort++;
> > + IC2IFP(ic)->if_ierrors++;
> > + m_freem(m);
> > + return;
> > + }
> > + if (len + sizeof(*rx_res) + sizeof(rx_pkt_status) > maxlen ||
> > +    len > IEEE80211_MAX_LEN) {
> > + IC2IFP(ic)->if_ierrors++;
> > + m_freem(m);
> > + return;
> > + }
> >  
> > - hw = le16toh(sc->rxq.stat->closed_rb_num) & 0xfff;
> > - hw &= (IWM_RX_RING_COUNT - 1);
> > - while (sc->rxq.cur != hw) {
> > - struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
> > - struct iwm_rx_packet *pkt;
> > - int qid, idx, code, handled = 1;
> > + memcpy(&rx_pkt_status, pkt->data + sizeof(*rx_res) + len,
> > +    sizeof(rx_pkt_status));
> > + rx_pkt_status = le32toh(rx_pkt_status);
> > + 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;
> > + }
> >  
> > - bus_dmamap_sync(sc->sc_dmat, data->map, 0, sizeof(*pkt),
> > -    BUS_DMASYNC_POSTREAD);
> > - pkt = mtod(data->m, struct iwm_rx_packet *);
> > + /* 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, rx_pkt_status, ml) != 0) {
> > + ifp->if_ierrors++;
> > + m_freem(m);
> > + }
> > +}
> >  
> > +int
> > +iwm_rx_pkt_valid(struct iwm_rx_packet *pkt)
> > +{
> > + int qid, idx, code;
> > +
> > + qid = pkt->hdr.qid & ~0x80;
> > + idx = pkt->hdr.idx;
> > + code = IWM_WIDE_ID(pkt->hdr.flags, pkt->hdr.code);
> > +
> > + return (!(qid == 0 && idx == 0 && code == 0) &&
> > +    pkt->len_n_flags != htole32(IWM_FH_RSCSR_FRAME_INVALID));
> > +}
> > +
> > +void
> > +iwm_rx_pkt(struct iwm_softc *sc, struct iwm_rx_data *data, struct mbuf_list *ml)
> > +{
> > + struct ifnet *ifp = IC2IFP(&sc->sc_ic);
> > + struct iwm_rx_packet *pkt, *nextpkt;
> > + uint32_t offset = 0, nextoff = 0, nmpdu = 0, len;
> > + struct mbuf *m0, *m;
> > + const size_t minsz = sizeof(pkt->len_n_flags) + sizeof(pkt->hdr);
> > + size_t remain = IWM_RBUF_SIZE;
> > + int qid, idx, code, handled = 1;
> > +
> > + bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
> > +    BUS_DMASYNC_POSTREAD);
> > +
> > + m0 = data->m;
> > + while (m0 && offset + minsz < IWM_RBUF_SIZE) {
> > + pkt = (struct iwm_rx_packet *)(m0->m_data + offset);
> >   qid = pkt->hdr.qid;
> >   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 & ~0x80) == 0
> > -    && idx == 0) || pkt->len_n_flags == htole32(0x55550000))) {
> > - ADVANCE_RXQ(sc);
> > - continue;
> > + if (!iwm_rx_pkt_valid(pkt))
> > + break;
> > +
> > + len = sizeof(pkt->len_n_flags) + iwm_rx_packet_len(pkt);
> > + if (len < sizeof(pkt->hdr) ||
> > +    len > (IWM_RBUF_SIZE - offset - minsz))
> > + 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);
> >   }
> >  
> >   switch (code) {
> > @@ -7564,10 +7606,40 @@ iwm_notif_intr(struct iwm_softc *sc)
> >   iwm_rx_rx_phy_cmd(sc, pkt, data);
> >   break;
> >  
> > - case IWM_REPLY_RX_MPDU_CMD:
> > - iwm_rx_rx_mpdu(sc, pkt, data, &ml);
> > - break;
> > + case IWM_REPLY_RX_MPDU_CMD: {
> > + nextoff = offset +
> > +    roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
> > + nextpkt = (struct iwm_rx_packet *)
> > +    (m0->m_data + nextoff);
> > + if (nextoff + minsz >= IWM_RBUF_SIZE ||
> > +    !iwm_rx_pkt_valid(nextpkt)) {
> > + /* No need to copy last frame in buffer. */
> > + if (offset > 0)
> > + m_adj(m0, offset);
> > + iwm_rx_mpdu(sc, m0, remain - minsz, ml);
> > + m0 = NULL; /* stack owns m0 now; abort loop */
> > + } else {
> > + /*
> > + * Create an mbuf which points to the current
> > + * packet. Always copy from offset zero to
> > + * preserve m_pkthdr.
> > + */
> > + m = m_copym(m0, 0, M_COPYALL, M_DONTWAIT);
> > + if (m == NULL) {
> > + ifp->if_ierrors++;
> > + break;
> > + }
> > + m_adj(m, offset);
> > + iwm_rx_mpdu(sc, m, remain - minsz, ml);
> > + }
> >  
> > + if (offset + minsz < remain)
> > + remain -= offset;
> > + else
> > + remain = minsz;
> > + break;
> > + }
> > +
> >   case IWM_TX_CMD:
> >   iwm_rx_tx_cmd(sc, pkt, data);
> >   break;
> > @@ -7818,6 +7890,27 @@ iwm_notif_intr(struct iwm_softc *sc)
> >   iwm_cmd_done(sc, qid, idx, code);
> >   }
> >  
> > + offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
> > + }
> > +
> > + if (m0 && m0 != data->m)
> > + m_freem(m0);
> > +}
> > +
> > +void
> > +iwm_notif_intr(struct iwm_softc *sc)
> > +{
> > + struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> > + 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;
> > + hw &= (IWM_RX_RING_COUNT - 1);
> > + while (sc->rxq.cur != hw) {
> > + struct iwm_rx_data *data = &sc->rxq.data[sc->rxq.cur];
> > + iwm_rx_pkt(sc, data, &ml);
> >   ADVANCE_RXQ(sc);
> >   }
> >   if_input(&sc->sc_ic.ic_if, &ml);
> >
>
> --
> I'm not entirely sure you are real.
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: iwm: support more than one frame per interrupt

Stuart Henderson
On 2019/11/09 15:28, Claudio Jeker wrote:

> On Sat, Nov 09, 2019 at 03:20:10PM +0100, Florian Obser wrote:
> > My 7260 doesn't like it. Same problem as last time.
> > These counters go through the roof and network stalls.
> > Also when packets are flowing throughput varies a lot.
> >         30131 input block ack window slides
> >         30171 input frames above block ack window end
> >         827766 expected input block ack frames never arrived
>
> I have similar issues with a 8265:
>         65731 input frames above block ack window end
>         65711 input block ack window slides
>         150884 expected input block ack frames never arrived
>
> It works but feels slower than before.

Now I know what to look for, I see that on 8260 too. And I do see some
minor stalls with some things (switching windows in tmux for example).