Re: [Ipw2100-devel] [PATCH] ipw2200: rework scan handling while associated

[Zhu Yi <yi.zhu@intel.com>]

On Thu, 2008-11-27 at 01:05 +0800, Helmut Schaa wrote:
> Could somebody please review the patch?
> Should I split the patch into three single patches?

Thank you for your patch. See my comments below.

> diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
> index c73173a..f17b5b2 100644
> --- a/drivers/net/wireless/ipw2x00/ipw2200.c
> +++ b/drivers/net/wireless/ipw2x00/ipw2200.c
> @@ -2310,10 +2310,10 @@ static void ipw_scan_check(void *data)
>  {
>  	struct ipw_priv *priv = data;
>  	if (priv->status & (STATUS_SCANNING | STATUS_SCAN_ABORTING)) {
> -		IPW_DEBUG_SCAN("Scan completion watchdog resetting "
> -			       "adapter after (%dms).\n",
> +		IPW_DEBUG_SCAN("Scan completion watchdog: aborting "
> +			       "scan after (%dms).\n",
>  			       jiffies_to_msecs(IPW_SCAN_CHECK_WATCHDOG));
> -		queue_work(priv->workqueue, &priv->adapter_restart);
> +		queue_work(priv->workqueue, &priv->abort_scan);
>  	}
>  }

We need something like the network Tx watchdog to prevent the firmware
stick forever when something is wrong during scan. At this time, no
command (include ABORT_SCAN) is going to be processed. We need something
can really move the firmware back to the correct state, that is the
adapter_restart. If you do see the watchdog files a lot, increasing
IPW_SCAN_CHECK_WATCHDOG should be the way to go.

> @@ -4346,7 +4346,8 @@ static void ipw_handle_missed_beacon(struct ipw_priv *priv,
>  		return;
>  	}
>  
> -	if (priv->status & STATUS_SCANNING) {
> +	if (priv->status & STATUS_SCANNING &&
> +	    missed_count > priv->cancel_scan_threshold) {
>  		/* Stop scan to keep fw from getting
>  		 * stuck (only if we aren't roaming --
>  		 * otherwise we'll never scan more than 2 or 3

I'm OK with this. Since you don't change priv->cancel_scan_threadhold
anywhere, can you use IPW_MB_SCAN_CANCEL_THRESHOLD directly? 

> @@ -6281,6 +6282,18 @@ static void ipw_add_scan_channels(struct ipw_priv *priv,
>  	}
>  }
>  
> +static int ipw_passive_dwell_time(struct ipw_priv *priv)
> +{
> +	/* staying on passive channels longer than the beacon interval causes
> +	 * the firmware to enter an infinite loop. Hence, don't stay on passive
> +	 * longer than the beacon interval.
> +	 */
> +	if (priv->status & STATUS_ASSOCIATED && priv->assoc_network->beacon_interval > 10)
> +		return priv->assoc_network->beacon_interval - 10;
> +	else
> +		return 120;
> +}

Is it still a problem with the above IPW_MB_SCAN_CANCEL_THRESHOLD used?
We assume most APs use 100ms beacon interval. But in case you are
associated with an AP which uses 20ms beacon interval, do you want to
set the passive_dwell time to 10ms here? 120ms makes sure we can at
least receive a beacon for normal APs.

BTW, another thing might help to resolve the scan-while-associate
problem. Currently we scan 5GHz band channels first, then 2.4GHz band
(see ipw_add_scan_channels). Normally most channels in 5GHz band are
passive ones. Thus they use longer dwell time. If we change to scan
2.4GHz band channels first, we can get more scan result before a scan
abort. Can you please try it?

Thanks,
-yi