| Message ID | 20131122140533.GA24796@magnum.frso.rivierawaves.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Nov 22, 2013 at 03:05:33PM +0100, Karl Beldan wrote: > 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 .. > Sorry, I was not very clear. What I meant is that, my change for hwsim is not correct because it assumed something like what's above that would prevent beacon_get() to return a beacon when csa is under completion. Instead, it should check for csa completion prior to getting the beacon. Checking your ath9k change for CSA, I am under the impression this is possible ? : {{{ ath9k_channel_switch_beacon() ath9k_beacon_tasklet() ath9k_csa_is_finished() // ret false .. ath9k_beacon_tasklet() ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true ieee80211_csa_finish() // schedules csa work ath9k_beacon_tasklet() ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL csa_finalize_work() // too late }}} Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Karl, > Sorry, I was not very clear. > What I meant is that, my change for hwsim is not correct because it > assumed something like what's above that would prevent beacon_get() to > return a beacon when csa is under completion. Instead, it should check > for csa completion prior to getting the beacon. > > Checking your ath9k change for CSA, I am under the impression this is > possible ? : {{{ > ath9k_channel_switch_beacon() > ath9k_beacon_tasklet() > ath9k_csa_is_finished() // ret false > .. > ath9k_beacon_tasklet() > ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true > ieee80211_csa_finish() // schedules csa work > > ath9k_beacon_tasklet() > ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL > > csa_finalize_work() // too late > }}} Yeah, you are right, if the worker is too late ath9k still generates a beacon and will hit a warning that the csa was already finished. Actually that helped quite a lot while developing the feature. In practice, I don't think it's that bad - the worker shouldn't take so long too respond, and also it does not hurt to generate/send the beacon one more time on the same channel with count = 0 (or 1). It's useful to get a warning in this case so that the root cause for the delay can be worked on. What do you think? I can change that easily in ath9k otherwise. :) Thanks, Simon -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 26, 2013 at 04:43:22PM +0100, Simon Wunderlich wrote: > Hey Karl, > > Sorry, I was not very clear. > > What I meant is that, my change for hwsim is not correct because it > > assumed something like what's above that would prevent beacon_get() to > > return a beacon when csa is under completion. Instead, it should check > > for csa completion prior to getting the beacon. > > > > Checking your ath9k change for CSA, I am under the impression this is > > possible ? : {{{ > > ath9k_channel_switch_beacon() > > ath9k_beacon_tasklet() > > ath9k_csa_is_finished() // ret false > > .. > > ath9k_beacon_tasklet() > > ath9k_csa_is_finished() // sets sc->csa_vif to NULL, ret true > > ieee80211_csa_finish() // schedules csa work > > > > ath9k_beacon_tasklet() > > ath9k_csa_is_finished() // ret false because sc->csa_vif==NULL > > > > csa_finalize_work() // too late > > }}} > > Yeah, you are right, if the worker is too late ath9k still generates a beacon > and will hit a warning that the csa was already finished. Actually that helped > quite a lot while developing the feature. > > In practice, I don't think it's that bad - the worker shouldn't take so long > too respond, and also it does not hurt to generate/send the beacon one more > time on the same channel with count = 0 (or 1). It's useful to get a warning > in this case so that the root cause for the delay can be worked on. > I think it's always nice to add a clean-room use of a new stack feature. I was more expecting a WARN_ON(!vif && vif->csa_active) in the driver than hitting a "forbidden" path in the stack. The v4 I sent for hwsim is immune to this race but I'd have expected the CSA code to handle this (e.g. with a vif->csa_finishing, or by letting ieee80211_beacon_get* return an ERR_PTR) since many drivers would? have to do this ATM. > What do you think? I can change that easily in ath9k otherwise. :) > I have no strong opinion on this, no pb with me keeping it as is, plus CSA being a recent addition, I doubt new ideas won't chime in. Karl -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 .. }}}