* pull request: iwlwifi 2013-02-12
@ 2014-02-12 8:51 Emmanuel Grumbach
2014-02-12 8:54 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach
2014-02-12 9:08 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach
0 siblings, 2 replies; 19+ messages in thread
From: Emmanuel Grumbach @ 2014-02-12 8:51 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, ilw
[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]
Hi John,
One single patch for this pull request for 3.14.
As explicitly written in the commit message, we prefer to disable Tx
AMPDU on NICs supported by iwldvm. This feature gives a big boost in Tx
performance, but the firmware is buggy and we can't rely on it.
Our hope is that most of the users out there want wifi to surf on the
web which means that they care more for Rx traffic than for Tx.
People who want to enable it can do so with a simple Kconfig entry.
Rx AMPDU is still enabled by default.
Please pull - thanks.
emmanuel
The following changes since commit c512865446e6dd5b6e91e81187e75b734ad7cfc7:
iwlwifi: mvm: don't allow A band if SKU forbids it (2014-02-02
11:08:42 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git
for-john
for you to fetch changes up to 930e106d90ca902c8ede0a8cb206cee1ef3c5cc0:
iwlwifi: disable TX AMPDU by default for iwldvm (2014-02-12 10:38:44
+0200)
----------------------------------------------------------------
Emmanuel Grumbach (1):
iwlwifi: disable TX AMPDU by default for iwldvm
drivers/net/wireless/iwlwifi/Kconfig | 10 ++++++++
drivers/net/wireless/iwlwifi/dvm/mac80211.c | 8 +++----
drivers/net/wireless/iwlwifi/dvm/tx.c | 32
++++++++++++++------------
drivers/net/wireless/iwlwifi/iwl-drv.c | 2 +-
drivers/net/wireless/iwlwifi/iwl-modparams.h | 1 -
drivers/net/wireless/iwlwifi/iwl-nvm-parse.c | 2 --
drivers/net/wireless/iwlwifi/mvm/mac80211.c | 8 -------
7 files changed, 32 insertions(+), 31 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 8:51 pull request: iwlwifi 2013-02-12 Emmanuel Grumbach @ 2014-02-12 8:54 ` Emmanuel Grumbach 2014-02-12 9:10 ` Stanislaw Gruszka 2014-02-12 9:08 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach 1 sibling, 1 reply; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 8:54 UTC (permalink / raw) To: linux-wireless; +Cc: Emmanuel Grumbach From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> We have had a bug in TX AMPDU in iwldvm for a very long time. This bug has been raised in many bugzillas and threads on linux-wireless mailing list. The bug is in firmware and we won't be able to fix it in the near future. Hence, we prefer to disable TX AMPDU by default in iwldvm. This doesn't affect iwlmvm which supports 7160 / 3160 and up. Reviewed-by: Johannes Berg <johannes.berg@intel.com> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- drivers/net/wireless/iwlwifi/Kconfig | 10 ++++++++ drivers/net/wireless/iwlwifi/dvm/mac80211.c | 8 +++---- drivers/net/wireless/iwlwifi/dvm/tx.c | 32 ++++++++++++++------------ drivers/net/wireless/iwlwifi/iwl-drv.c | 2 +- drivers/net/wireless/iwlwifi/iwl-modparams.h | 1 - drivers/net/wireless/iwlwifi/iwl-nvm-parse.c | 2 -- drivers/net/wireless/iwlwifi/mvm/mac80211.c | 8 ------- 7 files changed, 32 insertions(+), 31 deletions(-) diff --git a/drivers/net/wireless/iwlwifi/Kconfig b/drivers/net/wireless/iwlwifi/Kconfig index 3eb2102..ec28564 100644 --- a/drivers/net/wireless/iwlwifi/Kconfig +++ b/drivers/net/wireless/iwlwifi/Kconfig @@ -128,3 +128,13 @@ config IWLWIFI_DEVICE_TRACING If unsure, say Y so we can help you better when problems occur. endmenu + +config IWLWIFI_TX_AMPDU + bool "Enable TX AMPDU (EXPERIMENTAL)" + depends on IWLDVM + help + Say Y here to enable TX AMPDU. TX APMDU should give a + significant boost to TX throughput but the firmware has + a bug that prevents it from working properly. + + If unsure, say N which is the default. diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c index c24d1d3..12d3613 100644 --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c @@ -105,7 +105,6 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv, /* Tell mac80211 our characteristics */ hw->flags = IEEE80211_HW_SIGNAL_DBM | - IEEE80211_HW_AMPDU_AGGREGATION | IEEE80211_HW_NEED_DTIM_BEFORE_ASSOC | IEEE80211_HW_SPECTRUM_MGMT | IEEE80211_HW_REPORTS_TX_ACK_STATUS | @@ -126,7 +125,8 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv, if (priv->nvm_data->sku_cap_11n_enable) hw->flags |= IEEE80211_HW_SUPPORTS_DYNAMIC_SMPS | - IEEE80211_HW_SUPPORTS_STATIC_SMPS; + IEEE80211_HW_SUPPORTS_STATIC_SMPS | + IEEE80211_HW_AMPDU_AGGREGATION; /* * Enable 11w if advertised by firmware and software crypto @@ -729,10 +729,10 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: if (!priv->trans->ops->txq_enable) break; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) - break; +#ifdef CPTCFG_IWLWIFI_TX_AMPDU IWL_DEBUG_HT(priv, "start Tx\n"); ret = iwlagn_tx_agg_start(priv, vif, sta, tid, ssn); +#endif break; case IEEE80211_AMPDU_TX_STOP_FLUSH: case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT: diff --git a/drivers/net/wireless/iwlwifi/dvm/tx.c b/drivers/net/wireless/iwlwifi/dvm/tx.c index a6839df..f1a0386 100644 --- a/drivers/net/wireless/iwlwifi/dvm/tx.c +++ b/drivers/net/wireless/iwlwifi/dvm/tx.c @@ -480,21 +480,6 @@ drop_unlock_priv: return -1; } -static int iwlagn_alloc_agg_txq(struct iwl_priv *priv, int mq) -{ - int q; - - for (q = IWLAGN_FIRST_AMPDU_QUEUE; - q < priv->cfg->base_params->num_of_queues; q++) { - if (!test_and_set_bit(q, priv->agg_q_alloc)) { - priv->queue_to_mac80211[q] = mq; - return q; - } - } - - return -ENOSPC; -} - static void iwlagn_dealloc_agg_txq(struct iwl_priv *priv, int q) { clear_bit(q, priv->agg_q_alloc); @@ -592,6 +577,22 @@ turn_off: return 0; } +#ifdef CPTCFG_IWLWIFI_TX_AMPDU +static int iwlagn_alloc_agg_txq(struct iwl_priv *priv, int mq) +{ + int q; + + for (q = IWLAGN_FIRST_AMPDU_QUEUE; + q < priv->cfg->base_params->num_of_queues; q++) { + if (!test_and_set_bit(q, priv->agg_q_alloc)) { + priv->queue_to_mac80211[q] = mq; + return q; + } + } + + return -ENOSPC; +} + int iwlagn_tx_agg_start(struct iwl_priv *priv, struct ieee80211_vif *vif, struct ieee80211_sta *sta, u16 tid, u16 *ssn) { @@ -650,6 +651,7 @@ int iwlagn_tx_agg_start(struct iwl_priv *priv, struct ieee80211_vif *vif, return ret; } +#endif /* CPTCFG_IWLWIFI_TX_AMPDU */ int iwlagn_tx_agg_flush(struct iwl_priv *priv, struct ieee80211_vif *vif, struct ieee80211_sta *sta, u16 tid) diff --git a/drivers/net/wireless/iwlwifi/iwl-drv.c b/drivers/net/wireless/iwlwifi/iwl-drv.c index c372816..2a77602 100644 --- a/drivers/net/wireless/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/iwlwifi/iwl-drv.c @@ -1286,7 +1286,7 @@ module_param_named(swcrypto, iwlwifi_mod_params.sw_crypto, int, S_IRUGO); MODULE_PARM_DESC(swcrypto, "using crypto in software (default 0 [hardware])"); module_param_named(11n_disable, iwlwifi_mod_params.disable_11n, uint, S_IRUGO); MODULE_PARM_DESC(11n_disable, - "disable 11n functionality, bitmap: 1: full, 2: agg TX, 4: agg RX"); + "disable 11n functionality, bitmap: 1: full, 4: agg RX"); module_param_named(amsdu_size_8K, iwlwifi_mod_params.amsdu_size_8K, int, S_IRUGO); MODULE_PARM_DESC(amsdu_size_8K, "enable 8K amsdu size (default 0)"); diff --git a/drivers/net/wireless/iwlwifi/iwl-modparams.h b/drivers/net/wireless/iwlwifi/iwl-modparams.h index 0a84ade..9589c06 100644 --- a/drivers/net/wireless/iwlwifi/iwl-modparams.h +++ b/drivers/net/wireless/iwlwifi/iwl-modparams.h @@ -80,7 +80,6 @@ enum iwl_power_level { }; #define IWL_DISABLE_HT_ALL BIT(0) -#define IWL_DISABLE_HT_TXAGG BIT(1) #define IWL_DISABLE_HT_RXAGG BIT(2) /** diff --git a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c index 725e954..810c76d 100644 --- a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c +++ b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c @@ -369,8 +369,6 @@ iwl_parse_nvm_data(struct device *dev, const struct iwl_cfg *cfg, data->sku_cap_band_24GHz_enable = sku & NVM_SKU_CAP_BAND_24GHZ; data->sku_cap_band_52GHz_enable = sku & NVM_SKU_CAP_BAND_52GHZ; data->sku_cap_11n_enable = sku & NVM_SKU_CAP_11N_ENABLE; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_ALL) - data->sku_cap_11n_enable = false; /* check overrides (some devices have wrong NVM) */ if (cfg->valid_tx_ant) diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c index 6bf9766..b068799 100644 --- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c @@ -347,20 +347,12 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) { - ret = -EINVAL; - break; - } ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, *ssn, true); break; case IEEE80211_AMPDU_RX_STOP: ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, 0, false); break; case IEEE80211_AMPDU_TX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) { - ret = -EINVAL; - break; - } ret = iwl_mvm_sta_tx_agg_start(mvm, vif, sta, tid, ssn); break; case IEEE80211_AMPDU_TX_STOP_CONT: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 8:54 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach @ 2014-02-12 9:10 ` Stanislaw Gruszka 2014-02-12 9:21 ` Stanislaw Gruszka 0 siblings, 1 reply; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-12 9:10 UTC (permalink / raw) To: Emmanuel Grumbach; +Cc: linux-wireless, Emmanuel Grumbach On Wed, Feb 12, 2014 at 10:54:29AM +0200, Emmanuel Grumbach wrote: > From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > We have had a bug in TX AMPDU in iwldvm for a very long > time. This bug has been raised in many bugzillas and threads > on linux-wireless mailing list. > > The bug is in firmware and we won't be able to fix it in the > near future. Hence, we prefer to disable TX AMPDU by default > in iwldvm. This doesn't affect iwlmvm which supports 7160 / > 3160 and up. ... > #define IWL_DISABLE_HT_ALL BIT(0) > -#define IWL_DISABLE_HT_TXAGG BIT(1) Whouln't simple change, by setting disable_11=2 by default, make the same effect, and allow easly to enable TX aggregation (without kernel recompile) ? Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:10 ` Stanislaw Gruszka @ 2014-02-12 9:21 ` Stanislaw Gruszka 2014-02-12 9:35 ` Grumbach, Emmanuel 0 siblings, 1 reply; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-12 9:21 UTC (permalink / raw) To: Emmanuel Grumbach; +Cc: linux-wireless, Emmanuel Grumbach On Wed, Feb 12, 2014 at 10:10:08AM +0100, Stanislaw Gruszka wrote: > On Wed, Feb 12, 2014 at 10:54:29AM +0200, Emmanuel Grumbach wrote: > > From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > > > We have had a bug in TX AMPDU in iwldvm for a very long > > time. This bug has been raised in many bugzillas and threads > > on linux-wireless mailing list. > > > > The bug is in firmware and we won't be able to fix it in the > > near future. Hence, we prefer to disable TX AMPDU by default > > in iwldvm. This doesn't affect iwlmvm which supports 7160 / > > 3160 and up. > > ... > > > #define IWL_DISABLE_HT_ALL BIT(0) > > -#define IWL_DISABLE_HT_TXAGG BIT(1) > > Whouln't simple change, by setting disable_11=2 by default, make > the same effect, and allow easly to enable TX aggregation (without > kernel recompile) ? Also, does the bug affect all iwldvm devices ? They have different firmware images, so I wonder if this is needed for all DVM devices. Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:21 ` Stanislaw Gruszka @ 2014-02-12 9:35 ` Grumbach, Emmanuel 2014-02-12 9:43 ` Sujith Manoharan 2014-02-12 10:41 ` Stanislaw Gruszka 0 siblings, 2 replies; 19+ messages in thread From: Grumbach, Emmanuel @ 2014-02-12 9:35 UTC (permalink / raw) To: Stanislaw Gruszka, Emmanuel Grumbach Cc: linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) > > On Wed, Feb 12, 2014 at 10:10:08AM +0100, Stanislaw Gruszka wrote: > > On Wed, Feb 12, 2014 at 10:54:29AM +0200, Emmanuel Grumbach wrote: > > > From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > > > > > We have had a bug in TX AMPDU in iwldvm for a very long time. This > > > bug has been raised in many bugzillas and threads on linux-wireless > > > mailing list. > > > > > > The bug is in firmware and we won't be able to fix it in the near > > > future. Hence, we prefer to disable TX AMPDU by default in iwldvm. > > > This doesn't affect iwlmvm which supports 7160 / > > > 3160 and up. > > > > ... > > > > > #define IWL_DISABLE_HT_ALL BIT(0) > > > -#define IWL_DISABLE_HT_TXAGG BIT(1) > > > > Whouln't simple change, by setting disable_11=2 by default, make the > > same effect, and allow easly to enable TX aggregation (without kernel > > recompile) ? > I guess at that point, I prefer to have a compilation flag. In Redhat / Fedora you'd disable it by default right? Also the module parameter is for iwlwifi which includes iwlmvm. When I did this change I mainly thought about distros not wanting to change their modprobe.conf files. I want to give the possibility for distro to disable TX AMPDU for iwldvm and keep it enable for iwlmvm. We can move the module parameter to be iwldvm only, this would be the best, but it'd require distro to make changes... Ignoring the disable_11n module parameter in iwlmvm seems odd (even if this is what I do know for IWL_DISABLE_HT_ALL and IWL_DISABLE_HT_RXAGG). If you (as a distro representative) have a better idea, I am willing to hear. After all, this change is mostly intended to make distro's life easier... > Also, does the bug affect all iwldvm devices ? They have different firmware > images, so I wonder if this is needed for all DVM devices. I have seen reports from many users and many devices - and unfortunately I have never been able to reproduce - so I can't know if it affects all the devices. All I can see is that I saw reports on very old devices (5150) and with more recent devices (2230). ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:35 ` Grumbach, Emmanuel @ 2014-02-12 9:43 ` Sujith Manoharan 2014-02-12 9:45 ` Grumbach, Emmanuel 2014-02-12 10:41 ` Stanislaw Gruszka 1 sibling, 1 reply; 19+ messages in thread From: Sujith Manoharan @ 2014-02-12 9:43 UTC (permalink / raw) To: Grumbach, Emmanuel Cc: Stanislaw Gruszka, Emmanuel Grumbach, linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) Grumbach, Emmanuel wrote: > I have seen reports from many users and many devices - and unfortunately I > have never been able to reproduce - so I can't know if it affects all the > devices. All I can see is that I saw reports on very old devices (5150) and > with more recent devices (2230). Is 5100 a very old device ? I have one of those cards in a laptop and starting 11n TX traffic usually results in a crash like this: http://pastebin.com/HtUdCLR3 Sujith ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:43 ` Sujith Manoharan @ 2014-02-12 9:45 ` Grumbach, Emmanuel 2014-02-12 9:53 ` Sujith Manoharan 0 siblings, 1 reply; 19+ messages in thread From: Grumbach, Emmanuel @ 2014-02-12 9:45 UTC (permalink / raw) To: Sujith Manoharan Cc: Stanislaw Gruszka, Emmanuel Grumbach, linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) > > Grumbach, Emmanuel wrote: > > I have seen reports from many users and many devices - and > > unfortunately I have never been able to reproduce - so I can't know if > > it affects all the devices. All I can see is that I saw reports on > > very old devices (5150) and with more recent devices (2230). > > Is 5100 a very old device ? I have one of those cards in a laptop and starting > 11n TX traffic usually results in a crash like this: > http://pastebin.com/HtUdCLR3 > 5100 is the first device supported by iwldvm. I think it was shipped in 2008. ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:45 ` Grumbach, Emmanuel @ 2014-02-12 9:53 ` Sujith Manoharan 0 siblings, 0 replies; 19+ messages in thread From: Sujith Manoharan @ 2014-02-12 9:53 UTC (permalink / raw) To: Grumbach, Emmanuel Cc: Stanislaw Gruszka, Emmanuel Grumbach, linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) Grumbach, Emmanuel wrote: > 5100 is the first device supported by iwldvm. I think it was shipped in 2008. Ok, guess I can't test TX performance with this card. :) Sujith ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:35 ` Grumbach, Emmanuel 2014-02-12 9:43 ` Sujith Manoharan @ 2014-02-12 10:41 ` Stanislaw Gruszka 1 sibling, 0 replies; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-12 10:41 UTC (permalink / raw) To: Grumbach, Emmanuel Cc: Emmanuel Grumbach, linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) On Wed, Feb 12, 2014 at 09:35:03AM +0000, Grumbach, Emmanuel wrote: > > > > On Wed, Feb 12, 2014 at 10:10:08AM +0100, Stanislaw Gruszka wrote: > > > On Wed, Feb 12, 2014 at 10:54:29AM +0200, Emmanuel Grumbach wrote: > > > > From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > > > > > > > We have had a bug in TX AMPDU in iwldvm for a very long time. This > > > > bug has been raised in many bugzillas and threads on linux-wireless > > > > mailing list. > > > > > > > > The bug is in firmware and we won't be able to fix it in the near > > > > future. Hence, we prefer to disable TX AMPDU by default in iwldvm. > > > > This doesn't affect iwlmvm which supports 7160 / > > > > 3160 and up. > > > > > > ... > > > > > > > #define IWL_DISABLE_HT_ALL BIT(0) > > > > -#define IWL_DISABLE_HT_TXAGG BIT(1) > > > > > > Whouln't simple change, by setting disable_11=2 by default, make the > > > same effect, and allow easly to enable TX aggregation (without kernel > > > recompile) ? > > > > I guess at that point, I prefer to have a compilation flag. In Redhat / Fedora you'd disable it by default right? No, we do not have any extra options in /etc/modprobe.d/ for iwlwifi, all is running with default settings. > Also the module parameter is for iwlwifi which includes iwlmvm. When I did this change I mainly thought about distros not wanting to change their modprobe.conf files. I want to give the possibility for distro to disable TX AMPDU for iwldvm and keep it enable for iwlmvm. We can move the module parameter to be iwldvm only, this would be the best, but it'd require distro to make changes... Ignoring the disable_11n module parameter in iwlmvm seems odd (even if this is what I do know for IWL_DISABLE_HT_ALL and IWL_DISABLE_HT_RXAGG). > If you (as a distro representative) have a better idea, I am willing to hear. After all, this change is mostly intended to make distro's life easier... If somebody has working aggregation and we will remove that from kernel, we will get complains. Telling that he/she have to recompile kernel to get functionality back, will not make live easier to anyone. Setting module option is much much easier. In the past we have in iwlwifi module parameters initialized differently for different devices. Though, it does not work with many devices on the system, this is not an issue since normal users have only one device. I guess the same could be used here: 0 - default (device specific), 1 - disable TX AGG, 2 - disable RX AGG, 4 - force enable TX AGG, 8 - force enable RX AGG. > > Also, does the bug affect all iwldvm devices ? They have different firmware > > images, so I wonder if this is needed for all DVM devices. > > I have seen reports from many users and many devices - and unfortunately I have never been able to reproduce So, that is argument to do not remove support for TX agg. If someone has that working, he/she still should be able to use it. Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* pull request: iwlwifi 2014-02-12 (FIXED) 2014-02-12 8:51 pull request: iwlwifi 2013-02-12 Emmanuel Grumbach 2014-02-12 8:54 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach @ 2014-02-12 9:08 ` Emmanuel Grumbach 2014-02-12 9:09 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 11:02 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach 1 sibling, 2 replies; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 9:08 UTC (permalink / raw) To: John Linville; +Cc: linux-wireless, ilw [-- Attachment #1: Type: text/plain, Size: 1111 bytes --] On 02/12/2014 10:51 AM, Emmanuel Grumbach wrote: > Hi John, > > One single patch for this pull request for 3.14. > > As explicitly written in the commit message, we prefer to disable Tx > AMPDU on NICs supported by iwldvm. This feature gives a big boost in Tx > performance, but the firmware is buggy and we can't rely on it. > Our hope is that most of the users out there want wifi to surf on the > web which means that they care more for Rx traffic than for Tx. > People who want to enable it can do so with a simple Kconfig entry. > Rx AMPDU is still enabled by default. > > Please pull - thanks. > > emmanuel > > The following changes since commit c512865446e6dd5b6e91e81187e75b734ad7cfc7: > > iwlwifi: mvm: don't allow A band if SKU forbids it (2014-02-02 > 11:08:42 +0200) > > are available in the git repository at: > > > git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git > for-john > > for you to fetch changes up to 930e106d90ca902c8ede0a8cb206cee1ef3c5cc0: > I had to edit the patch - 63f1809aa672dc439e7750b551ff9926a79312e0 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:08 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach @ 2014-02-12 9:09 ` Emmanuel Grumbach 2014-02-12 11:19 ` [RFC] iwlwifi: refactor the TX / RX ampdu override Emmanuel Grumbach 2014-02-13 12:28 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 11:02 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach 1 sibling, 2 replies; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 9:09 UTC (permalink / raw) To: linux-wireless; +Cc: Emmanuel Grumbach From: Emmanuel Grumbach <emmanuel.grumbach@intel.com> We have had a bug in TX AMPDU in iwldvm for a very long time. This bug has been raised in many bugzillas and threads on linux-wireless mailing list. The bug is in firmware and we won't be able to fix it in the near future. Hence, we prefer to disable TX AMPDU by default in iwldvm. This doesn't affect iwlmvm which supports 7160 / 3160 and up. Reviewed-by: Johannes Berg <johannes.berg@intel.com> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- drivers/net/wireless/iwlwifi/Kconfig | 10 ++++++++ drivers/net/wireless/iwlwifi/dvm/mac80211.c | 8 +++---- drivers/net/wireless/iwlwifi/dvm/tx.c | 32 ++++++++++++++------------ drivers/net/wireless/iwlwifi/iwl-drv.c | 2 +- drivers/net/wireless/iwlwifi/iwl-modparams.h | 1 - drivers/net/wireless/iwlwifi/iwl-nvm-parse.c | 2 -- drivers/net/wireless/iwlwifi/mvm/mac80211.c | 8 ------- 7 files changed, 32 insertions(+), 31 deletions(-) diff --git a/drivers/net/wireless/iwlwifi/Kconfig b/drivers/net/wireless/iwlwifi/Kconfig index 3eb2102..ec28564 100644 --- a/drivers/net/wireless/iwlwifi/Kconfig +++ b/drivers/net/wireless/iwlwifi/Kconfig @@ -128,3 +128,13 @@ config IWLWIFI_DEVICE_TRACING If unsure, say Y so we can help you better when problems occur. endmenu + +config IWLWIFI_TX_AMPDU + bool "Enable TX AMPDU (EXPERIMENTAL)" + depends on IWLDVM + help + Say Y here to enable TX AMPDU. TX APMDU should give a + significant boost to TX throughput but the firmware has + a bug that prevents it from working properly. + + If unsure, say N which is the default. diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c index c24d1d3..e678c83 100644 --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c @@ -105,7 +105,6 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv, /* Tell mac80211 our characteristics */ hw->flags = IEEE80211_HW_SIGNAL_DBM | - IEEE80211_HW_AMPDU_AGGREGATION | IEEE80211_HW_NEED_DTIM_BEFORE_ASSOC | IEEE80211_HW_SPECTRUM_MGMT | IEEE80211_HW_REPORTS_TX_ACK_STATUS | @@ -126,7 +125,8 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv, if (priv->nvm_data->sku_cap_11n_enable) hw->flags |= IEEE80211_HW_SUPPORTS_DYNAMIC_SMPS | - IEEE80211_HW_SUPPORTS_STATIC_SMPS; + IEEE80211_HW_SUPPORTS_STATIC_SMPS | + IEEE80211_HW_AMPDU_AGGREGATION; /* * Enable 11w if advertised by firmware and software crypto @@ -729,10 +729,10 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: if (!priv->trans->ops->txq_enable) break; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) - break; +#ifdef CONFIG_IWLWIFI_TX_AMPDU IWL_DEBUG_HT(priv, "start Tx\n"); ret = iwlagn_tx_agg_start(priv, vif, sta, tid, ssn); +#endif break; case IEEE80211_AMPDU_TX_STOP_FLUSH: case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT: diff --git a/drivers/net/wireless/iwlwifi/dvm/tx.c b/drivers/net/wireless/iwlwifi/dvm/tx.c index a6839df..b19418c 100644 --- a/drivers/net/wireless/iwlwifi/dvm/tx.c +++ b/drivers/net/wireless/iwlwifi/dvm/tx.c @@ -480,21 +480,6 @@ drop_unlock_priv: return -1; } -static int iwlagn_alloc_agg_txq(struct iwl_priv *priv, int mq) -{ - int q; - - for (q = IWLAGN_FIRST_AMPDU_QUEUE; - q < priv->cfg->base_params->num_of_queues; q++) { - if (!test_and_set_bit(q, priv->agg_q_alloc)) { - priv->queue_to_mac80211[q] = mq; - return q; - } - } - - return -ENOSPC; -} - static void iwlagn_dealloc_agg_txq(struct iwl_priv *priv, int q) { clear_bit(q, priv->agg_q_alloc); @@ -592,6 +577,22 @@ turn_off: return 0; } +#ifdef CONFIG_IWLWIFI_TX_AMPDU +static int iwlagn_alloc_agg_txq(struct iwl_priv *priv, int mq) +{ + int q; + + for (q = IWLAGN_FIRST_AMPDU_QUEUE; + q < priv->cfg->base_params->num_of_queues; q++) { + if (!test_and_set_bit(q, priv->agg_q_alloc)) { + priv->queue_to_mac80211[q] = mq; + return q; + } + } + + return -ENOSPC; +} + int iwlagn_tx_agg_start(struct iwl_priv *priv, struct ieee80211_vif *vif, struct ieee80211_sta *sta, u16 tid, u16 *ssn) { @@ -650,6 +651,7 @@ int iwlagn_tx_agg_start(struct iwl_priv *priv, struct ieee80211_vif *vif, return ret; } +#endif /* CONFIG_IWLWIFI_TX_AMPDU */ int iwlagn_tx_agg_flush(struct iwl_priv *priv, struct ieee80211_vif *vif, struct ieee80211_sta *sta, u16 tid) diff --git a/drivers/net/wireless/iwlwifi/iwl-drv.c b/drivers/net/wireless/iwlwifi/iwl-drv.c index c372816..2a77602 100644 --- a/drivers/net/wireless/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/iwlwifi/iwl-drv.c @@ -1286,7 +1286,7 @@ module_param_named(swcrypto, iwlwifi_mod_params.sw_crypto, int, S_IRUGO); MODULE_PARM_DESC(swcrypto, "using crypto in software (default 0 [hardware])"); module_param_named(11n_disable, iwlwifi_mod_params.disable_11n, uint, S_IRUGO); MODULE_PARM_DESC(11n_disable, - "disable 11n functionality, bitmap: 1: full, 2: agg TX, 4: agg RX"); + "disable 11n functionality, bitmap: 1: full, 4: agg RX"); module_param_named(amsdu_size_8K, iwlwifi_mod_params.amsdu_size_8K, int, S_IRUGO); MODULE_PARM_DESC(amsdu_size_8K, "enable 8K amsdu size (default 0)"); diff --git a/drivers/net/wireless/iwlwifi/iwl-modparams.h b/drivers/net/wireless/iwlwifi/iwl-modparams.h index 0a84ade..9589c06 100644 --- a/drivers/net/wireless/iwlwifi/iwl-modparams.h +++ b/drivers/net/wireless/iwlwifi/iwl-modparams.h @@ -80,7 +80,6 @@ enum iwl_power_level { }; #define IWL_DISABLE_HT_ALL BIT(0) -#define IWL_DISABLE_HT_TXAGG BIT(1) #define IWL_DISABLE_HT_RXAGG BIT(2) /** diff --git a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c index 725e954..810c76d 100644 --- a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c +++ b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c @@ -369,8 +369,6 @@ iwl_parse_nvm_data(struct device *dev, const struct iwl_cfg *cfg, data->sku_cap_band_24GHz_enable = sku & NVM_SKU_CAP_BAND_24GHZ; data->sku_cap_band_52GHz_enable = sku & NVM_SKU_CAP_BAND_52GHZ; data->sku_cap_11n_enable = sku & NVM_SKU_CAP_11N_ENABLE; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_ALL) - data->sku_cap_11n_enable = false; /* check overrides (some devices have wrong NVM) */ if (cfg->valid_tx_ant) diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c index 6bf9766..b068799 100644 --- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c @@ -347,20 +347,12 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) { - ret = -EINVAL; - break; - } ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, *ssn, true); break; case IEEE80211_AMPDU_RX_STOP: ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, 0, false); break; case IEEE80211_AMPDU_TX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) { - ret = -EINVAL; - break; - } ret = iwl_mvm_sta_tx_agg_start(mvm, vif, sta, tid, ssn); break; case IEEE80211_AMPDU_TX_STOP_CONT: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 9:09 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach @ 2014-02-12 11:19 ` Emmanuel Grumbach 2014-02-12 12:13 ` Stanislaw Gruszka 2014-02-13 12:28 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 1 sibling, 1 reply; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 11:19 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: linux-wireless, Emmanuel Grumbach Note that this modifies the module parameter values / string which means that distro will need to update their *.conf file (if any). Change-Id: Icfd5a1a6100fcb126e560046551bfb0d1f28179c Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- drivers/net/wireless/iwlwifi/dvm/mac80211.c | 22 ++++++++++++++++++++-- drivers/net/wireless/iwlwifi/iwl-drv.c | 4 ++-- drivers/net/wireless/iwlwifi/iwl-eeprom-parse.c | 2 -- drivers/net/wireless/iwlwifi/iwl-modparams.h | 13 +++++++------ drivers/net/wireless/iwlwifi/iwl-nvm-parse.c | 2 -- drivers/net/wireless/iwlwifi/mvm/mac80211.c | 22 ++++++++++++++++++++-- 6 files changed, 49 insertions(+), 16 deletions(-) diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c index ad8f81d..0a9c2ec 100644 --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c @@ -697,6 +697,24 @@ static int iwlagn_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, return ret; } +static inline bool iwl_enable_rx_ampdu(struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.override_ampdu & IWL_DISABLE_HT_RXAGG) + return false; + return true; +} + +static inline bool iwl_enable_tx_ampdu(struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.override_ampdu & IWL_DISABLE_HT_TXAGG) + return false; + if (iwlwifi_mod_params.override_ampdu & IWL_ENABLE_HT_TXAGG) + return true; + + /* disabled by default */ + return false; +} + static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, @@ -718,7 +736,7 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) + if (!iwl_enable_rx_ampdu(priv->cfg)) break; IWL_DEBUG_HT(priv, "start Rx\n"); ret = iwl_sta_rx_agg_start(priv, sta, tid, *ssn); @@ -730,7 +748,7 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: if (!priv->trans->ops->txq_enable) break; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) + if (!iwl_enable_tx_ampdu(priv->cfg)) break; IWL_DEBUG_HT(priv, "start Tx\n"); ret = iwlagn_tx_agg_start(priv, vif, sta, tid, ssn); diff --git a/drivers/net/wireless/iwlwifi/iwl-drv.c b/drivers/net/wireless/iwlwifi/iwl-drv.c index 65bc944..68e8f3b 100644 --- a/drivers/net/wireless/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/iwlwifi/iwl-drv.c @@ -1661,8 +1661,8 @@ MODULE_PARM_DESC(xvt_default_mode, "xVT is the default operation mode (default: module_param_named(swcrypto, iwlwifi_mod_params.sw_crypto, int, S_IRUGO); MODULE_PARM_DESC(swcrypto, "using crypto in software (default 0 [hardware])"); -module_param_named(11n_disable, iwlwifi_mod_params.disable_11n, uint, S_IRUGO); -MODULE_PARM_DESC(11n_disable, +module_param_named(override_ampdu, iwlwifi_mod_params.override_ampdu, uint, S_IRUGO); +MODULE_PARM_DESC(override_ampdu, "disable 11n functionality, bitmap: 1: full, 2: agg TX, 4: agg RX"); module_param_named(amsdu_size_8K, iwlwifi_mod_params.amsdu_size_8K, int, S_IRUGO); diff --git a/drivers/net/wireless/iwlwifi/iwl-eeprom-parse.c b/drivers/net/wireless/iwlwifi/iwl-eeprom-parse.c index c44cf11..ae98cb7 100644 --- a/drivers/net/wireless/iwlwifi/iwl-eeprom-parse.c +++ b/drivers/net/wireless/iwlwifi/iwl-eeprom-parse.c @@ -893,8 +893,6 @@ iwl_parse_eeprom_data(struct device *dev, const struct iwl_cfg *cfg, data->sku_cap_band_24GHz_enable = sku & EEPROM_SKU_CAP_BAND_24GHZ; data->sku_cap_band_52GHz_enable = sku & EEPROM_SKU_CAP_BAND_52GHZ; data->sku_cap_ipan_enable = sku & EEPROM_SKU_CAP_IPAN_ENABLE; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_ALL) - data->sku_cap_11n_enable = false; data->nvm_version = iwl_eeprom_query16(eeprom, eeprom_size, EEPROM_VERSION); diff --git a/drivers/net/wireless/iwlwifi/iwl-modparams.h b/drivers/net/wireless/iwlwifi/iwl-modparams.h index 1d2f793..9022091 100644 --- a/drivers/net/wireless/iwlwifi/iwl-modparams.h +++ b/drivers/net/wireless/iwlwifi/iwl-modparams.h @@ -79,9 +79,10 @@ enum iwl_power_level { IWL_POWER_NUM }; -#define IWL_DISABLE_HT_ALL BIT(0) -#define IWL_DISABLE_HT_TXAGG BIT(1) -#define IWL_DISABLE_HT_RXAGG BIT(2) +#define IWL_DISABLE_HT_TXAGG BIT(0) +#define IWL_DISABLE_HT_RXAGG BIT(1) +#define IWL_ENABLE_HT_TXAGG BIT(2) +#define IWL_ENABLE_HT_RXAGG BIT(3) /** * struct iwl_mod_params @@ -89,8 +90,8 @@ enum iwl_power_level { * Holds the module parameters * * @sw_crypto: using hardware encryption, default = 0 - * @disable_11n: disable 11n capabilities, default = 0, - * use IWL_DISABLE_HT_* constants + * @override_ampdu: disable 11n capabilities, default = 0, + * use IWL_[DIS,EN]ABLE_HT_* constants * @amsdu_size_8K: enable 8K amsdu size, default = 0 * @restart_fw: restart firmware, default = 1 * @wd_disable: disable stuck queue check, default = 1 @@ -104,7 +105,7 @@ enum iwl_power_level { */ struct iwl_mod_params { int sw_crypto; - unsigned int disable_11n; + unsigned int override_ampdu; int amsdu_size_8K; bool restart_fw; int wd_disable; diff --git a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c index 3f3536c..9a4ae64 100644 --- a/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c +++ b/drivers/net/wireless/iwlwifi/iwl-nvm-parse.c @@ -498,8 +498,6 @@ iwl_parse_nvm_data(struct device *dev, const struct iwl_cfg *cfg, data->sku_cap_band_52GHz_enable = sku & NVM_SKU_CAP_BAND_52GHZ; data->sku_cap_11n_enable = sku & NVM_SKU_CAP_11N_ENABLE; data->sku_cap_11ac_enable = sku & NVM_SKU_CAP_11AC_ENABLE; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_ALL) - data->sku_cap_11n_enable = false; /* check overrides (some devices have wrong NVM) */ if (cfg->valid_tx_ant) diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c index 19925bb..daa7d9b 100644 --- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c @@ -468,6 +468,24 @@ static void iwl_mvm_mac_tx(struct ieee80211_hw *hw, ieee80211_free_txskb(hw, skb); } +static inline bool iwl_enable_rx_ampdu(struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.override_ampdu & IWL_DISABLE_HT_RXAGG) + return false; + return true; +} + +static inline bool iwl_enable_tx_ampdu(struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.override_ampdu & IWL_DISABLE_HT_TXAGG) + return false; + if (iwlwifi_mod_params.override_ampdu & IWL_ENABLE_HT_TXAGG) + return true; + + /* enabled by default */ + return true; +} + static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, @@ -487,7 +505,7 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) { + if (!iwl_enable_rx_ampdu(mvm->cfg)) { ret = -EINVAL; break; } @@ -497,7 +515,7 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, 0, false); break; case IEEE80211_AMPDU_TX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) { + if (!iwl_enable_tx_ampdu(mvm->cfg)) { ret = -EINVAL; break; } -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 11:19 ` [RFC] iwlwifi: refactor the TX / RX ampdu override Emmanuel Grumbach @ 2014-02-12 12:13 ` Stanislaw Gruszka 2014-02-12 12:27 ` Grumbach, Emmanuel 0 siblings, 1 reply; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-12 12:13 UTC (permalink / raw) To: Emmanuel Grumbach; +Cc: linux-wireless On Wed, Feb 12, 2014 at 01:19:49PM +0200, Emmanuel Grumbach wrote: > Note that this modifies the module parameter values / string > which means that distro will need to update their *.conf file > (if any). Hmm, this is not something that I would like to do ... > -#define IWL_DISABLE_HT_ALL BIT(0) > -#define IWL_DISABLE_HT_TXAGG BIT(1) > -#define IWL_DISABLE_HT_RXAGG BIT(2) > -#define IWL_DISABLE_HT_ALL BIT(1) > +#define IWL_DISABLE_HT_TXAGG BIT(1) > +#define IWL_DISABLE_HT_RXAGG BIT(1) > +#define IWL_ENABLE_HT_TXAGG BIT(2) > +#define IWL_ENABLE_HT_RXAGG BIT(3) I think we can keep IWL_DISABLE_HT_ALL option. I should be possible to make some per-device type default settings and if disable_11n == 0 use them. Otherwise use settings from module parameter. Will that work ? Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 12:13 ` Stanislaw Gruszka @ 2014-02-12 12:27 ` Grumbach, Emmanuel 2014-02-12 14:04 ` Stanislaw Gruszka 0 siblings, 1 reply; 19+ messages in thread From: Grumbach, Emmanuel @ 2014-02-12 12:27 UTC (permalink / raw) To: Stanislaw Gruszka Cc: linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) > Subject: Re: [RFC] iwlwifi: refactor the TX / RX ampdu override > > On Wed, Feb 12, 2014 at 01:19:49PM +0200, Emmanuel Grumbach wrote: > > Note that this modifies the module parameter values / string which > > means that distro will need to update their *.conf file (if any). > > Hmm, this is not something that I would like to do ... I thought you didn't care - ok - I'll respin > > > -#define IWL_DISABLE_HT_ALL BIT(0) > > -#define IWL_DISABLE_HT_TXAGG BIT(1) > > -#define IWL_DISABLE_HT_RXAGG BIT(2) > > -#define IWL_DISABLE_HT_ALL BIT(1) > > +#define IWL_DISABLE_HT_TXAGG BIT(1) > > +#define IWL_DISABLE_HT_RXAGG BIT(1) > > +#define IWL_ENABLE_HT_TXAGG BIT(2) > > +#define IWL_ENABLE_HT_RXAGG BIT(3) > > I think we can keep IWL_DISABLE_HT_ALL option. I should be possible to > make some per-device type default settings and if disable_11n == 0 use > them. Otherwise use settings from module parameter. Will that work ? I'd prefer to have the default based on the iwldvm vs. iwlmvm instead of adding yet another per-HW field. But yeah - it is possible. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 12:27 ` Grumbach, Emmanuel @ 2014-02-12 14:04 ` Stanislaw Gruszka 2014-02-12 20:28 ` Emmanuel Grumbach 0 siblings, 1 reply; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-12 14:04 UTC (permalink / raw) To: Grumbach, Emmanuel Cc: linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) On Wed, Feb 12, 2014 at 12:27:05PM +0000, Grumbach, Emmanuel wrote: > > I think we can keep IWL_DISABLE_HT_ALL option. I should be possible to > > make some per-device type default settings and if disable_11n == 0 use > > them. Otherwise use settings from module parameter. Will that work ? > > I'd prefer to have the default based on the iwldvm vs. iwlmvm instead of adding yet another per-HW field. But yeah - it is possible. I re-think changing default settings do disable TX AGG and must tell that this is very odd. I would prefer to stay defaults as they are now. Users can disable TX aggregation using module option. I also not sure if that is really a firmware bug, it is rather more probable that TX aggregation worked pretty fine on 5100 and other older devices, but some driver changes broke that. Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 14:04 ` Stanislaw Gruszka @ 2014-02-12 20:28 ` Emmanuel Grumbach 2014-02-13 7:54 ` Stanislaw Gruszka 0 siblings, 1 reply; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 20:28 UTC (permalink / raw) To: Stanislaw Gruszka, Grumbach, Emmanuel Cc: linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) On 02/12/2014 04:04 PM, Stanislaw Gruszka wrote: > On Wed, Feb 12, 2014 at 12:27:05PM +0000, Grumbach, Emmanuel wrote: >>> I think we can keep IWL_DISABLE_HT_ALL option. I should be possible to >>> make some per-device type default settings and if disable_11n == 0 use >>> them. Otherwise use settings from module parameter. Will that work ? >> >> I'd prefer to have the default based on the iwldvm vs. iwlmvm instead of adding yet another per-HW field. But yeah - it is possible. > > I re-think changing default settings do disable TX AGG and must tell > that this is very odd. I would prefer to stay defaults as they are now. > Users can disable TX aggregation using module option. I disagree. We have bugs there - it is pretty much obvious. I remember a tracing from 2.6.39 which was before the re-architecture I made in the driver with clear FW bugs. I don't remember what device though. This is why I want to disable this by default. You asked me to do this with a module parameter and not with a Kconfig option - I agreed. Now - if you want to ship with a different settings, you can... but we both know the risk. > > I also not sure if that is really a firmware bug, it is rather more > probable that TX aggregation worked pretty fine on 5100 and other older > devices, but some driver changes broke that. Frankly, I don't really remember that... I have to say that I might not have been in the business at that period... ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC] iwlwifi: refactor the TX / RX ampdu override 2014-02-12 20:28 ` Emmanuel Grumbach @ 2014-02-13 7:54 ` Stanislaw Gruszka 0 siblings, 0 replies; 19+ messages in thread From: Stanislaw Gruszka @ 2014-02-13 7:54 UTC (permalink / raw) To: Emmanuel Grumbach Cc: Grumbach, Emmanuel, linux-wireless, Intel Linux Wireless (ilw@linux.intel.com) On Wed, Feb 12, 2014 at 10:28:00PM +0200, Emmanuel Grumbach wrote: > On 02/12/2014 04:04 PM, Stanislaw Gruszka wrote: > > On Wed, Feb 12, 2014 at 12:27:05PM +0000, Grumbach, Emmanuel wrote: > >>> I think we can keep IWL_DISABLE_HT_ALL option. I should be possible to > >>> make some per-device type default settings and if disable_11n == 0 use > >>> them. Otherwise use settings from module parameter. Will that work ? > >> > >> I'd prefer to have the default based on the iwldvm vs. iwlmvm instead of adding yet another per-HW field. But yeah - it is possible. > > > > I re-think changing default settings do disable TX AGG and must tell > > that this is very odd. I would prefer to stay defaults as they are now. > > Users can disable TX aggregation using module option. > > I disagree. We have bugs there - it is pretty much obvious. I remember a > tracing from 2.6.39 which was before the re-architecture I made in the > driver with clear FW bugs. I don't remember what device though. > This is why I want to disable this by default. Sure, it's up to you. But anyway, disabling feature because it buggy, after having it run for 5 years or so, it's odd :-) Stanislaw ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm 2014-02-12 9:09 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 11:19 ` [RFC] iwlwifi: refactor the TX / RX ampdu override Emmanuel Grumbach @ 2014-02-13 12:28 ` Emmanuel Grumbach 1 sibling, 0 replies; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-13 12:28 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: linux-wireless, Emmanuel Grumbach NICs supported by iwldvm don't handle well TX AMPDU. Disable it by default, still leave the possibility to the user to force enable it with a debug parameter. NICs supported by iwlmvm don't suffer from the same issue, leave TX AMPDU enabled by default for these. Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- drivers/net/wireless/iwlwifi/dvm/mac80211.c | 22 ++++++++++++++++++++-- drivers/net/wireless/iwlwifi/iwl-drv.c | 2 +- drivers/net/wireless/iwlwifi/iwl-modparams.h | 11 +++++++---- drivers/net/wireless/iwlwifi/mvm/mac80211.c | 22 ++++++++++++++++++++-- 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c index ad8f81d..99bf6f9 100644 --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c @@ -697,6 +697,24 @@ static int iwlagn_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd, return ret; } +static inline bool iwl_enable_rx_ampdu(const struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) + return false; + return true; +} + +static inline bool iwl_enable_tx_ampdu(const struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) + return false; + if (iwlwifi_mod_params.disable_11n & IWL_ENABLE_HT_TXAGG) + return true; + + /* disabled by default */ + return false; +} + static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, @@ -718,7 +736,7 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) + if (!iwl_enable_rx_ampdu(priv->cfg)) break; IWL_DEBUG_HT(priv, "start Rx\n"); ret = iwl_sta_rx_agg_start(priv, sta, tid, *ssn); @@ -730,7 +748,7 @@ static int iwlagn_mac_ampdu_action(struct ieee80211_hw *hw, case IEEE80211_AMPDU_TX_START: if (!priv->trans->ops->txq_enable) break; - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) + if (!iwl_enable_tx_ampdu(priv->cfg)) break; IWL_DEBUG_HT(priv, "start Tx\n"); ret = iwlagn_tx_agg_start(priv, vif, sta, tid, ssn); diff --git a/drivers/net/wireless/iwlwifi/iwl-drv.c b/drivers/net/wireless/iwlwifi/iwl-drv.c index 65bc944..dfdeccd 100644 --- a/drivers/net/wireless/iwlwifi/iwl-drv.c +++ b/drivers/net/wireless/iwlwifi/iwl-drv.c @@ -1663,7 +1663,7 @@ module_param_named(swcrypto, iwlwifi_mod_params.sw_crypto, int, S_IRUGO); MODULE_PARM_DESC(swcrypto, "using crypto in software (default 0 [hardware])"); module_param_named(11n_disable, iwlwifi_mod_params.disable_11n, uint, S_IRUGO); MODULE_PARM_DESC(11n_disable, - "disable 11n functionality, bitmap: 1: full, 2: agg TX, 4: agg RX"); + "disable 11n functionality, bitmap: 1: full, 2: disable agg TX, 4: disable agg RX, 8 enable agg TX"); module_param_named(amsdu_size_8K, iwlwifi_mod_params.amsdu_size_8K, int, S_IRUGO); MODULE_PARM_DESC(amsdu_size_8K, "enable 8K amsdu size (default 0)"); diff --git a/drivers/net/wireless/iwlwifi/iwl-modparams.h b/drivers/net/wireless/iwlwifi/iwl-modparams.h index 1d2f793..34ce808 100644 --- a/drivers/net/wireless/iwlwifi/iwl-modparams.h +++ b/drivers/net/wireless/iwlwifi/iwl-modparams.h @@ -79,9 +79,12 @@ enum iwl_power_level { IWL_POWER_NUM }; -#define IWL_DISABLE_HT_ALL BIT(0) -#define IWL_DISABLE_HT_TXAGG BIT(1) -#define IWL_DISABLE_HT_RXAGG BIT(2) +enum iwl_disable_11n { + IWL_DISABLE_HT_ALL = BIT(0), + IWL_DISABLE_HT_TXAGG = BIT(1), + IWL_DISABLE_HT_RXAGG = BIT(2), + IWL_ENABLE_HT_TXAGG = BIT(3), +}; /** * struct iwl_mod_params @@ -90,7 +93,7 @@ enum iwl_power_level { * * @sw_crypto: using hardware encryption, default = 0 * @disable_11n: disable 11n capabilities, default = 0, - * use IWL_DISABLE_HT_* constants + * use IWL_[DIS,EN]ABLE_HT_* constants * @amsdu_size_8K: enable 8K amsdu size, default = 0 * @restart_fw: restart firmware, default = 1 * @wd_disable: disable stuck queue check, default = 1 diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c index 19925bb..068f785 100644 --- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c +++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c @@ -468,6 +468,24 @@ static void iwl_mvm_mac_tx(struct ieee80211_hw *hw, ieee80211_free_txskb(hw, skb); } +static inline bool iwl_enable_rx_ampdu(const struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) + return false; + return true; +} + +static inline bool iwl_enable_tx_ampdu(const struct iwl_cfg *cfg) +{ + if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) + return false; + if (iwlwifi_mod_params.disable_11n & IWL_ENABLE_HT_TXAGG) + return true; + + /* enabled by default */ + return true; +} + static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, enum ieee80211_ampdu_mlme_action action, @@ -487,7 +505,7 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, switch (action) { case IEEE80211_AMPDU_RX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_RXAGG) { + if (!iwl_enable_rx_ampdu(mvm->cfg)) { ret = -EINVAL; break; } @@ -497,7 +515,7 @@ static int iwl_mvm_mac_ampdu_action(struct ieee80211_hw *hw, ret = iwl_mvm_sta_rx_agg(mvm, sta, tid, 0, false); break; case IEEE80211_AMPDU_TX_START: - if (iwlwifi_mod_params.disable_11n & IWL_DISABLE_HT_TXAGG) { + if (!iwl_enable_tx_ampdu(mvm->cfg)) { ret = -EINVAL; break; } -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: pull request: iwlwifi 2014-02-12 (FIXED) 2014-02-12 9:08 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach 2014-02-12 9:09 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach @ 2014-02-12 11:02 ` Emmanuel Grumbach 1 sibling, 0 replies; 19+ messages in thread From: Emmanuel Grumbach @ 2014-02-12 11:02 UTC (permalink / raw) To: John Linville; +Cc: linux-wireless, ilw >> Hi John, >> >> One single patch for this pull request for 3.14. >> >> As explicitly written in the commit message, we prefer to disable Tx >> AMPDU on NICs supported by iwldvm. This feature gives a big boost in Tx >> performance, but the firmware is buggy and we can't rely on it. >> Our hope is that most of the users out there want wifi to surf on the >> web which means that they care more for Rx traffic than for Tx. >> People who want to enable it can do so with a simple Kconfig entry. >> Rx AMPDU is still enabled by default. >> >> Please pull - thanks. >> >> emmanuel >> >> The following changes since commit c512865446e6dd5b6e91e81187e75b734ad7cfc7: >> >> iwlwifi: mvm: don't allow A band if SKU forbids it (2014-02-02 >> 11:08:42 +0200) >> >> are available in the git repository at: >> >> >> git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git >> for-john >> >> for you to fetch changes up to 930e106d90ca902c8ede0a8cb206cee1ef3c5cc0: >> > > I had to edit the patch - 63f1809aa672dc439e7750b551ff9926a79312e0 > John - as you can see Stanislaw isn't happy with this. Please don't pull this right now - I'll try to find an agreed solution. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-02-13 12:30 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-12 8:51 pull request: iwlwifi 2013-02-12 Emmanuel Grumbach 2014-02-12 8:54 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 9:10 ` Stanislaw Gruszka 2014-02-12 9:21 ` Stanislaw Gruszka 2014-02-12 9:35 ` Grumbach, Emmanuel 2014-02-12 9:43 ` Sujith Manoharan 2014-02-12 9:45 ` Grumbach, Emmanuel 2014-02-12 9:53 ` Sujith Manoharan 2014-02-12 10:41 ` Stanislaw Gruszka 2014-02-12 9:08 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach 2014-02-12 9:09 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 11:19 ` [RFC] iwlwifi: refactor the TX / RX ampdu override Emmanuel Grumbach 2014-02-12 12:13 ` Stanislaw Gruszka 2014-02-12 12:27 ` Grumbach, Emmanuel 2014-02-12 14:04 ` Stanislaw Gruszka 2014-02-12 20:28 ` Emmanuel Grumbach 2014-02-13 7:54 ` Stanislaw Gruszka 2014-02-13 12:28 ` [PATCH] iwlwifi: disable TX AMPDU by default for iwldvm Emmanuel Grumbach 2014-02-12 11:02 ` pull request: iwlwifi 2014-02-12 (FIXED) Emmanuel Grumbach
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.