| Message ID | 1385224698-12294-1-git-send-email-karl.beldan@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sat, 2013-11-23 at 17:38 +0100, Karl Beldan wrote: > This adds a per vif bool csa_finished that is set after a call to > ieee80211_csa_finish() and used to skip beaconing while csa_active is > set in case a beacon is scheduled prior to csa_finalize_work completion. > This bool and the number of beacons transmitted during the CSA up to the > call to ieee80211_csa_finish() are reset in channel_switch_beacon op. Andrei says: Overall it seems ok, but all the purpose of csa_finished is not very clear.. It looks that this patch tries to avoid extra beaconing on the previous channel/hitting the warning.. But the problem is much bigger here, it means that we didn't switch in time (before the next beacon) so it's ok to hit the warning here and transmit extra beacon with count==1. So if we see a lot of such warnings, maybe we need to fix ieee80211_csa_finish instead (not using work for example) johannes -- 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, Dec 03, 2013 at 01:55:33PM +0100, Johannes Berg wrote: > On Sat, 2013-11-23 at 17:38 +0100, Karl Beldan wrote: > > > This adds a per vif bool csa_finished that is set after a call to > > ieee80211_csa_finish() and used to skip beaconing while csa_active is > > set in case a beacon is scheduled prior to csa_finalize_work completion. > > This bool and the number of beacons transmitted during the CSA up to the > > call to ieee80211_csa_finish() are reset in channel_switch_beacon op. > > Andrei says: > > Overall it seems ok, but all the purpose of csa_finished is not very > clear.. > It looks that this patch tries to avoid extra beaconing on the previous > channel/hitting the warning.. Yes, it avoids extra beacons. > But the problem is much bigger here, it means that we didn't switch in > time (before the next beacon) so it's ok to hit the warning here and > transmit extra beacon with count==1. Precisely what I discussed in the previous emails. However, I'd expect WARN*s to trigger on unexpected conditions .. though I haven't seen any warning, the codepath nearly seems to beg for it. > So if we see a lot of such warnings, maybe we need to fix > ieee80211_csa_finish instead (not using work for example) > The replies I got from the race I detailed felt like ppl wanted to prevent this in-driver, now we agree this race prone work is a flaw. Maybe I'll send a v5 for hwsim relying on future in-stack improvement to avoid extra beacon instead of doing this in-driver, with some appropriate comments. 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
On Thu, 2013-12-05 at 18:26 +0100, Karl Beldan wrote: > > Overall it seems ok, but all the purpose of csa_finished is not very > > clear.. > > It looks that this patch tries to avoid extra beaconing on the previous > > channel/hitting the warning.. > Yes, it avoids extra beacons. > > > But the problem is much bigger here, it means that we didn't switch in > > time (before the next beacon) so it's ok to hit the warning here and > > transmit extra beacon with count==1. > Precisely what I discussed in the previous emails. > However, I'd expect WARN*s to trigger on unexpected conditions .. though > I haven't seen any warning, the codepath nearly seems to beg for it. > > > So if we see a lot of such warnings, maybe we need to fix > > ieee80211_csa_finish instead (not using work for example) > The replies I got from the race I detailed felt like ppl wanted to > prevent this in-driver, now we agree this race prone work is a flaw. > Maybe I'll send a v5 for hwsim relying on future in-stack improvement to > avoid extra beacon instead of doing this in-driver, with some > appropriate comments. Generally, I think I'd prefer simpler code in the driver(s) over simpler code in the stack, since the former tends to get duplicated a lot. johannes -- 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
Karl Beldan <karl.beldan@gmail.com> writes: > From: Karl Beldan <karl.beldan@rivierawaves.com> > > This assigns the channel_switch_beacon op. > For hwsim, it comes down to calling ieee80211_csa_finish once > ieee80211_csa_is_complete is true. > Since channel_switch_beacon is not called if CSA count starts @ 0 or 1, > the check for ieee80211_csa_is_complete can be done after getting the > beacon (and this way it might trigger helpful warnings). > > This adds a per vif bool csa_finished that is set after a call to > ieee80211_csa_finish() and used to skip beaconing while csa_active is > set in case a beacon is scheduled prior to csa_finalize_work completion. > This bool and the number of beacons transmitted during the CSA up to the > call to ieee80211_csa_finish() are reset in channel_switch_beacon op. > > Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com> > Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de> What are we going to do with this patch? It would be convenient to get hwsim support CSA as it would help working with DFS issues. Was the issue blocking this patch the race when transmitting beacons? IMHO we can live with that and fix it properly later. It's not the only bug in DFS code right now ;) > @@ -1058,6 +1067,17 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac, > > mac80211_hwsim_tx_frame(hw, skb, > rcu_dereference(vif->chanctx_conf)->def.chan); > + > + if (vif->csa_active) { > + vp->csa_bcn_cnt++; > + if (ieee80211_csa_is_complete(vif)) { > + wiphy_debug(hw->wiphy, > + "%s CSA complete after %d beacons\n", > + __func__, vp->csa_bcn_cnt); > + ieee80211_csa_finish(vif); > + vp->csa_finished = 1; > + } > + } csa_finished is a bool so 'true' should be used here.
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index a11dc7c..0fb6bb4 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -169,6 +169,8 @@ struct hwsim_vif_priv { bool assoc; bool bcn_en; u16 aid; + int csa_bcn_cnt; + bool csa_finished; }; #define HWSIM_VIF_MAGIC 0x69537748 @@ -1024,6 +1026,7 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw, static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac, struct ieee80211_vif *vif) { + struct hwsim_vif_priv *vp = (void *)vif->drv_priv; struct mac80211_hwsim_data *data = arg; struct ieee80211_hw *hw = data->hw; struct ieee80211_tx_info *info; @@ -1038,6 +1041,12 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac, vif->type != NL80211_IFTYPE_ADHOC) return; + if (vif->csa_active && vp->csa_finished) { + wiphy_debug(hw->wiphy, "%s skip (CSA is active & finished)\n", + __func__); + return; + } + skb = ieee80211_beacon_get(hw, vif); if (skb == NULL) return; @@ -1058,6 +1067,17 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac, mac80211_hwsim_tx_frame(hw, skb, rcu_dereference(vif->chanctx_conf)->def.chan); + + if (vif->csa_active) { + vp->csa_bcn_cnt++; + if (ieee80211_csa_is_complete(vif)) { + wiphy_debug(hw->wiphy, + "%s CSA complete after %d beacons\n", + __func__, vp->csa_bcn_cnt); + ieee80211_csa_finish(vif); + vp->csa_finished = 1; + } + } } static enum hrtimer_restart @@ -1692,6 +1712,20 @@ static void mac80211_hwsim_unassign_vif_chanctx(struct ieee80211_hw *hw, hwsim_check_chanctx_magic(ctx); } +static void mac80211_hwsim_channel_switch_beacon(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + struct cfg80211_chan_def *chandef) +{ + struct hwsim_vif_priv *vp = (void *)vif->drv_priv; + + hwsim_check_magic(vif); + vp->csa_finished = 0; + vp->csa_bcn_cnt = 0; + wiphy_debug(hw->wiphy, "%s (freq=%d(%d - %d)/%s)\n", __func__, + chandef->chan->center_freq, chandef->center_freq1, + chandef->center_freq2, hwsim_chanwidths[chandef->width]); +} + static struct ieee80211_ops mac80211_hwsim_ops = { .tx = mac80211_hwsim_tx, @@ -1716,6 +1750,7 @@ static struct ieee80211_ops mac80211_hwsim_ops = .flush = mac80211_hwsim_flush, .get_tsf = mac80211_hwsim_get_tsf, .set_tsf = mac80211_hwsim_set_tsf, + .channel_switch_beacon = mac80211_hwsim_channel_switch_beacon, }; @@ -2359,7 +2394,9 @@ static int __init init_mac80211_hwsim(void) hw->wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS | WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL | - WIPHY_FLAG_AP_UAPSD; + WIPHY_FLAG_AP_UAPSD | + WIPHY_FLAG_HAS_CHANNEL_SWITCH; + hw->wiphy->features |= NL80211_FEATURE_ACTIVE_MONITOR; /* ask mac80211 to reserve space for magic */