[PATCH 1/2] mac80211: some fixes for ieee80211_do_stop while suspend

[PATCH 1/2] mac80211: some fixes for ieee80211_do_stop while suspend

[Stanislaw Gruszka]

Is possible that we close interface while we are suspended, that
can result warning like below (and some others similar):

WARNING: at net/mac80211/driver-ops.h:12 ieee80211_do_stop+0x62e/0x670 [mac80211]()
wlan0:  Failed check-sdata-in-driver check, flags: 0x4
Call Trace:
 [<ffffffff8105c9d6>] warn_slowpath_fmt+0x46/0x50
 [<ffffffffa045d46e>] ieee80211_do_stop+0x62e/0x670 [mac80211]
 [<ffffffffa045d4ca>] ieee80211_stop+0x1a/0x20 [mac80211]
 [<ffffffff815122ed>] __dev_close_many+0x7d/0xc0
 [<ffffffff81513af8>] dev_close_many+0x88/0x100
 [<ffffffff81513f2a>] dev_close+0x3a/0x50
 [<ffffffffa03c90ae>] cfg80211_rfkill_set_block+0x6e/0xa0 [cfg80211]
 [<ffffffffa03c9106>] cfg80211_rfkill_sync_work+0x26/0x30 [cfg80211]

Patch try to avoid calling most of drv callbacks when stopping interface
while suspended.

Some further work is probably needed to handle ROC, DFS, WDS and
ieee80211_{add,del}_virtual_monitor . However patch should fix issues
in most common scenario, i.e. managed mode without any new futures.
 
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
Patch is on top of my two pending patches:
 - [PATCH 1/2] mac80211: move sdata debugfs dir to vif
 - [PATCH 2/2] mac80211: remove vif debugfs driver callbacks

 net/mac80211/iface.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index dfe9cb9..c136050 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -744,8 +744,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 				 sdata->dev->addr_len);
 		spin_unlock_bh(&local->filter_lock);
 		netif_addr_unlock_bh(sdata->dev);
-
-		ieee80211_configure_filter(local);
+		/* configure filter latter (if not suspended) */
 	}
 
 	del_timer_sync(&local->dynamic_ps_timer);
@@ -810,10 +809,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		}
 
 		ieee80211_adjust_monitor_flags(sdata, -1);
-		ieee80211_configure_filter(local);
-		mutex_lock(&local->mtx);
-		ieee80211_recalc_idle(local);
-		mutex_unlock(&local->mtx);
+		/* tell driver latter (if not suspended) */
 		break;
 	case NL80211_IFTYPE_P2P_DEVICE:
 		/* relies on synchronize_rcu() below */
@@ -844,26 +840,29 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 	case NL80211_IFTYPE_AP:
 		skb_queue_purge(&sdata->skb_queue);
 
-		if (going_down)
+		if (going_down && !local->suspended)
 			drv_remove_interface(local, sdata);
 	}
 
 	sdata->bss = NULL;
 
-	ieee80211_recalc_ps(local, -1);
+	if (!local->suspended) {
+		if (local->open_count == 0) {
+			ieee80211_clear_tx_pending(local);
+			ieee80211_stop_device(local);
+		} else {
+			ieee80211_configure_filter(local);
+			ieee80211_recalc_ps(local, -1);
 
-	if (local->open_count == 0) {
-		ieee80211_clear_tx_pending(local);
-		ieee80211_stop_device(local);
+			mutex_lock(&local->mtx);
+			ieee80211_recalc_idle(local);
+			mutex_unlock(&local->mtx);
 
-		/* no reconfiguring after stop! */
-		hw_reconf_flags = 0;
+			if (hw_reconf_flags)
+				ieee80211_hw_config(local, hw_reconf_flags);
+		}
 	}
 
-	/* do after stop to avoid reconfiguring when we stop anyway */
-	if (hw_reconf_flags)
-		ieee80211_hw_config(local, hw_reconf_flags);
-
 	spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
 	for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
 		skb_queue_walk_safe(&local->pending[i], skb, tmp) {
-- 
1.7.1

[PATCH 2/2] mac80211: fixes for virtual monitor add/remove on suspend

[Stanislaw Gruszka]

Change suspend/resume and add/remove virtual monitor code (called at
ieee80211_do_stop) to avoid calling drv callback while we are suspended.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 net/mac80211/iface.c |   43 +++++++++++++++++++++++--------------------
 net/mac80211/pm.c    |    4 +++-
 net/mac80211/util.c  |   12 ++++++++++--
 3 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c136050..7893913 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -373,28 +373,30 @@ static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
 
 	ieee80211_set_default_queues(sdata);
 
-	ret = drv_add_interface(local, sdata);
-	if (WARN_ON(ret)) {
-		/* ok .. stupid driver, it asked for this! */
-		kfree(sdata);
-		goto out_unlock;
-	}
+	if (!local->suspended) {
+		ret = drv_add_interface(local, sdata);
+		if (WARN_ON(ret)) {
+			/* ok .. stupid driver, it asked for this! */
+			goto out_free;
+		}
 
-	ret = ieee80211_check_queues(sdata);
-	if (ret) {
-		kfree(sdata);
-		goto out_unlock;
-	}
+			ret = ieee80211_check_queues(sdata);
+		if (ret)
+			goto out_remove;
 
-	ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
-					IEEE80211_CHANCTX_EXCLUSIVE);
-	if (ret) {
-		drv_remove_interface(local, sdata);
-		kfree(sdata);
-		goto out_unlock;
+		ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
+						IEEE80211_CHANCTX_EXCLUSIVE);
+		if (ret)
+			goto out_remove;
 	}
 
 	rcu_assign_pointer(local->monitor_sdata, sdata);
+	goto out_unlock;
+
+ out_remove:
+	drv_remove_interface(local, sdata);
+ out_free:
+	kfree(sdata);
  out_unlock:
 	mutex_unlock(&local->iflist_mtx);
 	return ret;
@@ -417,9 +419,10 @@ static void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
 	rcu_assign_pointer(local->monitor_sdata, NULL);
 	synchronize_net();
 
-	ieee80211_vif_release_channel(sdata);
-
-	drv_remove_interface(local, sdata);
+	if (!local->suspended) {
+		ieee80211_vif_release_channel(sdata);
+		drv_remove_interface(local, sdata);
+	}
 
 	kfree(sdata);
  out_unlock:
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index b471a67..dbe42ea 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -100,8 +100,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
 	}
 
 	sdata = rtnl_dereference(local->monitor_sdata);
-	if (sdata)
+	if (sdata) {
+		ieee80211_vif_release_channel(sdata);
 		drv_remove_interface(local, sdata);
+	}
 
 	/*
 	 * We disconnected on all interfaces before suspend, all channel
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index b7a856e..8f30275 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1470,8 +1470,16 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 	}
 
 	sdata = rtnl_dereference(local->monitor_sdata);
-	if (sdata && ieee80211_sdata_running(sdata))
-		ieee80211_assign_chanctx(local, sdata);
+	if (sdata) {
+		res = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
+						IEEE80211_CHANCTX_EXCLUSIVE);
+		if (WARN_ON(res)) {
+			drv_remove_interface(local, sdata);
+			rcu_assign_pointer(local->monitor_sdata, NULL);
+			synchronize_net();
+			kfree(sdata);
+		}
+	}
 
 	/* add STAs back */
 	mutex_lock(&local->sta_mtx);
-- 
1.7.1

Re: [PATCH 2/2] mac80211: fixes for virtual monitor add/remove on suspend

[Stanislaw Gruszka]

On Mon, Mar 11, 2013 at 06:01:19PM +0100, Stanislaw Gruszka wrote:
> Change suspend/resume and add/remove virtual monitor code (called at
> ieee80211_do_stop) to avoid calling drv callback while we are suspended.
[snip]
> -	if (sdata && ieee80211_sdata_running(sdata))
> -		ieee80211_assign_chanctx(local, sdata);
> +	if (sdata) {
> +		res = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
> +						IEEE80211_CHANCTX_EXCLUSIVE);
> +		if (WARN_ON(res)) {
> +			drv_remove_interface(local, sdata);
> +			rcu_assign_pointer(local->monitor_sdata, NULL);
> +			synchronize_net();
> +			kfree(sdata);
> +		}
Hmm, this probably shouldn't be changed for hw restart case.

Stanislaw

[PATCH 2/2 v2] mac80211: fixes for virtual monitor add/remove on suspend

[Stanislaw Gruszka]

Change suspend/resume and add/remove virtual monitor code (called at
ieee80211_do_stop) to avoid calling drv callback while we are suspended.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v1->v2: on HW reset just assign chanctx to monitor sdata

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c136050..7893913 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -373,28 +373,30 @@ static int ieee80211_add_virtual_monitor(struct ieee80211_local *local)
 
 	ieee80211_set_default_queues(sdata);
 
-	ret = drv_add_interface(local, sdata);
-	if (WARN_ON(ret)) {
-		/* ok .. stupid driver, it asked for this! */
-		kfree(sdata);
-		goto out_unlock;
-	}
+	if (!local->suspended) {
+		ret = drv_add_interface(local, sdata);
+		if (WARN_ON(ret)) {
+			/* ok .. stupid driver, it asked for this! */
+			goto out_free;
+		}
 
-	ret = ieee80211_check_queues(sdata);
-	if (ret) {
-		kfree(sdata);
-		goto out_unlock;
-	}
+			ret = ieee80211_check_queues(sdata);
+		if (ret)
+			goto out_remove;
 
-	ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
-					IEEE80211_CHANCTX_EXCLUSIVE);
-	if (ret) {
-		drv_remove_interface(local, sdata);
-		kfree(sdata);
-		goto out_unlock;
+		ret = ieee80211_vif_use_channel(sdata, &local->monitor_chandef,
+						IEEE80211_CHANCTX_EXCLUSIVE);
+		if (ret)
+			goto out_remove;
 	}
 
 	rcu_assign_pointer(local->monitor_sdata, sdata);
+	goto out_unlock;
+
+ out_remove:
+	drv_remove_interface(local, sdata);
+ out_free:
+	kfree(sdata);
  out_unlock:
 	mutex_unlock(&local->iflist_mtx);
 	return ret;
@@ -417,9 +419,10 @@ static void ieee80211_del_virtual_monitor(struct ieee80211_local *local)
 	rcu_assign_pointer(local->monitor_sdata, NULL);
 	synchronize_net();
 
-	ieee80211_vif_release_channel(sdata);
-
-	drv_remove_interface(local, sdata);
+	if (!local->suspended) {
+		ieee80211_vif_release_channel(sdata);
+		drv_remove_interface(local, sdata);
+	}
 
 	kfree(sdata);
  out_unlock:
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index b471a67..dbe42ea 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -100,8 +100,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
 	}
 
 	sdata = rtnl_dereference(local->monitor_sdata);
-	if (sdata)
+	if (sdata) {
+		ieee80211_vif_release_channel(sdata);
 		drv_remove_interface(local, sdata);
+	}
 
 	/*
 	 * We disconnected on all interfaces before suspend, all channel
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index b7a856e..20e0d0e 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1470,8 +1470,21 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 	}
 
 	sdata = rtnl_dereference(local->monitor_sdata);
-	if (sdata && ieee80211_sdata_running(sdata))
-		ieee80211_assign_chanctx(local, sdata);
+	if (sdata) {
+		if (local->suspended) {
+			res = ieee80211_vif_use_channel(sdata,
+						&local->monitor_chandef,
+						IEEE80211_CHANCTX_EXCLUSIVE);
+			if (WARN_ON(res)) {
+				drv_remove_interface(local, sdata);
+				rcu_assign_pointer(local->monitor_sdata, NULL);
+				synchronize_net();
+				kfree(sdata);
+			}
+		} else {
+			ieee80211_assign_chanctx(local, sdata);
+		}
+	}
 
 	/* add STAs back */
 	mutex_lock(&local->sta_mtx);

Re: [PATCH 1/2] mac80211: some fixes for ieee80211_do_stop while suspend

[Johannes Berg]

On Mon, 2013-03-11 at 18:01 +0100, Stanislaw Gruszka wrote:
> Is possible that we close interface while we are suspended, that
> can result warning like below (and some others similar):

> Patch try to avoid calling most of drv callbacks when stopping interface
> while suspended.

try :-)

> Some further work is probably needed to handle ROC, DFS, WDS and
> ieee80211_{add,del}_virtual_monitor . However patch should fix issues

Fixing either any of those would be pretty tricky, I think.

> in most common scenario, i.e. managed mode without any new futures.

I hope you're not into the stock market now ;-)


> -		ieee80211_configure_filter(local);
> +		/* configure filter latter (if not suspended) */

later

> -		mutex_unlock(&local->mtx);
> +		/* tell driver latter (if not suspended) */

later

> -		if (going_down)
> +		if (going_down && !local->suspended)
>  			drv_remove_interface(local, sdata);

I really wonder if there's not a better solution ... I'll think about it
a bit.

johannes

Re: [PATCH 1/2] mac80211: some fixes for ieee80211_do_stop while suspend

[Stanislaw Gruszka]

On Fri, Mar 15, 2013 at 04:44:30PM +0100, Johannes Berg wrote:
> On Mon, 2013-03-11 at 18:01 +0100, Stanislaw Gruszka wrote:
> > Is possible that we close interface while we are suspended, that
> > can result warning like below (and some others similar):
> 
> > Patch try to avoid calling most of drv callbacks when stopping interface
> > while suspended.
> 
> try :-)

It basically fix my test case, but I'm not sure if there are no other 
problems, not covered by this test.
 
> I really wonder if there's not a better solution ... I'll think about it
> a bit.

I thought about 2 solutions:

1) Do not drv_remove_interface() and drv_stop() on suspend. Need to
check and possibly rewrite drivers to work with that, so this is not
preferred solution for me.

2) Add "if (local->started && !local->suspended) check in some drv_
callbacks, IOW silently ignore callbacks when suspended. This looks
as even worse hack than this patch.

Stanislaw

Re: [PATCH 1/2] mac80211: some fixes for ieee80211_do_stop while suspend

[Johannes Berg]

On Mon, 2013-03-18 at 10:30 +0100, Stanislaw Gruszka wrote:

> I thought about 2 solutions:
> 
> 1) Do not drv_remove_interface() and drv_stop() on suspend. Need to
> check and possibly rewrite drivers to work with that, so this is not
> preferred solution for me.

Yeah, that's probably not feasible. Actually this is what happens with
WoWLAN today, but still. OTOH, it'd only have to be done for drivers
that even can hotplug :)

> 2) Add "if (local->started && !local->suspended) check in some drv_
> callbacks, IOW silently ignore callbacks when suspended. This looks
> as even worse hack than this patch.

Agree, that's not really a great idea either.

I was more thinking along the lines of seeing whether during suspend
it'd be possible to put the interface into a state where it's safe to
not do any more cleanups at all, and then just not really execute the
do_stop() at all if it's still in that suspended state. But that looks
equally difficult to do.

johannes