Shuffle the call to ath_hal_setuplasttxdesc() to _after_ the rate control

code is called and remove it from ath_buf_set_rate().

For the legacy (non-11n API) TX routines, ath_hal_filltxdesc() takes care
of setting up the intermediary and final descriptors right, complete
with copying the rate control info into the final descriptor so the
rate modules can grab it.

The 11n version doesn't do this - ath_hal_chaintxdesc() doesn't
copy the rate control bits over, nor does it clear isaggr/moreaggr/
pad delimiters.  So the call to setuplasttxdesc() is needed here.

So:

* legacy NICs - never call the 11n rate control stuff, so filltxdesc
  copies the rate control info right;
* 11n NICs transmitting legacy or 11n non-aggregate frames -
  ath_hal_set11nratescenario() is called to setup rate control and
  then ath_hal_filltxdesc() chains them together - so the rate control
  info is right;
* 11n aggregate frames - set11nratescenario() is called, then
  ath_hal_chaintxdesc() is called to chain a list of aggregate and subframes
  together. This requires a call to ath_hal_setuplasttxdesc() to complete
  things.

Tested:

* AR9280 in station mode

TODO:

* I really should make sure that the descriptor contents get blanked
  out correctly or garbage left over from aggregate frames may show
  up in non-aggregate frames, leading to badness.

2 files changed, 7 insertions, 31 deletions

diff --git a/sys/dev/ath/if_ath_tx.c b/sys/dev/ath/if_ath_tx.c
index a1330aa..77be706 100644
--- a/sys/dev/ath/if_ath_tx.c
+++ b/sys/dev/ath/if_ath_tx.c
@@ -496,13 +496,6 @@ ath_tx_setds_11n(struct ath_softc *sc, struct ath_buf *bf_first)
 	    bf_first->bf_state.bfs_ctsduration);
 
 	/*
-	 * Setup the last descriptor in the list.
-	 * bf_prev points to the last; bf is NULL here.
-	 */
-	ath_hal_setuplasttxdesc(sc->sc_ah, bf_prev->bf_desc,
-	    bf_first->bf_desc);
-
-	/*
 	 * Set the first descriptor bf_lastds field to point to
 	 * the last descriptor in the last subframe, that's where
 	 * the status update will occur.
@@ -520,6 +513,13 @@ ath_tx_setds_11n(struct ath_softc *sc, struct ath_buf *bf_first)
 	 */
 	ath_tx_set_ratectrl(sc, bf_first->bf_node, bf_first);
 
+	/*
+	 * Setup the last descriptor in the list.
+	 * bf_prev points to the last; bf is NULL here.
+	 */
+	ath_hal_setuplasttxdesc(sc->sc_ah, bf_prev->bf_desc,
+	    bf_first->bf_desc);
+
 	DPRINTF(sc, ATH_DEBUG_SW_TX_AGGR, "%s: end\n", __func__);
 }
 
diff --git a/sys/dev/ath/if_ath_tx_ht.c b/sys/dev/ath/if_ath_tx_ht.c
index d08e2c4..6495a04 100644
--- a/sys/dev/ath/if_ath_tx_ht.c
+++ b/sys/dev/ath/if_ath_tx_ht.c
@@ -560,14 +560,12 @@ ath_rateseries_print(struct ath_softc *sc, HAL_11N_RATE_SERIES *series)
  * This isn't useful for sending beacon frames, which has different needs
  * wrt what's passed into the rate scenario function.
  */
-
 void
 ath_buf_set_rate(struct ath_softc *sc, struct ieee80211_node *ni,
     struct ath_buf *bf)
 {
 	HAL_11N_RATE_SERIES series[4];
 	struct ath_desc *ds = bf->bf_desc;
-	struct ath_desc *lastds = NULL;
 	struct ath_hal *ah = sc->sc_ah;
 	int is_pspoll = (bf->bf_state.bfs_atype == HAL_PKT_TYPE_PSPOLL);
 	int ctsrate = bf->bf_state.bfs_ctsrate;
@@ -578,13 +576,6 @@ ath_buf_set_rate(struct ath_softc *sc, struct ieee80211_node *ni,
 
 	ath_rateseries_setup(sc, ni, bf, series);
 
-	/* Enforce AR5416 aggregate limit - can't do RTS w/ an agg frame > 8k */
-
-	/* Enforce RTS and CTS are mutually exclusive */
-
-	/* Get a pointer to the last tx descriptor in the list */
-	lastds = bf->bf_lastds;
-
 #if 0
 	printf("pktlen: %d; flags 0x%x\n", pktlen, flags);
 	ath_rateseries_print(sc, series);
@@ -602,21 +593,6 @@ ath_buf_set_rate(struct ath_softc *sc, struct ieee80211_node *ni,
 	    4,		/* number of series */
 	    flags);
 
-	/* Setup the last descriptor in the chain */
-	/*
-	 * XXX Why is this done here, and not in the upper layer?
-	 * The rate control code stores a copy of the RC info in
-	 * the last descriptor as well as the first, then uses
-	 * the shadow copy in the last descriptor to see what the RC
-	 * decisions were.  I'm not sure why; perhaps earlier hardware
-	 * overwrote the first descriptor contents.
-	 *
-	 * In the 802.11n case, it also clears the moreaggr/delim
-	 * fields.  Again, this should be done by the caller of
-	 * ath_buf_set_rate().
-	 */
-	ath_hal_setuplasttxdesc(ah, lastds, ds);
-
 	/* Set burst duration */
 	/*
 	 * This is only required when doing 11n burst, not aggregation