On 2016-02-23 12:25, Johannes Berg wrote:

On Thu, 2016-02-18 at 19:39 +0100, Felix Fietkau wrote:

quoted

Requires software tx queueing support. frag_list support (for zero-
copy) is optional.

"optional" ;-)

quoted

@@ -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...

It's initialized to 0 by default, in which case it does not limit the
A-MSDU size. The driver does not need to set it, but it really should.

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?

Didn't find a better place to put it. Got any ideas?

quoted

+ * @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.

Will do.

quoted

+ * @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.

Sum of 1 (head) + n_frags for each skb in the fraglist.

quoted

+	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?

Yes. If A-MSDU in A-MPDU is not supported, dealing with starting and
stopping of A-MPDU sessions leaves all kinds of nasty corner cases
(since the txq is not flushed between sessions). In that case it's
better to just leave it disabled from that point on.

quoted

+	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?

I guess I forgot to change that after I got the code working.

quoted

+	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?

rfc1042_header + A-MSDU length field.

quoted

@@ -2820,6 +2983,10 @@ static bool ieee80211_xmit_fast(struct

ieee80211_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?

Yes. I'm relying on fast_tx fields to simplify dealing with headers and
to avoid adding extra checks for all kinds of stuff like no software
crypto, which ieee80211_check_fast_xmit already handles.

- Felix