iwlwifi workaround for mac80211 sequence number bug

iwlwifi workaround for mac80211 sequence number bug

[Johannes Berg]

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

Ok, so I'm frustrated. I spent a few hours today chasing docs, trying to
understand sequence numbering from the specs, how we implement it in
mac80211 for TX and RX, how hardware/firmware implements it. Finally, I
figure out that mac80211 is buggy, and come up with a buggy (now I know)
patch.

Then, now, I find out that iwlwifi contains a workaround for the
mac80211 sequence numbering, so you've known all along!

Why don't you tell anyone about the problems you find? It can't be that
hard. Are there any other lurking issues like that?

Here's the code from iwlwifi:

        if (ieee80211_is_data_qos(fc)) {
                qc = ieee80211_get_qos_ctl(hdr);
                tid = qc[0] & 0xf;
                seq_number = priv->stations[sta_id].tid[tid].seq_number &
                                IEEE80211_SCTL_SEQ;
                hdr->seq_ctrl = cpu_to_le16(seq_number) |
                        (hdr->seq_ctrl &
                                __constant_cpu_to_le16(IEEE80211_SCTL_FRAG));
                seq_number += 0x10;
                /* aggregation is on for this <sta,tid> */
                if (info->flags & IEEE80211_TX_CTL_AMPDU)
                        txq_id = priv->stations[sta_id].tid[tid].agg.txq_id;
                priv->stations[sta_id].tid[tid].tfds_in_queue++;
        }

We can implement that trivially in mac80211 and make it do the right
thing for all drivers instead of having iwlwifi work around the bug in
mac80211.

Additionally, we can then remove the ssn argument to the ampdu_action
callback.

johannes

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

Re: iwlwifi workaround for mac80211 sequence number bug

[Tomas Winkler]

On Thu, Jul 10, 2008 at 1:11 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Ok, so I'm frustrated. I spent a few hours today chasing docs, trying to
> understand sequence numbering from the specs, how we implement it in
> mac80211 for TX and RX, how hardware/firmware implements it. Finally, I
> figure out that mac80211 is buggy, and come up with a buggy (now I know)
> patch.
>
> Then, now, I find out that iwlwifi contains a workaround for the
> mac80211 sequence numbering, so you've known all along!
>
> Why don't you tell anyone about the problems you find? It can't be that
> hard. Are there any other lurking issues like that?
>
> Here's the code from iwlwifi:
>
>        if (ieee80211_is_data_qos(fc)) {
>                qc = ieee80211_get_qos_ctl(hdr);
>                tid = qc[0] & 0xf;
>                seq_number = priv->stations[sta_id].tid[tid].seq_number &
>                                IEEE80211_SCTL_SEQ;
>                hdr->seq_ctrl = cpu_to_le16(seq_number) |
>                        (hdr->seq_ctrl &
>                                __constant_cpu_to_le16(IEEE80211_SCTL_FRAG));
>                seq_number += 0x10;
>                /* aggregation is on for this <sta,tid> */
>                if (info->flags & IEEE80211_TX_CTL_AMPDU)
>                        txq_id = priv->stations[sta_id].tid[tid].agg.txq_id;
>                priv->stations[sta_id].tid[tid].tfds_in_queue++;
>        }
>
> We can implement that trivially in mac80211 and make it do the right
> thing for all drivers instead of having iwlwifi work around the bug in
> mac80211.
>
> Additionally, we can then remove the ssn argument to the ampdu_action
> callback.
>
This is not a bug in mac.
The sequence number can be committed only if the packet is not dropped
somewhere in the driver. See lower in the function.. There are plenty
of  places on the TX path when packet can be dropped this it's last
station.  There is no reason to pushing this into mac
Tomas



> johannes
>

Re: iwlwifi workaround for mac80211 sequence number bug

[Johannes Berg]

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

On Thu, 2008-07-10 at 01:17 +0300, Tomas Winkler wrote:

> This is not a bug in mac.
> The sequence number can be committed only if the packet is not dropped
> somewhere in the driver. See lower in the function.. There are plenty
> of  places on the TX path when packet can be dropped this it's last
> station.  There is no reason to pushing this into mac

I don't see it that way. In fact, in the code I'm looking at, there is
nothing that drops the frame before committing the sequence number
change after assigning it.

And all the cases where it drops the frame _before_ the piece of code I
quoted don't actually matter, the first bunch of those are bugs and the
not associated one is strange but most likely doesn't matter either (if
we're not associated sequence numbers don't matter), and the
"destination STA entry not found" one doesn't matter either since that
means there's some disconnect/bug between iwlwifi and mac80211 since
they should synchronise via sta_notify().

Which particular item do you see as problematic?

johannes

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

Re: iwlwifi workaround for mac80211 sequence number bug

[Tomas Winkler]

On Thu, Jul 10, 2008 at 1:24 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2008-07-10 at 01:17 +0300, Tomas Winkler wrote:
>
>> This is not a bug in mac.
>> The sequence number can be committed only if the packet is not dropped
>> somewhere in the driver. See lower in the function.. There are plenty
>> of  places on the TX path when packet can be dropped this it's last
>> station.  There is no reason to pushing this into mac
>
> I don't see it that way. In fact, in the code I'm looking at, there is
> nothing that drops the frame before committing the sequence number
> change after assigning it.
>
> And all the cases where it drops the frame _before_ the piece of code I
> quoted don't actually matter, the first bunch of those are bugs and the
> not associated one is strange but most likely doesn't matter either (if
> we're not associated sequence numbers don't matter), and the
> "destination STA entry not found" one doesn't matter either since that
> means there's some disconnect/bug between iwlwifi and mac80211 since
> they should synchronise via sta_notify().

Unfortunately sta_notify still does nothing in iwlwifi, need some more
work on this.
I've eliminated most dropping places over the time if you look in the
history it wasn't like that before and not all of the parts of the
code develops in the same speed.
Anyhow it doesn't matter for the packet that was dropped but it matter
for the packet that will come.  I think that advancing sequence number
should be low as possible. I'm not sure about other drivers' drop
points.

> Which particular item do you see as problematic?
Most problem at the time was found a good place for it in the mac80211.

Tomas