| Message ID | 1383921579-22373-1-git-send-email-luciano.coelho@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2013-11-08 at 16:39 +0200, Luciano Coelho wrote: > A beacon should never have a Channel Switch Announcement information > element with a count of 0, because a count of 1 means switch just > before the next beacon. So, if a count of 0 was valid in a beacon, it > would have been transmitted in the next channel already, which is > useless. A CSA count equal to zero is only meaningful in action > frames or probe_responses. > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > functions accordingly. These seem fine to me - maybe we should have some CSA tests in Jouni's hwsim test scripts, for the various cases? :) 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
> A beacon should never have a Channel Switch Announcement information > element with a count of 0, because a count of 1 means switch just > before the next beacon. So, if a count of 0 was valid in a beacon, it > would have been transmitted in the next channel already, which is > useless. A CSA count equal to zero is only meaningful in action > frames or probe_responses. > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > functions accordingly. Just to make future bisecting easier, shouldn't this patch go after "only set CSA beacon when at least one beacon must be transmitted" in the series? Otherwise userspace may change the channel with count=0 and hit a warning here, because this is not supported anymore. Apart from that, the series looks fine. I'd like to test the next v4 then. I guess we still need to do the action frame part - for IBSS mode we already have code but we probably need to make sure that these packets go out before actually changing the channel. Do you plan to work on that? Thanks for your work. :) 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 Mon, 2013-11-11 at 14:59 +0100, Johannes Berg wrote: > On Fri, 2013-11-08 at 16:39 +0200, Luciano Coelho wrote: > > A beacon should never have a Channel Switch Announcement information > > element with a count of 0, because a count of 1 means switch just > > before the next beacon. So, if a count of 0 was valid in a beacon, it > > would have been transmitted in the next channel already, which is > > useless. A CSA count equal to zero is only meaningful in action > > frames or probe_responses. > > > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > > functions accordingly. > > These seem fine to me - maybe we should have some CSA tests in Jouni's > hwsim test scripts, for the various cases? :) Cool that this is fine with you. I can proceed making the final changes for v4 then. ;) I'll take a look into the hwsim test scripts. -- Cheers, Luca.
On Mon, 2013-11-11 at 15:57 +0100, Simon Wunderlich wrote: > > A beacon should never have a Channel Switch Announcement information > > element with a count of 0, because a count of 1 means switch just > > before the next beacon. So, if a count of 0 was valid in a beacon, it > > would have been transmitted in the next channel already, which is > > useless. A CSA count equal to zero is only meaningful in action > > frames or probe_responses. > > > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > > functions accordingly. > > Just to make future bisecting easier, shouldn't this patch go after "only set > CSA beacon when at least one beacon must be transmitted" in the series? > Otherwise userspace may change the channel with count=0 and hit a warning > here, because this is not supported anymore. My plan was to squash them, because they really belong together. What do you think? > Apart from that, the series looks fine. I'd like to test the next v4 then. Great! I'll try to send v4 soon, hopefully the missing mesh part doesn't turn out to be too complicated. ;) > I guess we still need to do the action frame part - for IBSS mode we already > have code but we probably need to make sure that these packets go out before > actually changing the channel. Do you plan to work on that? Yes, I'll look into it. I think it should be pretty simple, especially now that we already have IBSS and MESH sending it out. > Thanks for your work. :) Thanks for your help! ;) -- Luca.
> > Just to make future bisecting easier, shouldn't this patch go after "only > > set CSA beacon when at least one beacon must be transmitted" in the > > series? Otherwise userspace may change the channel with count=0 and hit > > a warning here, because this is not supported anymore. > > My plan was to squash them, because they really belong together. What > do you think? > Good idea, it belongs together anyway. > > Apart from that, the series looks fine. I'd like to test the next v4 > > then. > > Great! I'll try to send v4 soon, hopefully the missing mesh part doesn't > turn out to be too complicated. ;) OK > > I guess we still need to do the action frame part - for IBSS mode we > > already have code but we probably need to make sure that these packets > > go out before actually changing the channel. Do you plan to work on > > that? > > Yes, I'll look into it. I think it should be pretty simple, especially > now that we already have IBSS and MESH sending it out. Great, 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
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 7ceed99..ec6ed6d 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -2680,10 +2680,10 @@ enum ieee80211_roc_type { * @channel_switch_beacon: Starts a channel switch to a new channel. * Beacons are modified to include CSA or ECSA IEs before calling this * function. The corresponding count fields in these IEs must be - * decremented, and when they reach zero the driver must call + * decremented, and when they reach 1 the driver must call * ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get() * get the csa counter decremented by mac80211, but must check if it is - * zero using ieee80211_csa_is_complete() after the beacon has been + * 1 using ieee80211_csa_is_complete() after the beacon has been * transmitted and then call ieee80211_csa_finish(). * * @join_ibss: Join an IBSS (on an IBSS interface); this is called after all @@ -3383,13 +3383,13 @@ static inline struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw, * @vif: &struct ieee80211_vif pointer from the add_interface callback. * * After a channel switch announcement was scheduled and the counter in this - * announcement hit zero, this function must be called by the driver to + * announcement hits 1, this function must be called by the driver to * notify mac80211 that the channel can be changed. */ void ieee80211_csa_finish(struct ieee80211_vif *vif); /** - * ieee80211_csa_is_complete - find out if counters reached zero + * ieee80211_csa_is_complete - find out if counters reached 1 * @vif: &struct ieee80211_vif pointer from the add_interface callback. * * This function returns whether the channel switch counters reached zero. diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index c558b24..57d9feb 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2409,8 +2409,12 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata, if (WARN_ON(counter_offset_beacon >= beacon_data_len)) return; - /* warn if the driver did not check for/react to csa completeness */ - if (WARN_ON(beacon_data[counter_offset_beacon] == 0)) + /* Warn if the driver did not check for/react to csa + * completeness. A beacon with CSA counter set to 0 should + * never occur, because a counter of 1 means switch just + * before the next beacon. + */ + if (WARN_ON(beacon_data[counter_offset_beacon] == 1)) return; beacon_data[counter_offset_beacon]--; @@ -2476,7 +2480,7 @@ bool ieee80211_csa_is_complete(struct ieee80211_vif *vif) if (WARN_ON(counter_beacon > beacon_data_len)) goto out; - if (beacon_data[counter_beacon] == 0) + if (beacon_data[counter_beacon] == 1) ret = true; out: rcu_read_unlock();
A beacon should never have a Channel Switch Announcement information element with a count of 0, because a count of 1 means switch just before the next beacon. So, if a count of 0 was valid in a beacon, it would have been transmitted in the next channel already, which is useless. A CSA count equal to zero is only meaningful in action frames or probe_responses. Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() functions accordingly. Signed-off-by: Luciano Coelho <luciano.coelho@intel.com> --- In v2, updated the documentation to reflect the change. This is going to be sent to linux-wireless for comments from the community too. In v3, removed the part of the documentation that belongs in the next patch. include/net/mac80211.h | 8 ++++---- net/mac80211/tx.c | 10 +++++++--- 2 files changed, 11 insertions(+), 7 deletions(-)