[PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Wey-Yi Guy]

When uCode error condition detected, driver try to perform either
rf reset or firmware reload in order bring device back to
working condition.

If rf reset is required and scan is in process, there is no need
to issue rf reset since scan already reset the rf.

If firmware reload is required and scan is in process, skip the
reload request. There is a possibility firmware reload during
scan cause problem.

[  485.804046] WARNING: at net/mac80211/main.c:310 ieee80211_restart_hw+0x28/0x62()
[  485.804049] Hardware name: Latitude E6400
[  485.804052] ieee80211_restart_hw called with hardware scan in progress
[  485.804054] Modules linked in: iwlagn iwlcore bnep sco rfcomm l2cap crc16 bluetooth [last unloaded: iwlcore]
[  485.804069] Pid: 812, comm: kworker/u:3 Tainted: G        W   2.6.36-rc3-wl+ #74
[  485.804072] Call Trace:
[  485.804079]  [<c103019a>] warn_slowpath_common+0x60/0x75
[  485.804084]  [<c1030213>] warn_slowpath_fmt+0x26/0x2a
[  485.804089]  [<c145da67>] ieee80211_restart_hw+0x28/0x62
[  485.804102]  [<f8b35dc6>] iwl_bg_restart+0x113/0x150 [iwlagn]
[  485.804108]  [<c10415d5>] process_one_work+0x181/0x25c
[  485.804119]  [<f8b35cb3>] ? iwl_bg_restart+0x0/0x150 [iwlagn]
[  485.804124]  [<c104190a>] worker_thread+0xf9/0x1f2
[  485.804128]  [<c1041811>] ? worker_thread+0x0/0x1f2
[  485.804133]  [<c10451b0>] kthread+0x64/0x69
[  485.804137]  [<c104514c>] ? kthread+0x0/0x69
[  485.804141]  [<c1002df6>] kernel_thread_helper+0x6/0x10
[  485.804145] ---[ end trace 3d4ebdc02d524bbb ]---
[  485.804153] Pid: 812, comm: kworker/u:3 Tainted: G        W   2.6.36-rc3-wl+ #74
[  485.804156] Call Trace:
[  485.804161]  [<c145da9b>] ? ieee80211_restart_hw+0x5c/0x62
[  485.804172]  [<f8b35dcb>] iwl_bg_restart+0x118/0x150 [iwlagn]
[  485.804177]  [<c10415d5>] process_one_work+0x181/0x25c
[  485.804188]  [<f8b35cb3>] ? iwl_bg_restart+0x0/0x150 [iwlagn]
[  485.804192]  [<c104190a>] worker_thread+0xf9/0x1f2
[  485.804197]  [<c1041811>] ? worker_thread+0x0/0x1f2
[  485.804201]  [<c10451b0>] kthread+0x64/0x69
[  485.804205]  [<c104514c>] ? kthread+0x0/0x69
[  485.804209]  [<c1002df6>] kernel_thread_helper+0x6/0x10

Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
---
this patch is also available from wireless-2.6 branch on
 git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-2.6.git

 drivers/net/wireless/iwlwifi/iwl-core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 07dbc27..e23c406 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -2613,6 +2613,11 @@ int iwl_force_reset(struct iwl_priv *priv, int mode, bool external)
 	if (test_bit(STATUS_EXIT_PENDING, &priv->status))
 		return -EINVAL;
 
+	if (test_bit(STATUS_SCANNING, &priv->status)) {
+		IWL_DEBUG_INFO(priv, "scan in progress.\n");
+		return -EINVAL;
+	}
+
 	if (mode >= IWL_MAX_FORCE_RESET) {
 		IWL_DEBUG_INFO(priv, "invalid reset request.\n");
 		return -EINVAL;
-- 
1.7.0.4

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Stanislaw Gruszka]

Hi Wey

On Fri, 17 Sep 2010 14:24:17 -0700
Wey-Yi Guy <wey-yi.w.guy@intel.com> wrote:
> When uCode error condition detected, driver try to perform either
> rf reset or firmware reload in order bring device back to
> working condition.
> 
> If rf reset is required and scan is in process, there is no need
> to issue rf reset since scan already reset the rf.

Yes, and that is already handled by iwl_scan_initiate().

> If firmware reload is required and scan is in process, skip the
> reload request. There is a possibility firmware reload during
> scan cause problem.

If we skip restart request now, next will be scheduled lately (correct?,
I think there are firmware reset requests that are not repeatable). But we
still will have scan pending since firmware is in bad shape and will not
finish scan. So until scan_check delayed work (7s) will not finish scan,
will not be able to reset firmware. I do not think that is what we want.
I think patch is good for .36, but after my current scan patches, it is
not be needed and actually it should be reverted (see below).

> [  485.804046] WARNING: at net/mac80211/main.c:310 ieee80211_restart_hw+0x28/0x62()
> [  485.804049] Hardware name: Latitude E6400
> [  485.804052] ieee80211_restart_hw called with hardware scan in progress
> [  485.804054] Modules linked in: iwlagn iwlcore bnep sco rfcomm l2cap crc16 bluetooth [last unloaded: iwlcore]
> [  485.804069] Pid: 812, comm: kworker/u:3 Tainted: G        W   2.6.36-rc3-wl+ #74
> [  485.804072] Call Trace:
> [  485.804079]  [<c103019a>] warn_slowpath_common+0x60/0x75ieee80211_restart_hw
> [  485.804084]  [<c1030213>] warn_slowpath_fmt+0x26/0x2a
> [  485.804089]  [<c145da67>] ieee80211_restart_hw+0x28/0x62
> [  485.804102]  [<f8b35dc6>] iwl_bg_restart+0x113/0x150 [iwlagn]

In iwl_bg_restart() we cancel scan. First we try to send abort command
via __iwl_down() ->  iwl_scan_cancel_timeout() -> iwl_do_scan_abort().
If sending abort command fail we will complete scan in mac80211, otherwise
if firmware do not finish scan, we will complete scan in
iwl_cancel_deferred_work() -> iwl_cancel_scan_deferred_work(). Hence we should
be safe.

So why we can see this warning? During my testing I saw it also.
There is race regarding SCAN_HW_SCANNING bit, usually we set/clear
this bit under local->mtx, but not in ieee80211_restart_hw()

cpu0						cpu1
__ieee80211_start_scan
  __set_bit(SCAN_HW_SCANNING, &local->scanning);
  
						iwl_bg_restart()
						  ieee80211_restart_hw()
						    WARN
  
   drv_hw_scan
     iwl_mac_hw_scan
     (OK, fail new scan, return error)
   
   local->scanning = 0;

So nothing wrong will happen except printing a call trace. If we want fix that
I would suggest patch like this:

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 7c85426..31993c3 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -302,16 +302,20 @@ static void ieee80211_restart_work(struct work_struct *work)
 void ieee80211_restart_hw(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
+	bool sw_scan = false;
 
 	trace_api_restart_hw(local);
 
 	/* wait for scan work complete */
 	flush_workqueue(local->workqueue);
 
+	mutex_lock(&local->mtx);
 	WARN(test_bit(SCAN_HW_SCANNING, &local->scanning),
 		"%s called with hardware scan in progress\n", __func__);
+	sw_scan = test_bit(SCAN_SW_SCANNING, &local->scanning);
+	mutex_unlock(&local->mtx);
 
-	if (unlikely(test_bit(SCAN_SW_SCANNING, &local->scanning)))
+	if (unlikely(sw_scan))
 		ieee80211_scan_cancel(local);
 
 	/* use this reason, ieee80211_reconfig will unblock it */

Or just leave code as is, many (harmless) warning's can be printed when restarting
iwlwifi firmware.

Stanislaw

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Guy, Wey-Yi]

Hi Gruszka,

On Wed, 2010-09-22 at 01:57 -0700, Stanislaw Gruszka wrote:
> Hi Wey
> 
> On Fri, 17 Sep 2010 14:24:17 -0700
> Wey-Yi Guy <wey-yi.w.guy@intel.com> wrote:
> > When uCode error condition detected, driver try to perform either
> > rf reset or firmware reload in order bring device back to
> > working condition.
> > 
> > If rf reset is required and scan is in process, there is no need
> > to issue rf reset since scan already reset the rf.
> 
> Yes, and that is already handled by iwl_scan_initiate().
> 
> > If firmware reload is required and scan is in process, skip the
> > reload request. There is a possibility firmware reload during
> > scan cause problem.
> 
> If we skip restart request now, next will be scheduled lately (correct?,

That is correct, if we still encounter the problem.

> I think there are firmware reset requests that are not repeatable). But we
> still will have scan pending since firmware is in bad shape and will not
> finish scan. So until scan_check delayed work (7s) will not finish scan,
> will not be able to reset firmware. I do not think that is what we want.
> I think patch is good for .36, but after my current scan patches, it is
> not be needed and actually it should be reverted (see below).
> 
Agree, with your recent patch, it improve a lot on how scan abort work.
I still don't like the warning, but like you say, it might be ok since
we reloading firmware.

I will ask John to revert the patch

Thanks
Wey

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Johannes Berg]

On Wed, 2010-09-22 at 10:57 +0200, Stanislaw Gruszka wrote:

> So why we can see this warning? During my testing I saw it also.
> There is race regarding SCAN_HW_SCANNING bit, usually we set/clear
> this bit under local->mtx, but not in ieee80211_restart_hw()
> 
> cpu0						cpu1
> __ieee80211_start_scan
>   __set_bit(SCAN_HW_SCANNING, &local->scanning);
>   
> 						iwl_bg_restart()
> 						  ieee80211_restart_hw()
> 						    WARN

Why can't iwlwifi's bg_restart() force-abort the scan? It'll be aborted
anyway...

johannes

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Stanislaw Gruszka]

On Wed, 22 Sep 2010 16:39:57 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Wed, 2010-09-22 at 10:57 +0200, Stanislaw Gruszka wrote:
> 
> > So why we can see this warning? During my testing I saw it also.
> > There is race regarding SCAN_HW_SCANNING bit, usually we set/clear
> > this bit under local->mtx, but not in ieee80211_restart_hw()
> > 
> > cpu0						cpu1
> > __ieee80211_start_scan
> >   __set_bit(SCAN_HW_SCANNING, &local->scanning);
> >   
> > 						iwl_bg_restart()
> > 						  ieee80211_restart_hw()
> > 						    WARN
> 
> Why can't iwlwifi's bg_restart() force-abort the scan? It'll be aborted
> anyway...

Yes, bg_restart() force scan abort, but not if no scan is pending. It looks
like that:

cpu0						cpu1

						iwl_bg_restart()
						  __iwl_down()
						     iwl_scan_cancel_timeout
						     (do nothing since no pending scan)
			  
__ieee80211_start_scan
  __set_bit(SCAN_HW_SCANNING, &local->scanning);

						  ieee80211_restart_hw()
						    WARN
  
   drv_hw_scan
     iwl_mac_hw_scan
     (OK, fail new scan, return error)
   
   local->scanning = 0;

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Johannes Berg]

On Wed, 2010-09-22 at 16:51 +0200, Stanislaw Gruszka wrote:

> Yes, bg_restart() force scan abort, but not if no scan is pending. It looks
> like that:
> 
> cpu0						cpu1
> 
> 						iwl_bg_restart()
> 						  __iwl_down()
> 						     iwl_scan_cancel_timeout
> 						     (do nothing since no pending scan)
> 			  
> __ieee80211_start_scan
>   __set_bit(SCAN_HW_SCANNING, &local->scanning);

Ah, ok, makes sense. I think some mutex trickery could possibly help
here though? But I'm not sure it's worth it...

johannes

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Stanislaw Gruszka]

On Wed, 22 Sep 2010 17:10:57 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Wed, 2010-09-22 at 16:51 +0200, Stanislaw Gruszka wrote:
> 
> > Yes, bg_restart() force scan abort, but not if no scan is pending. It looks
> > like that:
> > 
> > cpu0						cpu1
> > 
> > 						iwl_bg_restart()
> > 						  __iwl_down()
> > 						     iwl_scan_cancel_timeout
> > 						     (do nothing since no pending scan)
> > 			  
> > __ieee80211_start_scan
> >   __set_bit(SCAN_HW_SCANNING, &local->scanning);
> 
> Ah, ok, makes sense. I think some mutex trickery could possibly help
> here though? But I'm not sure it's worth it...

I showed possible patch earlier in this thread, I will rethink about it and
eventually post. Perhaps applying the patch is good idea, since we will stop
seeing the warning it that harmless case, and be sure something is really
wrong when see it again.

Stanislaw

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Johannes Berg]

On Wed, 2010-09-22 at 17:23 +0200, Stanislaw Gruszka wrote:

> > > Yes, bg_restart() force scan abort, but not if no scan is pending. It looks
> > > like that:
> > > 
> > > cpu0						cpu1
> > > 
> > > 						iwl_bg_restart()
> > > 						  __iwl_down()
> > > 						     iwl_scan_cancel_timeout
> > > 						     (do nothing since no pending scan)
> > > 			  
> > > __ieee80211_start_scan
> > >   __set_bit(SCAN_HW_SCANNING, &local->scanning);
> > 
> > Ah, ok, makes sense. I think some mutex trickery could possibly help
> > here though? But I'm not sure it's worth it...
> 
> I showed possible patch earlier in this thread, I will rethink about it and
> eventually post. Perhaps applying the patch is good idea, since we will stop
> seeing the warning it that harmless case, and be sure something is really
> wrong when see it again.

Well I'm thinking that iwlwifi will reject or queue a scan while it's
down, and if we do the check while holding the mutex that we also hold
here, it should close the race? And then we'll only accept the scan
again when we've been started by mac80211 again, or something?

johannes

Re: [PATCH wireless-2.6] iwlwifi: do not perferm force reset while doing scan

[Guy, Wey-Yi]

On Wed, 2010-09-22 at 08:23 -0700, Stanislaw Gruszka wrote:
> On Wed, 22 Sep 2010 17:10:57 +0200
> Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> > On Wed, 2010-09-22 at 16:51 +0200, Stanislaw Gruszka wrote:
> > 
> > > Yes, bg_restart() force scan abort, but not if no scan is pending. It looks
> > > like that:
> > > 
> > > cpu0						cpu1
> > > 
> > > 						iwl_bg_restart()
> > > 						  __iwl_down()
> > > 						     iwl_scan_cancel_timeout
> > > 						     (do nothing since no pending scan)
> > > 			  
> > > __ieee80211_start_scan
> > >   __set_bit(SCAN_HW_SCANNING, &local->scanning);
> > 
> > Ah, ok, makes sense. I think some mutex trickery could possibly help
> > here though? But I'm not sure it's worth it...
> 
> I showed possible patch earlier in this thread, I will rethink about it and
> eventually post. Perhaps applying the patch is good idea, since we will stop
> seeing the warning it that harmless case, and be sure something is really
> wrong when see it again.
> 

I will prefer not seeing "warning" even it is harmless in this case

Thanks
Wey