ath9k fragmentation

ath9k fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]

Hi,

Just a quick note -- was testing fragmentation and happened to use ath9k
as the test station, this failed utterly, it sent increasing sequence
numbers and no fragment number.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

ath9k fragmentation

[Sujith]

Johannes Berg wrote:
> Just a quick note -- was testing fragmentation and happened to use ath9k
> as the test station, this failed utterly, it sent increasing sequence
> numbers and no fragment number.

Sequence number handling is pretty much borked in ath9k.
There is a comment in assign_aggr_tid_seqno@xmit.c - and it doesn't make much sense.
Removing all crappy code in ath9k dealing with this and leaving everything
to mac80211 would certainly be the better option, but we still have to figure
out how to manage BA windows.

Sujith

Re: ath9k fragmentation

[Sujith]

Sujith wrote:
> Johannes Berg wrote:
> > Just a quick note -- was testing fragmentation and happened to use ath9k
> > as the test station, this failed utterly, it sent increasing sequence
> > numbers and no fragment number.
> 
> Sequence number handling is pretty much borked in ath9k.
> There is a comment in assign_aggr_tid_seqno@xmit.c - and it doesn't make much sense.
> Removing all crappy code in ath9k dealing with this and leaving everything
> to mac80211 would certainly be the better option, but we still have to figure
> out how to manage BA windows.
> 

And a quick hack to fix this would be to not increment the internal sequence
number when the frame is fragmented. But that is what it would be, a hack. :)

Sujith

Re: ath9k fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Fri, 2009-03-13 at 07:34 +0530, Sujith wrote:

> Sequence number handling is pretty much borked in ath9k.
> There is a comment in assign_aggr_tid_seqno@xmit.c - and it doesn't make much sense.

Indeed, that doesn't seem to reflect the code at all.

> Removing all crappy code in ath9k dealing with this and leaving everything
> to mac80211 would certainly be the better option, but we still have to figure
> out how to manage BA windows.

Can you explain a little what is required?

For ampdu_action(), I recently saw something, somewhere, saying
"mac80211 expects us to fill in the seqno variable" or so, but this is
only true if you don't use mac80211's sequence numbers, otherwise it is
fine to leave it at the pre-assigned value.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: ath9k fragmentation

[Sujith]

Johannes Berg wrote:
> > Removing all crappy code in ath9k dealing with this and leaving everything
> > to mac80211 would certainly be the better option, but we still have to figure
> > out how to manage BA windows.
> 
> Can you explain a little what is required?
> 

Managing per-TID state to handle Block ACKs, failed sub-frames,
sub-frame retries and other window management stuff.
ath9k manages all this internally.

> For ampdu_action(), I recently saw something, somewhere, saying
> "mac80211 expects us to fill in the seqno variable" or so, but this is
> only true if you don't use mac80211's sequence numbers, otherwise it is
> fine to leave it at the pre-assigned value.

ampdu_action() requires the driver to set the starting sequence number
for initiating an ADDBA session, ath9k was filling it incorrectly.
This was fixed in the patch, "ath9k: Fix bug in TX aggregation".

Sujith

Re: ath9k fragmentation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Fri, 2009-03-13 at 16:18 +0530, Sujith wrote:

> Managing per-TID state to handle Block ACKs, failed sub-frames,
> sub-frame retries and other window management stuff.
> ath9k manages all this internally.

Ok. I'd have to look into it to understand what we could do there.

> > For ampdu_action(), I recently saw something, somewhere, saying
> > "mac80211 expects us to fill in the seqno variable" or so, but this is
> > only true if you don't use mac80211's sequence numbers, otherwise it is
> > fine to leave it at the pre-assigned value.
> 
> ampdu_action() requires the driver to set the starting sequence number
> for initiating an ADDBA session, ath9k was filling it incorrectly.
> This was fixed in the patch, "ath9k: Fix bug in TX aggregation".

Yes, but it only requires it to do that if the driver assigns sequence
numbers, if it relies on mac80211 then it doesn't need to touch the
value :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]