[PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/agg-tx.c | 2 +-
 net/mac80211/ht.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 13b7683..ce9633a 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -107,7 +107,7 @@ static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
 	mgmt->u.action.u.addba_req.start_seq_num =
 					cpu_to_le16(start_seq_num << 4);
 
-	ieee80211_tx_skb_tid(sdata, skb, tid);
+	ieee80211_tx_skb(sdata, skb);
 }
 
 void ieee80211_send_bar(struct ieee80211_vif *vif, u8 *ra, u16 tid, u16 ssn)
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index fab7b91..dc3c280 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -375,7 +375,7 @@ void ieee80211_send_delba(struct ieee80211_sub_if_data *sdata,
 	mgmt->u.action.u.delba.params = cpu_to_le16(params);
 	mgmt->u.action.u.delba.reason_code = cpu_to_le16(reason_code);
 
-	ieee80211_tx_skb_tid(sdata, skb, tid);
+	ieee80211_tx_skb(sdata, skb);
 }
 
 void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
-- 
1.8.5.1.19.gdaad3aa

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Johannes Berg]

Hmm. I guess you're right about the spec, but I vaguely remember races
in this with the delBA going out too soon or so?

johannes

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

On Wed, Jan 22, 2014 at 01:34:39PM +0100, Johannes Berg wrote:
> Hmm. I guess you're right about the spec, but I vaguely remember races
> in this with the delBA going out too soon or so?
> 

Indeed, this was intended by cf6bb79 ("Use appropriate TID for sending
BAR, ADDBA and DELBA frames") and I overlooked it .. will look into it,
thanks.
I Cced Helmut who authored the said commit.
 
Karl

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Helmut Schaa]

On Wed, Jan 22, 2014 at 2:09 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> On Wed, Jan 22, 2014 at 01:34:39PM +0100, Johannes Berg wrote:
>> Hmm. I guess you're right about the spec, but I vaguely remember races
>> in this with the delBA going out too soon or so?
>>
>
> Indeed, this was intended by cf6bb79 ("Use appropriate TID for sending
> BAR, ADDBA and DELBA frames") and I overlooked it .. will look into it,
> thanks.
> I Cced Helmut who authored the said commit.

You're right. There were some issues with that, but that was 2 years ago :)

Sending ADDBA over AV_VO should be safe.
If a DELBA is sent as AC_VO it might get received before the last AMPDU
of the BlockAck session. So, the pending AMPDUs will get dropped at the
receiver.

In theory this could also be avoided by properly flushing all pending AMPDUs
of the TID in question from the hw queues or by waiting for the tx status
of all pending AMPDUs.

Helmut

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

On Wed, Jan 22, 2014 at 02:33:14PM +0100, Helmut Schaa wrote:
> On Wed, Jan 22, 2014 at 2:09 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> > On Wed, Jan 22, 2014 at 01:34:39PM +0100, Johannes Berg wrote:
> >> Hmm. I guess you're right about the spec, but I vaguely remember races
> >> in this with the delBA going out too soon or so?
> >>
> >
> > Indeed, this was intended by cf6bb79 ("Use appropriate TID for sending
> > BAR, ADDBA and DELBA frames") and I overlooked it .. will look into it,
> > thanks.
> > I Cced Helmut who authored the said commit.
> 
> You're right. There were some issues with that, but that was 2 years ago :)
> 
> Sending ADDBA over AV_VO should be safe.
> If a DELBA is sent as AC_VO it might get received before the last AMPDU
> of the BlockAck session. So, the pending AMPDUs will get dropped at the
> receiver.
> 
> In theory this could also be avoided by properly flushing all pending AMPDUs
> of the TID in question from the hw queues or by waiting for the tx status
> of all pending AMPDUs.
> 

I just looked at the code with your change in mind and couldn't find any
issue caused by sending {add,del}ba on AC_VO.
As of today, a delba is sent only after a driver has called 'purposely'
ieee80211_stop_tx_ba_cb_irqsafe so I see no issue with the delba either,
do you see one today ?

Johannes, does it look good to you ? maybe add some comments in the
changelog relating to cf6bb79 ?

 
Karl

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Helmut Schaa]

On Wed, Jan 22, 2014 at 4:28 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> On Wed, Jan 22, 2014 at 02:33:14PM +0100, Helmut Schaa wrote:
>> On Wed, Jan 22, 2014 at 2:09 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
>> > On Wed, Jan 22, 2014 at 01:34:39PM +0100, Johannes Berg wrote:
>> >> Hmm. I guess you're right about the spec, but I vaguely remember races
>> >> in this with the delBA going out too soon or so?
>> >>
>> >
>> > Indeed, this was intended by cf6bb79 ("Use appropriate TID for sending
>> > BAR, ADDBA and DELBA frames") and I overlooked it .. will look into it,
>> > thanks.
>> > I Cced Helmut who authored the said commit.
>>
>> You're right. There were some issues with that, but that was 2 years ago :)
>>
>> Sending ADDBA over AV_VO should be safe.
>> If a DELBA is sent as AC_VO it might get received before the last AMPDU
>> of the BlockAck session. So, the pending AMPDUs will get dropped at the
>> receiver.
>>
>> In theory this could also be avoided by properly flushing all pending AMPDUs
>> of the TID in question from the hw queues or by waiting for the tx status
>> of all pending AMPDUs.
>>
>
> I just looked at the code with your change in mind and couldn't find any
> issue caused by sending {add,del}ba on AC_VO.
> As of today, a delba is sent only after a driver has called 'purposely'
> ieee80211_stop_tx_ba_cb_irqsafe so I see no issue with the delba either,
> do you see one today ?

If the driver does the right thing by flushing the right frames in the
hw tx queues
I think this should be ok. At least rt2x00 is not doing that, but
maybe that's just
an issue in rt2x00 :(

Helmut

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

On Wed, Jan 22, 2014 at 04:36:14PM +0100, Helmut Schaa wrote:
> On Wed, Jan 22, 2014 at 4:28 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> > On Wed, Jan 22, 2014 at 02:33:14PM +0100, Helmut Schaa wrote:
> >> On Wed, Jan 22, 2014 at 2:09 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> >> > On Wed, Jan 22, 2014 at 01:34:39PM +0100, Johannes Berg wrote:
> >> >> Hmm. I guess you're right about the spec, but I vaguely remember races
> >> >> in this with the delBA going out too soon or so?
> >> >>
> >> >
> >> > Indeed, this was intended by cf6bb79 ("Use appropriate TID for sending
> >> > BAR, ADDBA and DELBA frames") and I overlooked it .. will look into it,
> >> > thanks.
> >> > I Cced Helmut who authored the said commit.
> >>
> >> You're right. There were some issues with that, but that was 2 years ago :)
> >>
> >> Sending ADDBA over AV_VO should be safe.
> >> If a DELBA is sent as AC_VO it might get received before the last AMPDU
> >> of the BlockAck session. So, the pending AMPDUs will get dropped at the
> >> receiver.
> >>
> >> In theory this could also be avoided by properly flushing all pending AMPDUs
> >> of the TID in question from the hw queues or by waiting for the tx status
> >> of all pending AMPDUs.
> >>
> >
> > I just looked at the code with your change in mind and couldn't find any
> > issue caused by sending {add,del}ba on AC_VO.
> > As of today, a delba is sent only after a driver has called 'purposely'
> > ieee80211_stop_tx_ba_cb_irqsafe so I see no issue with the delba either,
> > do you see one today ?
> 
> If the driver does the right thing by flushing the right frames in the
> hw tx queues
> I think this should be ok. At least rt2x00 is not doing that, but
> maybe that's just
> an issue in rt2x00 :(
> 


I see you are one of the maintainers of the ralink drivers, do you
intend to fix it in those ?

Callers of ieee80211_stop_tx_ba_cb_irqsafe are:
ath9k
ath9k_htc
carl9170
wcn36xx
brcm80211
iwlegacy
iwlwifi
mwl8k
rt2x00
rtlwifi

Johannes, what is your impression ?

 
Karl

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Johannes Berg]

On Wed, 2014-01-22 at 17:41 +0100, Karl Beldan wrote:

> > >> If a DELBA is sent as AC_VO it might get received before the last AMPDU
> > >> of the BlockAck session. So, the pending AMPDUs will get dropped at the
> > >> receiver.
> > >>
> > >> In theory this could also be avoided by properly flushing all pending AMPDUs
> > >> of the TID in question from the hw queues or by waiting for the tx status
> > >> of all pending AMPDUs.

Let's get the issue straight first.

The (current) documentation for the TX_STOP ampdu actions says:

 * @IEEE80211_AMPDU_TX_STOP_CONT: stop TX aggregation but continue
transmitting
 *      queued packets, now unaggregated. After all packets are
transmitted the
 *      driver has to call ieee80211_stop_tx_ba_cb_irqsafe().
 * @IEEE80211_AMPDU_TX_STOP_FLUSH: stop TX aggregation and flush all
packets,
 *      called when the station is removed. There's no need or reason to
call
 *      ieee80211_stop_tx_ba_cb_irqsafe() in this case as mac80211
assumes the
 *      session is gone and removes the station.
 * @IEEE80211_AMPDU_TX_STOP_FLUSH_CONT: called when TX aggregation is
stopped
 *      but the driver hasn't called ieee80211_stop_tx_ba_cb_irqsafe()
yet and
 *      now the connection is dropped and the station will be removed.
Drivers
 *      should clean up and drop remaining packets when this is called.

I say "current" because we only completed this fairly recently (couple
months ago?) to make it complete with the three different possibilities.

Therefore, I don't think there's a need to ever *flush* the queues,
although the term "flush" is getting confusing and we should probably
call this IEEE80211_AMPDU_TX_STOP_DROP instead of _FLUSH, as that's what
it really means, drop all the remaining frames (if any.)

Given the fact that we only send the frame from
ieee80211_stop_tx_ba_cb() I don't see any problem. Even if we were to
send the frame directly after calling the ampdu_action, it seems it
would be fine, since the callback (now) requires sending the remaining
frames unaggregated. (Given that, I'm not even sure why we required the
packets to be sent unaggregated, Emmanuel, do you remember?)

> Callers of ieee80211_stop_tx_ba_cb_irqsafe are:
> ath9k
> ath9k_htc
> carl9170
> wcn36xx
> brcm80211
> iwlegacy
> iwlwifi
> mwl8k
> rt2x00
> rtlwifi
> 
> Johannes, what is your impression ?

I'm pretty sure iwlwifi should be safe. ath9k/htc/carl9170/wcn36xx are
probably fine, I can't really say anything about the other ones. I'd
guess brcm80211 should be OK since it builds aggregates in software and
should be able to easily stop transmitting aggregates. But see above - I
don't see how even if the driver didn't stop sending aggregates it could
be wrong. Unless the driver called ieee80211_stop_tx_ba_cb() immediately
but didn't actually stop sending. That's pretty much a bug anyway
though.

johannes

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

On Wed, Jan 22, 2014 at 07:51:53PM +0100, Johannes Berg wrote:
> On Wed, 2014-01-22 at 17:41 +0100, Karl Beldan wrote:
> 
> > > >> If a DELBA is sent as AC_VO it might get received before the last AMPDU
> > > >> of the BlockAck session. So, the pending AMPDUs will get dropped at the
> > > >> receiver.
> > > >>
> > > >> In theory this could also be avoided by properly flushing all pending AMPDUs
> > > >> of the TID in question from the hw queues or by waiting for the tx status
> > > >> of all pending AMPDUs.
> 
> Let's get the issue straight first.
> 
> The (current) documentation for the TX_STOP ampdu actions says:
> 
>  * @IEEE80211_AMPDU_TX_STOP_CONT: stop TX aggregation but continue
> transmitting
>  *      queued packets, now unaggregated. After all packets are
> transmitted the
>  *      driver has to call ieee80211_stop_tx_ba_cb_irqsafe().
>  * @IEEE80211_AMPDU_TX_STOP_FLUSH: stop TX aggregation and flush all
> packets,
>  *      called when the station is removed. There's no need or reason to
> call
>  *      ieee80211_stop_tx_ba_cb_irqsafe() in this case as mac80211
> assumes the
>  *      session is gone and removes the station.
>  * @IEEE80211_AMPDU_TX_STOP_FLUSH_CONT: called when TX aggregation is
> stopped
>  *      but the driver hasn't called ieee80211_stop_tx_ba_cb_irqsafe()
> yet and
>  *      now the connection is dropped and the station will be removed.
> Drivers
>  *      should clean up and drop remaining packets when this is called.
> 
> I say "current" because we only completed this fairly recently (couple
> months ago?) to make it complete with the three different possibilities.
> 
> Therefore, I don't think there's a need to ever *flush* the queues,
> although the term "flush" is getting confusing and we should probably
> call this IEEE80211_AMPDU_TX_STOP_DROP instead of _FLUSH, as that's what
> it really means, drop all the remaining frames (if any.)
> 
> Given the fact that we only send the frame from
> ieee80211_stop_tx_ba_cb() I don't see any problem. Even if we were to
> send the frame directly after calling the ampdu_action, it seems it
> would be fine, since the callback (now) requires sending the remaining
> frames unaggregated. (Given that, I'm not even sure why we required the
> packets to be sent unaggregated, Emmanuel, do you remember?)
> 
I'd expect most device to not block ack such frames, and they'd be
right to do so, sending them unaggregated seems the right thing to do.

> > Callers of ieee80211_stop_tx_ba_cb_irqsafe are:
> > ath9k
> > ath9k_htc
> > carl9170
> > wcn36xx
> > brcm80211
> > iwlegacy
> > iwlwifi
> > mwl8k
> > rt2x00
> > rtlwifi
> > 
> > Johannes, what is your impression ?
> 
> I'm pretty sure iwlwifi should be safe. ath9k/htc/carl9170/wcn36xx are
> probably fine, I can't really say anything about the other ones. I'd
> guess brcm80211 should be OK since it builds aggregates in software and
> should be able to easily stop transmitting aggregates. But see above - I
> don't see how even if the driver didn't stop sending aggregates it could
> be wrong. Unless the driver called ieee80211_stop_tx_ba_cb() immediately
> but didn't actually stop sending. That's pretty much a bug anyway
> though.
> 
So, I guess you are taking what I sent ?

 
Karl

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Johannes Berg]

On Wed, 2014-01-22 at 20:16 +0100, Karl Beldan wrote:

> > Given the fact that we only send the frame from
> > ieee80211_stop_tx_ba_cb() I don't see any problem. Even if we were to
> > send the frame directly after calling the ampdu_action, it seems it
> > would be fine, since the callback (now) requires sending the remaining
> > frames unaggregated. (Given that, I'm not even sure why we required the
> > packets to be sent unaggregated, Emmanuel, do you remember?)
> > 
> I'd expect most device to not block ack such frames, and they'd be
> right to do so, sending them unaggregated seems the right thing to do.

Oh, I roughly remember now - we didn't want to separate the cases of us
sending a delBA and us receiving a delBA. If we receive a delBA, we
should stop sending aggregated frames immediately (actually for iwlwifi
the firmware will do that) or as quickly as possible, hence the
requirement

If we decide to tear down the session ourselves then we could continue
sending until later, but it's not worth it.

> So, I guess you are taking what I sent ?

Haven't really made up my mind yet ... I think it's more correct, so I
should, but I also don't really want to break the ralink drivers over
what seems to me to be a fairly small issue.

johannes

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Helmut Schaa]

Johannes Berg <johannes@sipsolutions.net> schrieb:
>On Wed, 2014-01-22 at 20:16 +0100, Karl Beldan wrote:
>
>> > Given the fact that we only send the frame from
>> > ieee80211_stop_tx_ba_cb() I don't see any problem. Even if we were
>to
>> > send the frame directly after calling the ampdu_action, it seems it
>> > would be fine, since the callback (now) requires sending the
>remaining
>> > frames unaggregated. (Given that, I'm not even sure why we required
>the
>> > packets to be sent unaggregated, Emmanuel, do you remember?)
>> > 
>> I'd expect most device to not block ack such frames, and they'd be
>> right to do so, sending them unaggregated seems the right thing to
>do.
>
>Oh, I roughly remember now - we didn't want to separate the cases of us
>sending a delBA and us receiving a delBA. If we receive a delBA, we
>should stop sending aggregated frames immediately (actually for iwlwifi
>the firmware will do that) or as quickly as possible, hence the
>requirement
>
>If we decide to tear down the session ourselves then we could continue
>sending until later, but it's not worth it.
>
>> So, I guess you are taking what I sent ?
>
>Haven't really made up my mind yet ... I think it's more correct, so I
>should, but I also don't really want to break the ralink drivers over
>what seems to me to be a fairly small issue.

I think I'm fine with this now. Let's just see if someone experiences any issues ...

 Furthermore I think i even remember that you can force ralink HW to stop a BA session. But all in all lets better comply with the spec ...

Helmut

RE: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Grumbach, Emmanuel]

PiBKb2hhbm5lcyBCZXJnIDxqb2hhbm5lc0BzaXBzb2x1dGlvbnMubmV0PiBzY2hyaWViOg0KPiA+
T24gV2VkLCAyMDE0LTAxLTIyIGF0IDIwOjE2ICswMTAwLCBLYXJsIEJlbGRhbiB3cm90ZToNCj4g
Pg0KPiA+PiA+IEdpdmVuIHRoZSBmYWN0IHRoYXQgd2Ugb25seSBzZW5kIHRoZSBmcmFtZSBmcm9t
DQo+ID4+ID4gaWVlZTgwMjExX3N0b3BfdHhfYmFfY2IoKSBJIGRvbid0IHNlZSBhbnkgcHJvYmxl
bS4gRXZlbiBpZiB3ZSB3ZXJlDQo+ID50bw0KPiA+PiA+IHNlbmQgdGhlIGZyYW1lIGRpcmVjdGx5
IGFmdGVyIGNhbGxpbmcgdGhlIGFtcGR1X2FjdGlvbiwgaXQgc2VlbXMgaXQNCj4gPj4gPiB3b3Vs
ZCBiZSBmaW5lLCBzaW5jZSB0aGUgY2FsbGJhY2sgKG5vdykgcmVxdWlyZXMgc2VuZGluZyB0aGUN
Cj4gPnJlbWFpbmluZw0KPiA+PiA+IGZyYW1lcyB1bmFnZ3JlZ2F0ZWQuIChHaXZlbiB0aGF0LCBJ
J20gbm90IGV2ZW4gc3VyZSB3aHkgd2UgcmVxdWlyZWQNCj4gPnRoZQ0KPiA+PiA+IHBhY2tldHMg
dG8gYmUgc2VudCB1bmFnZ3JlZ2F0ZWQsIEVtbWFudWVsLCBkbyB5b3UgcmVtZW1iZXI/KQ0KPiA+
PiA+DQo+ID4+IEknZCBleHBlY3QgbW9zdCBkZXZpY2UgdG8gbm90IGJsb2NrIGFjayBzdWNoIGZy
YW1lcywgYW5kIHRoZXknZCBiZQ0KPiA+PiByaWdodCB0byBkbyBzbywgc2VuZGluZyB0aGVtIHVu
YWdncmVnYXRlZCBzZWVtcyB0aGUgcmlnaHQgdGhpbmcgdG8NCj4gPmRvLg0KPiA+DQo+ID5PaCwg
SSByb3VnaGx5IHJlbWVtYmVyIG5vdyAtIHdlIGRpZG4ndCB3YW50IHRvIHNlcGFyYXRlIHRoZSBj
YXNlcyBvZiB1cw0KPiA+c2VuZGluZyBhIGRlbEJBIGFuZCB1cyByZWNlaXZpbmcgYSBkZWxCQS4g
SWYgd2UgcmVjZWl2ZSBhIGRlbEJBLCB3ZQ0KPiA+c2hvdWxkIHN0b3Agc2VuZGluZyBhZ2dyZWdh
dGVkIGZyYW1lcyBpbW1lZGlhdGVseSAoYWN0dWFsbHkgZm9yIGl3bHdpZmkNCj4gPnRoZSBmaXJt
d2FyZSB3aWxsIGRvIHRoYXQpIG9yIGFzIHF1aWNrbHkgYXMgcG9zc2libGUsIGhlbmNlIHRoZQ0K
PiA+cmVxdWlyZW1lbnQNCj4gPg0KPiA+SWYgd2UgZGVjaWRlIHRvIHRlYXIgZG93biB0aGUgc2Vz
c2lvbiBvdXJzZWx2ZXMgdGhlbiB3ZSBjb3VsZCBjb250aW51ZQ0KPiA+c2VuZGluZyB1bnRpbCBs
YXRlciwgYnV0IGl0J3Mgbm90IHdvcnRoIGl0Lg0KPiA+DQoNCkV4YWN0bHkgLSB3ZSB3YW50ZWQg
dG8gdW5pZnkgdGhlIGluaXRpYXRvciAodGhlIFRYaW5nIHNpZGUgZGVjaWRlcyB0byBzdG9wIHRo
ZSBCQSBhZ3JlZW1lbnQpIGFuZCB0aGUgcmVjaXBpZW50ICh0aGUgUlhpbmcgc2lkZSBkZWNpZGVz
IHRvIHN0b3AgdGhlIEJBIGFncmVlbWVudCkgY2FzZXMuIFNvIGl0IGdvZXMgbGlrZSB0aGlzOg0K
SWYgeW91IGdldCBhcmUgVFhpbmcgYW5kIHlvdSBnZXQgYSAocmVjaXBpZW50KSBERUxCQSwgdGhl
biB5b3UgbmVlZCB0byBzdG9wIHNlbmRpbmcgQU1QRFVzIHN0cmFpZ2h0IGF3YXkuIFRoaXMgaXMg
d2h5IEludGVsIGNhcmRzIGhhdmUgdGhlIGZpcm13YXJlIGRvIHRoaXMuIEJ1dCBzaW5jZSBpd2x3
aWZpIHVzZXMgZGlmZmVyZW50IHF1ZXVlcyBmb3Igbm9uLWFnZ3JlZ2F0ZWQgdHJhZmZpYyBhbmQg
QU1QRFVzLCB3ZSBtdXN0IHdhaXQgdW50aWwgYWxsIHRoZSBwYWNrZXRzIGluIHRoZSBBTVBEVSBx
dWV1ZSB3aWxsIGRyYWluLCBhbmQgb25seSB0aGVuLCBhbGxvdyBtYWM4MDIxMSB0byBzZW5kIHBh
Y2tldHMgdG8gdGhlIG5vbi1hZ2dyZWdhdGVkIHRyYWZmaWMgcXVldWUuIE5vdyAtIGV2ZW4gdmVu
ZG9ycyB0aGF0IGRvbid0IHByZXZlbnQgdGhlIEFNUERVcyBpbiBmaXJtd2FyZSBzaG91bGQgc3Rp
bGwgZG8gdGhlIHNhbWUgKHByb3ZpZGVkIHRoYXQgdGhleSB1c2UgZGlmZmVyZW50IHF1ZXVlcyBm
b3IgQU1QRFUgYW5kIG5vbi1BTVBEVSB0cmFmZmljKS4gVGhpcyBpcyBub3QgdG8gcHJldmVudCBz
ZW5kaW5nIEFNUERVcyBhZnRlciBERUxCQSwgYnV0IG1vcmUgdG8gcHJldmVudCBvdXQtb2Ytb3Jk
ZXIgVFguDQpJbiB0aGUgY2FzZSBvZiBvcmlnaW5hdG9yIERFTEJBLCBpdCBpcyBzbGlnaHRseSBk
aWZmZXJlbnQuIFdlIGNhbiBjaG9vc2Ugd2hlbiB0byBzZW5kIHRoZSBERUxCQSBhZnRlciBhbGwu
IE9mIGNvdXJzZSB3ZSBzdGlsbCBoYXZlIHRoZSBvdXQtb2Ytb3JkZXIgdGhpbmcgdG8gdGFrZSBj
YXJlIGFib3V0LCBidXQgYXQgbGVhc3QsIHdlIGNhbiBhdm9pZCB0aGUgIkFNUERVcyBhZnRlciBE
RUxCQSIgZXZlbiB3aXRob3V0IGZpcm13YXJlIHN1cHBvcnQuDQpUaGUgb25seSB0aGluZyB5b3Ug
aGF2ZSB0byBkbyBpcyB0byBtYWtlIHN1cmUgdGhhdCBpZWVlODAyMTFfc3RvcF90eF9iYV9jYiBp
cyBjYWxsZWQgb25seSAqYWZ0ZXIqIHRoZSBkcml2ZXIgaXMgc3VyZSB0aGF0IGFsbCB0aGUgcGFj
a2V0cyBpbiB0aGUgQU1QRFUgcXVldWUgYXJlIHNlbnQuIE1hYzgwMjExIHdvbid0IHNlbmQgdGhl
IERFTEJBIGJlZm9yZSB0aGUgbG93IGxldmVsIGRyaXZlciBjYWxscyBpZWVlODAyMTFfc3RvcF90
eF9iYV9jYi4gU28sIGlmIHRoZSBkcml2ZXIgaXMgYWJsZSB0byB0ZWxsIG1hYzgwMjExIHdoZW4g
aXQgaXMgZmluaXNoZWQgd2l0aCB0aGUgQU1QRFUgcGFja2V0cywgeW91IGNhbiBzYWZlbHkgc2Vu
ZCBpdCBpbiBWTyBhbmQgbm90IGZlYXIgYW55ICJBTVBEVSBhZnRlciBERUxCQSIgcHJvYmxlbS4N
CkFub3RoZXIgKHNpZGUpIHBvaW50LiBFdmVuIGlmIHlvdSBjYW4gc3RvcCBzZW5kaW5nIEFNUERV
cyBzdHJhaWdodCBhd2F5IChpd2x3aWZpIGNhbiBkbyB0aGF0IHdpdGggYSBjb21tYW5kIHRvIHRo
ZSBmaXJtd2FyZSB0aGF0IHdpbGwgYmUgbXVjaCBmYXN0ZXIgdGhhbiB3YWl0aW5nIGZvciBxdWV1
ZXMgdG8gZHJhaW4pLCB5b3Ugc3RpbGwgaGF2ZSB0aGUgVHggb3V0LW9mLW9yZGVyIHByb2JsZW0u
DQpTbyB0aGF0IHRlY2huaWNhbGx5LCB3ZSBjb3VsZCBoYXZlIGEgbW9yZSBmaW5lIGdyYWluIHJl
c29sdXRpb24gaW4gbWFjODAyMTEgdGhhdCBhbGxvd3MgdGhlIGxvdyBsZXZlbCBkcml2ZXIgdG8g
aGF2ZSBtYWM4MDIxMSBzZW5kIHRoZSBERUxCQSBlYXJsaWVyLCBidXQgc3RpbGwgcHJldmVudHMg
bWFjODAyMTEgZnJvbSBzZW5kaW5nIHBhY2tldHMgdG8gdGhlIGxvdyBsZXZlbCBkcml2ZXIgdW50
aWwgdGhlIGFsbCB0aGUgcXVldWVzIGFyZSBkcmFpbmVkLg0KQnV0IHRoaXMgaXMgZGVmaW5pdGVs
eSBub3Qgd29ydGggdGhlIGNvbXBsZXhpdHkgc2luY2UgZGVsYXlpbmcgdGhlIERFTEJBIGJ5IGEg
Yml0IGRvZXNuJ3QgY29zdCBhbnl0aGluZy4NCg0KSG9wZSB0aGF0IGhlbHBzIDopDQoNCj4gPj4g
U28sIEkgZ3Vlc3MgeW91IGFyZSB0YWtpbmcgd2hhdCBJIHNlbnQgPw0KPiA+DQo+ID5IYXZlbid0
IHJlYWxseSBtYWRlIHVwIG15IG1pbmQgeWV0IC4uLiBJIHRoaW5rIGl0J3MgbW9yZSBjb3JyZWN0
LCBzbyBJDQo+ID5zaG91bGQsIGJ1dCBJIGFsc28gZG9uJ3QgcmVhbGx5IHdhbnQgdG8gYnJlYWsg
dGhlIHJhbGluayBkcml2ZXJzIG92ZXINCj4gPndoYXQgc2VlbXMgdG8gbWUgdG8gYmUgYSBmYWly
bHkgc21hbGwgaXNzdWUuDQo+IA0KPiBJIHRoaW5rIEknbSBmaW5lIHdpdGggdGhpcyBub3cuIExl
dCdzIGp1c3Qgc2VlIGlmIHNvbWVvbmUgZXhwZXJpZW5jZXMgYW55DQo+IGlzc3VlcyAuLi4NCj4g
DQo+ICBGdXJ0aGVybW9yZSBJIHRoaW5rIGkgZXZlbiByZW1lbWJlciB0aGF0IHlvdSBjYW4gZm9y
Y2UgcmFsaW5rIEhXIHRvIHN0b3AgYQ0KPiBCQSBzZXNzaW9uLiBCdXQgYWxsIGluIGFsbCBsZXRz
IGJldHRlciBjb21wbHkgd2l0aCB0aGUgc3BlYyAuLi4NCg0KV2VsbCAtIHlvdSBuZWVkIHRvIHNl
ZSB3aGF0ICJzdG9wIGEgQkEgc2Vzc2lvbiIgbWVhbi4gSWYgeW91IGFyZSBhYmxlIHRvIHRlbGwg
dGhlIEhXIG5vIEFNUERVcyBhbnltb3JlICpub3cqLCB0aGVuIG9rIC0gYnV0IHlvdSBzdGlsbCBo
YXZlIHRoZSBvdXQtb2Ytb3JkZXIgVFggcHJvYmxlbS4gVW5sZXNzIHlvdSB1c2UgdGhlIHNhbWUg
VFggcXVldWUgZm9yIEFNUFVzIGFuZCBub24tQU1QRFVzIGFuZCBkb24ndCBmZWFyIHJlb3JkZXJp
bmcgaXNzdWVzDQoNCj4gDQo+IEhlbG11dA0K

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Helmut Schaa]

On Thu, Jan 23, 2014 at 7:08 AM, Grumbach, Emmanuel
<emmanuel.grumbach@intel.com> wrote:
>> >> So, I guess you are taking what I sent ?
>> >
>> >Haven't really made up my mind yet ... I think it's more correct, so I
>> >should, but I also don't really want to break the ralink drivers over
>> >what seems to me to be a fairly small issue.
>>
>> I think I'm fine with this now. Let's just see if someone experiences any
>> issues ...
>>
>>  Furthermore I think i even remember that you can force ralink HW to stop a
>> BA session. But all in all lets better comply with the spec ...
>
> Well - you need to see what "stop a BA session" mean. If you are able to tell the HW no AMPDUs anymore *now*, then ok - but you still have the out-of-order TX problem. Unless you use the same TX queue for AMPUs and non-AMPDUs and don't fear reordering issues

We shouldn't have reordering issues in rt2x00 since AMPDUs and MPDUs
go through the same hw queue.
However, at the moment the hw will keep on sending AMPDUs that are in
the TX queue even after we received a DELBA.
But this is an issue with the ralink drivers ...

Helmut

Re: [PATCH] mac80211: send {add,del}ba on AC_VO like other mgmt frames, as per spec

[Johannes Berg]

On Thu, 2014-01-23 at 14:42 +0100, Helmut Schaa wrote:
> On Thu, Jan 23, 2014 at 7:08 AM, Grumbach, Emmanuel
> <emmanuel.grumbach@intel.com> wrote:
> >> >> So, I guess you are taking what I sent ?
> >> >
> >> >Haven't really made up my mind yet ... I think it's more correct, so I
> >> >should, but I also don't really want to break the ralink drivers over
> >> >what seems to me to be a fairly small issue.
> >>
> >> I think I'm fine with this now. Let's just see if someone experiences any
> >> issues ...
> >>
> >>  Furthermore I think i even remember that you can force ralink HW to stop a
> >> BA session. But all in all lets better comply with the spec ...
> >
> > Well - you need to see what "stop a BA session" mean. If you are able to tell the HW no AMPDUs anymore *now*, then ok - but you still have the out-of-order TX problem. Unless you use the same TX queue for AMPUs and non-AMPDUs and don't fear reordering issues
> 
> We shouldn't have reordering issues in rt2x00 since AMPDUs and MPDUs
> go through the same hw queue.
> However, at the moment the hw will keep on sending AMPDUs that are in
> the TX queue even after we received a DELBA.
> But this is an issue with the ralink drivers ...

Ok.

Karl, can you rewrite the commit log, maybe referencing the original
commit and this discussion (I'd suggest using
http://mid.gmane.org/1390391564-18481-1-git-send-email-karl.beldan@gmail.com) please? Then I'll apply that.

Thanks,
johannes

[PATCH v2] mac80211: send {ADD,DEL}BA on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

ATM, {ADD,DEL}BA and BAR frames are sent on the AC matching the TID of
the BA parameters. Johannes recalled [1] that it fixed some races with
the DELBA and indeed this behavior was introduced in [2].
While [2] is right for the BARs, the part queueing the {ADD,DEL}BAs on
their BA params TID AC violates the spec and is more a workaround for
some drivers. Helmut expressed [2] some concerns wrt such drivers, in
particular DELBAs in rt2x00.
ATM, DELBAs are sent after a driver has called (hence "purposely")
ieee80211_start_tx_ba_cb_irqsafe and Johannes and Emmanuel gave [2] some
details wrt intentions behind the split of the IEEE80211_AMPDU_TX_STOP_*
given to the drv ampdu_action supposed to call this function, which
could prove handy to people trying to do the right thing in faulty
drivers (if their fw/hw don't get in their way).

[1] Message-Id: http://mid.gmane.org/1390391564-18481-1-git-send-email-karl.beldan@gmail.com
[2] Commit: cf6bb79ad8287cd9fe87 ("mac80211: Use appropriate TID for sending BAR, ADDBA and DELBA frames")

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
 net/mac80211/agg-tx.c | 2 +-
 net/mac80211/ht.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 13b7683..ce9633a 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -107,7 +107,7 @@ static void ieee80211_send_addba_request(struct ieee80211_sub_if_data *sdata,
 	mgmt->u.action.u.addba_req.start_seq_num =
 					cpu_to_le16(start_seq_num << 4);
 
-	ieee80211_tx_skb_tid(sdata, skb, tid);
+	ieee80211_tx_skb(sdata, skb);
 }
 
 void ieee80211_send_bar(struct ieee80211_vif *vif, u8 *ra, u16 tid, u16 ssn)
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index fab7b91..dc3c280 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -375,7 +375,7 @@ void ieee80211_send_delba(struct ieee80211_sub_if_data *sdata,
 	mgmt->u.action.u.delba.params = cpu_to_le16(params);
 	mgmt->u.action.u.delba.reason_code = cpu_to_le16(reason_code);
 
-	ieee80211_tx_skb_tid(sdata, skb, tid);
+	ieee80211_tx_skb(sdata, skb);
 }
 
 void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
-- 
1.8.5.1.19.gdaad3aa

Re: [PATCH v2] mac80211: send {ADD,DEL}BA on AC_VO like other mgmt frames, as per spec

[Johannes Berg]

On Thu, 2014-01-23 at 20:06 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> ATM, {ADD,DEL}BA and BAR frames are sent on the AC matching the TID of
> the BA parameters. Johannes recalled [1] that it fixed some races with
> the DELBA and indeed this behavior was introduced in [2].

Applied. I made some minor edits to the commit log, notably to fix that
you had too many [2] (I guess originally you wanted to link to all the
different emails?)

johannes

Re: [PATCH v2] mac80211: send {ADD,DEL}BA on AC_VO like other mgmt frames, as per spec

[Karl Beldan]

On Thu, Jan 23, 2014 at 08:15:24PM +0100, Johannes Berg wrote:
> On Thu, 2014-01-23 at 20:06 +0100, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > ATM, {ADD,DEL}BA and BAR frames are sent on the AC matching the TID of
> > the BA parameters. Johannes recalled [1] that it fixed some races with
> > the DELBA and indeed this behavior was introduced in [2].
> 
> Applied. I made some minor edits to the commit log, notably to fix that
> you had too many [2] (I guess originally you wanted to link to all the
> different emails?)
> 
Yes, only the 2 first [2]s were intended [2]s .. in the very end of the
day I had to struggle to write this, I guess it shows.
 
Karl