Re: rt2x00 & mac80211: correct usage of ieee80211_beacon_get_tim?

[Johannes Berg <johannes@sipsolutions.net>]

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