rt2x00 update

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Signed-off-by Ivo van Doorn <[email protected]>

---
 net/mac80211/tx.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

net/mac80211/tx.c +5 -0

index 359cb36..0e7e1f3 100644
@@ -1876,6 +1876,8 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 		hdr->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
 						  IEEE80211_STYPE_BEACON);
 
+		ieee80211_include_sequence(sdata, hdr);
+
 		num_beacons = &ifsta->num_beacons;
 	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		/* headroom, head length, tail length and maximum TIM length */

@@ -1896,6 +1898,9 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 			cpu_to_le16(local->hw.conf.beacon_int);
 		mgmt->u.beacon.capab_info = 0x0; /* 0x0 for MPs */
 
+		ieee80211_include_sequence(sdata,
+					   (struct ieee80211_hdr *)skb->data);
+
 		pos = skb_put(skb, 2);
 		*pos++ = WLAN_EID_SSID;
 		*pos++ = 0x0;

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Signed-off-by Ivo van Doorn <[email protected]>

---
 net/mac80211/tx.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

net/mac80211/tx.c +5 -0

index 359cb36..0e7e1f3 100644
@@ -1876,6 +1876,8 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 		hdr->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
 						  IEEE80211_STYPE_BEACON);
 
+		ieee80211_include_sequence(sdata, hdr);
+
 		num_beacons = &ifsta->num_beacons;
 	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		/* headroom, head length, tail length and maximum TIM length */

@@ -1896,6 +1898,9 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 			cpu_to_le16(local->hw.conf.beacon_int);
 		mgmt->u.beacon.capab_info = 0x0; /* 0x0 for MPs */
 
+		ieee80211_include_sequence(sdata,
+					   (struct ieee80211_hdr *)skb->data);
+
 		pos = skb_put(skb, 2);
 		*pos++ = WLAN_EID_SSID;
 		*pos++ = 0x0;

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

Quoted 28 lines

Signed-off-by Ivo van Doorn <[email protected]>

---
 net/mac80211/tx.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

net/mac80211/tx.c +5 -0

index 359cb36..0e7e1f3 100644
@@ -1876,6 +1876,8 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 		hdr->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
 						  IEEE80211_STYPE_BEACON);
 
+		ieee80211_include_sequence(sdata, hdr);
+
 		num_beacons = &ifsta->num_beacons;
 	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		/* headroom, head length, tail length and maximum TIM length */

@@ -1896,6 +1898,9 @@ struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
 			cpu_to_le16(local->hw.conf.beacon_int);
 		mgmt->u.beacon.capab_info = 0x0; /* 0x0 for MPs */
 
+		ieee80211_include_sequence(sdata,
+					   (struct ieee80211_hdr *)skb->data);
+
 		pos = skb_put(skb, 2);
 		*pos++ = WLAN_EID_SSID;
 		*pos++ = 0x0;

Quoted 5 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Quoted 3 lines

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

For rt2x00 the beacon is requested and queued within interrupt context or during resume when it doesn't allow mac80211 to send any frames yet.

The problem would be when the config_interface() is called, if during that call mac80211 also sends out TX frames, then you get problems in the sequence counting.

Quoted 6 lines · Ivo van Doorn

Quoted 5 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Quoted 6 lines

Quoted 3 lines

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

For rt2x00 the beacon is requested and queued within interrupt context or during resume when it doesn't allow mac80211 to send any frames yet.

Ok, in interrupt context definitely works.

Quoted 3 lines

The problem would be when the config_interface() is called, if during that call mac80211 also sends out TX frames, then you get problems in the sequence counting.

Well, if you care about the beacon in config_interface() you're implementing hwseq since otherwise you'd only request it when you need it, no?

Quoted 3 lines

Well, if you care about the beacon in config_interface() you're implementing hwseq since otherwise you'd only request it when you need it, no?

No not really, because from what moment can the driver start requesting beacons? I would think when mac80211 calls config_interface with the IEEE80211_IFCC_BEACON flag. ;)

At that moment the first beacon is loaded, and the device will trigger the interrupt to request a new beacon every x milliseconds.

Quoted 8 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

For rt2x00 the beacon is requested and queued within interrupt context

Quoted 10 lines · On Wednesday 09 July 2008 16:04:50 Ivo van Doorn wrote:

Quoted 8 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

Quoted 1 line

For rt2x00 the beacon is requested and queued within interrupt context

Well, another CPU could be in progress of walking down the mac80211 TX code and aquire a sequence number in the meantime before you requested the beacon. However that frame will be blocked by your driver locks, so the two seq numbers of the beacon and the other frame will be swapped, as the driver will queue the beacon first.

Quoted 11 lines · Michael Buesch

Quoted 10 lines · On Wednesday 09 July 2008 16:04:50 Ivo van Doorn wrote:

Quoted 8 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

Quoted 7 lines

Quoted 1 line

For rt2x00 the beacon is requested and queued within interrupt context

Well, another CPU could be in progress of walking down the mac80211 TX code and aquire a sequence number in the meantime before you requested the beacon. However that frame will be blocked by your driver locks, so the two seq numbers of the beacon and the other frame will be swapped, as the driver will queue the beacon first.

Indeed, I was wrong.

I wonder if we should remove the hwseq support completely. It's much easier for the driver to do this, especially since we pass a vif pointer with driver-private data to all relevant functions and the driver could keep the current sequence number in there. Or, just like hwseq would, keep a global sequence number?

How does that affect multi-bss support btw? Do we have to use sw seqno for that?

If rt2x00 would use a global lock to block all TX and beacons, then yes. But rt2x00 uses per-queue locking. When a beacon is being updated rt2x00 will still allow regular frames to be queued.

As far as rt2x00 is concerned, all hardware that supports multi-bss also support HW sequence counters. rt2400pci and rt2500pci are the only ones requiring SW sequence counters, and they can't do multi-bss.

Quoted 4 lines

If rt2x00 would use a global lock to block all TX and beacons, then yes. But rt2x00 uses per-queue locking. When a beacon is being updated rt2x00 will still allow regular frames to be queued.

That just widens the window. And if you have multiple queues then you can't do sw sequence numbers anyway because the hardware might reorder the frames.

Quoted 3 lines

As far as rt2x00 is concerned, all hardware that supports multi-bss also support HW sequence counters. rt2400pci and rt2500pci are the only ones requiring SW sequence counters, and they can't do multi-bss.

Do they keep per-BSS counters?

Quoted 5 lines · Johannes Berg

Quoted 4 lines

If rt2x00 would use a global lock to block all TX and beacons, then yes. But rt2x00 uses per-queue locking. When a beacon is being updated rt2x00 will still allow regular frames to be queued.

Quoted 3 lines

That just widens the window. And if you have multiple queues then you can't do sw sequence numbers anyway because the hardware might reorder the frames.

That doesn't seem to be a problem in the legacy drivers, so I guess the hardware does *something* to prevent problems.

Quoted 10 lines · On Wednesday 09 July 2008 17:08:04 Ivo van Doorn wrote:

Quoted 8 lines · Johannes Berg

Quoted 4 lines

If rt2x00 would use a global lock to block all TX and beacons, then yes. But rt2x00 uses per-queue locking. When a beacon is being updated rt2x00 will still allow regular frames to be queued.

That just widens the window. And if you have multiple queues then you can't do sw sequence numbers anyway because the hardware might reorder the frames.

Quoted 2 lines

That doesn't seem to be a problem in the legacy drivers, so I guess the hardware does *something* to prevent problems.

Well, legacy drivers don't use mac80211. Sequence counting can trivially be fixed by doing it _right_ before queueing the packet to the hardware in the driver, with the TX queue lock held, that must already be there.

True, I have to double check it, but the legacy drivers use per-queue locking as well. And it would still be possible for the driver to send out frames with a higher sequence number before one with a lower number.

But I'll put out a request on the rt2400-devel list to see if anybody is interested in looking into this issue for rt2400pci and rt2500pci. In if there is nobody, I'll disable adhoc and master mode for those drivers to prevent problems.

Quoted 3 lines · Ivo van Doorn

True, I have to double check it, but the legacy drivers use per-queue locking as well. And it would still be possible for the driver to send out frames with a higher sequence number before one with a lower number.

The legacy drivers hopefully also implement proper sequence numbering per TID. I'll send another mail on the topic.

Quoted 3 lines

But I'll put out a request on the rt2400-devel list to see if anybody is interested in looking into this issue for rt2400pci and rt2500pci. In if there is nobody, I'll disable adhoc and master mode for those drivers to prevent problems.

Heh, that works. Just disable it and tell folks who want it to work to fix it :)

Quoted 6 lines · Johannes Berg

Quoted 3 lines · Ivo van Doorn

True, I have to double check it, but the legacy drivers use per-queue locking as well. And it would still be possible for the driver to send out frames with a higher sequence number before one with a lower number.

Quoted 2 lines

The legacy drivers hopefully also implement proper sequence numbering per TID. I'll send another mail on the topic.

Heh, I seriously doubt that. As far as I know it is working using a single counter in the main interface structure. Just the same way it currently works in mac80211 (which isn't surprising, since I based the mac80211 patch on the legacy driver implementation).

Ok so the thing is this: 802.11-2007 7.1.3.4.1 requires that we keep sequence counters per TID (and one for "not QoS"), which we only assign rather late in the wme code. Because even b43 hardware only keeps track of a single sequence counter, it requires not using that counter for QoS frames. I suspect no driver except the overly smart iwlwifi gets this right.

On Wed, 2008-07-09 at 17:49 +0200, Johannes Berg wrote:
=20

Quoted 1 line

Ok so the thing is this: 802.11-2007 7.1.3.4.1 requires that we kee=

Quoted 1 line

sequence counters per TID (and one for "not QoS"), which we only as=

Quoted 1 line

rather late in the wme code. Because even b43 hardware only keeps t=

Quoted 1 line

of a single sequence counter, it requires not using that counter fo=

Quoted 1 line

frames. =EF=BB=BFI suspect no driver except the overly smart iwlwif=

Quoted 1 line

right.

=20
Even if we fix this, we still haven't solved the problem of the beaco=

sequence number. I'm currently favouring putting the burden on the driver since only =EF=BB=BFrt2400pci and rt2500pci seem to require it=

fairly simple to keep a race-free per-VIF counter for non-QoS frames =

the driver (you can just do it under the queue lock), but doing it race-free in mac80211 will need a bunch of extra locking. =20
I'll put off the fix until after davem's TX MQ rework though.

Quoted 1 line

I'll put off the fix until after davem's TX MQ rework though.

Ok, that would allow some time for rt2x00 to come up with the implementation for the rt2400pci and rt2500pci drivers.

And this is extra motivation for a volunteer to look into this, because when mac80211 removes the seq counting, it is not just adhoc and master mode that are broken. ;)

Quoted 1 line · Ivo van Doorn

Quoted 1 line

I'll put off the fix until after davem's TX MQ rework though.

Actually, I don't have to.

Quoted 3 lines

Ok, that would allow some time for rt2x00 to come up with the implementation for the rt2400pci and rt2500pci drivers.

Quoted 3 lines

And this is extra motivation for a volunteer to look into this, because when mac80211 removes the seq counting, it is not just adhoc and master mode that are broken. ;)

Here's a patch to try it out:

http://johannes.sipsolutions.net/patches/kernel/all/2008-07-09-17%3a38/022-mac80211-fix-hwseq.patch

Quoted 1 line

http://johannes.sipsolutions.net/patches/kernel/all/2008-07-09-17%3a38/022-mac80211-fix-hwseq.patch

Personally I have my doubts about the meaningfullness of the flag

	IEEE80211_TX_CTL_QOS_FRAME
since drivers will probably use ieee80211_is_data_qos() instead.
Perhaps it is better to have the flag named something like:
	IEEE80211_TX_CTS_SW_SEQUENCE
or
	IEEE80211_TX_CTS_HAS_SEQUENCE

That way it is a bit clearer what the intention of the flag is.

Quoted 4 lines · Ivo van Doorn

Quoted 3 lines · Johannes Berg

On Wed, 2008-07-09 at 17:36 +0200, Ivo van Doorn wrote:
=20

Quoted 1 line

True, I have to double check it, but the legacy drivers use per=

Quoted 1 line

Quoted 1 line

Quoted 1 line

And it would still be possible for the driver to send out frame=

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

sequence number before one with a lower number.

=20
The legacy drivers hopefully also implement proper sequence numbe=

Quoted 3 lines

Quoted 1 line

per TID. I'll send another mail on the topic.

=20
Heh, I seriously doubt that. As far as I know it is working using a=

Quoted 1 line

counter in the main interface structure. Just the same way it curre=

Quoted 1 line

works in mac80211 (which isn't surprising, since I based the mac802=

Quoted 1 line

on the legacy driver implementation).

=20
Ok so the thing is this: 802.11-2007 7.1.3.4.1 requires that we keep sequence counters per TID (and one for "not QoS"), which we only assi=

rather late in the wme code. Because even b43 hardware only keeps tra=

of a single sequence counter, it requires not using that counter for =

frames. =EF=BB=BFI suspect no driver except the overly smart iwlwifi =

right.
=20
Do you have flags to turn off hwseq support for the other ralink devices?

rt2500usb: Not per-frame, it can only be disabled globally
rt61pci: Both per-frame as well as globally
rt73usb: Both per-frame as well as globally
rt2800pci: Not per-frame, not sure about globally
rt2800usb: Not per-frame, not sure about globally

Note that I don't know if these devices handle the TID and the not QOS sequence counting in the hardware. I'll have to consult the specsheets for that.

Quoted 3 lines

That just widens the window. And if you have multiple queues then you can't do sw sequence numbers anyway because the hardware might reorder the frames.

That doesn't seem to be a problem in the legacy drivers, so I guess the hardware does *something* to prevent problems.

Quoted 5 lines

Quoted 3 lines

As far as rt2x00 is concerned, all hardware that supports multi-bss also support HW sequence counters. rt2400pci and rt2500pci are the only ones requiring SW sequence counters, and they can't do multi-bss.

Do they keep per-BSS counters?

Not sure what you mean, but rt2400pci and rt2500pci don't support multi-bss, so they wouldn't have per-BSS counters.
rt61pci and rt73usb do support multi-bss and keep the sequence counting completely in firmware/hardware and I am not sure how they can be read by the driver. So I don't know how those are stored either.

Quoted 12 lines · Johannes Berg

Quoted 11 lines · Michael Buesch

Quoted 10 lines · On Wednesday 09 July 2008 16:04:50 Ivo van Doorn wrote:

Quoted 8 lines · Michael Buesch

Quoted 3 lines · On Wednesday 09 July 2008 15:11:45 Ivo van Doorn wrote:

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Does software seq numbering even work at all?
What about packets that get sent between the driver requested the beacon and the driver does actually queue it?

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

For rt2x00 the beacon is requested and queued within interrupt context

Well, another CPU could be in progress of walking down the mac80211 TX code and aquire a sequence number in the meantime before you requested the beacon. However that frame will be blocked by your driver locks, so the two seq numbers of the beacon and the other frame will be swapped, as the driver will queue the beacon first.

If rt2x00 would use a global lock to block all TX and beacons, then yes. But rt2x00 uses per-queue locking. When a beacon is being updated rt2x00 will still allow regular frames to be queued.

But we can't halt the TX queues for the beacon update either, because ieee80211_stop_queues() should only be called from within the TX path.

Currently only beacons generated in AP mode have the software sequence number inserted. This means IBSS and Mesh mode are broken for all hardware that require software sequence numbers.

Signed-off-by Ivo van Doorn <[email protected]>