Re: [PATCH 1/2] mac80211: add A-MSDU tx support
From: Johannes Berg <johannes@sipsolutions.net>
Date: 2016-02-23 11:26:00
On Thu, 2016-02-18 at 19:39 +0100, Felix Fietkau wrote:
Requires software tx queueing support. frag_list support (for zero- copy) is optional.
"optional" ;-)
quoted hunk ↗ jump to hunk
@@ -1751,6 +1754,7 @@ struct ieee80211_sta {bool mfp; u8 max_amsdu_subframes; u16 max_amsdu_len; + u16 max_rc_amsdu_len;
I may have missed this, but I didn't see you initialize this to something. I think that's probably a bad idea, since you've now created a situation in which a driver wanting to use txq support needs to also use minstrel or update this field... Apart from that, I'm not sure that having a field here that works like this is a good idea at all - all the other fields in the vicinity kinda go from mac80211 to the driver, where here you have something that potentially goes from the driver to mac80211. Perhaps we can find a better way of doing this?
+ * @IEEE80211_HW_TX_AMSDU: Hardware (or driver) supports software aggregated + * A-MSDU frames. Requires software tx queueing support.
Or at least document that it also requires integration with the max_rc_amsdu_len thing.
+ * @max_tx_fragments: maximum fragments per (A-)MSDU.
"fragments" is a bit unclear - I guess this should refer to fraglist length only? Or is it meant to apply as a limit to the sum of pieces in the fraglist? I think that should be clarified.
+ txq = sta->sta.txq[tid]; + if (!amsdu && txq) + set_bit(IEEE80211_TXQ_NO_AMSDU, &to_txq_info(txq)-quoted
flags);
Did you intentionally not clear this on aggregation stop?
+ if (skb_headroom(skb) < sizeof(amsdu_hdr) ||
skb_tailroom(skb) < 3) {
+ I802_DEBUG_INC(local->tx_expand_skb_head);
+
+ if (pskb_expand_head(skb, sizeof(amsdu_hdr), 3,
GFP_ATOMIC)) {You should be able to precalculate the amount of padding needed based on the length of the skb, so why statically use 3 here?
+ if (skb_headroom(skb) < 8 || skb_tailroom(skb) < 3) {
+ I802_DEBUG_INC(local->tx_expand_skb_head);
+
+ if (pskb_expand_head(skb, 8, 3, GFP_ATOMIC)) {similarly here? Where does the 8 come from?
quoted hunk ↗ jump to hunk
@@ -2820,6 +2983,10 @@ static bool ieee80211_xmit_fast(structieee80211_sub_if_data *sdata, ieee80211_tx_stats(dev, skb->len + extra_head); + if ((hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) && + ieee80211_amsdu_aggregate(sdata, sta, fast_tx, skb)) + return true;
Only on xmit_fast? johannes