From e1e8b05256525553c236491d8ec9c450079d2721 Mon Sep 17 00:00:00 2001 From: Adrian Chadd Date: Wed, 4 May 2016 01:36:19 +0000 Subject: [PATCH] [bwn] accurately(ish) account transmit/recieve failures for rate control. I noticed that it'd associate fine, but it'd quickly stop exchanging traffic. Receive was okay, but transmit just failed. Then I went "wlandebug +rate". I discovered it started at 36M OFDM, and then quickly rose to 54M, which then showed 0% transmit success. Then, I dug into how the completion path works. We are reading 'ack=0' in the TX status side, so .. then I discovered we were only processing the TX completion status /if/ ack=1. So, we'd only ever count successes; we'd never count failures, and thus the rate control code thought everything was a-ok. We also have to set retrycnt to something non-zero so it indeed does bring the rate down upon failure. So: * Delete the rate control completion code from the tx completion routine, it's just duplicate and never worked. Putting it behind 'if (status->ack) was pointless. * Move it to the PIO and DMA completion routines which actually do free the node reference and mbuf. We know at that point what the status is, so do it there. * Fake a retrycnt of 1 for now, so we at least count failures. Also: * Start adding comments about weird stuff I find with rate selection. In this instance, we shouldn't be selecting a fallback rate that doesn't match the currently configured mode (11a, 11b, 11g, etc.) This isn't perfect - AMRR does try 54mbit and takes a few packets before it figures out it's a bad idea - but it's better than nothing. This makes the bwn(4) driver actually useful for the first time since I've tried using it - and that dates back to 2011. I've resisted successfully until now. Tested: * Broadcom BCM4312 802.11b/g Wireless, STA mode WLAN (chipid 0x4312 rev 15) PHY (analog 6 type 5 rev 1) RADIO (manuf 0x17f ver 0x2062 rev 2) TODO: * See if the fallback rate actually /is/ working * Question my own sanity over touching this driver in the first place. --- sys/dev/bwn/if_bwn.c | 77 ++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 46 deletions(-) diff --git a/sys/dev/bwn/if_bwn.c b/sys/dev/bwn/if_bwn.c index 8766ffa4ac75..f7dc50c952d7 100644 --- a/sys/dev/bwn/if_bwn.c +++ b/sys/dev/bwn/if_bwn.c @@ -5132,16 +5132,8 @@ bwn_dma_rxeof(struct bwn_dma_ring *dr, int *slot) static void bwn_handle_txeof(struct bwn_mac *mac, const struct bwn_txstatus *status) { - struct bwn_dma_ring *dr; - struct bwn_dmadesc_generic *desc; - struct bwn_dmadesc_meta *meta; - struct bwn_pio_txqueue *tq; - struct bwn_pio_txpkt *tp = NULL; struct bwn_softc *sc = mac->mac_sc; struct bwn_stats *stats = &mac->mac_stats; - struct ieee80211_node *ni; - struct ieee80211vap *vap; - int retrycnt = 0, slot; BWN_ASSERT_LOCKED(mac->mac_sc); @@ -5157,46 +5149,8 @@ bwn_handle_txeof(struct bwn_mac *mac, const struct bwn_txstatus *status) } if (mac->mac_flags & BWN_MAC_FLAG_DMA) { - if (status->ack) { - dr = bwn_dma_parse_cookie(mac, status, - status->cookie, &slot); - if (dr == NULL) { - device_printf(sc->sc_dev, - "failed to parse cookie\n"); - return; - } - while (1) { - dr->getdesc(dr, slot, &desc, &meta); - if (meta->mt_islast) { - ni = meta->mt_ni; - vap = ni->ni_vap; - ieee80211_ratectl_tx_complete(vap, ni, - status->ack ? - IEEE80211_RATECTL_TX_SUCCESS : - IEEE80211_RATECTL_TX_FAILURE, - &retrycnt, 0); - break; - } - slot = bwn_dma_nextslot(dr, slot); - } - } bwn_dma_handle_txeof(mac, status); } else { - if (status->ack) { - tq = bwn_pio_parse_cookie(mac, status->cookie, &tp); - if (tq == NULL) { - device_printf(sc->sc_dev, - "failed to parse cookie\n"); - return; - } - ni = tp->tp_ni; - vap = ni->ni_vap; - ieee80211_ratectl_tx_complete(vap, ni, - status->ack ? - IEEE80211_RATECTL_TX_SUCCESS : - IEEE80211_RATECTL_TX_FAILURE, - &retrycnt, 0); - } bwn_pio_handle_txeof(mac, status); } @@ -5538,6 +5492,7 @@ bwn_dma_handle_txeof(struct bwn_mac *mac, struct bwn_dmadesc_meta *meta; struct bwn_softc *sc = mac->mac_sc; int slot; + int retrycnt = 0; BWN_ASSERT_LOCKED(sc); @@ -5562,6 +5517,16 @@ bwn_dma_handle_txeof(struct bwn_mac *mac, KASSERT(meta->mt_m != NULL, ("%s:%d: fail", __func__, __LINE__)); + /* XXX */ + if (status->ack == 0) + retrycnt = 1; + else + retrycnt = 0; + ieee80211_ratectl_tx_complete(meta->mt_ni->ni_vap, meta->mt_ni, + status->ack ? + IEEE80211_RATECTL_TX_SUCCESS : + IEEE80211_RATECTL_TX_FAILURE, + &retrycnt, 0); ieee80211_tx_complete(meta->mt_ni, meta->mt_m, 0); meta->mt_ni = NULL; meta->mt_m = NULL; @@ -5589,6 +5554,7 @@ bwn_pio_handle_txeof(struct bwn_mac *mac, struct bwn_pio_txqueue *tq; struct bwn_pio_txpkt *tp = NULL; struct bwn_softc *sc = mac->mac_sc; + int retrycnt = 0; BWN_ASSERT_LOCKED(sc); @@ -5604,6 +5570,18 @@ bwn_pio_handle_txeof(struct bwn_mac *mac, * Do any tx complete callback. Note this must * be done before releasing the node reference. */ + + /* XXX */ + if (status->ack == 0) + retrycnt = 1; + else + retrycnt = 0; + ieee80211_ratectl_tx_complete(tp->tp_ni->ni_vap, tp->tp_ni, + status->ack ? + IEEE80211_RATECTL_TX_SUCCESS : + IEEE80211_RATECTL_TX_FAILURE, + &retrycnt, 0); + if (tp->tp_m->m_flags & M_TXCB) ieee80211_process_callback(tp->tp_ni, tp->tp_m, 0); ieee80211_free_node(tp->tp_ni); @@ -5751,6 +5729,7 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, else if (tp->ucastrate != IEEE80211_FIXED_RATE_NONE) rate = rate_fb = tp->ucastrate; else { + /* XXX TODO: don't fall back to CCK rates for OFDM */ rix = ieee80211_ratectl_rate(ni, NULL, 0); rate = ni->ni_txrate; @@ -5763,6 +5742,7 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, sc->sc_tx_rate = rate; + /* Note: this maps the select ieee80211 rate to hardware rate */ rate = bwn_ieeerate2hwrate(sc, rate); rate_fb = bwn_ieeerate2hwrate(sc, rate_fb); @@ -5771,6 +5751,7 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, bcopy(wh->i_fc, txhdr->macfc, sizeof(txhdr->macfc)); bcopy(wh->i_addr1, txhdr->addr1, IEEE80211_ADDR_LEN); + /* XXX rate/rate_fb is the hardware rate */ if ((rate_fb == rate) || (*(u_int16_t *)wh->i_dur & htole16(0x8000)) || (*(u_int16_t *)wh->i_dur == htole16(0))) @@ -5792,6 +5773,7 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, txhdr->chan = phy->chan; phyctl |= (BWN_ISOFDMRATE(rate)) ? BWN_TX_PHY_ENC_OFDM : BWN_TX_PHY_ENC_CCK; + /* XXX preamble? obey net80211 */ if (isshort && (rate == BWN_CCK_RATE_2MB || rate == BWN_CCK_RATE_5MB || rate == BWN_CCK_RATE_11MB)) phyctl |= BWN_TX_PHY_SHORTPRMBL; @@ -5828,9 +5810,11 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, if (ic->ic_flags & IEEE80211_F_USEPROT) { /* XXX RTS rate is always 1MB??? */ + /* XXX TODO: don't fall back to CCK rates for OFDM */ rts_rate = BWN_CCK_RATE_1MB; rts_rate_fb = bwn_get_fbrate(rts_rate); + /* XXX 'rate' here is hardware rate now, not the net80211 rate */ protdur = ieee80211_compute_duration(ic->ic_rt, m->m_pkthdr.len, rate, isshort) + + ieee80211_ack_duration(ic->ic_rt, rate, isshort); @@ -5851,6 +5835,7 @@ bwn_set_txhdr(struct bwn_mac *mac, struct ieee80211_node *ni, rts = (struct ieee80211_frame_rts *)(BWN_ISOLDFMT(mac) ? (txhdr->body.old.rts_frame) : (txhdr->body.new.rts_frame)); + /* XXX rate/rate_fb is the hardware rate */ protdur += ieee80211_ack_duration(ic->ic_rt, rate, isshort); mprot = ieee80211_alloc_rts(ic, wh->i_addr1,