[PATCH] mac80211: be more careful in suspend/resume

[PATCH] mac80211: be more careful in suspend/resume

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When suspending with all netdevs down, the device
is stopped but we still call a number of driver
callbacks that the driver might not expect. The
same happens during resume, we might call a few
callbacks without starting the driver. Fix this
by checking open_count around more things and
exiting quickly if it is 0.

Also, while at this I noticed that the coverage
class isn't reprogrammed after resume, so add
that.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/pm.c   |    3 ++
 net/mac80211/util.c |   54 ++++++++++++++++++++++++++--------------------------
 2 files changed, 31 insertions(+), 26 deletions(-)

--- a/net/mac80211/pm.c	2011-07-14 16:21:12.000000000 +0200
+++ b/net/mac80211/pm.c	2011-07-14 16:21:28.000000000 +0200
@@ -34,6 +34,9 @@ int __ieee80211_suspend(struct ieee80211
 	struct ieee80211_sub_if_data *sdata;
 	struct sta_info *sta;
 
+	if (!local->open_count)
+		goto suspend;
+
 	ieee80211_scan_cancel(local);
 
 	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
--- a/net/mac80211/util.c	2011-07-14 16:21:12.000000000 +0200
+++ b/net/mac80211/util.c	2011-07-14 16:21:28.000000000 +0200
@@ -1157,27 +1157,37 @@ int ieee80211_reconfig(struct ieee80211_
 	}
 #endif
 
-	/* restart hardware */
-	if (local->open_count) {
-		/*
-		 * Upon resume hardware can sometimes be goofy due to
-		 * various platform / driver / bus issues, so restarting
-		 * the device may at times not work immediately. Propagate
-		 * the error.
-		 */
-		res = drv_start(local);
-		if (res) {
-			WARN(local->suspended, "Hardware became unavailable "
-			     "upon resume. This could be a software issue "
-			     "prior to suspend or a hardware issue.\n");
-			return res;
-		}
+	/* setup fragmentation threshold */
+	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
+
+	/* setup RTS threshold */
+	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
+
+	/* reset coverage class */
+	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
 
-		ieee80211_led_radio(local, true);
-		ieee80211_mod_tpt_led_trig(local,
-					   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
+	/* everything else happens only if HW was up & running */
+	if (!local->open_count)
+		goto wake_up;
+
+	/*
+	 * Upon resume hardware can sometimes be goofy due to
+	 * various platform / driver / bus issues, so restarting
+	 * the device may at times not work immediately. Propagate
+	 * the error.
+	 */
+	res = drv_start(local);
+	if (res) {
+		WARN(local->suspended, "Hardware became unavailable "
+		     "upon resume. This could be a software issue "
+		     "prior to suspend or a hardware issue.\n");
+		return res;
 	}
 
+	ieee80211_led_radio(local, true);
+	ieee80211_mod_tpt_led_trig(local,
+				   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
+
 	/* add interfaces */
 	list_for_each_entry(sdata, &local->interfaces, list) {
 		if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
@@ -1201,12 +1211,6 @@ int ieee80211_reconfig(struct ieee80211_
 	}
 	mutex_unlock(&local->sta_mtx);
 
-	/* setup fragmentation threshold */
-	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
-
-	/* setup RTS threshold */
-	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
-
 	/* reconfigure hardware */
 	ieee80211_hw_config(local, ~0);
 
@@ -1287,9 +1291,7 @@ int ieee80211_reconfig(struct ieee80211_
 		if (ieee80211_sdata_running(sdata))
 			ieee80211_enable_keys(sdata);
 
-#ifdef CONFIG_PM
  wake_up:
-#endif
 	ieee80211_wake_queues_by_reason(hw,
 			IEEE80211_QUEUE_STOP_REASON_SUSPEND);
 

Re: [PATCH] mac80211: be more careful in suspend/resume

[Hauke Mehrtens]

Hi Johannes,

On 07/14/2011 04:24 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> When suspending with all netdevs down, the device
> is stopped but we still call a number of driver
> callbacks that the driver might not expect. The
> same happens during resume, we might call a few
> callbacks without starting the driver. Fix this
> by checking open_count around more things and
> exiting quickly if it is 0.
> 
> Also, while at this I noticed that the coverage
> class isn't reprogrammed after resume, so add
> that.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/mac80211/pm.c   |    3 ++
>  net/mac80211/util.c |   54 ++++++++++++++++++++++++++--------------------------
>  2 files changed, 31 insertions(+), 26 deletions(-)
> 
> --- a/net/mac80211/pm.c	2011-07-14 16:21:12.000000000 +0200
> +++ b/net/mac80211/pm.c	2011-07-14 16:21:28.000000000 +0200
> @@ -34,6 +34,9 @@ int __ieee80211_suspend(struct ieee80211
>  	struct ieee80211_sub_if_data *sdata;
>  	struct sta_info *sta;
>  
> +	if (!local->open_count)
> +		goto suspend;
> +
>  	ieee80211_scan_cancel(local);
>  
>  	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> --- a/net/mac80211/util.c	2011-07-14 16:21:12.000000000 +0200
> +++ b/net/mac80211/util.c	2011-07-14 16:21:28.000000000 +0200
> @@ -1157,27 +1157,37 @@ int ieee80211_reconfig(struct ieee80211_
>  	}
>  #endif
>  
> -	/* restart hardware */
> -	if (local->open_count) {
> -		/*
> -		 * Upon resume hardware can sometimes be goofy due to
> -		 * various platform / driver / bus issues, so restarting
> -		 * the device may at times not work immediately. Propagate
> -		 * the error.
> -		 */
> -		res = drv_start(local);
> -		if (res) {
> -			WARN(local->suspended, "Hardware became unavailable "
> -			     "upon resume. This could be a software issue "
> -			     "prior to suspend or a hardware issue.\n");
> -			return res;
> -		}
> +	/* setup fragmentation threshold */
> +	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
> +
> +	/* setup RTS threshold */
> +	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
> +
> +	/* reset coverage class */
> +	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);

Shouldn't this be
drv_set_coverage_class(local, hw->wiphy->coverage_class);

>  
> -		ieee80211_led_radio(local, true);
> -		ieee80211_mod_tpt_led_trig(local,
> -					   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
> +	/* everything else happens only if HW was up & running */
> +	if (!local->open_count)
> +		goto wake_up;
> +
> +	/*
> +	 * Upon resume hardware can sometimes be goofy due to
> +	 * various platform / driver / bus issues, so restarting
> +	 * the device may at times not work immediately. Propagate
> +	 * the error.
> +	 */
> +	res = drv_start(local);
> +	if (res) {
> +		WARN(local->suspended, "Hardware became unavailable "
> +		     "upon resume. This could be a software issue "
> +		     "prior to suspend or a hardware issue.\n");
> +		return res;
>  	}
>  
> +	ieee80211_led_radio(local, true);
> +	ieee80211_mod_tpt_led_trig(local,
> +				   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
> +
>  	/* add interfaces */
>  	list_for_each_entry(sdata, &local->interfaces, list) {
>  		if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
> @@ -1201,12 +1211,6 @@ int ieee80211_reconfig(struct ieee80211_
>  	}
>  	mutex_unlock(&local->sta_mtx);
>  
> -	/* setup fragmentation threshold */
> -	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
> -
> -	/* setup RTS threshold */
> -	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
> -
>  	/* reconfigure hardware */
>  	ieee80211_hw_config(local, ~0);
>  
> @@ -1287,9 +1291,7 @@ int ieee80211_reconfig(struct ieee80211_
>  		if (ieee80211_sdata_running(sdata))
>  			ieee80211_enable_keys(sdata);
>  
> -#ifdef CONFIG_PM
>   wake_up:
> -#endif
>  	ieee80211_wake_queues_by_reason(hw,
>  			IEEE80211_QUEUE_STOP_REASON_SUSPEND);
>  
> 
> 
> --
> 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

Re: [PATCH] mac80211: be more careful in suspend/resume

[Johannes Berg]

On Thu, 2011-07-14 at 16:42 +0200, Hauke Mehrtens wrote:

> > +	/* reset coverage class */
> > +	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
> 
> Shouldn't this be
> drv_set_coverage_class(local, hw->wiphy->coverage_class);

Err, of course, good catch. Thanks!

johannes

[PATCH v2] mac80211: be more careful in suspend/resume

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When suspending with all netdevs down, the device
is stopped but we still call a number of driver
callbacks that the driver might not expect. The
same happens during resume, we might call a few
callbacks without starting the driver. Fix this
by checking open_count around more things and
exiting quickly if it is 0.

Also, while at this I noticed that the coverage
class isn't reprogrammed after resume, so add
that.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v2: really fix coverage class (pointed out by Hauke)

 net/mac80211/pm.c   |    3 ++
 net/mac80211/util.c |   54 ++++++++++++++++++++++++++--------------------------
 2 files changed, 31 insertions(+), 26 deletions(-)

--- a/net/mac80211/pm.c	2011-07-14 16:21:12.000000000 +0200
+++ b/net/mac80211/pm.c	2011-07-14 16:21:28.000000000 +0200
@@ -34,6 +34,9 @@ int __ieee80211_suspend(struct ieee80211
 	struct ieee80211_sub_if_data *sdata;
 	struct sta_info *sta;
 
+	if (!local->open_count)
+		goto suspend;
+
 	ieee80211_scan_cancel(local);
 
 	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
--- a/net/mac80211/util.c	2011-07-14 16:21:12.000000000 +0200
+++ b/net/mac80211/util.c	2011-07-14 16:47:18.000000000 +0200
@@ -1157,27 +1157,37 @@ int ieee80211_reconfig(struct ieee80211_
 	}
 #endif
 
-	/* restart hardware */
-	if (local->open_count) {
-		/*
-		 * Upon resume hardware can sometimes be goofy due to
-		 * various platform / driver / bus issues, so restarting
-		 * the device may at times not work immediately. Propagate
-		 * the error.
-		 */
-		res = drv_start(local);
-		if (res) {
-			WARN(local->suspended, "Hardware became unavailable "
-			     "upon resume. This could be a software issue "
-			     "prior to suspend or a hardware issue.\n");
-			return res;
-		}
+	/* setup fragmentation threshold */
+	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
 
-		ieee80211_led_radio(local, true);
-		ieee80211_mod_tpt_led_trig(local,
-					   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
+	/* setup RTS threshold */
+	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
+
+	/* reset coverage class */
+	drv_set_coverage_class(local, hw->wiphy->coverage_class);
+
+	/* everything else happens only if HW was up & running */
+	if (!local->open_count)
+		goto wake_up;
+
+	/*
+	 * Upon resume hardware can sometimes be goofy due to
+	 * various platform / driver / bus issues, so restarting
+	 * the device may at times not work immediately. Propagate
+	 * the error.
+	 */
+	res = drv_start(local);
+	if (res) {
+		WARN(local->suspended, "Hardware became unavailable "
+		     "upon resume. This could be a software issue "
+		     "prior to suspend or a hardware issue.\n");
+		return res;
 	}
 
+	ieee80211_led_radio(local, true);
+	ieee80211_mod_tpt_led_trig(local,
+				   IEEE80211_TPT_LEDTRIG_FL_RADIO, 0);
+
 	/* add interfaces */
 	list_for_each_entry(sdata, &local->interfaces, list) {
 		if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
@@ -1201,12 +1211,6 @@ int ieee80211_reconfig(struct ieee80211_
 	}
 	mutex_unlock(&local->sta_mtx);
 
-	/* setup fragmentation threshold */
-	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
-
-	/* setup RTS threshold */
-	drv_set_rts_threshold(local, hw->wiphy->rts_threshold);
-
 	/* reconfigure hardware */
 	ieee80211_hw_config(local, ~0);
 
@@ -1287,9 +1291,7 @@ int ieee80211_reconfig(struct ieee80211_
 		if (ieee80211_sdata_running(sdata))
 			ieee80211_enable_keys(sdata);
 
-#ifdef CONFIG_PM
  wake_up:
-#endif
 	ieee80211_wake_queues_by_reason(hw,
 			IEEE80211_QUEUE_STOP_REASON_SUSPEND);
 

Re: [PATCH v2] mac80211: be more careful in suspend/resume

[Jouni Malinen]

On Thu, Jul 14, 2011 at 04:48:54PM +0200, Johannes Berg wrote:
> When suspending with all netdevs down, the device
> is stopped but we still call a number of driver
> callbacks that the driver might not expect. The
> same happens during resume, we might call a few
> callbacks without starting the driver. Fix this
> by checking open_count around more things and
> exiting quickly if it is 0.
> 
> Also, while at this I noticed that the coverage
> class isn't reprogrammed after resume, so add
> that.

Hmm.. This patch seems to pretty much kill P2P at least with hwsim. Many
of the EAPOL frames seem to be dropped at some point and the end result
of this is in the WPS provisioning taking 10 seconds (two 5 second
timeouts from the authenticator due to EAPOL frames dropped). This by
itself may not be enough to hit the group formation timeout, but similar
issue in 4-way handshake makes the authenticator miss msg 4/4 and hit
the state mismatch with supplicant having configured the PTK and
authenticator not..

I'm not yet sure what exactly is causing the issue, but this commit came
up in git bisect and reverting it from the current wireless-testing.git
made P2P work again in hwsim.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2] mac80211: be more careful in suspend/resume

[Jouni Malinen]

On Mon, Jul 25, 2011 at 10:57:47PM +0300, Jouni Malinen wrote:
> Hmm.. This patch seems to pretty much kill P2P at least with hwsim. Many
> of the EAPOL frames seem to be dropped at some point and the end result
> of this is in the WPS provisioning taking 10 seconds (two 5 second
> timeouts from the authenticator due to EAPOL frames dropped). This by
> itself may not be enough to hit the group formation timeout, but similar
> issue in 4-way handshake makes the authenticator miss msg 4/4 and hit
> the state mismatch with supplicant having configured the PTK and
> authenticator not..
> 
> I'm not yet sure what exactly is causing the issue, but this commit came
> up in git bisect and reverting it from the current wireless-testing.git
> made P2P work again in hwsim.

Actually, this does not make any sense.. Neither of the modified
functions are even called in my hwsim test. In addition, I can "fix" the
issue by adding a printk call into the beginning of (the not even
called) ieee80211_reconfig() function.

This sounds like some kind of build issue, but I've now reproduced this
issue multiple times and even after a full rebuild of the kernel to make
sure there are no build dependency issues etc. No idea what could be
causing this, but it looks very clear that some of the EAPOL frames get
dropped in some of the builds and just a single printk call seems to be
enough to move things enough to avoid this.. hwsim0 interface shows the
EAPOL frames, so they seem to be transmitted (or well, at least
mac80211_hwsim tries to transmit them), but the recipient does not
report them to wpa_supplicant (mac80211 dropping the frame?).

This is with x86_64 build and UML and on a new test host (i.e., new gcc
4.5.2-8ubuntu4 from Ubuntu 11.04), so my previous tests with hwsim may
have not had some odd issue that starts showing up now for whatever
reason.

-- 
Jouni Malinen                                            PGP id EFC895FA