[RFC PATCH 0/3] mac80211 dissasociation

[RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

This series should fix mac problems in disassociation
There are two basic problems disassociation in current implementation.
The order of steps in the dissociation flow and missing proper
disassociation when moving from one AP to another. 
 
One more missing piece not addressed by these patches is how
to notify mac80211 about rfkill. It can be done via mac_notify callback
or by rf kill notification chain. I didn't explore the later yet.
 
I'm releasing this as RFC because I didn't tested it after rebasing
to current wireless-testing. (Will do soon)
 
Tomas


---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Tomas Winkler]

This patch restructure the flow of disassociation and deauthentication
flows, and adds the removal of the obsolete sta form station table

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/mac80211/mlme.c |   83 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index c396c35..ba255e2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -561,8 +561,6 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
 		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
 		ieee80211_sta_send_associnfo(sdata, ifsta);
 	} else {
-		netif_carrier_off(sdata->dev);
-		ieee80211_sta_tear_down_BA_sessions(sdata, ifsta->bssid);
 		ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
 		changed |= ieee80211_reset_erp_info(sdata);
 
@@ -585,18 +583,6 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
 	wireless_send_event(sdata->dev, SIOCGIWAP, &wrqu, NULL);
 }
 
-static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
-				   struct ieee80211_if_sta *ifsta, int deauth)
-{
-	if (deauth) {
-		ifsta->direct_probe_tries = 0;
-		ifsta->auth_tries = 0;
-	}
-	ifsta->assoc_scan_tries = 0;
-	ifsta->assoc_tries = 0;
-	ieee80211_set_associated(sdata, ifsta, 0);
-}
-
 void ieee80211_sta_tx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
 		      int encrypt)
 {
@@ -990,6 +976,49 @@ static void ieee80211_send_disassoc(struct ieee80211_sub_if_data *sdata,
 	ieee80211_sta_tx(sdata, skb, 0);
 }
 
+static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
+				   struct ieee80211_if_sta *ifsta, int deauth,
+				   int self_disconnected, u16 reason)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta;
+
+	rcu_read_lock();
+
+	sta = sta_info_get(local, ifsta->bssid);
+	if (!sta) {
+		rcu_read_unlock();
+		return;
+	}
+
+	if (deauth) {
+		ifsta->direct_probe_tries = 0;
+		ifsta->auth_tries = 0;
+	}
+	ifsta->assoc_scan_tries = 0;
+	ifsta->assoc_tries = 0;
+
+	netif_carrier_off(sdata->dev);
+
+	ieee80211_sta_tear_down_BA_sessions(sdata, sta->addr);
+
+	if (self_disconnected) {
+		if (deauth)
+			ieee80211_send_deauth(sdata, ifsta, reason);
+		else
+			ieee80211_send_disassoc(sdata, ifsta, reason);
+	}
+
+	ieee80211_set_associated(sdata, ifsta, 0);
+
+	if (self_disconnected)
+		ifsta->state = IEEE80211_STA_MLME_DISABLED;
+
+	rcu_read_unlock();
+
+	sta_info_unlink(&sta);
+	sta_info_destroy(sta);
+}
 
 static int ieee80211_privacy_mismatch(struct ieee80211_sub_if_data *sdata,
 				      struct ieee80211_if_sta *ifsta)
@@ -1104,16 +1133,12 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata,
 
 	rcu_read_unlock();
 
-	if (disassoc && sta)
-		sta_info_destroy(sta);
-
-	if (disassoc) {
-		ifsta->state = IEEE80211_STA_MLME_DISABLED;
-		ieee80211_set_associated(sdata, ifsta, 0);
-	} else {
+	if (disassoc)
+		ieee80211_set_disassoc(sdata, ifsta, 1, 1,
+					WLAN_REASON_PREV_AUTH_NOT_VALID);
+	else
 		mod_timer(&ifsta->timer, jiffies +
 				      IEEE80211_MONITORING_INTERVAL);
-	}
 }
 
 
@@ -1978,7 +2003,7 @@ static void ieee80211_rx_mgmt_deauth(struct ieee80211_sub_if_data *sdata,
 				      IEEE80211_RETRY_AUTH_INTERVAL);
 	}
 
-	ieee80211_set_disassoc(sdata, ifsta, 1);
+	ieee80211_set_disassoc(sdata, ifsta, 1, 0, 0);
 	ifsta->flags &= ~IEEE80211_STA_AUTHENTICATED;
 }
 
@@ -2008,7 +2033,7 @@ static void ieee80211_rx_mgmt_disassoc(struct ieee80211_sub_if_data *sdata,
 				      IEEE80211_RETRY_AUTH_INTERVAL);
 	}
 
-	ieee80211_set_disassoc(sdata, ifsta, 0);
+	ieee80211_set_disassoc(sdata, ifsta, 0, 0, 0);
 }
 
 
@@ -3343,8 +3368,8 @@ void ieee80211_sta_work(struct work_struct *work)
 		printk(KERN_DEBUG "%s: privacy configuration mismatch and "
 		       "mixed-cell disabled - disassociate\n", sdata->dev->name);
 
-		ieee80211_send_disassoc(sdata, ifsta, WLAN_REASON_UNSPECIFIED);
-		ieee80211_set_disassoc(sdata, ifsta, 0);
+		ieee80211_set_disassoc(sdata, ifsta, 0, 1,
+					WLAN_REASON_UNSPECIFIED);
 	}
 }
 
@@ -4379,8 +4404,7 @@ int ieee80211_sta_deauthenticate(struct ieee80211_sub_if_data *sdata, u16 reason
 	    sdata->vif.type != IEEE80211_IF_TYPE_IBSS)
 		return -EINVAL;
 
-	ieee80211_send_deauth(sdata, ifsta, reason);
-	ieee80211_set_disassoc(sdata, ifsta, 1);
+	ieee80211_set_disassoc(sdata, ifsta, 1, 1, reason);
 	return 0;
 }
 
@@ -4398,8 +4422,7 @@ int ieee80211_sta_disassociate(struct ieee80211_sub_if_data *sdata, u16 reason)
 	if (!(ifsta->flags & IEEE80211_STA_ASSOCIATED))
 		return -1;
 
-	ieee80211_send_disassoc(sdata, ifsta, reason);
-	ieee80211_set_disassoc(sdata, ifsta, 0);
+	ieee80211_set_disassoc(sdata, ifsta, 0, 1, reason);
 	return 0;
 }
 
-- 
1.5.4.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[RFC PATCH 2/3] mac80211: disassociate when moving to new BSS

[Tomas Winkler]

This patch cleans current association when leaving to new BSS. this is
needed as old configuration and data were still stored in station table
and in low level driver, causing problems (e.g. 11n mlme) in new BSS

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/mac80211/mlme.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index ba255e2..aa5367e 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -3414,9 +3414,14 @@ void ieee80211_sta_req_auth(struct ieee80211_sub_if_data *sdata,
 		return;
 
 	if ((ifsta->flags & (IEEE80211_STA_BSSID_SET |
-				IEEE80211_STA_AUTO_BSSID_SEL)) &&
+			     IEEE80211_STA_AUTO_BSSID_SEL)) &&
 	    (ifsta->flags & (IEEE80211_STA_SSID_SET |
-				IEEE80211_STA_AUTO_SSID_SEL))) {
+			     IEEE80211_STA_AUTO_SSID_SEL))) {
+
+		if (ifsta->state == IEEE80211_STA_MLME_ASSOCIATED)
+			ieee80211_set_disassoc(sdata, ifsta, 1, 1,
+					       WLAN_REASON_DEAUTH_LEAVING);
+
 		set_bit(IEEE80211_STA_REQ_AUTH, &ifsta->request);
 		queue_work(local->hw.workqueue, &ifsta->work);
 	}
-- 
1.5.4.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[RFC PATCH 3/3] mac80211: remove disassociation code from ieee80211_set_associated

[Tomas Winkler]

This patch moves disassociation code from ieee80211_set_associated
to ieee80211_set_disassoc for this it also  introduces
ieee80211_sta_send_apinfo function to reduce code duplication.
In addition it marks bss_info_change BSS_CHANGED_HT also
in disassociation flow

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/mac80211/mlme.c |  102 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 56 insertions(+), 46 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index aa5367e..2564553 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -497,6 +497,17 @@ int ieee80211_ht_addt_info_ie_to_ht_bss_info(
 	return 0;
 }
 
+static void ieee80211_sta_send_apinfo(struct ieee80211_sub_if_data *sdata,
+					struct ieee80211_if_sta *ifsta)
+{
+	union iwreq_data wrqu;
+	memset(&wrqu, 0, sizeof(wrqu));
+	if (ifsta->flags & IEEE80211_STA_ASSOCIATED)
+		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
+	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
+	wireless_send_event(sdata->dev, SIOCGIWAP, &wrqu, NULL);
+}
+
 static void ieee80211_sta_send_associnfo(struct ieee80211_sub_if_data *sdata,
 					 struct ieee80211_if_sta *ifsta)
 {
@@ -519,68 +530,53 @@ static void ieee80211_sta_send_associnfo(struct ieee80211_sub_if_data *sdata,
 
 
 static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
-				     struct ieee80211_if_sta *ifsta,
-				     bool assoc)
+				     struct ieee80211_if_sta *ifsta)
 {
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_conf *conf = &local_to_hw(local)->conf;
-	union iwreq_data wrqu;
 	u32 changed = BSS_CHANGED_ASSOC;
 
-	if (assoc) {
-		struct ieee80211_sta_bss *bss;
-
-		ifsta->flags |= IEEE80211_STA_ASSOCIATED;
+	struct ieee80211_sta_bss *bss;
 
-		if (sdata->vif.type != IEEE80211_IF_TYPE_STA)
-			return;
+	ifsta->flags |= IEEE80211_STA_ASSOCIATED;
 
-		bss = ieee80211_rx_bss_get(local, ifsta->bssid,
-					   conf->channel->center_freq,
-					   ifsta->ssid, ifsta->ssid_len);
-		if (bss) {
-			/* set timing information */
-			sdata->bss_conf.beacon_int = bss->beacon_int;
-			sdata->bss_conf.timestamp = bss->timestamp;
-			sdata->bss_conf.dtim_period = bss->dtim_period;
+	if (sdata->vif.type != IEEE80211_IF_TYPE_STA)
+		return;
 
-			changed |= ieee80211_handle_bss_capability(sdata, bss);
+	bss = ieee80211_rx_bss_get(local, ifsta->bssid,
+				   conf->channel->center_freq,
+				   ifsta->ssid, ifsta->ssid_len);
+	if (bss) {
+		/* set timing information */
+		sdata->bss_conf.beacon_int = bss->beacon_int;
+		sdata->bss_conf.timestamp = bss->timestamp;
+		sdata->bss_conf.dtim_period = bss->dtim_period;
 
-			ieee80211_rx_bss_put(local, bss);
-		}
+		changed |= ieee80211_handle_bss_capability(sdata, bss);
 
-		if (conf->flags & IEEE80211_CONF_SUPPORT_HT_MODE) {
-			changed |= BSS_CHANGED_HT;
-			sdata->bss_conf.assoc_ht = 1;
-			sdata->bss_conf.ht_conf = &conf->ht_conf;
-			sdata->bss_conf.ht_bss_conf = &conf->ht_bss_conf;
-		}
+		ieee80211_rx_bss_put(local, bss);
+	}
 
-		ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
-		memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
-		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
-		ieee80211_sta_send_associnfo(sdata, ifsta);
-	} else {
-		ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
-		changed |= ieee80211_reset_erp_info(sdata);
+	if (conf->flags & IEEE80211_CONF_SUPPORT_HT_MODE) {
+		changed |= BSS_CHANGED_HT;
+		sdata->bss_conf.assoc_ht = 1;
+		sdata->bss_conf.ht_conf = &conf->ht_conf;
+		sdata->bss_conf.ht_bss_conf = &conf->ht_bss_conf;
+	}
 
-		sdata->bss_conf.assoc_ht = 0;
-		sdata->bss_conf.ht_conf = NULL;
-		sdata->bss_conf.ht_bss_conf = NULL;
+	ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
+	memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
+	ieee80211_sta_send_associnfo(sdata, ifsta);
 
-		memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
-	}
 	ifsta->last_probe = jiffies;
-	ieee80211_led_assoc(local, assoc);
+	ieee80211_led_assoc(local, 1);
 
-	sdata->bss_conf.assoc = assoc;
+	sdata->bss_conf.assoc = 1;
 	ieee80211_bss_info_change_notify(sdata, changed);
 
-	if (assoc)
-		netif_carrier_on(sdata->dev);
+	netif_carrier_on(sdata->dev);
 
-	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
-	wireless_send_event(sdata->dev, SIOCGIWAP, &wrqu, NULL);
+	ieee80211_sta_send_apinfo(sdata, ifsta);
 }
 
 void ieee80211_sta_tx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
@@ -982,6 +978,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 {
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta;
+	u32 changed = BSS_CHANGED_ASSOC;
 
 	rcu_read_lock();
 
@@ -1009,7 +1006,20 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 			ieee80211_send_disassoc(sdata, ifsta, reason);
 	}
 
-	ieee80211_set_associated(sdata, ifsta, 0);
+	ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
+	changed |= ieee80211_reset_erp_info(sdata);
+
+	if (sdata->bss_conf.assoc_ht)
+		changed |= BSS_CHANGED_HT;
+
+	sdata->bss_conf.assoc_ht = 0;
+	sdata->bss_conf.ht_conf = NULL;
+	sdata->bss_conf.ht_bss_conf = NULL;
+
+	ieee80211_led_assoc(local, 0);
+	sdata->bss_conf.assoc = 0;
+
+	ieee80211_sta_send_apinfo(sdata, ifsta);
 
 	if (self_disconnected)
 		ifsta->state = IEEE80211_STA_MLME_DISABLED;
@@ -2227,7 +2237,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 	 * ieee80211_set_associated() will tell the driver */
 	bss_conf->aid = aid;
 	bss_conf->assoc_capability = capab_info;
-	ieee80211_set_associated(sdata, ifsta, 1);
+	ieee80211_set_associated(sdata, ifsta);
 
 	ieee80211_associated(sdata, ifsta);
 }
-- 
1.5.4.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [RFC PATCH 3/3] mac80211: remove disassociation code from ieee80211_set_associated

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:

> -	ieee80211_led_assoc(local, assoc);
> +	ieee80211_led_assoc(local, 1);
>  
> -	sdata->bss_conf.assoc = assoc;
> +	sdata->bss_conf.assoc = 1;

I think you should use "true" at least for the latter "1"

> +	sdata->bss_conf.assoc = 0;

and "false" here, and if the LED interface uses "bool" then for that
too.

The actual patch looks good to me though.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 3/3] mac80211: remove disassociation code from ieee80211_set_associated

[Tomas Winkler]

On Sun, Sep 7, 2008 at 1:30 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
>
>> -     ieee80211_led_assoc(local, assoc);
>> +     ieee80211_led_assoc(local, 1);
>>
>> -     sdata->bss_conf.assoc = assoc;
>> +     sdata->bss_conf.assoc = 1;
>
> I think you should use "true" at least for the latter "1"
>
>> +     sdata->bss_conf.assoc = 0;
>
> and "false" here, and if the LED interface uses "bool" then for that
> too.
>
> The actual patch looks good to me though.

Will do
Thanks
Tomas

Re: [RFC PATCH 2/3] mac80211: disassociate when moving to new BSS

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
> This patch cleans current association when leaving to new BSS. this is
> needed as old configuration and data were still stored in station table
> and in low level driver, causing problems (e.g. 11n mlme) in new BSS
> 
> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  net/mac80211/mlme.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index ba255e2..aa5367e 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -3414,9 +3414,14 @@ void ieee80211_sta_req_auth(struct ieee80211_sub_if_data *sdata,
>  		return;
>  
>  	if ((ifsta->flags & (IEEE80211_STA_BSSID_SET |
> -				IEEE80211_STA_AUTO_BSSID_SEL)) &&
> +			     IEEE80211_STA_AUTO_BSSID_SEL)) &&
>  	    (ifsta->flags & (IEEE80211_STA_SSID_SET |
> -				IEEE80211_STA_AUTO_SSID_SEL))) {
> +			     IEEE80211_STA_AUTO_SSID_SEL))) {
> +
> +		if (ifsta->state == IEEE80211_STA_MLME_ASSOCIATED)
> +			ieee80211_set_disassoc(sdata, ifsta, 1, 1,
> +					       WLAN_REASON_DEAUTH_LEAVING);
> +
>  		set_bit(IEEE80211_STA_REQ_AUTH, &ifsta->request);
>  		queue_work(local->hw.workqueue, &ifsta->work);
>  	}

Looks sane to me

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]


> 
> +static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> +                                  struct ieee80211_if_sta *ifsta, int deauth,
> +                                  int self_disconnected, u16 reason)

I think you should use bool for those two (deauth, self)

> +	rcu_read_unlock();
> +
> +	sta_info_unlink(&sta);

Those need to be the other way around.


> +       ieee80211_set_associated(sdata, ifsta, 0);

This whole set_associated(0) vs. ieee80211_set_disassoc() business seems
fishy to me, can't we just move the set_associated(0) code into
set_disassoc? AFAICT there's only one caller now that calls with (0),
and that's in set_disassoc().

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Sun, 2008-09-07 at 00:25 +0200, Johannes Berg wrote:

> > +       ieee80211_set_associated(sdata, ifsta, 0);
> 
> This whole set_associated(0) vs. ieee80211_set_disassoc() business seems
> fishy to me, can't we just move the set_associated(0) code into
> set_disassoc? AFAICT there's only one caller now that calls with (0),
> and that's in set_disassoc().

Ah, I see you're doing that in 3/3

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Tomas Winkler]

On Sun, Sep 7, 2008 at 1:25 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>>
>> +static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>> +                                  struct ieee80211_if_sta *ifsta, int deauth,
>> +                                  int self_disconnected, u16 reason)
>
> I think you should use bool for those two (deauth, self)

I'm thinking rather using some enum values just for readability
foo(DEAUTH, ORIGIN_SELF) vs foo(tree,true)
foo(DISASSOC, ORIGIN_PEER)  vs foo(false, false)


>> +     rcu_read_unlock();
>> +
>> +     sta_info_unlink(&sta);
>
> Those need to be the other way around.

Will do,
Thanks
Tomas

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Sun, 2008-09-07 at 01:32 +0300, Tomas Winkler wrote:
> On Sun, Sep 7, 2008 at 1:25 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> >>
> >> +static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> >> +                                  struct ieee80211_if_sta *ifsta, int deauth,
> >> +                                  int self_disconnected, u16 reason)
> >
> > I think you should use bool for those two (deauth, self)
> 
> I'm thinking rather using some enum values just for readability
> foo(DEAUTH, ORIGIN_SELF) vs foo(tree,true)
> foo(DISASSOC, ORIGIN_PEER)  vs foo(false, false)

That works too, sure, though seems a little overkill, the function is
called what, three times? Anyway, whatever you prefer, I just don't like
bare "int" as a bool.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Tomas Winkler]

On Sun, Sep 7, 2008 at 1:42 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2008-09-07 at 01:32 +0300, Tomas Winkler wrote:
>> On Sun, Sep 7, 2008 at 1:25 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> >
>> >>
>> >> +static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>> >> +                                  struct ieee80211_if_sta *ifsta, int deauth,
>> >> +                                  int self_disconnected, u16 reason)
>> >
>> > I think you should use bool for those two (deauth, self)
>>
>> I'm thinking rather using some enum values just for readability
>> foo(DEAUTH, ORIGIN_SELF) vs foo(tree,true)
>> foo(DISASSOC, ORIGIN_PEER)  vs foo(false, false)
>
> That works too, sure, though seems a little overkill, the function is
> called what, three times? Anyway, whatever you prefer, I just don't like
> bare "int" as a bool.

Do u think it is sane using IEEE80211_STYPE_DISASSOC and
IEEE80211_STYPE_DEAUTH for this?
Thanks
Tomas

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Sun, 2008-09-07 at 02:24 +0300, Tomas Winkler wrote:
> On Sun, Sep 7, 2008 at 1:42 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Sun, 2008-09-07 at 01:32 +0300, Tomas Winkler wrote:
> >> On Sun, Sep 7, 2008 at 1:25 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> >> >
> >> >>
> >> >> +static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> >> >> +                                  struct ieee80211_if_sta *ifsta, int deauth,
> >> >> +                                  int self_disconnected, u16 reason)
> >> >
> >> > I think you should use bool for those two (deauth, self)
> >>
> >> I'm thinking rather using some enum values just for readability
> >> foo(DEAUTH, ORIGIN_SELF) vs foo(tree,true)
> >> foo(DISASSOC, ORIGIN_PEER)  vs foo(false, false)
> >
> > That works too, sure, though seems a little overkill, the function is
> > called what, three times? Anyway, whatever you prefer, I just don't like
> > bare "int" as a bool.
> 
> Do u think it is sane using IEEE80211_STYPE_DISASSOC and
> IEEE80211_STYPE_DEAUTH for this?

Dunno, were you thinking of unifying ieee80211_send_disassoc and
ieee80211_send_deauth into a single function? That might be worthwhile
anyway, the frames are identical...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:

> @@ -1104,16 +1133,12 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata,
>  
>  	rcu_read_unlock();
>  
> -	if (disassoc && sta)
> -		sta_info_destroy(sta);
> -
> -	if (disassoc) {
> -		ifsta->state = IEEE80211_STA_MLME_DISABLED;
> -		ieee80211_set_associated(sdata, ifsta, 0);
> -	} else {
> +	if (disassoc)
> +		ieee80211_set_disassoc(sdata, ifsta, 1, 1,
> +					WLAN_REASON_PREV_AUTH_NOT_VALID);
> +	else
>  		mod_timer(&ifsta->timer, jiffies +
>  				      IEEE80211_MONITORING_INTERVAL);
> -	}
>  }
>  
> 

This hunk has another bug, or rather, we need to add this hunk:

@@ -1084,7 +1114,6 @@ static void ieee80211_associated(struct 
                                       "range\n",
                                       sdata->dev->name, print_mac(mac, ifsta->bssid));
                                disassoc = 1;
-                               sta_info_unlink(&sta);
                        } else
                                ieee80211_send_probe_req(sdata, ifsta->bssid,
                                                         local->scan_ssid,

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Tomas Winkler]

On Mon, Sep 8, 2008 at 11:23 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
>
>> @@ -1104,16 +1133,12 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata,
>>
>>       rcu_read_unlock();
>>
>> -     if (disassoc && sta)
>> -             sta_info_destroy(sta);
>> -
>> -     if (disassoc) {
>> -             ifsta->state = IEEE80211_STA_MLME_DISABLED;
>> -             ieee80211_set_associated(sdata, ifsta, 0);
>> -     } else {
>> +     if (disassoc)
>> +             ieee80211_set_disassoc(sdata, ifsta, 1, 1,
>> +                                     WLAN_REASON_PREV_AUTH_NOT_VALID);
>> +     else
>>               mod_timer(&ifsta->timer, jiffies +
>>                                     IEEE80211_MONITORING_INTERVAL);
>> -     }
>>  }
>>
>>
>
> This hunk has another bug, or rather, we need to add this hunk:
>
> @@ -1084,7 +1114,6 @@ static void ieee80211_associated(struct
>                                       "range\n",
>                                       sdata->dev->name, print_mac(mac, ifsta->bssid));
>                                disassoc = 1;
> -                               sta_info_unlink(&sta);
>                        } else
>                                ieee80211_send_probe_req(sdata, ifsta->bssid,
>                                                         local->scan_ssid,
>
Yep
This is rebasing bug. This line is newer than the original patch.
I will added this hunk.
Thanks

Re: [RFC PATCH 1/3] mac80211: restructure disassoc/deauth flows

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Mon, 2008-09-08 at 11:55 +0300, Tomas Winkler wrote:

> > @@ -1084,7 +1114,6 @@ static void ieee80211_associated(struct
> >                                       "range\n",
> >                                       sdata->dev->name, print_mac(mac, ifsta->bssid));
> >                                disassoc = 1;
> > -                               sta_info_unlink(&sta);
> >                        } else
> >                                ieee80211_send_probe_req(sdata, ifsta->bssid,
> >                                                         local->scan_ssid,
> >
> Yep
> This is rebasing bug. This line is newer than the original patch.
> I will added this hunk.

Alright. I already have it in my version but since I've rebased my
patches on top of these three anyway, you can simply send them in when
you've verified that they're ok (maybe with the queue stop I just sent
you) and then I'll do mine after. I have a few I'll be re-sending but
they don't conflict.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Marcel Holtmann]

Hi Tomas,

> This series should fix mac problems in disassociation
> There are two basic problems disassociation in current implementation.
> The order of steps in the dissociation flow and missing proper
> disassociation when moving from one AP to another. 

it would be nice if you add the problem description into the commit
messages of the actual patches. This helps to actually figure out the
reason for these kind of changes and they are then stored together with
the patch. We really have to be more verbose why we are doing things. If
anyone has to go through one year old kernels and figure out why a
change has been made, they are basically lost. I had to go through 7
years old kernels in the past and every extra piece of information is so
useful then.

Regards

Marcel

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Sun, Sep 7, 2008 at 4:08 AM, Marcel Holtmann
<holtmann@linux.intel.com> wrote:
> Hi Tomas,
>
>> This series should fix mac problems in disassociation
>> There are two basic problems disassociation in current implementation.
>> The order of steps in the dissociation flow and missing proper
>> disassociation when moving from one AP to another.
>
> it would be nice if you add the problem description into the commit
> messages of the actual patches. This helps to actually figure out the
> reason for these kind of changes and they are then stored together with
> the patch. We really have to be more verbose why we are doing things. If
> anyone has to go through one year old kernels and figure out why a
> change has been made, they are basically lost. I had to go through 7
> years old kernels in the past and every extra piece of information is so
> useful then.

Sure, this is just RFC series
Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
> This series should fix mac problems in disassociation
> There are two basic problems disassociation in current implementation.
> The order of steps in the dissociation flow and missing proper
> disassociation when moving from one AP to another. 

Mind if I adopt this patch series? I've done only slight modifications
so far (the int->bool thing I asked for) and have rebased all the
patches I had already posted on top of it. The arguments to
ieee80211_set_disassoc are changing a lot in my patch series anyway so
I'll just work on top of the bool/bool thing and  then fix it up, ok?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Sun, Sep 7, 2008 at 4:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
>> This series should fix mac problems in disassociation
>> There are two basic problems disassociation in current implementation.
>> The order of steps in the dissociation flow and missing proper
>> disassociation when moving from one AP to another.
>
> Mind if I adopt this patch series? I've done only slight modifications
> so far (the int->bool thing I asked for) and have rebased all the
> patches I had already posted on top of it. The arguments to
> ieee80211_set_disassoc are changing a lot in my patch series anyway so
> I'll just work on top of the bool/bool thing and  then fix it up, ok?

Don't mind just need to test them properly and update commit messages.
Any suggestion how we do that?
Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Sun, 2008-09-07 at 17:24 +0300, Tomas Winkler wrote:
> On Sun, Sep 7, 2008 at 4:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
> >> This series should fix mac problems in disassociation
> >> There are two basic problems disassociation in current implementation.
> >> The order of steps in the dissociation flow and missing proper
> >> disassociation when moving from one AP to another.
> >
> > Mind if I adopt this patch series? I've done only slight modifications
> > so far (the int->bool thing I asked for) and have rebased all the
> > patches I had already posted on top of it. The arguments to
> > ieee80211_set_disassoc are changing a lot in my patch series anyway so
> > I'll just work on top of the bool/bool thing and  then fix it up, ok?
> 
> Don't mind just need to test them properly and update commit messages.
> Any suggestion how we do that?

I've already changed the commit messages slightly, you can find my
versions at http://johannes.sipsolutions.net/patches/kernel/all/LATEST/
(disassoc-part-{1,2,3}.patch). If you want to test those by themselves
that would probably be useful, and if you want to test my complete mlme
rewrite I'd appreciate that too, but I'm not done rebasing yet :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Sun, Sep 7, 2008 at 5:40 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2008-09-07 at 17:24 +0300, Tomas Winkler wrote:
>> On Sun, Sep 7, 2008 at 4:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
>> >> This series should fix mac problems in disassociation
>> >> There are two basic problems disassociation in current implementation.
>> >> The order of steps in the dissociation flow and missing proper
>> >> disassociation when moving from one AP to another.
>> >
>> > Mind if I adopt this patch series? I've done only slight modifications
>> > so far (the int->bool thing I asked for) and have rebased all the
>> > patches I had already posted on top of it. The arguments to
>> > ieee80211_set_disassoc are changing a lot in my patch series anyway so
>> > I'll just work on top of the bool/bool thing and  then fix it up, ok?
>>
>> Don't mind just need to test them properly and update commit messages.
>> Any suggestion how we do that?
>
> I've already changed the commit messages slightly, you can find my
> versions at http://johannes.sipsolutions.net/patches/kernel/all/LATEST/
> (disassoc-part-{1,2,3}.patch). If you want to test those by themselves
> that would probably be useful, and if you want to test my complete mlme
> rewrite I'd appreciate that too, but I'm not done rebasing yet :)

Okay I'll take first the 3 patches then now and I will check the
rewrites after that .
Thanks
Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Sun, Sep 7, 2008 at 5:50 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Sun, Sep 7, 2008 at 5:40 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Sun, 2008-09-07 at 17:24 +0300, Tomas Winkler wrote:
>>> On Sun, Sep 7, 2008 at 4:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>>> > On Sun, 2008-09-07 at 01:14 +0300, Tomas Winkler wrote:
>>> >> This series should fix mac problems in disassociation
>>> >> There are two basic problems disassociation in current implementation.
>>> >> The order of steps in the dissociation flow and missing proper
>>> >> disassociation when moving from one AP to another.
>>> >
>>> > Mind if I adopt this patch series? I've done only slight modifications
>>> > so far (the int->bool thing I asked for) and have rebased all the
>>> > patches I had already posted on top of it. The arguments to
>>> > ieee80211_set_disassoc are changing a lot in my patch series anyway so
>>> > I'll just work on top of the bool/bool thing and  then fix it up, ok?
>>>
>>> Don't mind just need to test them properly and update commit messages.
>>> Any suggestion how we do that?
>>
>> I've already changed the commit messages slightly, you can find my
>> versions at http://johannes.sipsolutions.net/patches/kernel/all/LATEST/
>> (disassoc-part-{1,2,3}.patch). If you want to test those by themselves
>> that would probably be useful, and if you want to test my complete mlme
>> rewrite I'd appreciate that too, but I'm not done rebasing yet :)
>
> Okay I'll take first the 3 patches then now and I will check the
> rewrites after that .

In general it works but I didn't test all the corner cases yet. Not
sure this is related but.
why I'm hitting unathorized prot messag for subif_start_xmit.
Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
before.

Thanks
Tomas

> Thanks
> Tomas
>

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:

> In general it works but I didn't test all the corner cases yet. Not
> sure this is related but.
> why I'm hitting unathorized prot messag for subif_start_xmit.
> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
> before.

In client mode? Then something is really gone wrong, APs always have the
WLAN_STA_AUTHORIZED bit set once we get an assoc response.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:

> why I'm hitting unathorized prot messag for subif_start_xmit.
> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this

and no, netif_carrier_off() doesn't immediately stop the queues, it just
schedules the link watch which will _eventually_ remove the qdiscs and
attach the noop qdisc.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Mon, Sep 8, 2008 at 10:58 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:
>
>> why I'm hitting unathorized prot messag for subif_start_xmit.
>> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
>
> and no, netif_carrier_off() doesn't immediately stop the queues, it just
> schedules the link watch which will _eventually_ remove the qdiscs and
> attach the noop qdisc.

So what you are saying this is expected behavior?

Please remember I'm testing the disassociation
1. AP is should down
2. Switching to another AP -> Legacy <-> HT
3. WPA supplicnat (it uses ieee80211_ioctl_siwmlme)
4. The BA tear down will be unfortunately untested, for now

Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Mon, 2008-09-08 at 11:18 +0300, Tomas Winkler wrote:
> On Mon, Sep 8, 2008 at 10:58 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:
> >
> >> why I'm hitting unathorized prot messag for subif_start_xmit.
> >> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
> >
> > and no, netif_carrier_off() doesn't immediately stop the queues, it just
> > schedules the link watch which will _eventually_ remove the qdiscs and
> > attach the noop qdisc.
> 
> So what you are saying this is expected behavior?

Sort of, yes.

> Please remember I'm testing the disassociation
> 1. AP is should down
> 2. Switching to another AP -> Legacy <-> HT
> 3. WPA supplicnat (it uses ieee80211_ioctl_siwmlme)
> 4. The BA tear down will be unfortunately untested, for now

When you shut down the AP, it probably sends you a deauth. Frames we
want to send out for it might still be pending in our queue. Since
you're removing the sta_info for it right away and not stopping queues,
you get the "unauthorized port" message because there's no sta_info
struct.

The reason you haven't seen that before is that the sta_info wasn't
deleted, so we could still send out a few frames before
netif_carrier_down took effect.

To fix it, we probably want to simply stop the queue for that subif too.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Mon, 2008-09-08 at 10:30 +0200, Johannes Berg wrote:

> When you shut down the AP, it probably sends you a deauth. 

and if it doesn't, you run into the same situation, the old code however
should have run into the same situation too if it didn't deauth frames,
because in the case where the AP simply suddenly stopped responding to
probe requests we removed the sta_info too. Hence my assumption it does
send deauth frames because you haven't seen this problem before.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:

> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
> before.

The other question is whether we should be doing netif_carrier_off() at
all since that throws away frames from the queue and attaches a noop
qdisc. When we're roaming quickly between two APs, we don't necessarily
want that, but we don't actually know whether or not we'll be able to
get back to an AP quickly when we disassociate for whatever reason.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Mon, Sep 8, 2008 at 11:38 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:
>
>> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
>> before.
>
> The other question is whether we should be doing netif_carrier_off() at
> all since that throws away frames from the queue and attaches a noop
> qdisc. When we're roaming quickly between two APs, we don't necessarily
> want that, but we don't actually know whether or not we'll be able to
> get back to an AP quickly when we disassociate for whatever reason.
>
we should stop the wlan0 not the master device. Only data frames
should be stopped.
Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

On Mon, 2008-09-08 at 11:46 +0300, Tomas Winkler wrote:
> On Mon, Sep 8, 2008 at 11:38 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:
> >
> >> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
> >> before.
> >
> > The other question is whether we should be doing netif_carrier_off() at
> > all since that throws away frames from the queue and attaches a noop
> > qdisc. When we're roaming quickly between two APs, we don't necessarily
> > want that, but we don't actually know whether or not we'll be able to
> > get back to an AP quickly when we disassociate for whatever reason.
> >
> we should stop the wlan0 not the master device. Only data frames
> should be stopped.

That's what we're doing of course, but should we really drop all the
frames that might still be in the queue? If we're just roaming we could
send them out via the next AP, but it's hard to know, and since it's
working let's not touch it for now.

How exactly are you triggering that "unauthorized port" message? I can't
seem to reproduce to see if stopping the queue helps, but I'm fairly
sure, try the patch below that fixes this.

johannes

--- everything.orig/net/mac80211/mlme.c	2008-09-08 10:54:28.000000000 +0200
+++ everything/net/mac80211/mlme.c	2008-09-08 10:57:30.000000000 +0200
@@ -574,6 +574,7 @@ static void ieee80211_set_associated(str
 	sdata->bss_conf.assoc = 1;
 	ieee80211_bss_info_change_notify(sdata, changed);
 
+	netif_tx_start_all_queues(sdata->dev);
 	netif_carrier_on(sdata->dev);
 
 	ieee80211_sta_send_apinfo(sdata, ifsta);
@@ -995,6 +996,7 @@ static void ieee80211_set_disassoc(struc
 	ifsta->assoc_scan_tries = 0;
 	ifsta->assoc_tries = 0;
 
+	netif_tx_stop_all_queues(sdata->dev);
 	netif_carrier_off(sdata->dev);
 
 	ieee80211_sta_tear_down_BA_sessions(sdata, sta->addr);
@@ -3413,6 +3415,7 @@ static void ieee80211_sta_reset_auth(str
 	ifsta->direct_probe_tries = 0;
 	ifsta->auth_tries = 0;
 	ifsta->assoc_tries = 0;
+	netif_tx_stop_all_queues(sdata->dev);
 	netif_carrier_off(sdata->dev);
 }
 

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Mon, Sep 8, 2008 at 11:58 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-09-08 at 11:46 +0300, Tomas Winkler wrote:
>> On Mon, Sep 8, 2008 at 11:38 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Mon, 2008-09-08 at 02:14 +0300, Tomas Winkler wrote:
>> >
>> >> Shouldn't carrier_off(stada->dev) stop the queue?  I haven't seen this
>> >> before.
>> >
>> > The other question is whether we should be doing netif_carrier_off() at
>> > all since that throws away frames from the queue and attaches a noop
>> > qdisc. When we're roaming quickly between two APs, we don't necessarily
>> > want that, but we don't actually know whether or not we'll be able to
>> > get back to an AP quickly when we disassociate for whatever reason.
>> >
>> we should stop the wlan0 not the master device. Only data frames
>> should be stopped.
>
> That's what we're doing of course, but should we really drop all the
> frames that might still be in the queue? If we're just roaming we could
> send them out via the next AP, but it's hard to know, and since it's
> working let's not touch it for now.

In current situation we are dropping the frames anyway.
How do you know you are roaming and not moving to another network?

> How exactly are you triggering that "unauthorized port" message? I can't
> seem to reproduce to see if stopping the queue helps, but I'm fairly
> sure, try the patch below that fixes this.

Powering off AP, I think I've seen only Cisco AP that are able to send
deauth packet on power off. So this was without.

> johannes
>
> --- everything.orig/net/mac80211/mlme.c 2008-09-08 10:54:28.000000000 +0200
> +++ everything/net/mac80211/mlme.c      2008-09-08 10:57:30.000000000 +0200
> @@ -574,6 +574,7 @@ static void ieee80211_set_associated(str
>        sdata->bss_conf.assoc = 1;
>        ieee80211_bss_info_change_notify(sdata, changed);
>
> +       netif_tx_start_all_queues(sdata->dev);
>        netif_carrier_on(sdata->dev);
>
>        ieee80211_sta_send_apinfo(sdata, ifsta);
> @@ -995,6 +996,7 @@ static void ieee80211_set_disassoc(struc
>        ifsta->assoc_scan_tries = 0;
>        ifsta->assoc_tries = 0;
>
> +       netif_tx_stop_all_queues(sdata->dev);
>        netif_carrier_off(sdata->dev);
>
>        ieee80211_sta_tear_down_BA_sessions(sdata, sta->addr);
> @@ -3413,6 +3415,7 @@ static void ieee80211_sta_reset_auth(str
>        ifsta->direct_probe_tries = 0;
>        ifsta->auth_tries = 0;
>        ifsta->assoc_tries = 0;
> +       netif_tx_stop_all_queues(sdata->dev);
>        netif_carrier_off(sdata->dev);
>  }
>
Will be able to test only at night.
Tomas
>
>
>

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Mon, 2008-09-08 at 12:06 +0300, Tomas Winkler wrote:

> > That's what we're doing of course, but should we really drop all the
> > frames that might still be in the queue? If we're just roaming we could
> > send them out via the next AP, but it's hard to know, and since it's
> > working let's not touch it for now.
> 
> In current situation we are dropping the frames anyway.
> How do you know you are roaming and not moving to another network?

Yeah exactly, it's not easy to know.

> > How exactly are you triggering that "unauthorized port" message? I can't
> > seem to reproduce to see if stopping the queue helps, but I'm fairly
> > sure, try the patch below that fixes this.
> 
> Powering off AP, I think I've seen only Cisco AP that are able to send
> deauth packet on power off. So this was without.

Odd. So you did get the "No ProbeResp from current AP - assume out of
range" message?

In that case I would have expected you to get the same message, but of
course it's both timing and frame sensitive. I'm a bit surprised you saw
the message at all. Maybe I can reproduce by doing a ping flood to the
AP.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Mon, 2008-09-08 at 12:06 +0300, Tomas Winkler wrote:

> > How exactly are you triggering that "unauthorized port" message? I can't
> > seem to reproduce to see if stopping the queue helps, but I'm fairly
> > sure, try the patch below that fixes this.
> 
> Powering off AP, I think I've seen only Cisco AP that are able to send
> deauth packet on power off. So this was without.

Alright, I have an AP where I can log in and turn off the radio :))

I've been able to trigger the message _without_ your patches, but not
_with_ your patches so far!

I still think we should add the queue stopping so we can't run into the
messages. Do you want me to send them out then?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Tomas Winkler]

On Mon, Sep 8, 2008 at 12:24 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-09-08 at 12:06 +0300, Tomas Winkler wrote:
>
>> > How exactly are you triggering that "unauthorized port" message? I can't
>> > seem to reproduce to see if stopping the queue helps, but I'm fairly
>> > sure, try the patch below that fixes this.
>>
>> Powering off AP, I think I've seen only Cisco AP that are able to send
>> deauth packet on power off. So this was without.
>
> Alright, I have an AP where I can log in and turn off the radio :))
>
> I've been able to trigger the message _without_ your patches, but not
> _with_ your patches so far!
>
> I still think we should add the queue stopping so we can't run into the
> messages. Do you want me to send them out then?

I've added new commit message to the first patch and the new hunk
otherwise no changes.
I want to test your stopping queue stuff in my environment  but I get
to the lab only very late today.
Thanks for your effort
Tomas

Re: [RFC PATCH 0/3] mac80211 dissasociation

[Johannes Berg]

On Mon, 2008-09-08 at 12:32 +0300, Tomas Winkler wrote:

> I want to test your stopping queue stuff in my environment  but I get
> to the lab only very late today.

Ok, sure, works for me. I decided to do the stopping in a separate patch
as I have verified that it was already "wrong" before your changes,
patch inlined below (it's more a cosmetic thing, but still wrong).

Subject: mac80211: stop queues before carrier off

During testing of the disassociation fixes, Tomas noticed that it
was possible to run into a situation where you'd suddenly get a
few "wlan0: dropped frame to <AP> (unauthorized port)" messages
and I found this to be due to the AP's sta_info having been
removed but netif_carrier_off not having removed/stopped traffic
yet. To avoid that, stop the queue for the interface (and avoid
bringing them up when another vif scans when they weren't up.)

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/mlme.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

--- everything.orig/net/mac80211/mlme.c	2008-09-08 11:27:39.000000000 +0200
+++ everything/net/mac80211/mlme.c	2008-09-08 11:30:41.000000000 +0200
@@ -574,6 +574,7 @@ static void ieee80211_set_associated(str
 	sdata->bss_conf.assoc = 1;
 	ieee80211_bss_info_change_notify(sdata, changed);
 
+	netif_tx_start_all_queues(sdata->dev);
 	netif_carrier_on(sdata->dev);
 
 	ieee80211_sta_send_apinfo(sdata, ifsta);
@@ -995,6 +996,7 @@ static void ieee80211_set_disassoc(struc
 	ifsta->assoc_scan_tries = 0;
 	ifsta->assoc_tries = 0;
 
+	netif_tx_stop_all_queues(sdata->dev);
 	netif_carrier_off(sdata->dev);
 
 	ieee80211_sta_tear_down_BA_sessions(sdata, sta->addr);
@@ -3413,6 +3415,7 @@ static void ieee80211_sta_reset_auth(str
 	ifsta->direct_probe_tries = 0;
 	ifsta->auth_tries = 0;
 	ifsta->assoc_tries = 0;
+	netif_tx_stop_all_queues(sdata->dev);
 	netif_carrier_off(sdata->dev);
 }
 
@@ -3889,13 +3892,15 @@ void ieee80211_scan_completed(struct iee
 	rcu_read_lock();
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		/* Tell AP we're back */
-		if (sdata->vif.type == IEEE80211_IF_TYPE_STA &&
-		    sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED)
-			ieee80211_send_nullfunc(local, sdata, 0);
+		if (sdata->vif.type == IEEE80211_IF_TYPE_STA) {
+			if (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
+				ieee80211_send_nullfunc(local, sdata, 0);
+				netif_tx_wake_all_queues(sdata->dev);
+			}
+		} else
+			netif_tx_wake_all_queues(sdata->dev);
 
 		ieee80211_restart_sta_timer(sdata);
-
-		netif_wake_queue(sdata->dev);
 	}
 	rcu_read_unlock();
 
@@ -4053,10 +4058,13 @@ static int ieee80211_sta_start_scan(stru
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-		netif_stop_queue(sdata->dev);
-		if (sdata->vif.type == IEEE80211_IF_TYPE_STA &&
-		    (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED))
-			ieee80211_send_nullfunc(local, sdata, 1);
+		if (sdata->vif.type == IEEE80211_IF_TYPE_STA) {
+			if (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
+				netif_tx_stop_all_queues(sdata->dev);
+				ieee80211_send_nullfunc(local, sdata, 1);
+			}
+		} else
+			netif_tx_stop_all_queues(sdata->dev);
 	}
 	rcu_read_unlock();