[PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Stanislaw Gruszka]

In scan code completion send set TX power command again, as according
to comment it can be deferred during scan. Currently this comment is
only partially true - on 4965 device iwl4965_send_tx_power will fail
when scanning, but the new tx power value is lost. On other device
we do not defer setting tx power.

This patch change code to:
- save newly requested tx power value
- defer sending command to firmware when scanning on all devices
- add WARN_ONCE in all *_send_tx_power methods
- on 5xxx and 6xxx change to send command synchronously, to not
  start other commands when setting tx is pending, like we do for older
  devices

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/iwlwifi/iwl-3945.c     |    4 ++++
 drivers/net/wireless/iwlwifi/iwl-4965.c     |    8 ++------
 drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |    9 ++++++---
 drivers/net/wireless/iwlwifi/iwl-agn.c      |    1 +
 drivers/net/wireless/iwlwifi/iwl-core.c     |    8 ++++++++
 drivers/net/wireless/iwlwifi/iwl-dev.h      |    1 +
 drivers/net/wireless/iwlwifi/iwl-scan.c     |    2 +-
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    1 +
 8 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-3945.c b/drivers/net/wireless/iwlwifi/iwl-3945.c
index 176e525..6fd79f2 100644
--- a/drivers/net/wireless/iwlwifi/iwl-3945.c
+++ b/drivers/net/wireless/iwlwifi/iwl-3945.c
@@ -1451,6 +1451,10 @@ static int iwl3945_send_tx_power(struct iwl_priv *priv)
 	};
 	u16 chan;
 
+	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
+		      "TX Power requested while scanning!\n"))
+		return -EAGAIN;
+
 	chan = le16_to_cpu(priv->contexts[IWL_RXON_CTX_BSS].active.channel);
 
 	txpower.band = (priv->band == IEEE80211_BAND_5GHZ) ? 0 : 1;
diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index b207e3e..93e1696 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -1377,13 +1377,9 @@ static int iwl4965_send_tx_power(struct iwl_priv *priv)
 	u8 ctrl_chan_high = 0;
 	struct iwl_rxon_context *ctx = &priv->contexts[IWL_RXON_CTX_BSS];
 
-	if (test_bit(STATUS_SCANNING, &priv->status)) {
-		/* If this gets hit a lot, switch it to a BUG() and catch
-		 * the stack trace to find out who is calling this during
-		 * a scan. */
-		IWL_WARN(priv, "TX Power requested while scanning!\n");
+	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
+		      "TX Power requested while scanning!\n"))
 		return -EAGAIN;
-	}
 
 	band = priv->band == IEEE80211_BAND_2GHZ;
 
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
index c1a3898..b717ac0 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
@@ -496,6 +496,10 @@ int iwlagn_send_tx_power(struct iwl_priv *priv)
 	struct iwlagn_tx_power_dbm_cmd tx_power_cmd;
 	u8 tx_ant_cfg_cmd;
 
+	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
+		      "TX Power requested while scanning!\n"))
+		return -EAGAIN;
+
 	/* half dBm need to multiply */
 	tx_power_cmd.global_lmt = (s8)(2 * priv->tx_power_user_lmt);
 
@@ -522,9 +526,8 @@ int iwlagn_send_tx_power(struct iwl_priv *priv)
 	else
 		tx_ant_cfg_cmd = REPLY_TX_POWER_DBM_CMD;
 
-	return  iwl_send_cmd_pdu_async(priv, tx_ant_cfg_cmd,
-				       sizeof(tx_power_cmd), &tx_power_cmd,
-				       NULL);
+	return iwl_send_cmd_pdu(priv, tx_ant_cfg_cmd, sizeof(tx_power_cmd),
+				&tx_power_cmd);
 }
 
 void iwlagn_temperature(struct iwl_priv *priv)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 6771c40..0881452 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -4179,6 +4179,7 @@ static int iwl_init_drv(struct iwl_priv *priv)
 	 * this value will get overwritten by channel max power avg
 	 * from eeprom */
 	priv->tx_power_user_lmt = IWLAGN_TX_POWER_TARGET_POWER_MIN;
+	priv->tx_power_next = IWLAGN_TX_POWER_TARGET_POWER_MIN;
 
 	ret = iwl_init_channel_map(priv);
 	if (ret) {
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index b3efbe0..2a5d57f 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -1211,6 +1211,8 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
 	int ret = 0;
 	s8 prev_tx_power = priv->tx_power_user_lmt;
 
+	lockdep_assert_held(&priv->mutex);
+
 	if (tx_power < IWLAGN_TX_POWER_TARGET_POWER_MIN) {
 		IWL_WARN(priv,
 			 "Requested user TXPOWER %d below lower limit %d.\n",
@@ -1226,6 +1228,12 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
 		return -EINVAL;
 	}
 
+	if (test_bit(STATUS_SCANNING, &priv->status)) {
+		priv->tx_power_next = tx_power;
+		IWL_DEBUG_INFO(priv, "Deferring tx power set while scanning\n");
+		return 0;
+	}
+
 	if (priv->tx_power_user_lmt != tx_power)
 		force = true;
 
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index de43e13..4c09d21 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -1517,6 +1517,7 @@ struct iwl_priv {
 	s8 tx_power_user_lmt;
 	s8 tx_power_device_lmt;
 	s8 tx_power_lmt_in_half_dbm; /* max tx power in half-dBm format */
+	s8 tx_power_next;
 
 
 #ifdef CONFIG_IWLWIFI_DEBUG
diff --git a/drivers/net/wireless/iwlwifi/iwl-scan.c b/drivers/net/wireless/iwlwifi/iwl-scan.c
index 67da312..1424267 100644
--- a/drivers/net/wireless/iwlwifi/iwl-scan.c
+++ b/drivers/net/wireless/iwlwifi/iwl-scan.c
@@ -605,7 +605,7 @@ out_settings:
 
 	/* Since setting the TXPOWER may have been deferred while
 	 * performing the scan, fire one off */
-	iwl_set_tx_power(priv, priv->tx_power_user_lmt, true);
+	iwl_set_tx_power(priv, priv->tx_power_next, true);
 
 	priv->cfg->ops->utils->post_scan(priv);
 
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 980c609..9d0f736 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -3866,6 +3866,7 @@ static int iwl3945_init_drv(struct iwl_priv *priv)
 	priv->missed_beacon_threshold = IWL_MISSED_BEACON_THRESHOLD_DEF;
 
 	priv->tx_power_user_lmt = IWL_DEFAULT_TX_POWER;
+	priv->tx_power_next = IWL_DEFAULT_TX_POWER;
 
 	if (eeprom->version < EEPROM_3945_EEPROM_VERSION) {
 		IWL_WARN(priv, "Unsupported EEPROM version: 0x%04X\n",
-- 
1.7.1

[PATCH 2/2] iwlwifi: one less commit_rxon while scan

[Stanislaw Gruszka]

Almost anywhere in the code we avoid committing rxon while performing
scan, and make rxon commit when scan complete. Some current patches do
not follow that rule. We have that problem at least in
iwlagn_confirue_filter(), iwl_update_chain_flags() and
iwl_bg_bt_full_concurrency(). This patch try to resolve the first
function, formers are much harder to resolve so left them by now.

Since we do not commit directly in iwl3945_configure_filter, we can
also do the same for agn, so I just remove iwlcore_commit_rxon()
function and add a comment. Also change comment for iwl3945.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/iwlwifi/iwl-agn.c      |    6 +++++-
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 0881452..5a881c9 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -3984,7 +3984,11 @@ static void iwlagn_configure_filter(struct ieee80211_hw *hw,
 	for_each_context(priv, ctx) {
 		ctx->staging.filter_flags &= ~filter_nand;
 		ctx->staging.filter_flags |= filter_or;
-		iwlcore_commit_rxon(priv, ctx);
+
+		/*
+		 * Not committing directly because hardware can perform a scan,
+		 * but we'll eventually commit the filter flags change anyway.
+		 */
 	}
 
 	mutex_unlock(&priv->mutex);
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 9d0f736..4ff4b2e 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -3407,9 +3407,9 @@ static void iwl3945_configure_filter(struct ieee80211_hw *hw,
 	ctx->staging.filter_flags |= filter_or;
 
 	/*
-	 * Committing directly here breaks for some reason,
-	 * but we'll eventually commit the filter flags
-	 * change anyway.
+	 * Not committing directly because hardware can perfrom a scan,
+	 * but even if hw is ready, commiting here breaks for some reason,
+	 * we'll eventually commit the filter flags change anyway.
 	 */
 
 	mutex_unlock(&priv->mutex);
-- 
1.7.1

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Guy, Wey-Yi]

Hi Gruszka,

On Wed, 2010-10-13 at 06:39 -0700, Stanislaw Gruszka wrote:
> In scan code completion send set TX power command again, as according
> to comment it can be deferred during scan. Currently this comment is
> only partially true - on 4965 device iwl4965_send_tx_power will fail
> when scanning, but the new tx power value is lost. On other device
> we do not defer setting tx power.
> 
> This patch change code to:
> - save newly requested tx power value
> - defer sending command to firmware when scanning on all devices
> - add WARN_ONCE in all *_send_tx_power methods
> - on 5xxx and 6xxx change to send command synchronously, to not
>   start other commands when setting tx is pending, like we do for older
>   devices
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-3945.c     |    4 ++++
>  drivers/net/wireless/iwlwifi/iwl-4965.c     |    8 ++------
>  drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |    9 ++++++---
>  drivers/net/wireless/iwlwifi/iwl-agn.c      |    1 +
>  drivers/net/wireless/iwlwifi/iwl-core.c     |    8 ++++++++
>  drivers/net/wireless/iwlwifi/iwl-dev.h      |    1 +
>  drivers/net/wireless/iwlwifi/iwl-scan.c     |    2 +-
>  drivers/net/wireless/iwlwifi/iwl3945-base.c |    1 +
>  8 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-3945.c b/drivers/net/wireless/iwlwifi/iwl-3945.c
> index 176e525..6fd79f2 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-3945.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-3945.c
> @@ -1451,6 +1451,10 @@ static int iwl3945_send_tx_power(struct iwl_priv *priv)
>  	};
>  	u16 chan;
>  
> +	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
> +		      "TX Power requested while scanning!\n"))
> +		return -EAGAIN;
> +
>  	chan = le16_to_cpu(priv->contexts[IWL_RXON_CTX_BSS].active.channel);
>  
>  	txpower.band = (priv->band == IEEE80211_BAND_5GHZ) ? 0 : 1;
> diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
> index b207e3e..93e1696 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-4965.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
> @@ -1377,13 +1377,9 @@ static int iwl4965_send_tx_power(struct iwl_priv *priv)
>  	u8 ctrl_chan_high = 0;
>  	struct iwl_rxon_context *ctx = &priv->contexts[IWL_RXON_CTX_BSS];
>  
> -	if (test_bit(STATUS_SCANNING, &priv->status)) {
> -		/* If this gets hit a lot, switch it to a BUG() and catch
> -		 * the stack trace to find out who is calling this during
> -		 * a scan. */
> -		IWL_WARN(priv, "TX Power requested while scanning!\n");
> +	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
> +		      "TX Power requested while scanning!\n"))
>  		return -EAGAIN;
> -	}
>  
>  	band = priv->band == IEEE80211_BAND_2GHZ;
>  
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> index c1a3898..b717ac0 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> @@ -496,6 +496,10 @@ int iwlagn_send_tx_power(struct iwl_priv *priv)
>  	struct iwlagn_tx_power_dbm_cmd tx_power_cmd;
>  	u8 tx_ant_cfg_cmd;
>  
> +	if (WARN_ONCE(test_bit(STATUS_SCANNING, &priv->status),
> +		      "TX Power requested while scanning!\n"))
> +		return -EAGAIN;
> +
>  	/* half dBm need to multiply */
>  	tx_power_cmd.global_lmt = (s8)(2 * priv->tx_power_user_lmt);
>  
> @@ -522,9 +526,8 @@ int iwlagn_send_tx_power(struct iwl_priv *priv)
>  	else
>  		tx_ant_cfg_cmd = REPLY_TX_POWER_DBM_CMD;
>  
> -	return  iwl_send_cmd_pdu_async(priv, tx_ant_cfg_cmd,
> -				       sizeof(tx_power_cmd), &tx_power_cmd,
> -				       NULL);
> +	return iwl_send_cmd_pdu(priv, tx_ant_cfg_cmd, sizeof(tx_power_cmd),
> +				&tx_power_cmd);
>  }
>  
>  void iwlagn_temperature(struct iwl_priv *priv)
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
> index 6771c40..0881452 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
> @@ -4179,6 +4179,7 @@ static int iwl_init_drv(struct iwl_priv *priv)
>  	 * this value will get overwritten by channel max power avg
>  	 * from eeprom */
>  	priv->tx_power_user_lmt = IWLAGN_TX_POWER_TARGET_POWER_MIN;
> +	priv->tx_power_next = IWLAGN_TX_POWER_TARGET_POWER_MIN;
>  
>  	ret = iwl_init_channel_map(priv);
>  	if (ret) {
> diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
> index b3efbe0..2a5d57f 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-core.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-core.c
> @@ -1211,6 +1211,8 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
>  	int ret = 0;
>  	s8 prev_tx_power = priv->tx_power_user_lmt;
>  
> +	lockdep_assert_held(&priv->mutex);
> +
>  	if (tx_power < IWLAGN_TX_POWER_TARGET_POWER_MIN) {
>  		IWL_WARN(priv,
>  			 "Requested user TXPOWER %d below lower limit %d.\n",
> @@ -1226,6 +1228,12 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
>  		return -EINVAL;
>  	}
>  
> +	if (test_bit(STATUS_SCANNING, &priv->status)) {
> +		priv->tx_power_next = tx_power;
> +		IWL_DEBUG_INFO(priv, "Deferring tx power set while scanning\n");
> +		return 0;
> +	}
> +
>  	if (priv->tx_power_user_lmt != tx_power)
>  		force = true;
>  
> diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
> index de43e13..4c09d21 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-dev.h
> +++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
> @@ -1517,6 +1517,7 @@ struct iwl_priv {
>  	s8 tx_power_user_lmt;
>  	s8 tx_power_device_lmt;
>  	s8 tx_power_lmt_in_half_dbm; /* max tx power in half-dBm format */
> +	s8 tx_power_next;
>  
> 
>  #ifdef CONFIG_IWLWIFI_DEBUG
> diff --git a/drivers/net/wireless/iwlwifi/iwl-scan.c b/drivers/net/wireless/iwlwifi/iwl-scan.c
> index 67da312..1424267 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-scan.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-scan.c
> @@ -605,7 +605,7 @@ out_settings:
>  
>  	/* Since setting the TXPOWER may have been deferred while
>  	 * performing the scan, fire one off */
> -	iwl_set_tx_power(priv, priv->tx_power_user_lmt, true);
> +	iwl_set_tx_power(priv, priv->tx_power_next, true);
>  
Looks good, the only thing is if priv->tx_power_user_lmt ==
priv->tx_power_next, we don't even have to call set_tx_power, but I
guess calling it won't hurt, so its your decision check or not.

Thanks
Wey

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Stanislaw Gruszka]

Hi Wey

On Wed, Oct 13, 2010 at 08:18:20AM -0700, Guy, Wey-Yi wrote:
> >  		IWL_WARN(priv,
> >  			 "Requested user TXPOWER %d below lower limit %d.\n",
> > @@ -1226,6 +1228,12 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (test_bit(STATUS_SCANNING, &priv->status)) {
> > +		priv->tx_power_next = tx_power;
> > +		IWL_DEBUG_INFO(priv, "Deferring tx power set while scanning\n");
> > +		return 0;
> > +	}
> > +
[snip]
> >  	/* Since setting the TXPOWER may have been deferred while
> >  	 * performing the scan, fire one off */
> > -	iwl_set_tx_power(priv, priv->tx_power_user_lmt, true);
> > +	iwl_set_tx_power(priv, priv->tx_power_next, true);

Doh, this patch introduce a bug, when tx_power_next is not set (because
there was no scan during set_tx_power), but then when someone request
a scan and it finish, tx_power_next is written to hardware. I should
read tx_power using device sysfs not iwconfig when testing, will fix
that.

> Looks good, the only thing is if priv->tx_power_user_lmt ==
> priv->tx_power_next, we don't even have to call set_tx_power, but I
> guess calling it won't hurt, so its your decision check or not.

I'll will call iwl_set_tx_power( ... , false); what seems to be right
thing to do.

Stanislaw

Re: [PATCH 2/2] iwlwifi: one less commit_rxon while scan

[Stanislaw Gruszka]

On Wed, Oct 13, 2010 at 03:39:53PM +0200, Stanislaw Gruszka wrote:
> Almost anywhere in the code we avoid committing rxon while performing
> scan, and make rxon commit when scan complete. Some current patches do
> not follow that rule. We have that problem at least in
> iwlagn_confirue_filter(), iwl_update_chain_flags() and
> iwl_bg_bt_full_concurrency().

Any comments about iwl_update_chain_flags, iwl_bg_bt_full_concurrency ?
I would like to know how to deal with them, should we deffer commit_rxon
to scan complete, or cancel the scan. Or maybe this is no problem
at all, because in example committing rxon vs scan was problem of
older firmware?

Stanislaw

Re: [PATCH 2/2] iwlwifi: one less commit_rxon while scan

[Guy, Wey-Yi]

Hi Stanislaw,

On Thu, 2010-10-14 at 01:42 -0700, Stanislaw Gruszka wrote:
> On Wed, Oct 13, 2010 at 03:39:53PM +0200, Stanislaw Gruszka wrote:
> > Almost anywhere in the code we avoid committing rxon while performing
> > scan, and make rxon commit when scan complete. Some current patches do
> > not follow that rule. We have that problem at least in
> > iwlagn_confirue_filter(), iwl_update_chain_flags() and
> > iwl_bg_bt_full_concurrency().
> 
> Any comments about iwl_update_chain_flags, iwl_bg_bt_full_concurrency ?
> I would like to know how to deal with them, should we deffer commit_rxon
> to scan complete, or cancel the scan. Or maybe this is no problem
> at all, because in example committing rxon vs scan was problem of
> older firmware?
> 

iwl_update_chain_flags() is used when PSP mode change,
iwl_bg_bt_full_concurrency() is used only for BT coex and in BT full
concurrency mode. For both case, I do not see any reason we can not
defer to scan complete.

Thanks
Wey

Re: [PATCH 2/2] iwlwifi: one less commit_rxon while scan

[Stanislaw Gruszka]

Hello

On Thu, Oct 14, 2010 at 08:54:39AM -0700, Guy, Wey-Yi wrote:
> > Any comments about iwl_update_chain_flags, iwl_bg_bt_full_concurrency ?
> > I would like to know how to deal with them, should we deffer commit_rxon
> > to scan complete, or cancel the scan. Or maybe this is no problem
> > at all, because in example committing rxon vs scan was problem of
> > older firmware?
> > 
> 
> iwl_update_chain_flags() is used when PSP mode change,
> iwl_bg_bt_full_concurrency() is used only for BT coex and in BT full
> concurrency mode. For both case, I do not see any reason we can not
> defer to scan complete.

I think deferring could be a bit hard for iwl_update_chain_flags, since
according to the comments we need perform commit_rxon in order regarding
other commands sending to the device.

Looking more closely at this:

* iwl_update_chain_flags is called from:

1) iwl_chain_noise_calibration
2) iwlagn_bt_traffic_change_work 
3) iwl_power_update_mode

Ad 1) iwl_chain_noise_calibration:

Called only from iwl_bg_run_time_calib_work, we check STATUS_SCANNING
there.

Ad 2) iwlagn_bt_traffic_change_work 

Queued as work from iwl_rx_scan_complete_notif, since scan_completed
work is queued first we, should not have pending scan, as long as new
scan request do not income in the maintime. Adding STATUS_SCANNING 
check and return is probably what we need to prevent that corner case.

Ad 3) iwl_power_update_mode

Called from many places. In some of them we for sure not perform scanning,
because we do the check or scan cancel before, or this is alive start.
Fixing remaining calls and add WARNING in iwl_power_update_mode in is
what I plan to do.

* iwl_bg_bt_full_concurrency is queued from rs_bt_update_lq. 

I think we can check if scanning is pending and schedule that work for
late time, or set additional bit that we need to change full_concurrency,
and based on that bit do bg_bt_full_concurrency from scan_completed.

Stanislaw

Re: [PATCH 2/2] iwlwifi: one less commit_rxon while scan

[Guy, Wey-Yi]

Hi Stanislaw,

On Fri, 2010-10-15 at 07:51 -0700, Stanislaw Gruszka wrote:
> Hello
> 
> On Thu, Oct 14, 2010 at 08:54:39AM -0700, Guy, Wey-Yi wrote:
> > > Any comments about iwl_update_chain_flags, iwl_bg_bt_full_concurrency ?
> > > I would like to know how to deal with them, should we deffer commit_rxon
> > > to scan complete, or cancel the scan. Or maybe this is no problem
> > > at all, because in example committing rxon vs scan was problem of
> > > older firmware?
> > > 
> > 
> > iwl_update_chain_flags() is used when PSP mode change,
> > iwl_bg_bt_full_concurrency() is used only for BT coex and in BT full
> > concurrency mode. For both case, I do not see any reason we can not
> > defer to scan complete.
> 
> I think deferring could be a bit hard for iwl_update_chain_flags, since
> according to the comments we need perform commit_rxon in order regarding
> other commands sending to the device.
> 
> Looking more closely at this:
> 
> * iwl_update_chain_flags is called from:
> 
> 1) iwl_chain_noise_calibration
> 2) iwlagn_bt_traffic_change_work 
> 3) iwl_power_update_mode
> 
> Ad 1) iwl_chain_noise_calibration:
> 
> Called only from iwl_bg_run_time_calib_work, we check STATUS_SCANNING
> there.

This should only call once when we first associate, and we check
STATUS_SCANNING, justlike you mention, should be ok

> 
> Ad 2) iwlagn_bt_traffic_change_work 
> 
> Queued as work from iwl_rx_scan_complete_notif, since scan_completed
> work is queued first we, should not have pending scan, as long as new
> scan request do not income in the maintime. Adding STATUS_SCANNING 
> check and return is probably what we need to prevent that corner case.
> 
Agree

> Ad 3) iwl_power_update_mode
> 
> Called from many places. In some of them we for sure not perform scanning,
> because we do the check or scan cancel before, or this is alive start.
> Fixing remaining calls and add WARNING in iwl_power_update_mode in is
> what I plan to do.
> 
> * iwl_bg_bt_full_concurrency is queued from rs_bt_update_lq. 
> 
> I think we can check if scanning is pending and schedule that work for
> late time, or set additional bit that we need to change full_concurrency,
> and based on that bit do bg_bt_full_concurrency from scan_completed.
> 

overall, I agree with your plan, for both iwl_bg_bt_full_concurrency and
iwlagn_bt_traffic_change_work can be delay to scan complete.

Thanks
Wey

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Stanislaw Gruszka]

On Thu, Oct 14, 2010 at 10:32:05AM +0200, Stanislaw Gruszka wrote:
> > Looks good, the only thing is if priv->tx_power_user_lmt ==
> > priv->tx_power_next, we don't even have to call set_tx_power, but I
> > guess calling it won't hurt, so its your decision check or not.
> 
> I'll will call iwl_set_tx_power( ... , false); what seems to be right
> thing to do.

Set tx power have to be forces. Without that I get  

iwlagn 0000:40:00.0: low ack count detected, restart firmware
iwlagn 0000:40:00.0: On demand firmware reload
iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 1
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
iwlagn 0000:40:00.0: low ack count detected, restart firmware
iwlagn 0000:40:00.0: On demand firmware reload
iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
iwlagn 0000:40:00.0: low ack count detected, restart firmware
iwlagn 0000:40:00.0: On demand firmware reload
iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 4
iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0

on 5300 and device was unusable in general. Hence I will only comment
that forcing send tx power after scan is needed.

Stanislaw

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Guy, Wey-Yi]

On Thu, 2010-10-21 at 06:13 -0700, Stanislaw Gruszka wrote:
> On Thu, Oct 14, 2010 at 10:32:05AM +0200, Stanislaw Gruszka wrote:
> > > Looks good, the only thing is if priv->tx_power_user_lmt ==
> > > priv->tx_power_next, we don't even have to call set_tx_power, but I
> > > guess calling it won't hurt, so its your decision check or not.
> > 
> > I'll will call iwl_set_tx_power( ... , false); what seems to be right
> > thing to do.
> 
> Set tx power have to be forces. Without that I get  
> 
Thanks

Wey

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Stanislaw Gruszka]

On Thu, Oct 21, 2010 at 03:13:48PM +0200, Stanislaw Gruszka wrote:
> On Thu, Oct 14, 2010 at 10:32:05AM +0200, Stanislaw Gruszka wrote:
> > > Looks good, the only thing is if priv->tx_power_user_lmt ==
> > > priv->tx_power_next, we don't even have to call set_tx_power, but I
> > > guess calling it won't hurt, so its your decision check or not.
> > 
> > I'll will call iwl_set_tx_power( ... , false); what seems to be right
> > thing to do.
> 
> Set tx power have to be forces. Without that I get  
> 
> iwlagn 0000:40:00.0: low ack count detected, restart firmware
> iwlagn 0000:40:00.0: On demand firmware reload
> iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 1
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> iwlagn 0000:40:00.0: low ack count detected, restart firmware
> iwlagn 0000:40:00.0: On demand firmware reload
> iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> iwlagn 0000:40:00.0: low ack count detected, restart firmware
> iwlagn 0000:40:00.0: On demand firmware reload
> iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 4
> iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> 
> on 5300 and device was unusable in general. Hence I will only comment
> that forcing send tx power after scan is needed.

Not true. I have the same problem with vanilla 2.6.36 without any of my
patches (further investigation show this is 2.6.34 -> 2.6.35
regression). Problem happens only on 5Ghz, that confused me 
because NetworkManager in some cases connects to 5Ghz, in others to
2.4Ghz on my system.

Since my patches do not screw up (at least not that thing :) I'm going
to repost them, also further bisect this "low ack count detected" issue.

Stanislaw

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Guy, Wey-Yi]

Hi Stanislaw,

On Fri, 2010-10-22 at 05:56 -0700, Stanislaw Gruszka wrote:
> On Thu, Oct 21, 2010 at 03:13:48PM +0200, Stanislaw Gruszka wrote:
> > On Thu, Oct 14, 2010 at 10:32:05AM +0200, Stanislaw Gruszka wrote:
> > > > Looks good, the only thing is if priv->tx_power_user_lmt ==
> > > > priv->tx_power_next, we don't even have to call set_tx_power, but I
> > > > guess calling it won't hurt, so its your decision check or not.
> > > 
> > > I'll will call iwl_set_tx_power( ... , false); what seems to be right
> > > thing to do.
> > 
> > Set tx power have to be forces. Without that I get  
> > 
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 1
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 4
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > 
> > on 5300 and device was unusable in general. Hence I will only comment
> > that forcing send tx power after scan is needed.
> 
> Not true. I have the same problem with vanilla 2.6.36 without any of my
> patches (further investigation show this is 2.6.34 -> 2.6.35
> regression). Problem happens only on 5Ghz, that confused me 
> because NetworkManager in some cases connects to 5Ghz, in others to
> 2.4Ghz on my system.
> 
> Since my patches do not screw up (at least not that thing :) I'm going
> to repost them, also further bisect this "low ack count detected" issue.
> 

the error message indicate "low ack count" received by driver. not sure
how it happen? is it only happen on 5GHz, or the problem on both band?

Thanks
Wey

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Stanislaw Gruszka]

Hello

On Fri, Oct 22, 2010 at 07:39:27AM -0700, Guy, Wey-Yi wrote:
> > Not true. I have the same problem with vanilla 2.6.36 without any of my
> > patches (further investigation show this is 2.6.34 -> 2.6.35
> > regression). Problem happens only on 5Ghz, that confused me 
> > because NetworkManager in some cases connects to 5Ghz, in others to
> > 2.4Ghz on my system.
> > 
> > Since my patches do not screw up (at least not that thing :) I'm going
> > to repost them, also further bisect this "low ack count detected" issue.
> > 
> 
> the error message indicate "low ack count" received by driver. not sure
> how it happen? is it only happen on 5GHz, or the problem on both band?

Only on 5GHz and iwl 5300:

40:00.0 Network controller [0280]: Intel Corporation Ultimate N WiFi Link 5300 [8086:4235]
	Subsystem: Intel Corporation Device [8086:1001]
	Physical Slot: 3
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 91
	Region 0: Memory at f3100000 (64-bit, non-prefetchable) [size=8K]
	Capabilities: [c8] Power Management version 3
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee0f00c  Data: 4193
	Capabilities: [e0] Express (v1) Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 unlimited
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal+ Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ FLReset-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
		LnkCap:	Port #1, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <128ns, L1 <32us
			ClockPM+ Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [100] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
		AERCap:	First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn-
	Capabilities: [140] Device Serial Number 00-21-6a-ff-ff-6d-18-8e
	Kernel driver in use: iwlagn
	Kernel modules: iwlagn

Not reproducible on 2.6.34.7 , 100% reproducible on 2.6.35.7 and 2.6.36 .
I'm going to bisect ... or maybe there is some better way of debug
this problem?

Stanislaw

Re: [PATCH 1/2] iwlwifi: fix set_tx_power vs scan

[Dan Williams]

On Fri, 2010-10-22 at 14:56 +0200, Stanislaw Gruszka wrote:
> On Thu, Oct 21, 2010 at 03:13:48PM +0200, Stanislaw Gruszka wrote:
> > On Thu, Oct 14, 2010 at 10:32:05AM +0200, Stanislaw Gruszka wrote:
> > > > Looks good, the only thing is if priv->tx_power_user_lmt ==
> > > > priv->tx_power_next, we don't even have to call set_tx_power, but I
> > > > guess calling it won't hurt, so its your decision check or not.
> > > 
> > > I'll will call iwl_set_tx_power( ... , false); what seems to be right
> > > thing to do.
> > 
> > Set tx power have to be forces. Without that I get  
> > 
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 1
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: iwlagn_tx_agg_start on ra = 00:23:69:35:d1:3f tid = 0
> > iwlagn 0000:40:00.0: low ack count detected, restart firmware
> > iwlagn 0000:40:00.0: On demand firmware reload
> > iwlagn 0000:40:00.0: Stopping AGG while state not ON or starting
> > iwlagn 0000:40:00.0: queue number out of range: 0, must be 10 to 19
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 4
> > iwlagn 0000:40:00.0: Aggregation not enabled for tid 0 because load = 0
> > 
> > on 5300 and device was unusable in general. Hence I will only comment
> > that forcing send tx power after scan is needed.
> 
> Not true. I have the same problem with vanilla 2.6.36 without any of my
> patches (further investigation show this is 2.6.34 -> 2.6.35
> regression). Problem happens only on 5Ghz, that confused me 
> because NetworkManager in some cases connects to 5Ghz, in others to
> 2.4Ghz on my system.

NM doesn't yet send a list of approved frequencies down to
wpa_supplicant for a network, which is how we'd implement "locking" a
connection to a particular band.  So at this point, which band the
driver uses depends on wpa_supplicant scan results and the driver
itself.  Something I intend to fix soon; we've always had a "band"
config option, it's just not hooked up to anything.

Dan