Re: iwn: fix tx result reporting
On Mon, Apr 06, 2020 at 05:02:00PM +0200, Stefan Sperling wrote:
> On Thu, Apr 02, 2020 at 03:52:11PM +0200, Stefan Sperling wrote:
> > While working on iwm Tx aggregation and revisiting some parts of MiRA,
> > I have noticed a rate-control problem in iwm and iwx (this also affects
> > iwn, but I am leaving that for later since iwn already does Tx aggregation
> > and requires a more elaborate fix).
>
> Here is the corresponding diff for iwn(4).
>
> Fixes an automatic Tx rate control issue in iwn(4).
> Same change as for iwm(4) and iwx(4), but also accounts for block ack.
>
> Also fix this driver's handling of ic_fixed_mcs.
Block ack is hard :(
The previous diff had a bug in evaluating the block ack bitmap.
We must align our own block ack window with the current window used by
firmware before looking at bits in the ACK bitmap provided by firmware.
Otherwise ACK/non-ACK status could be associated with the wrong frames.
Fixed in this version.
diff refs/heads/master refs/heads/iwn
blob - 72e19d7e35646822faaed4f2ebd157297a8ec907
blob + b50cd5642bd70be70e8d6d70f70e665208c0
--- sys/dev/pci/if_iwn.c
+++ sys/dev/pci/if_iwn.c
@@ -166,8 +166,8 @@ voidiwn_rx_statistics(struct iwn_softc *,
struct iwn
void iwn_ampdu_txq_advance(struct iwn_softc *, struct iwn_tx_ring *,
int, int);
void iwn_ampdu_tx_done(struct iwn_softc *, struct iwn_tx_ring *,
- struct iwn_rx_desc *, uint16_t, struct iwn_txagg_status *,
- int, uint32_t);
+ struct iwn_rx_desc *, uint16_t, uint8_t, uint8_t, uint8_t,
+ int, uint32_t, struct iwn_txagg_status *);
void iwn4965_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
struct iwn_rx_data *);
void iwn5000_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
@@ -176,7 +176,7 @@ voidiwn_tx_done_free_txdata(struct
iwn_softc *,
struct iwn_tx_data *);
void iwn_clear_oactive(struct iwn_softc *, struct iwn_tx_ring *);
void iwn_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
- uint8_t, int, int, uint16_t);
+ uint8_t, uint8_t, int, int, uint16_t);
void iwn_cmd_done(struct iwn_softc *, struct iwn_rx_desc *);
void iwn_notif_intr(struct iwn_softc *);
void iwn_wakeup_intr(struct iwn_softc *);
@@ -2259,6 +2259,7 @@ void
iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_rx_desc *desc,
struct iwn_rx_data *data)
{
+ struct iwn_ops *ops = >ops;
struct iwn_compressed_ba *cba = (struct iwn_compressed_ba *)(desc + 1);
struct ieee80211com *ic = >sc_ic;
struct ieee80211_node *ni;
@@ -2268,6 +2269,9 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
uint16_t ssn, idx;
int qid;
+ if (ic->ic_state != IEEE80211_S_RUN)
+ return;
+
bus_dmamap_sync(sc->sc_dmat, data->map, sizeof (*desc), sizeof (*cba),
BUS_DMASYNC_POSTREAD);
@@ -2282,47 +2286,76 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
return;
txq = >txq[qid];
- ssn = le16toh(cba->ssn); /* BA window starting sequence number */
- idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
/* Protect against a firmware bug where the queue/TID are off. */
if (qid != sc->first_agg_txq + cba->tid)
return;
- /*
-* Update Tx rate statistics.
-*/
- if (ic->ic_state == IEEE80211_S_RUN && cba->nframes_sent > 0) {
- uint8_t nframes = cba->nframes_sent;
- int read = txq->read;
- wn->mn.agglen = 0;
- wn->mn.ampdu_size = 0;
- /* Add up the lengths of all frames before the window. */
- while (nframes && read != idx) {
- struct iwn_tx_data *txdata = >data[read];
- wn->mn.agglen++;
- wn->mn.ampdu_size += txdata->totlen + IEEE80211_CRC_LEN;
- read = (read + 1) % IWN_TX_RING_COUNT;
- nframes--;
- }
- wn->mn.frames += cba->nframes_sent;
- /* If firmware reports a bogus ACK counter, fix it up. */
- if (cba->nframes_acked > cba->nframes_sent)
- cba->nframes_acked = cba->nframes_sent;
- wn->mn.retries += cba->nframes_sent - cba->nframes_acked;
- if (wn->mn.txfail > wn->mn.frames)
- wn->mn.txfail = wn->mn.frames;
- if (wn->mn.ampdu_size > 0)
- ieee80211_mira_choose(>mn, ic, ni);
- }
ba = >ni_tx_ba[cba->tid];
+ if (ba->ba_state != IEEE80211_BA_AGREED)
+ return;
+ ssn = le16toh(cba->ssn); /* BA window starting sequence number */
if (!SEQ_LT(ssn, ba->ba_winstart)) {
iwn: fix tx result reporting
On Thu, Apr 02, 2020 at 03:52:11PM +0200, Stefan Sperling wrote:
> While working on iwm Tx aggregation and revisiting some parts of MiRA,
> I have noticed a rate-control problem in iwm and iwx (this also affects
> iwn, but I am leaving that for later since iwn already does Tx aggregation
> and requires a more elaborate fix).
Here is the corresponding diff for iwn(4).
Fixes an automatic Tx rate control issue in iwn(4).
Same change as for iwm(4) and iwx(4), but also accounts for block ack.
Also fix this driver's handling of ic_fixed_mcs.
diff 5c2ccc6893e39ccee38b5e7e6ecf6487be392b1e
efc01515a309caa279d79c1a251c73cdbcc68bba
blob - 72e19d7e35646822faaed4f2ebd157297a8ec907
blob + 30a936bedb20e7958f73ee789e9aced79626e580
--- sys/dev/pci/if_iwn.c
+++ sys/dev/pci/if_iwn.c
@@ -166,8 +166,8 @@ voidiwn_rx_statistics(struct iwn_softc *,
struct iwn
void iwn_ampdu_txq_advance(struct iwn_softc *, struct iwn_tx_ring *,
int, int);
void iwn_ampdu_tx_done(struct iwn_softc *, struct iwn_tx_ring *,
- struct iwn_rx_desc *, uint16_t, struct iwn_txagg_status *,
- int, uint32_t);
+ struct iwn_rx_desc *, uint16_t, uint8_t, uint8_t, uint8_t,
+ int, uint32_t, struct iwn_txagg_status *);
void iwn4965_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
struct iwn_rx_data *);
void iwn5000_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
@@ -176,7 +176,7 @@ voidiwn_tx_done_free_txdata(struct
iwn_softc *,
struct iwn_tx_data *);
void iwn_clear_oactive(struct iwn_softc *, struct iwn_tx_ring *);
void iwn_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
- uint8_t, int, int, uint16_t);
+ uint8_t, uint8_t, int, int, uint16_t);
void iwn_cmd_done(struct iwn_softc *, struct iwn_rx_desc *);
void iwn_notif_intr(struct iwn_softc *);
void iwn_wakeup_intr(struct iwn_softc *);
@@ -2259,6 +2259,7 @@ void
iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_rx_desc *desc,
struct iwn_rx_data *data)
{
+ struct iwn_ops *ops = >ops;
struct iwn_compressed_ba *cba = (struct iwn_compressed_ba *)(desc + 1);
struct ieee80211com *ic = >sc_ic;
struct ieee80211_node *ni;
@@ -2268,6 +2269,9 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
uint16_t ssn, idx;
int qid;
+ if (ic->ic_state != IEEE80211_S_RUN)
+ return;
+
bus_dmamap_sync(sc->sc_dmat, data->map, sizeof (*desc), sizeof (*cba),
BUS_DMASYNC_POSTREAD);
@@ -2282,41 +2286,66 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
return;
txq = >txq[qid];
- ssn = le16toh(cba->ssn); /* BA window starting sequence number */
- idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
/* Protect against a firmware bug where the queue/TID are off. */
if (qid != sc->first_agg_txq + cba->tid)
return;
+
+ ba = >ni_tx_ba[cba->tid];
+ if (ba->ba_state != IEEE80211_BA_AGREED)
+ return;
+
/*
* Update Tx rate statistics.
+* Skip rate control if our Tx rate is fixed.
*/
- if (ic->ic_state == IEEE80211_S_RUN && cba->nframes_sent > 0) {
- uint8_t nframes = cba->nframes_sent;
- int read = txq->read;
+ if (ic->ic_fixed_mcs == -1 && cba->nframes_sent > 0) {
+ int end_idx = IWN_AGG_SSN_TO_TXQ_IDX(ba->ba_winend);
+ int bit = 0, nsent = cba->nframes_sent;
+
wn->mn.agglen = 0;
wn->mn.ampdu_size = 0;
- /* Add up the lengths of all frames before the window. */
- while (nframes && read != idx) {
- struct iwn_tx_data *txdata = >data[read];
- wn->mn.agglen++;
- wn->mn.ampdu_size += txdata->totlen + IEEE80211_CRC_LEN;
- read = (read + 1) % IWN_TX_RING_COUNT;
- nframes--;
+
+ ssn = le16toh(ba->ba_winstart);
+ idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
+ while (nsent && idx != end_idx) {
+ struct iwn_tx_data *txdata = >data[idx];
+ int have_ack = (le64toh(cba->bitmap) & (1 << bit++));
+
+ if (txdata->m != NULL) {
+ /*
+* Don't report frames to MiRA which were sent
+* at a different Tx rate than ni->ni_txmcs.
+*/
+ if (txdata->actual_txmcs == ni->ni_txmcs) {
+ wn->mn.frames++;
+ wn->mn.agglen++;
+ wn->mn.ampdu_size += txdata->totlen +
