On Fri, Nov 22, 2013 at 01:08:11PM +0100, Simon Wunderlich wrote:
> Hello Karl,
>
> >
> > Hmm, I thought the CSA code would make ieee80211_beacon_get_tim return
> > NULL (or do something alike) after the last ieee80211_beacon_get_tim
> > returned a beacon with a null CSA count until the config is done - but
> > it seems it doesn't - in that case this change would be race prone.
> > Did I miss something ?
>
> No, you have to do the check yourself (as you appearently did). In ath9k I'm
> checking if the CSA finished before scheduling the next beacon. Can you do the
> same for hwsim? Where do you see the race?
>
Hi Simon,
I would see a race in such scenario: {{{
cmd: stack/channel_switch(count=1);
bcn-intr: driver/beacon_get();
vif->csa_active == true
ieee80211_csa_is_complete(); // == true
ieee80211_csa_finish(); // schedules csa work
bcn-intr: driver/beacon_get(); // beacon but on the old channel
vif->csa_active still true
ieee80211_csa_is_complete(); // still true
csa worker: csa_finalize_work(); ----> too late
change_channel(); ----> too late
}}}
To prevent this, I thought that there was something like : {{{
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3e2dfcb..97b0382 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2405,7 +2405,7 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
}
EXPORT_SYMBOL(ieee80211_csa_finish);
-static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
+static bool ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
struct beacon_data *beacon)
{
struct probe_resp *resp;
@@ -2428,14 +2428,14 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
beacon_data_len = beacon->head_len;
break;
default:
- return;
+ return false;
}
if (WARN_ON(counter_offset_beacon >= beacon_data_len))
- return;
+ return false;
/* warn if the driver did not check for/react to csa completeness */
if (WARN_ON(beacon_data[counter_offset_beacon] == 0))
- return;
+ return false;
beacon_data[counter_offset_beacon]--;
@@ -2446,11 +2446,13 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
/* if nl80211 accepted the offset, this should not happen. */
if (WARN_ON(!resp)) {
rcu_read_unlock();
- return;
+ return true;
}
resp->data[counter_offset_presp]--;
rcu_read_unlock();
}
+
+ return true;
}
bool ieee80211_csa_is_complete(struct ieee80211_vif *vif)
@@ -2540,7 +2542,8 @@ struct sk_buff *ieee80211_beacon_get_tim(struct ieee80211_hw *hw,
if (beacon) {
if (sdata->vif.csa_active)
- ieee80211_update_csa(sdata, beacon);
+ if (!ieee80211_update_csa(sdata, beacon))
+ goto out;
... blah blah for mesh .. ibss ..
}}}
.. Of course there are other means to achieve this, just an example.
However I might have overlooked some things in the CSA code so ..
> Simon
>
> P.S.: Please use my new e-mail address, the old one will go out of service by
> end of the year.
Sure
Karl
--
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
