[PATCH] mac80211: cancel scan in ieee80211_restart_hw

[PATCH] mac80211: cancel scan in ieee80211_restart_hw

[John W. Linville]

This function exists to clean-up after a hardware error or something
similar.  The restart is accomplished using the same infrastructure used
to resume after a suspend.  The suspend path cancels running scans, so
it seems appropriate to do that here as well.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/main.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 93194f6..8b43d85 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -305,7 +305,9 @@ void ieee80211_restart_hw(struct ieee80211_hw *hw)
 
 	trace_api_restart_hw(local);
 
-	/* use this reason, __ieee80211_resume will unblock it */
+	ieee80211_scan_cancel(local);
+
+	/* use this reason, ieee80211_reconfig will unblock it */
 	ieee80211_stop_queues_by_reason(hw,
 		IEEE80211_QUEUE_STOP_REASON_SUSPEND);
 
-- 
1.7.2.2

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw

[Johannes Berg]

On Mon, 2010-08-30 at 15:14 -0400, John W. Linville wrote:
> This function exists to clean-up after a hardware error or something
> similar.  The restart is accomplished using the same infrastructure used
> to resume after a suspend.  The suspend path cancels running scans, so
> it seems appropriate to do that here as well.

> -	/* use this reason, __ieee80211_resume will unblock it */
> +	ieee80211_scan_cancel(local);
> +
> +	/* use this reason, ieee80211_reconfig will unblock it */

Hmm, yes .. but how will this interact with hw scan?

johannes

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw

[John W. Linville]

On Mon, Aug 30, 2010 at 09:17:59PM +0200, Johannes Berg wrote:
> On Mon, 2010-08-30 at 15:14 -0400, John W. Linville wrote:
> > This function exists to clean-up after a hardware error or something
> > similar.  The restart is accomplished using the same infrastructure used
> > to resume after a suspend.  The suspend path cancels running scans, so
> > it seems appropriate to do that here as well.
> 
> > -	/* use this reason, __ieee80211_resume will unblock it */
> > +	ieee80211_scan_cancel(local);
> > +
> > +	/* use this reason, ieee80211_reconfig will unblock it */
> 
> Hmm, yes .. but how will this interact with hw scan?

You tell me -- I would think that calling ieee80211_restart_hw
would effectively abandon all hope of a hardware scan completing.
Should the caller call ieee80211_scan_completed(..., 1)?  Or should
we do that (or equivalent) here too?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw

[Johannes Berg]

On Mon, 2010-08-30 at 15:37 -0400, John W. Linville wrote:

> > > -	/* use this reason, __ieee80211_resume will unblock it */
> > > +	ieee80211_scan_cancel(local);
> > > +
> > > +	/* use this reason, ieee80211_reconfig will unblock it */
> > 
> > Hmm, yes .. but how will this interact with hw scan?
> 
> You tell me -- I would think that calling ieee80211_restart_hw
> would effectively abandon all hope of a hardware scan completing.
> Should the caller call ieee80211_scan_completed(..., 1)?  Or should
> we do that (or equivalent) here too?

Yeah, I guess the caller probably should since it may have internal
state that needs resetting for scans as well. Maybe we should
WARN_ON(scan in progress) here for the hw scan case, and do what you're
proposing only for SW scan?

johannes

[PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[John W. Linville]

This function exists to clean-up after a hardware error or something
similar.  The restart is accomplished using the same infrastructure used
to resume after a suspend.  The suspend path cancels running scans, so
it seems appropriate to do that here as well for software-based scans.
If a hardware-based scan is pending, issue a warning message since this
indicates that the drivers has failed to clean-up after itself.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/main.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 93194f6..a06b6ee 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -305,7 +305,13 @@ void ieee80211_restart_hw(struct ieee80211_hw *hw)
 
 	trace_api_restart_hw(local);
 
-	/* use this reason, __ieee80211_resume will unblock it */
+	WARN(test_bit(SCAN_HW_SCANNING, &local->scanning),
+		"%s called with hardware scan in progress\n", __func__);
+
+	if (unlikely(test_bit(SCAN_SW_SCANNING, &local->scanning)))
+		ieee80211_scan_cancel(local);
+
+	/* use this reason, ieee80211_reconfig will unblock it */
 	ieee80211_stop_queues_by_reason(hw,
 		IEEE80211_QUEUE_STOP_REASON_SUSPEND);
 
-- 
1.7.2.2

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[Johannes Berg]

On Tue, 2010-08-31 at 15:25 -0400, John W. Linville wrote:
> This function exists to clean-up after a hardware error or something
> similar.  The restart is accomplished using the same infrastructure used
> to resume after a suspend.  The suspend path cancels running scans, so
> it seems appropriate to do that here as well for software-based scans.
> If a hardware-based scan is pending, issue a warning message since this
> indicates that the drivers has failed to clean-up after itself.

Makes sense.

I guess unrelated to this, I wonder if we should warn in the suspend
case as well, rather than doing it unconditionally. Hmm.

johannes

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[John W. Linville]

On Wed, Sep 01, 2010 at 12:29:38PM +0200, Johannes Berg wrote:
> On Tue, 2010-08-31 at 15:25 -0400, John W. Linville wrote:
> > This function exists to clean-up after a hardware error or something
> > similar.  The restart is accomplished using the same infrastructure used
> > to resume after a suspend.  The suspend path cancels running scans, so
> > it seems appropriate to do that here as well for software-based scans.
> > If a hardware-based scan is pending, issue a warning message since this
> > indicates that the drivers has failed to clean-up after itself.
> 
> Makes sense.
> 
> I guess unrelated to this, I wonder if we should warn in the suspend
> case as well, rather than doing it unconditionally. Hmm.

Maybe so, but I'm not sure what path we would hook for that.
Doesn't wiphy_suspend run before the hardware bus suspend functions?
How would the driver now in advance to cancel the hardware scan?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[John W. Linville]

On Wed, Sep 01, 2010 at 02:58:57PM -0400, John W. Linville wrote:
> On Wed, Sep 01, 2010 at 12:29:38PM +0200, Johannes Berg wrote:
> > On Tue, 2010-08-31 at 15:25 -0400, John W. Linville wrote:
> > > This function exists to clean-up after a hardware error or something
> > > similar.  The restart is accomplished using the same infrastructure used
> > > to resume after a suspend.  The suspend path cancels running scans, so
> > > it seems appropriate to do that here as well for software-based scans.
> > > If a hardware-based scan is pending, issue a warning message since this
> > > indicates that the drivers has failed to clean-up after itself.
> > 
> > Makes sense.
> > 
> > I guess unrelated to this, I wonder if we should warn in the suspend
> > case as well, rather than doing it unconditionally. Hmm.
> 
> Maybe so, but I'm not sure what path we would hook for that.
> Doesn't wiphy_suspend run before the hardware bus suspend functions?
> How would the driver now in advance to cancel the hardware scan?

s/now/know/ -- English is hard...

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[Johannes Berg]

On Wed, 2010-09-01 at 14:58 -0400, John W. Linville wrote:
> On Wed, Sep 01, 2010 at 12:29:38PM +0200, Johannes Berg wrote:
> > On Tue, 2010-08-31 at 15:25 -0400, John W. Linville wrote:
> > > This function exists to clean-up after a hardware error or something
> > > similar.  The restart is accomplished using the same infrastructure used
> > > to resume after a suspend.  The suspend path cancels running scans, so
> > > it seems appropriate to do that here as well for software-based scans.
> > > If a hardware-based scan is pending, issue a warning message since this
> > > indicates that the drivers has failed to clean-up after itself.
> > 
> > Makes sense.
> > 
> > I guess unrelated to this, I wonder if we should warn in the suspend
> > case as well, rather than doing it unconditionally. Hmm.
> 
> Maybe so, but I'm not sure what path we would hook for that.
> Doesn't wiphy_suspend run before the hardware bus suspend functions?
> How would the driver now in advance to cancel the hardware scan?

Good point! I was just thinking that if we do a mac80211 cancel only,
then the scan request struct pointer that we gave to the driver earlier
suddenly becomes invalid.

johannes

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[John W. Linville]

On Wed, Sep 01, 2010 at 09:10:26PM +0200, Johannes Berg wrote:
> On Wed, 2010-09-01 at 14:58 -0400, John W. Linville wrote:
> > On Wed, Sep 01, 2010 at 12:29:38PM +0200, Johannes Berg wrote:
> > > On Tue, 2010-08-31 at 15:25 -0400, John W. Linville wrote:
> > > > This function exists to clean-up after a hardware error or something
> > > > similar.  The restart is accomplished using the same infrastructure used
> > > > to resume after a suspend.  The suspend path cancels running scans, so
> > > > it seems appropriate to do that here as well for software-based scans.
> > > > If a hardware-based scan is pending, issue a warning message since this
> > > > indicates that the drivers has failed to clean-up after itself.
> > > 
> > > Makes sense.
> > > 
> > > I guess unrelated to this, I wonder if we should warn in the suspend
> > > case as well, rather than doing it unconditionally. Hmm.
> > 
> > Maybe so, but I'm not sure what path we would hook for that.
> > Doesn't wiphy_suspend run before the hardware bus suspend functions?
> > How would the driver now in advance to cancel the hardware scan?
> 
> Good point! I was just thinking that if we do a mac80211 cancel only,
> then the scan request struct pointer that we gave to the driver earlier
> suddenly becomes invalid.

I see...how about we only cancel the software scans, then warn on
resume if the hardware scan is still running?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH] mac80211: only cancel software-based scans on suspend

[John W. Linville]

Otherwise the hardware scan handler could access an invalid scan request
structure.  The driver should cancel any pending hardware scans during
the suspend process anyway, so also add a warning if the hardware scan
is still pending when the device resumes.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/ieee80211_i.h |    6 ++++++
 net/mac80211/pm.c          |    3 ++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 16f7fb1..4e635e2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1186,6 +1186,12 @@ int __ieee80211_suspend(struct ieee80211_hw *hw);
 
 static inline int __ieee80211_resume(struct ieee80211_hw *hw)
 {
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	WARN(test_bit(SCAN_HW_SCANNING, &local->scanning),
+		"%s: resume with hardware scan still in progress\n",
+		wiphy_name(hw->wiphy));
+
 	return ieee80211_reconfig(hw_to_local(hw));
 }
 #else
diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index d287fde..ce671df 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -12,7 +12,8 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 	struct ieee80211_sub_if_data *sdata;
 	struct sta_info *sta;
 
-	ieee80211_scan_cancel(local);
+	if (unlikely(test_bit(SCAN_SW_SCANNING, &local->scanning)))
+		ieee80211_scan_cancel(local);
 
 	ieee80211_stop_queues_by_reason(hw,
 			IEEE80211_QUEUE_STOP_REASON_SUSPEND);
-- 
1.7.2.2

Re: [PATCH] mac80211: cancel scan in ieee80211_restart_hw if software scan pending

[Johannes Berg]

On Wed, 2010-09-01 at 16:17 -0400, John W. Linville wrote:

> > Good point! I was just thinking that if we do a mac80211 cancel only,
> > then the scan request struct pointer that we gave to the driver earlier
> > suddenly becomes invalid.
> 
> I see...how about we only cancel the software scans, then warn on
> resume if the hardware scan is still running?

That seems reasonable, although the driver can only cancel it after
mac80211 has already suspended, which is kinda odd (but should work
fine)

johannes