TDLS adds a STA entry before full information about the STA is
available. This leaves rate control algorithms in pretty odd state.
Avoid this by claiming the lowest rate to be supported if no supported
rates are indicated and then update the rate control again when the rate
set changes.
Signed-off-by: Jouni Malinen <[email protected]>
---
net/mac80211/cfg.c | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)
Note: Without this, some rate control algorithms can get quite unhappy
with the TDLS entries and maybe other use cases that could result in no
supported rates. For example, minstral has a pretty horrible loop in
init_sample_table() going to n_srates = n_rates(0) - 1...
Are all rate control algorithms fine with the second
rate_control_rate_init() call? That is needed in the TDLS use case where
the supported rate set is known only after the STA entry has been
added. I guess it would be possible to delay addition of the STA entry
for TDLS until the supported rates are known, but I did not look at the
details on what exactly that would require.
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a9ded52..b9dae9d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -682,7 +682,7 @@ static void ieee80211_send_layer2_update(struct sta_info *sta)
netif_rx_ni(skb);
}
-static void sta_apply_parameters(struct ieee80211_local *local,
+static bool sta_apply_parameters(struct ieee80211_local *local,
struct sta_info *sta,
struct station_parameters *params)
{
@@ -691,6 +691,7 @@ static void sta_apply_parameters(struct ieee80211_local *local,
struct ieee80211_supported_band *sband;
struct ieee80211_sub_if_data *sdata = sta->sdata;
u32 mask, set;
+ bool supp_rates_changed = false;
sband = local->hw.wiphy->bands[local->oper_channel->band];
@@ -774,6 +775,16 @@ static void sta_apply_parameters(struct ieee80211_local *local,
rates |= BIT(j);
}
}
+ if (rates == 0) {
+ /*
+ * Rate control algorithms may not like this.. Enable
+ * the lowest rate even if we do not know the exact
+ * supported rate set yet.
+ */
+ rates = BIT(0);
+ }
+ if (sta->sta.supp_rates[local->oper_channel->band] != rates)
+ supp_rates_changed = true;
sta->sta.supp_rates[local->oper_channel->band] = rates;
}
@@ -806,6 +817,8 @@ static void sta_apply_parameters(struct ieee80211_local *local,
}
#endif
}
+
+ return supp_rates_changed;
}
static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
@@ -929,7 +942,8 @@ static int ieee80211_change_station(struct wiphy *wiphy,
ieee80211_send_layer2_update(sta);
}
- sta_apply_parameters(local, sta, params);
+ if (sta_apply_parameters(local, sta, params))
+ rate_control_rate_init(sta);
rcu_read_unlock();
--
1.7.4.1
--
Jouni Malinen PGP id EFC895FA
On Sun, Oct 23, 2011 at 21:40, Jouni Malinen <[email protected]> wrote:
>
> Are all rate control algorithms fine with the second
> rate_control_rate_init() call? That is needed in the TDLS use case where
> the supported rate set is known only after the STA entry has been
Note that the sta-rates are not really required before second addition
(which also sets them). Initially the STA is not authorized for direct
Tx (as determined by the WLAN_STA_TDLS_PEER_AUTH bit).
We don't expect direct frames during link setup.
> added. I guess it would be possible to delay addition of the STA entry
> for TDLS until the supported rates are known, but I did not look at the
> details on what exactly that would require.
The STA is added to let us know we should drop all non-setup packets
between TDLS peers currently in link setup. We could have used a
different indication, but the STA entry is make the most sense here
since it is automatically cleaned up when we are disconnected from the
AP (as all TDLS state should be).
How about this one instead (tested hwsim with it):
>From d8b7acc16073e50f4fda3365d98ad01b21e2c631 Mon Sep 17 00:00:00 2001
From: Arik Nemtsov <[email protected]>
Date: Mon, 24 Oct 2011 13:44:07 +0200
Subject: [RFC] mac80211: init rate-control for TDLS sta when supp-rates are
known
Initialize rate control algorithms only when supported rates are known
for a TDLS peer sta. Direct Tx between peers is not allowed before the
link is enabled. In turn, this only occurs after a change_station()
call that sets supported rates.
---
net/mac80211/cfg.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1309bb9..9f05416d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -829,7 +829,12 @@ static int ieee80211_add_station(struct wiphy
*wiphy, struct net_device *dev,
sdata->vif.type == NL80211_IFTYPE_STATION))
return -ENOTSUPP;
- rate_control_rate_init(sta);
+ /*
+ * for TDLS, rate control should be initialized only when supported
+ * rates are known.
+ */
+ if (!test_sta_flag(sta, WLAN_STA_TDLS_PEER))
+ rate_control_rate_init(sta);
layer2_update = sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
sdata->vif.type == NL80211_IFTYPE_AP;
@@ -913,6 +918,9 @@ static int ieee80211_change_station(struct wiphy *wiphy,
sta_apply_parameters(local, sta, params);
+ if (test_sta_flag(sta, WLAN_STA_TDLS_PEER) && params->supported_rates)
+ rate_control_rate_init(sta);
+
rcu_read_unlock();
if (sdata->vif.type == NL80211_IFTYPE_STATION &&
--
1.7.5.4
On Mon, Oct 24, 2011 at 14:15, Jouni Malinen <[email protected]> wrote:
> On Mon, Oct 24, 2011 at 01:59:45PM +0200, Arik Nemtsov wrote:
>> Note that the sta-rates are not really required before second addition
>> (which also sets them). Initially the STA is not authorized for direct
>> Tx (as determined by the WLAN_STA_TDLS_PEER_AUTH bit).
>> We don't expect direct frames during link setup.
>
> OK.. Though, there can still be some direct frames like TDLS Discovery
> Response.
You have a point there - this one is not related to link setup. At
this point we really have no idea about peer supported rates (and no
STA entry).
I just tested this with hwsim and it seems to work. The rate chosen is
the the smallest non-basic rate (haven't looked at why that happens).
With cards that use HW rate control (like wl12xx), this will of course
be implementation defined.
>
>> The STA is added to let us know we should drop all non-setup packets
>> between TDLS peers currently in link setup. We could have used a
>> different indication, but the STA entry is make the most sense here
>> since it is automatically cleaned up when we are disconnected from the
>> AP (as all TDLS state should be).
>
> When you say "drop", I hope you mean "buffer for later delivery" and
> when you say "non-setup packets", I hope you mean "non-setup Data
> frames" ;-).
No I mean "drop". Initially a buffer was planned, but in reality the
setup process is extremely fast. I setup iperf between two regular
stations (about 15mb/s) and proceeded to setup TDLS. I managed to lose
only about 3-4 frames each time.
Of course there may be anomalies (bad link quality etc.), but IMHO for
real-world situations the extra complexity of a buffering mechanism is
not warranted. Also higher level protocols (like TCP) will generally
make up for any loss.
I'll change the second one to "non-setup Data frames" :)
>
>> How about this one instead (tested hwsim with it):
>> Subject: [RFC] mac80211: init rate-control for TDLS sta when supp-rates are
>> ?known
>>
>> Initialize rate control algorithms only when supported rates are known
>> for a TDLS peer sta. Direct Tx between peers is not allowed before the
>> link is enabled. In turn, this only occurs after a change_station()
>> call that sets supported rates.
>
> I guess this could be fine, too, assuming TDLS is the only use case for
> this type of changing of STA supported rates.
Currently it is - we even check this explicitly higher up in
nl80211_set_station().
Arik
On Mon, Oct 24, 2011 at 01:59:45PM +0200, Arik Nemtsov wrote:
> Note that the sta-rates are not really required before second addition
> (which also sets them). Initially the STA is not authorized for direct
> Tx (as determined by the WLAN_STA_TDLS_PEER_AUTH bit).
> We don't expect direct frames during link setup.
OK.. Though, there can still be some direct frames like TDLS Discovery
Response.
> The STA is added to let us know we should drop all non-setup packets
> between TDLS peers currently in link setup. We could have used a
> different indication, but the STA entry is make the most sense here
> since it is automatically cleaned up when we are disconnected from the
> AP (as all TDLS state should be).
When you say "drop", I hope you mean "buffer for later delivery" and
when you say "non-setup packets", I hope you mean "non-setup Data
frames" ;-).
> How about this one instead (tested hwsim with it):
> Subject: [RFC] mac80211: init rate-control for TDLS sta when supp-rates are
> known
>
> Initialize rate control algorithms only when supported rates are known
> for a TDLS peer sta. Direct Tx between peers is not allowed before the
> link is enabled. In turn, this only occurs after a change_station()
> call that sets supported rates.
I guess this could be fine, too, assuming TDLS is the only use case for
this type of changing of STA supported rates.
--
Jouni Malinen PGP id EFC895FA