[PATCH 3.3] iwlwifi: always monitor for stuck queues

[PATCH 3.3] iwlwifi: always monitor for stuck queues

[Wey-Yi Guy]

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

If we only monitor while associated, the following
can happen:
 - we're associated, and the queue stuck check
   runs, setting the queue "touch" time to X
 - we disassociate, stopping the monitoring,
   which leaves the time set to X
 - almost 2s later, we associate, and enqueue
   a frame
 - before the frame is transmitted, we monitor
   for stuck queues, and find the time set to
   X, although it is now later than X + 2000ms,
   so we decide that the queue is stuck and
   erroneously restart the device

It happens more with P2P because there we can
go between associated/unassociated frequently.

Cc: stable@vger.kernel.org
Reported-by: Ben Cahill <ben.m.cahill@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
---
this patch will be also available from wireless branch on
 git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi.git

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

diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 7bcfa78..3abe9ed 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -1465,20 +1465,10 @@ void iwl_bg_watchdog(unsigned long data)
 	if (timeout == 0)
 		return;
 
-	/* monitor and check for stuck cmd queue */
-	if (iwl_check_stuck_queue(priv, priv->shrd->cmd_queue))
-		return;
-
-	/* monitor and check for other stuck queues */
-	if (iwl_is_any_associated(priv)) {
-		for (cnt = 0; cnt < hw_params(priv).max_txq_num; cnt++) {
-			/* skip as we already checked the command queue */
-			if (cnt == priv->shrd->cmd_queue)
-				continue;
-			if (iwl_check_stuck_queue(priv, cnt))
-				return;
-		}
-	}
+	/* monitor and check for stuck queues */
+	for (cnt = 0; cnt < hw_params(priv).max_txq_num; cnt++)
+		if (iwl_check_stuck_queue(priv, cnt))
+			return;
 
 	mod_timer(&priv->watchdog, jiffies +
 		  msecs_to_jiffies(IWL_WD_TICK(timeout)));
-- 
1.7.0.4

Re: [PATCH 3.3] iwlwifi: always monitor for stuck queues

[John W. Linville]

On Sun, Mar 04, 2012 at 08:50:46AM -0800, Wey-Yi Guy wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If we only monitor while associated, the following
> can happen:
>  - we're associated, and the queue stuck check
>    runs, setting the queue "touch" time to X
>  - we disassociate, stopping the monitoring,
>    which leaves the time set to X
>  - almost 2s later, we associate, and enqueue
>    a frame
>  - before the frame is transmitted, we monitor
>    for stuck queues, and find the time set to
>    X, although it is now later than X + 2000ms,
>    so we decide that the queue is stuck and
>    erroneously restart the device
> 
> It happens more with P2P because there we can
> go between associated/unassociated frequently.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Ben Cahill <ben.m.cahill@intel.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> ---

So what is the effect of this bug?  An unnecessary firmware restart?  How often does this happen?

It is very late in the 3.3 cycle.  Fixes should be for regressions, crashes, etc.

John

> this patch will be also available from wireless branch on
>  git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi.git
> 
>  drivers/net/wireless/iwlwifi/iwl-core.c |   18 ++++--------------
>  1 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
> index 7bcfa78..3abe9ed 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-core.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-core.c
> @@ -1465,20 +1465,10 @@ void iwl_bg_watchdog(unsigned long data)
>  	if (timeout == 0)
>  		return;
>  
> -	/* monitor and check for stuck cmd queue */
> -	if (iwl_check_stuck_queue(priv, priv->shrd->cmd_queue))
> -		return;
> -
> -	/* monitor and check for other stuck queues */
> -	if (iwl_is_any_associated(priv)) {
> -		for (cnt = 0; cnt < hw_params(priv).max_txq_num; cnt++) {
> -			/* skip as we already checked the command queue */
> -			if (cnt == priv->shrd->cmd_queue)
> -				continue;
> -			if (iwl_check_stuck_queue(priv, cnt))
> -				return;
> -		}
> -	}
> +	/* monitor and check for stuck queues */
> +	for (cnt = 0; cnt < hw_params(priv).max_txq_num; cnt++)
> +		if (iwl_check_stuck_queue(priv, cnt))
> +			return;
>  
>  	mod_timer(&priv->watchdog, jiffies +
>  		  msecs_to_jiffies(IWL_WD_TICK(timeout)));
> -- 
> 1.7.0.4
> 
> 

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

Re: [PATCH 3.3] iwlwifi: always monitor for stuck queues

[Johannes Berg]

On Mon, 2012-03-05 at 14:46 -0500, John W. Linville wrote:
> On Sun, Mar 04, 2012 at 08:50:46AM -0800, Wey-Yi Guy wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > If we only monitor while associated, the following
> > can happen:
> >  - we're associated, and the queue stuck check
> >    runs, setting the queue "touch" time to X
> >  - we disassociate, stopping the monitoring,
> >    which leaves the time set to X
> >  - almost 2s later, we associate, and enqueue
> >    a frame
> >  - before the frame is transmitted, we monitor
> >    for stuck queues, and find the time set to
> >    X, although it is now later than X + 2000ms,
> >    so we decide that the queue is stuck and
> >    erroneously restart the device
> > 
> > It happens more with P2P because there we can
> > go between associated/unassociated frequently.
> > 
> > Cc: stable@vger.kernel.org
> > Reported-by: Ben Cahill <ben.m.cahill@intel.com>
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> > ---
> 
> So what is the effect of this bug?  An unnecessary firmware restart?  How often does this happen?

Oh, guess I forgot that. The effect is essentially that the firmware
restarts unnecessarily, which causes strange things to happen.

> It is very late in the 3.3 cycle.  Fixes should be for regressions, crashes, etc.

Since unfortunately P2P isn't really stable yet and before P2P hardly
anyone saw this I suppose it doesn't matter all that much ...

johannes