[PATCH] mac80211: fix some unforgotten items on suspend

[PATCH] mac80211: fix some unforgotten items on suspend

[Luis R. Rodriguez]

We forgot to clear items which stuffed things into the our
station's workqueues. In particular we forgot to deal with
the dynamic_ps_timer, ifmgd->timer, ifmgd->chswitch_timer.

While at it we go ahead and add a warning in ieee80211_sta_work()
if its run while the suspend->resume cycle is in effect. This
should not happen and if it does it would indicate there is
a bug lurking in either mac80211 or mac80211 drivers.

With this now wpa_supplicant doesn't blink when I go to suspend
and resume where as before there where issues with any of the items
in the workqueue running during the suspend->resume cycle. This
caused a lot of incorrect assumptions and would at times bring
back the device in incoherent, but mostly recoverable, states.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/ieee80211_i.h |    9 +++++++++
 net/mac80211/mlme.c        |   28 ++++++++++++++++++++++++----
 net/mac80211/pm.c          |   19 +++++++++++++++++++
 net/mac80211/util.c        |   11 ++++++++++-
 4 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 9d15147..3e60f46 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -609,6 +609,13 @@ struct ieee80211_local {
 	unsigned int filter_flags; /* FIF_* */
 	struct iw_statistics wstats;
 	bool tim_in_locked_section; /* see ieee80211_beacon_get() */
+	/*
+	 * suspended is true if we finished all the suspend _and_ we have
+	 * not yet come up from resume. This is to be used by mac80211
+	 * to ensure driver sanity during suspend and mac80211's own
+	 * sanity. It can eventually be used for WoW as well.
+	 */
+	bool suspended;
 	int tx_headroom; /* required headroom for hardware/radiotap */
 
 	/* Tasklet and skb queue to process calls from IRQ mode. All frames
@@ -994,6 +1001,7 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
 void ieee80211_remove_interfaces(struct ieee80211_local *local);
 u32 __ieee80211_recalc_idle(struct ieee80211_local *local);
 void ieee80211_recalc_idle(struct ieee80211_local *local);
+void ieee80211_sta_setup_sdata_timers(struct ieee80211_sub_if_data *sdata);
 
 /* tx handling */
 void ieee80211_clear_tx_pending(struct ieee80211_local *local);
@@ -1052,6 +1060,7 @@ int __ieee80211_suspend(struct ieee80211_hw *hw);
 
 static inline int __ieee80211_resume(struct ieee80211_hw *hw)
 {
+	hw_to_local(hw)->suspended = false;
 	return ieee80211_reconfig(hw_to_local(hw));
 }
 #else
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index ae03068..d891967 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2129,6 +2129,17 @@ static void ieee80211_sta_work(struct work_struct *work)
 
 	if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_STATION))
 		return;
+
+	/*
+	 * Nothing should have been stuffed into the workqueue during
+	 * the suspend->resume cycle. If this WARN is seen then there
+	 * is a bug with either the driver suspend or something in
+	 * mac80211 stuffing into the workqueue which we haven't yet
+	 * cleared during mac80211's suspend cycle.
+	 */
+	if (WARN_ON(local->suspended))
+		return;
+
 	ifmgd = &sdata->u.mgd;
 
 	while ((skb = skb_dequeue(&ifmgd->skb_queue)))
@@ -2196,6 +2207,18 @@ static void ieee80211_restart_sta_timer(struct ieee80211_sub_if_data *sdata)
 	}
 }
 
+void ieee80211_sta_setup_sdata_timers(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_if_managed *ifmgd;
+
+	ifmgd = &sdata->u.mgd;
+
+	setup_timer(&ifmgd->timer, ieee80211_sta_timer,
+		    (unsigned long) sdata);
+	setup_timer(&ifmgd->chswitch_timer, ieee80211_chswitch_timer,
+		    (unsigned long) sdata);
+}
+
 /* interface setup */
 void ieee80211_sta_setup_sdata(struct ieee80211_sub_if_data *sdata)
 {
@@ -2206,10 +2229,7 @@ void ieee80211_sta_setup_sdata(struct ieee80211_sub_if_data *sdata)
 	INIT_WORK(&ifmgd->work, ieee80211_sta_work);
 	INIT_WORK(&ifmgd->chswitch_work, ieee80211_chswitch_work);
 	INIT_WORK(&ifmgd->beacon_loss_work, ieee80211_beacon_loss_work);
-	setup_timer(&ifmgd->timer, ieee80211_sta_timer,
-		    (unsigned long) sdata);
-	setup_timer(&ifmgd->chswitch_timer, ieee80211_chswitch_timer,
-		    (unsigned long) sdata);
+	ieee80211_sta_setup_sdata_timers(sdata);
 	skb_queue_head_init(&ifmgd->skb_queue);
 
 	ifmgd->capab = WLAN_CAPABILITY_ESS;
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index 9d3d89a..20fc091 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -18,6 +18,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 
 	flush_workqueue(local->hw.workqueue);
 
+	/* Don't stuff our workqueue during suspend->resume cycle */
+	del_timer_sync(&local->dynamic_ps_timer);
+	cancel_work_sync(&local->dynamic_ps_enable_work);
+
 	/* disable keys */
 	list_for_each_entry(sdata, &local->interfaces, list)
 		ieee80211_disable_keys(sdata);
@@ -52,6 +56,18 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 
 	/* remove all interfaces */
 	list_for_each_entry(sdata, &local->interfaces, list) {
+		/*
+		 * This prevents us from stuffing our workqueue during
+		 * during the suspend->resume cycle.
+		 */
+		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+			struct ieee80211_if_managed *ifmgd;
+			ifmgd = &sdata->u.mgd;
+
+			del_timer_sync(&ifmgd->timer);
+			del_timer_sync(&ifmgd->chswitch_timer);
+		}
+
 		if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
 		    sdata->vif.type != NL80211_IFTYPE_MONITOR &&
 		    netif_running(sdata->dev)) {
@@ -60,11 +76,14 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 			conf.mac_addr = sdata->dev->dev_addr;
 			drv_remove_interface(local, &conf);
 		}
+
 	}
 
 	/* flush again, in case driver queued work */
 	flush_workqueue(local->hw.workqueue);
 
+	local->suspended = true;
+
 	/* stop hardware */
 	if (local->open_count) {
 		ieee80211_led_radio(local, false);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 0689a8f..8d3d6c8 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1121,12 +1121,21 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 	}
 
 	/* add back keys */
-	list_for_each_entry(sdata, &local->interfaces, list)
+	list_for_each_entry(sdata, &local->interfaces, list) {
 		if (netif_running(sdata->dev))
 			ieee80211_enable_keys(sdata);
+		if (local->suspended &&
+		    sdata->vif.type == NL80211_IFTYPE_STATION)
+			ieee80211_sta_setup_sdata_timers(sdata);
+	}
 
 	ieee80211_wake_queues_by_reason(hw,
 			IEEE80211_QUEUE_STOP_REASON_SUSPEND);
 
+	if (local->suspended) {
+		setup_timer(&local->dynamic_ps_timer,
+			    ieee80211_dynamic_ps_timer, (unsigned long) local);
+	}
+
 	return 0;
 }
-- 
1.6.0.6

Re: [PATCH] mac80211: fix some unforgotten items on suspend

[Luis R. Rodriguez]

On Thu, May 14, 2009 at 9:48 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> We forgot to clear items which stuffed things into the our
> station's workqueues. In particular we forgot to deal with
> the dynamic_ps_timer, ifmgd->timer, ifmgd->chswitch_timer.
>
> While at it we go ahead and add a warning in ieee80211_sta_work()
> if its run while the suspend->resume cycle is in effect. This
> should not happen and if it does it would indicate there is
> a bug lurking in either mac80211 or mac80211 drivers.
>
> With this now wpa_supplicant doesn't blink when I go to suspend
> and resume where as before there where issues with any of the items
> in the workqueue running during the suspend->resume cycle. This
> caused a lot of incorrect assumptions and would at times bring
> back the device in incoherent, but mostly recoverable, states.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

Please hold on this -- with ath9k with legacy APs I get crap
throughput after pm-suspend if it disassociates. Even worse, with 11n
APs if it disassociates I get a crash. I haven't seen the oops yet...
This is with ath9k PS patches as well but also with the mac80211
debugfs patches pending, HT pending patches (debugfs and HT paches
should have no effect), and the WoW patches (these should have not
have an effect)

  Luis

Re: [PATCH] mac80211: fix some unforgotten items on suspend

[Johannes Berg]

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

On Fri, 2009-05-15 at 00:48 -0400, Luis R. Rodriguez wrote:

[...]

Looks pretty good, some comments.

>  static inline int __ieee80211_resume(struct ieee80211_hw *hw)
>  {
> +	hw_to_local(hw)->suspended = false;
>  	return ieee80211_reconfig(hw_to_local(hw));

Shouldn't we do that only somewhere in the middle of reconfig? i.e.
after we actually reconfig the hw, but before we restart our timers?

> @@ -2196,6 +2207,18 @@ static void ieee80211_restart_sta_timer(struct ieee80211_sub_if_data *sdata)
>  	}
>  }
>  
> +void ieee80211_sta_setup_sdata_timers(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_if_managed *ifmgd;
> +
> +	ifmgd = &sdata->u.mgd;
> +
> +	setup_timer(&ifmgd->timer, ieee80211_sta_timer,
> +		    (unsigned long) sdata);
> +	setup_timer(&ifmgd->chswitch_timer, ieee80211_chswitch_timer,
> +		    (unsigned long) sdata);
> +}

That doesn't seem necessary? The timers should still be set up after you
delete them.

> +++ b/net/mac80211/pm.c
> @@ -18,6 +18,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>  
>  	flush_workqueue(local->hw.workqueue);
>  
> +	/* Don't stuff our workqueue during suspend->resume cycle */
> +	del_timer_sync(&local->dynamic_ps_timer);
> +	cancel_work_sync(&local->dynamic_ps_enable_work);
> +

ack, but you forgot to add a comment why dynamic_ps_disable_work doesn't
need to be handled (actually it does need to handled with the current
code but shouldn't be necessary... up for a challenge? -- why? and how
to fix that better than handling it here?)

If you ever suspend in AP mode there's also an issue with each station's
cleanup timer, and for mesh each station's plink timer.

> @@ -52,6 +56,18 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>  
>  	/* remove all interfaces */
>  	list_for_each_entry(sdata, &local->interfaces, list) {
> +		/*
> +		 * This prevents us from stuffing our workqueue during
> +		 * during the suspend->resume cycle.
> +		 */
> +		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> +			struct ieee80211_if_managed *ifmgd;
> +			ifmgd = &sdata->u.mgd;
> +
> +			del_timer_sync(&ifmgd->timer);
> +			del_timer_sync(&ifmgd->chswitch_timer);
> +		}

Might make sense to use a switch statement, put the break; from
netif_running after it and for the two types the driver doesn't care
into it, like this
		if (!netif_running())
			continue;

		switch (type) {
		case station:
			del_timer_sync etc
			break;
		case ap_vlan:
		case monitor:
			continue;
		case ...
		}

		conf stuff

because you forgot IBSS and mesh.

> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -1121,12 +1121,21 @@ int ieee80211_reconfig(struct ieee80211_local *local)
>  	}
>  
>  	/* add back keys */
> -	list_for_each_entry(sdata, &local->interfaces, list)
> +	list_for_each_entry(sdata, &local->interfaces, list) {
>  		if (netif_running(sdata->dev))
>  			ieee80211_enable_keys(sdata);
> +		if (local->suspended &&
> +		    sdata->vif.type == NL80211_IFTYPE_STATION)
> +			ieee80211_sta_setup_sdata_timers(sdata);
> +	}

I don't think that helps, like I said the timers are still set up, you
need to actually queue the work now. However that kinda sucks for small
suspend times. Not really sure what to do.

What we should do is probably add new functions in mlme.c, ibss.c,
mesh.c for suspend() and resume() that contain all this logic instead of
putting it here -- that way mesh is easier to handle without ifdefs in
the code too.
 
> +	if (local->suspended) {
> +		setup_timer(&local->dynamic_ps_timer,
> +			    ieee80211_dynamic_ps_timer, (unsigned long) local);
> +	}
> +

I think that one is completely unnecessary since now traffic is flowing
which will call mod_timer anyway.

johannes

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

Re: [PATCH] mac80211: fix some unforgotten items on suspend

[Johannes Berg]

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

On Fri, 2009-05-15 at 10:22 +0200, Johannes Berg wrote:

> > +++ b/net/mac80211/pm.c
> > @@ -18,6 +18,10 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
> >  
> >  	flush_workqueue(local->hw.workqueue);
> >  
> > +	/* Don't stuff our workqueue during suspend->resume cycle */
> > +	del_timer_sync(&local->dynamic_ps_timer);
> > +	cancel_work_sync(&local->dynamic_ps_enable_work);
> > +
> 
> ack, but you forgot to add a comment why dynamic_ps_disable_work doesn't
> need to be handled (actually it does need to handled with the current
> code but shouldn't be necessary... up for a challenge? -- why? and how
> to fix that better than handling it here?)
> 
> If you ever suspend in AP mode there's also an issue with each station's
> cleanup timer, and for mesh each station's plink timer.

Hmm, actually, this isn't safe. We might still be receiving frames at
this point, which would start the timer again.

> I don't think that helps, like I said the timers are still set up, you
> need to actually queue the work now. However that kinda sucks for small
> suspend times. Not really sure what to do.

Instead of doing the setup_timer dance you were doing, we should simply
do add_timer /iff/ del_timer_sync() returned 1, because that means we
actually cancelled the timer.

johannes


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