rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Hi,

I've just reviewed the beacon handling in rt2x00 in AP mode and experienced
some inconsistencies. The DTIM count is not correctly updated: sometimes
multiple beacons are sent out using the same DTIM count.

rt2x00 calls ieee80211_beacon_get_tim right after the current beacon was
sent out to fetch the next one. However, rt2x00 also implements the set_tim
callback and updates the beacon in each call to set_tim. As far as I
understood the code in mac80211 the set_tim callback is called when the
first frame for a powersaving station gets queued.

Since every call to ieee80211_beacon_get_tim updates the DTIM count the
following can happen (assuming a DTIM period of 2):

- the hw sends out the current beacon (DTIM count == 0)
- call to ieee80211_beacon_get_tim fetches the next beacon (DTIM count == 1)
- the first frame for a PS STA gets queued -> set_tim
- again call ieee80211_beacon_get_tim (DTIM count == 0)
- hw sends out the beacon with incorrect DTIM count

A proper way of fixing this issue would be not to use the set_tim callback but
just fetch the next beacon right before it gets send out (like ath* does).
However, that's not easily possible with rt2x00 devices older then rt2800 as
they only generate beacon_done interrupts (which is obviously too late for
fetching the current beacon ;) ).

So, is the current implementation in rt2x00 supposed to work and mac80211
needs fixing?

Could we add a parameter to ieee80211_beacon_get_tim that indicates if a _new_
beacon should be generated or if the _current_ beacon should be updated in
response to the set_tim callback?

Any other ideas?

Thanks,
Helmut

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[John W. Linville]

On Thu, Jun 24, 2010 at 03:21:47PM +0200, Helmut Schaa wrote:

> So, is the current implementation in rt2x00 supposed to work and mac80211
> needs fixing?
> 
> Could we add a parameter to ieee80211_beacon_get_tim that indicates if a _new_
> beacon should be generated or if the _current_ beacon should be updated in
> response to the set_tim callback?
> 
> Any other ideas?

Since we seem to want to allow ieee80211_beacon_get_tim to be called
from the set_tim callbacks, maybe we could manage bss->dtim_count
somewhere other than ieee80211_beacon_add_tim?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Thu, 2010-06-24 at 15:21 +0200, Helmut Schaa wrote:

> I've just reviewed the beacon handling in rt2x00 in AP mode and experienced
> some inconsistencies. The DTIM count is not correctly updated: sometimes
> multiple beacons are sent out using the same DTIM count.

Er, so there's your problem already.

> rt2x00 calls ieee80211_beacon_get_tim right after the current beacon was
> sent out to fetch the next one. However, rt2x00 also implements the set_tim
> callback and updates the beacon in each call to set_tim. As far as I
> understood the code in mac80211 the set_tim callback is called when the
> first frame for a powersaving station gets queued.
> 
> Since every call to ieee80211_beacon_get_tim updates the DTIM count the
> following can happen (assuming a DTIM period of 2):
> 
> - the hw sends out the current beacon (DTIM count == 0)
> - call to ieee80211_beacon_get_tim fetches the next beacon (DTIM count == 1)
> - the first frame for a PS STA gets queued -> set_tim
> - again call ieee80211_beacon_get_tim (DTIM count == 0)
> - hw sends out the beacon with incorrect DTIM count
> 
> A proper way of fixing this issue would be not to use the set_tim callback but
> just fetch the next beacon right before it gets send out (like ath* does).
> However, that's not easily possible with rt2x00 devices older then rt2800 as
> they only generate beacon_done interrupts (which is obviously too late for
> fetching the current beacon ;) ).
> 
> So, is the current implementation in rt2x00 supposed to work and mac80211
> needs fixing?
> 
> Could we add a parameter to ieee80211_beacon_get_tim that indicates if a _new_
> beacon should be generated or if the _current_ beacon should be updated in
> response to the set_tim callback?
> 
> Any other ideas?

No, your entire description makes no sense. There are two possibilities:

 a) You get a new beacon from mac80211 each time you send it. Then you
    don't have to worry about the set_tim() callback at all -- don't
    assign it!

 b) You need to get a new beacon frame from mac80211 only when it
    changes. You can do this from set_tim(). HOWEVER: since you're not
    getting a new one from mac80211 all the time anyway, you NEED to
    have the driver or firmware overwrite the DTIM count, like b43's
    firmware for example will do.

Ok so maybe there are more possibilities like the firmware filling the
TIM IE differently and you would use set_tim() differently then.

However, *fundamentally*, any time you don't get a new skb from mac80211
for each transmitted beacon you NEED to overwrite the DTIM count in it.

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Thu, 2010-06-24 at 17:51 +0200, Johannes Berg wrote:

>  b) You need to get a new beacon frame from mac80211 only when it
>     changes. You can do this from set_tim(). HOWEVER: since you're not
>     getting a new one from mac80211 all the time anyway, you NEED to
>     have the driver or firmware overwrite the DTIM count, like b43's
>     firmware for example will do.

Oh and "when it changes" doesn't include DTIM period changes, obviously.

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Thu, 2010-06-24 at 17:53 +0200, Johannes Berg wrote:
> On Thu, 2010-06-24 at 17:51 +0200, Johannes Berg wrote:
> 
> >  b) You need to get a new beacon frame from mac80211 only when it
> >     changes. You can do this from set_tim(). HOWEVER: since you're not
> >     getting a new one from mac80211 all the time anyway, you NEED to
> >     have the driver or firmware overwrite the DTIM count, like b43's
> >     firmware for example will do.
> 
> Oh and "when it changes" doesn't include DTIM period changes, obviously.

Er, it does include "DTIM period" changes (shouldn't actually happen
anyway), I meant "DTIM count field changes".

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Am Donnerstag 24 Juni 2010 schrieb Johannes Berg:
>  a) You get a new beacon from mac80211 each time you send it. Then you
>     don't have to worry about the set_tim() callback at all -- don't
>     assign it!
> 
>  b) You need to get a new beacon frame from mac80211 only when it
>     changes. You can do this from set_tim(). HOWEVER: since you're not
>     getting a new one from mac80211 all the time anyway, you NEED to
>     have the driver or firmware overwrite the DTIM count, like b43's
>     firmware for example will do.
> 
> Ok so maybe there are more possibilities like the firmware filling the
> TIM IE differently and you would use set_tim() differently then.
> 
> However, *fundamentally*, any time you don't get a new skb from mac80211
> for each transmitted beacon you NEED to overwrite the DTIM count in it.

So, to answer my own question:

mac80211 behaves as designed and we need to fix rt2x00 to correctly update
the DTIM parameters when we stick to the set_tim callback for updating the
beacon template (option b).

Or we would implement option a) to get a new beacon prior to transmission
and would get a correct DTIM count from mac80211.

Thanks for the clarification,
Helmut

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Am Donnerstag 24 Juni 2010 schrieb Johannes Berg:
>  b) You need to get a new beacon frame from mac80211 only when it
>     changes. You can do this from set_tim(). HOWEVER: since you're not
>     getting a new one from mac80211 all the time anyway, you NEED to
>     have the driver or firmware overwrite the DTIM count, like b43's
>     firmware for example will do.

And the broad- and multicast buffering also needs to be done in the driver
(when the fw/hw cannot handle it) as mac80211 uses its own DTIM count for
deciding when to "release" buffered frames to the driver.

Helmut

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[John W. Linville]

On Fri, Jun 25, 2010 at 06:01:06PM +0200, Helmut Schaa wrote:
> Am Donnerstag 24 Juni 2010 schrieb Johannes Berg:
> >  b) You need to get a new beacon frame from mac80211 only when it
> >     changes. You can do this from set_tim(). HOWEVER: since you're not
> >     getting a new one from mac80211 all the time anyway, you NEED to
> >     have the driver or firmware overwrite the DTIM count, like b43's
> >     firmware for example will do.
> 
> And the broad- and multicast buffering also needs to be done in the driver
> (when the fw/hw cannot handle it) as mac80211 uses its own DTIM count for
> deciding when to "release" buffered frames to the driver.

That seems a bit...yucky...

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Fri, 2010-06-25 at 18:01 +0200, Helmut Schaa wrote:
> Am Donnerstag 24 Juni 2010 schrieb Johannes Berg:
> >  b) You need to get a new beacon frame from mac80211 only when it
> >     changes. You can do this from set_tim(). HOWEVER: since you're not
> >     getting a new one from mac80211 all the time anyway, you NEED to
> >     have the driver or firmware overwrite the DTIM count, like b43's
> >     firmware for example will do.
> 
> And the broad- and multicast buffering also needs to be done in the driver
> (when the fw/hw cannot handle it) as mac80211 uses its own DTIM count for
> deciding when to "release" buffered frames to the driver.

Well, yes, but I don't get it. Are you trying to conjure a third way of
doing things? How would you like it to work?

I don't understand why you expect to have a correct DTIM count field
when you don't pull every beacon from mac80211. If you pull each beacon
it's all correct. If you don't, you need to take care of everything DTIM
related, there's simply no way to do it differently.

Does your device fill the DTIM count field or does it not? That's really
the only thing you need to know, no?

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Am Sonntag 27 Juni 2010 schrieb Johannes Berg:
> On Fri, 2010-06-25 at 18:01 +0200, Helmut Schaa wrote:
> > Am Donnerstag 24 Juni 2010 schrieb Johannes Berg:
> > >  b) You need to get a new beacon frame from mac80211 only when it
> > >     changes. You can do this from set_tim(). HOWEVER: since you're not
> > >     getting a new one from mac80211 all the time anyway, you NEED to
> > >     have the driver or firmware overwrite the DTIM count, like b43's
> > >     firmware for example will do.
> > 
> > And the broad- and multicast buffering also needs to be done in the driver
> > (when the fw/hw cannot handle it) as mac80211 uses its own DTIM count for
> > deciding when to "release" buffered frames to the driver.
> 
> Well, yes, but I don't get it. Are you trying to conjure a third way of
> doing things? How would you like it to work?
> 
> I don't understand why you expect to have a correct DTIM count field
> when you don't pull every beacon from mac80211.

rt2x00 pulls every beacon. But not directly _prior_ to transmission as the 
hw lacks an interrupt for that. Instead the next beacon gets pulled _after_
the beacondone interrupt (which is obviously triggered directly after the
beacon was sent). So, all TIM changes that happen during the next beacon
interval won't be included in the next beacon. Hence, rt2x00 also implements
the set_tim callback and updates the beacon through these as well.

This gives us a correct TIM but as I explained earlier breaks the DTIM
count (and thus bc/mc buffering which is done in mac80211 fot rt2x00).

One possible option to fix this in rt2x00 would be to delay the beacon
update (as it is already put on a workqueue we could simply replace it by
a delayed work) by beaconinterval - 10ms or something. But I'm not how
accurate that would be (and of course remove the set_tim callback).

Ivo, would that be an option you would agree on? On rt2800 we would simply
use the pretbtt interrupt and on older hardware trigger the beacon update
from a delayed work, and remove the set_tim callback?

> If you pull each beacon
> it's all correct. If you don't, you need to take care of everything DTIM
> related, there's simply no way to do it differently.

Yes, that's what I tried to understand when I wrote the first mail as I didn't
know enough about that part in mac80211.

> Does your device fill the DTIM count field or does it not? That's really
> the only thing you need to know, no?

The rt2x00 devices don't do any semantic changes to the beacon (only update
the timestamp). So basically the approach to update the beacon once during
each beacon interval would be the nicest solution. But due to the lack of a
suitable interrupt (on devices older then rt2800) rt2x00 does it as explained
above.

Helmut

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Sun, 2010-06-27 at 21:00 +0200, Helmut Schaa wrote:

> rt2x00 pulls every beacon. But not directly _prior_ to transmission as the 
> hw lacks an interrupt for that. Instead the next beacon gets pulled _after_
> the beacondone interrupt (which is obviously triggered directly after the
> beacon was sent). So, all TIM changes that happen during the next beacon
> interval won't be included in the next beacon. Hence, rt2x00 also implements
> the set_tim callback and updates the beacon through these as well.
> 
> This gives us a correct TIM but as I explained earlier breaks the DTIM
> count (and thus bc/mc buffering which is done in mac80211 fot rt2x00).
> 
> One possible option to fix this in rt2x00 would be to delay the beacon
> update (as it is already put on a workqueue we could simply replace it by
> a delayed work) by beaconinterval - 10ms or something. But I'm not how
> accurate that would be (and of course remove the set_tim callback).

Well, after a beacon is before a beacon. I think iwlwifi also pulls the
next beacon after the previous one was sent. That just means you get a
potential higher delay, but otherwise it doesn't really matter that
much. You'll never be able to close the race fully anyway, unless your
device itself is capable of generating the TIM IE _right before_ the
beacon gets transmitted.

Therefore, with standard beacon intervals of 100 TU, I don't think it
matters all that much whether it's before or after?

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Am Freitag 02 Juli 2010 schrieb Johannes Berg:
> On Sun, 2010-06-27 at 21:00 +0200, Helmut Schaa wrote:
> 
> > rt2x00 pulls every beacon. But not directly _prior_ to transmission as the 
> > hw lacks an interrupt for that. Instead the next beacon gets pulled _after_
> > the beacondone interrupt (which is obviously triggered directly after the
> > beacon was sent). So, all TIM changes that happen during the next beacon
> > interval won't be included in the next beacon. Hence, rt2x00 also implements
> > the set_tim callback and updates the beacon through these as well.
> > 
> > This gives us a correct TIM but as I explained earlier breaks the DTIM
> > count (and thus bc/mc buffering which is done in mac80211 fot rt2x00).
> > 
> > One possible option to fix this in rt2x00 would be to delay the beacon
> > update (as it is already put on a workqueue we could simply replace it by
> > a delayed work) by beaconinterval - 10ms or something. But I'm not how
> > accurate that would be (and of course remove the set_tim callback).
> 
> Well, after a beacon is before a beacon.

Yes ;)

> I think iwlwifi also pulls the
> next beacon after the previous one was sent. That just means you get a
> potential higher delay, 

Agreed.

> but otherwise it doesn't really matter that
> much. You'll never be able to close the race fully anyway,

No, but I'd like to get as close as possible. The idea with the delayed
work was intended to reduce the latency as good as we can.

> unless your
> device itself is capable of generating the TIM IE _right before_ the
> beacon gets transmitted.

Agreed. At least on rt2800 we can use the pre tbtt interrupt to update the
beacon just before it is sent out.

> Therefore, with standard beacon intervals of 100 TU, I don't think it
> matters all that much whether it's before or after?

Yeah, maybe that's just fine. I guess I'll just drop the set_tim callback
from all rt2x00 pci variants and implement the pre tbtt interrupt on rt2800.

Thanks,
Helmut

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg]

On Fri, 2010-07-02 at 19:59 +0200, Helmut Schaa wrote:

> > unless your
> > device itself is capable of generating the TIM IE _right before_ the
> > beacon gets transmitted.
> 
> Agreed. At least on rt2800 we can use the pre tbtt interrupt to update the
> beacon just before it is sent out.

Careful though. You don't know how far in advance it is triggered (or
maybe you do) or if the time will be sufficient to generate and upload
the beacon to the device. You could even hit lock contention I think.

johannes

Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Helmut Schaa]

Am Freitag 02 Juli 2010 schrieb Johannes Berg:
> On Fri, 2010-07-02 at 19:59 +0200, Helmut Schaa wrote:
> 
> > > unless your
> > > device itself is capable of generating the TIM IE _right before_ the
> > > beacon gets transmitted.
> > 
> > Agreed. At least on rt2800 we can use the pre tbtt interrupt to update the
> > beacon just before it is sent out.
> 
> Careful though. You don't know how far in advance it is triggered (or
> maybe you do) or if the time will be sufficient to generate and upload
> the beacon to the device. You could even hit lock contention I think.

Yeah, the delay is configurable. And yes, we cannot be 100% sure but
I already did some tests and on my MIPS board anything >1ms was
enough to generate + upload a new beacon to the device. And we can still
increase that to a safe default of maybe 5ms or so.

Helmut