Search Linux Wireless

Re: [PATCH] mac80211: enable IBSS merging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wednesday 23 January 2008 23:48:49 Johannes Berg wrote:
> > enable IBSS cell merging. if an IBSS beacon with the same ESSID and a TSF
> > higher than the local TSF (mactime) is received, we have to join its
> > BSSID.
>
> Can you back that up with standard references? I see no such requirement
> in the standard text. I can see that under some circumstances this may
> be desirable, but maybe not.

sure. the relevant sections of the standard are 11.1.2.3 "Beacon reception":

"STAs in an IBSS shall use other information in any received Beacon frame for 
which the IBSS subfield of the Capability field is set to 1 and the content 
of the SSID element is equal to the SSID of the IBSS. Use of this information 
is specified in 11.1.4."

and in 11.1.4 "Adjusting STA timers":
(emphasis and brackets mine):

"In an IBSS, a STA shall always adopt the information in the contents of a 
Beacon or Probe Response frame when that frame contains a matching SSID and 
the value of the time stamp is later than the STA’s TSF timer. [In response 
to an MLME-Join.request, a STA shall initialize its TSF timer to 0 and shall 
not transmit a beacon or probe response until it hears a beacon or probe 
response from a member of the IBSS with a matching SSID.]"

"All Beacon and Probe Response frames carry a Timestamp field. A STA receiving 
such a frame from another STA in an IBSS with the same SSID shall compare the 
Timestamp field with its own TSF time. If the Timestamp field of the received 
frame is later than its own TSF time, the STA shall adopt *all* parameters 
contained in the Beacon frame."

i can see now how you could interpret this as only applying to timers, but i 
think that does just not make sense - why would you want to sync timers and 
adapt beacon intervals if you don't share the same BSSID?

i agree that the standard is quite ambigious when it comes to IBSS, but 
practically speaking, it is necessary to merge IBSS cells most of the time if 
you want to have IBSS working as expected. therefore i tend to interpret the 
ambigious parts of the standard in a way that will improve functionality.

just think about two IBSS nodes which are configured with the same ESSID and 
which start up at nearly the same time. they would both scan, not find an 
existing BSS and start their own. current mac80211 implementation would after 
a while detect that they are alone on their BSS and start another scan, which 
would finally cause one of them to reconfigure its BSSID, but this is not 
sufficient: if you happen to have 2 nodes in each BSSID a merge would never 
happen.
there are similar situations like this in real life, the most obvious example 
beeing a city wide IBSS where you can easily have two separate groups of 
nodes starting up with a different BSSID. suddenly an obstacle is removed or 
a new node comes up in the middle of these two groups (with connections to 
both) - which BSSID is it supposed to join? the one with the higher TSF, but 
that would leave us with 2 distinct BSSIDs. would all involved nodes restart, 
they would all share the same BSSID now. i think the only sensible answer to 
this problem is that they all should join the BSSID with the higher TSF. 

this is also how all drivers and (also firmware based) cards i know handle 
IBSS - they will merge in a similar fashion based on the ESSID. (i know... 
that might not be a good argument, since most drivers are broken in one way 
or the other, especially when it comes to IBSS mode.)

> In fact, this seems to *break* standard behaviour, as per 11.1.3.1:
> (emphasis mine)
>
>         When a STA starts a BSS, that STA shall determine the BSSID of
>         the BSS. If the BSSType indicates an infrastructure BSS, then
>         the STA shall start an infrastructure BSS and the BSSID shall be
>         equal to the STA’s dot11StationID. ***The value of the BSSID
>         shall remain unchanged***, even if the value of dot11StationID
>         is changed after the completion of the MLME-START.request.

i understand this to apply to infrastructure STAs only.

>         If 
>         the BSSType indicates an IBSS, the STA shall start an IBSS, and
>         the BSSID shall be an individual locally administered IEEE MAC
>         address as defined in 5.2 of IEEE Std 802-1990. The remaining 46
>         bits of that MAC address shall be a number selected in a manner
>         that minimizes the probability of STAs generating the same
>         number, even when those STAs are subjected to the same initial
>         conditions.
>
> Comments on the code now.
>
> > --- a/net/mac80211/ieee80211_sta.c
> > +++ b/net/mac80211/ieee80211_sta.c
> > @@ -80,6 +80,9 @@ static void ieee80211_rx_bss_put(struct net_device
> > *dev, struct ieee80211_sta_bss *bss); static int
> > ieee80211_sta_find_ibss(struct net_device *dev,
> >                                    struct ieee80211_if_sta *ifsta);
> > +static int ieee80211_sta_join_ibss(struct net_device *dev,
> > +                                  struct ieee80211_if_sta *ifsta,
> > +                                  struct ieee80211_sta_bss *bss);
>
> No way, order the code properly, this mess needs to be cleaned up not
> added to.

ok. i just didn't want to make my patch too unclear by moving stuff around in 
addition to adding functionality. i'll make 2 separate patches, once we 
agreed that this whole merge behaviour is wanted in mac80211 at all.

> > -	if (bss->probe_resp && beacon) {
> > +	if (sdata->vif.type != IEEE80211_IF_TYPE_IBSS &&
> > +	    bss->probe_resp && beacon) {
> >  		/* Do not allow beacon to override data from Probe Response. */
>
> Ahem. Comment update required.

ok.

> > +	if (sdata->vif.type == IEEE80211_IF_TYPE_IBSS && beacon &&
> > +	    !local->sta_sw_scanning && !local->sta_hw_scanning &&
> > +	    mgmt->u.beacon.capab_info & WLAN_CAPABILITY_IBSS &&
> > +	    memcmp(elems.ssid, sdata->u.sta.ssid, sdata->u.sta.ssid_len) == 0)
> > {
>
> I think that needs a check "&& ssid_len" or something. Someone's bound
> to try making an IBSS with a hidden SSID and we really don't want that
> to work.

yep.

> > +#ifdef CONFIG_MAC80211_IBSS_DEBUG
> > +		static unsigned long last_tsf_debug;
> > +#endif
>
> Let's just get rid of that, it's not usable with multiple devices.

i'm happy to do that, i just wanted to preserve as much as possible of the 
original code since i wasn't sure if anybody needed it.

> > +		if (rx_status->flag & RX_FLAG_TSFT)
> > +			mactime = rx_status->mactime;
> > +		else {
> > +			mactime = -1LLU;
> > +			printk(KERN_WARNING "%s: IBSS mode needs mactime for "
> > +				"beacons\n", dev->name);
>
> Very bad, you'll be flooded by this when others send beacons. Also, I
> doubt its truthfulness.

please see my last post where i suggest to use:

+               if (rx_status->flag & RX_FLAG_TSFT)
+                       /* in order for correct IBSS merging we need mactime*/
+                       mactime = rx_status->mactime;
+               else if (local && local->ops && local->ops->get_tsf)
+                       /* second best option: get current TSF */
+                       mactime = local->ops->get_tsf(local_to_hw(local));
+               else
+                       /* can't merge without knowing the TSF */
+                       mactime = -1LLU;

instead.

> > +			printk(KERN_DEBUG "%s: beacon TSF higher than local TSF"
> > +				" -> IBSS merge with BSSID %s\n",
> > +				dev->name, print_mac(mac, mgmt->bssid));
>
> No way. At the very least you need to ratelimit this.

ok, i'll add a ratelimit.

bruno
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux