On Mon, Dec 07, 2020 at 01:31:09PM +0100, Tobias Heider wrote:
> Some APs request a BA agreement and continue to send QOS packets
> for the same tid (with normal ack policy). Currently, these packets
> make it to the higher layers without going through BA reordering or the
> BA buffer. This results in reduced performance later on as the sequence
> numbers are expected by BA reordering.
>
> To fix this, we should use BA agreement immediately after it is
> requested by the AP. This causes the sequence number counter in
> the BA agreement to advance for the normal qos packets and the gap
> wait later on is avoided.
>
> ok?
Not yet, see below:
> Index: ieee80211_input.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.221
> diff -u -p -r1.221 ieee80211_input.c
> --- ieee80211_input.c 28 Aug 2020 12:01:48 -0000 1.221
> +++ ieee80211_input.c 7 Dec 2020 11:57:19 -0000
> @@ -358,6 +358,18 @@ ieee80211_inputm(struct ifnet *ifp, stru
> /* go through A-MPDU reordering */
> ieee80211_input_ba(ic, m, ni, tid, rxi, ml);
> return; /* don't free m! */
> + } else if (ba_state == IEEE80211_BA_REQUESTED &&
> + (qos & IEEE80211_QOS_ACK_POLICY_MASK) ==
> + IEEE80211_QOS_ACK_POLICY_NORMAL) {
> + /*
> + * Apparently, qos frames for a tid where a
> + * block ack agreement was requested but not
> + * yet confirmed by us should still contribute
> + * to the sequence number for this tid.
> + * XXX Add quote from the standard...
Please remove the last XXX line from this comment.
Whatever the standard says about this is irrelevant.
The fact is there are 802.11 implementations which require block ack to
operate before the WPA handshake is done, and some that only enable block
ack when WPA2 has been set up.
The former implementation choice is problematic (why remember state for a
peer that hasn't even been authenticated? WTF!) but we have to accept such
behaviour from peers for interop, regardless of the standard's opinion
on this.
> + */
> + ieee80211_input_ba(ic, m, ni, tid, rxi, ml);
> + return; /* don't free m! */
> }
> }
>
> @@ -2698,6 +2710,9 @@ ieee80211_recv_addba_req(struct ieee8021
> ssn = LE_READ_2(&frm[7]) >> 4;
>
> ba = &ni->ni_rx_ba[tid];
> + /* The driver is still processing an ADDBA request for this tid. */
> + if (ba->ba_state == IEEE80211_BA_REQUESTED)
> + return;
> /* check if we already have a Block Ack agreement for this RA/TID */
> if (ba->ba_state == IEEE80211_BA_AGREED) {
> /* XXX should we update the timeout value? */
> @@ -2737,7 +2752,7 @@ ieee80211_recv_addba_req(struct ieee8021
> goto refuse;
>
> /* setup Block Ack agreement */
> - ba->ba_state = IEEE80211_BA_INIT;
> + ba->ba_state = IEEE80211_BA_REQUESTED;
> ba->ba_timeout_val = timeout * IEEE80211_DUR_TU;
> ba->ba_ni = ni;
> ba->ba_token = token;
> @@ -2811,11 +2826,20 @@ void
> ieee80211_addba_req_refuse(struct ieee80211com *ic, struct ieee80211_node
> *ni,
> uint8_t tid)
> {
> +#ifdef IEEE80211_DEBUG
> + struct ifnet *ifp = &ic->ic_if;
> +#endif
> struct ieee80211_rx_ba *ba = &ni->ni_rx_ba[tid];
>
> + if (ba->ba_state != IEEE80211_BA_REQUESTED) {
> + DPRINTF(("%s: Invalid ba_state=%d\n", ifp->if_xname,
> + ba->ba_state));
> + }
I think the above debug hunk should be dropped. There's way too much
debug printf in interrupt context happening already in wifi, to the
point where some drivers fail to work if DEBUG is enabled. It is better
to keep debug prints as local changes where needed.
> +
> free(ba->ba_buf, M_DEVBUF,
> IEEE80211_BA_MAX_WINSZ * sizeof(*ba->ba_buf));
> ba->ba_buf = NULL;
> + ba->ba_state = IEEE80211_BA_INIT;
>
> /* MLME-ADDBA.response */
> IEEE80211_SEND_ACTION(ic, ni, IEEE80211_CATEG_BA,
>
