From: Luca Coelho <[email protected]>
Hi,
This is my first patchset with fixes for v5.7.
The changes are:
* Remove ACK enabled aggregation support flag, since we never really
supported it;
* A few fixes for the queues configuration on the 9000 family of
devices that were causing FW hangs;
* Fix an RCU issue;
* A fix for the TCM statistics gathering code;
As usual, I'm pushing this to a pending branch, for kbuild bot. And
since these are fixes for the rc series, feel free to take them
directly to wireless-drivers.git.
Cheers,
Luca.
Ilan Peer (1):
iwlwifi: mvm: Do not declare support for ACK Enabled Aggregation
Johannes Berg (5):
iwlwifi: pcie: actually release queue memory in TVQM
iwlwifi: pcie: indicate correct RB size to device
iwlwifi: mvm: limit maximum queue appropriately
iwlwifi: mvm: fix inactive TID removal return value usage
iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU
Mordechay Goodstein (2):
iwlwifi: mvm: beacon statistics shouldn't go backwards
iwlwifi: msix: limit max RX queues for 9000 family
.../net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
.../net/wireless/intel/iwlwifi/iwl-nvm-parse.c | 6 ++----
drivers/net/wireless/intel/iwlwifi/iwl-trans.h | 1 +
drivers/net/wireless/intel/iwlwifi/mvm/rx.c | 13 +++++++++++--
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 17 ++++++++++-------
.../intel/iwlwifi/pcie/ctxt-info-gen3.c | 18 ++++++++++++++----
.../net/wireless/intel/iwlwifi/pcie/trans.c | 6 +++++-
.../net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 3 +++
8 files changed, 49 insertions(+), 21 deletions(-)
--
2.25.1
From: Johannes Berg <[email protected]>
The iwl_trans_pcie_dyn_txq_free() function only releases the frames
that may be left on the queue by calling iwl_pcie_gen2_txq_unmap(),
but doesn't actually free the DMA ring or byte-count tables for the
queue. This leads to pretty large memory leaks (at least before my
queue size improvements), in particular in monitor/sniffer mode on
channel hopping since this happens on every channel change.
This was also now more evident after the move to a DMA pool for the
byte count tables, showing messages such as
BUG iwlwifi:bc (...): Objects remaining in iwlwifi:bc on __kmem_cache_shutdown()
This fixes https://bugzilla.kernel.org/show_bug.cgi?id=206811.
Signed-off-by: Johannes Berg <[email protected]>
Fixes: 6b35ff91572f ("iwlwifi: pcie: introduce a000 TX queues management")
Cc: [email protected] # v4.14+
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
index 86fc00167817..9664dbc70ef1 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
@@ -1418,6 +1418,9 @@ void iwl_trans_pcie_dyn_txq_free(struct iwl_trans *trans, int queue)
iwl_pcie_gen2_txq_unmap(trans, queue);
+ iwl_pcie_gen2_txq_free_memory(trans, trans_pcie->txq[queue]);
+ trans_pcie->txq[queue] = NULL;
+
IWL_DEBUG_TX_QUEUES(trans, "Deactivate queue %d\n", queue);
}
--
2.25.1
From: Johannes Berg <[email protected]>
The function iwl_mvm_remove_inactive_tids() returns bool, so we
should just check "if (ret)", not "if (ret >= 0)" (which would
do nothing useful here). We obviously therefore cannot use the
return value of the function for the free_queue, we need to use
the queue (i) we're currently dealing with instead.
Cc: [email protected] # v5.4+
Signed-off-by: Johannes Berg <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 251d6fbb1da5..56ae72debb96 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -1169,9 +1169,9 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
inactive_tid_bitmap,
&unshare_queues,
&changetid_queues);
- if (ret >= 0 && free_queue < 0) {
+ if (ret && free_queue < 0) {
queue_owner = sta;
- free_queue = ret;
+ free_queue = i;
}
/* only unlock sta lock - we still need the queue info lock */
spin_unlock_bh(&mvmsta->lock);
--
2.25.1
From: Johannes Berg <[email protected]>
iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
some circumstances, so don't call it under RCU. There doesn't appear
to be a need for RCU protection around this particular call.
Cc: [email protected] # v5.4+
Signed-off-by: Johannes Berg <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 56ae72debb96..9ca433fdf634 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -1184,17 +1184,15 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
for_each_set_bit(i, &changetid_queues, IWL_MAX_HW_QUEUES)
iwl_mvm_change_queue_tid(mvm, i);
+ rcu_read_unlock();
+
if (free_queue >= 0 && alloc_for_sta != IWL_MVM_INVALID_STA) {
ret = iwl_mvm_free_inactive_queue(mvm, free_queue, queue_owner,
alloc_for_sta);
- if (ret) {
- rcu_read_unlock();
+ if (ret)
return ret;
- }
}
- rcu_read_unlock();
-
return free_queue;
}
--
2.25.1
From: Mordechay Goodstein <[email protected]>
There is an issue in the HW DMA engine in the 9000 family of devices
when more than 6 RX queues are used. The issue is that the FW may
hang when IWL_MVM_RXQ_NSSN_SYNC notifications are sent.
Fix this by limiting the number of RX queues to 6 in the 9000 family
of devices.
Cc: [email protected]
Signed-off-by: Mordechay Goodstein <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/iwl-trans.h | 1 +
drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 6 +++++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-trans.h b/drivers/net/wireless/intel/iwlwifi/iwl-trans.h
index bba527b339b5..ff5f6b67334a 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-trans.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-trans.h
@@ -316,6 +316,7 @@ static inline void iwl_free_rxb(struct iwl_rx_cmd_buffer *r)
#define IWL_MGMT_TID 15
#define IWL_FRAME_LIMIT 64
#define IWL_MAX_RX_HW_QUEUES 16
+#define IWL_9000_MAX_RX_HW_QUEUES 6
/**
* enum iwl_wowlan_status - WoWLAN image/device status
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index e4cbd8daa7c6..e291c60024c2 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1625,11 +1625,15 @@ iwl_pcie_set_interrupt_capa(struct pci_dev *pdev,
struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
int max_irqs, num_irqs, i, ret;
u16 pci_cmd;
+ u32 max_rx_queues = IWL_MAX_RX_HW_QUEUES;
if (!cfg_trans->mq_rx_supported)
goto enable_msi;
- max_irqs = min_t(u32, num_online_cpus() + 2, IWL_MAX_RX_HW_QUEUES);
+ if (cfg_trans->device_family <= IWL_DEVICE_FAMILY_9000)
+ max_rx_queues = IWL_9000_MAX_RX_HW_QUEUES;
+
+ max_irqs = min_t(u32, num_online_cpus() + 2, max_rx_queues);
for (i = 0; i < max_irqs; i++)
trans_pcie->msix_entries[i].entry = i;
--
2.25.1
From: Mordechay Goodstein <[email protected]>
We reset statistics also in case that we didn't reassoc so in
this cases keep last beacon counter.
Cc: [email protected] # v4.19+
Signed-off-by: Mordechay Goodstein <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/mvm/rx.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rx.c b/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
index 5ee33c8ae9d2..77b8def26edb 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
@@ -8,7 +8,7 @@
* Copyright(c) 2012 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
* Copyright(c) 2016 - 2017 Intel Deutschland GmbH
- * Copyright(c) 2018 - 2019 Intel Corporation
+ * Copyright(c) 2018 - 2020 Intel Corporation
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of version 2 of the GNU General Public License as
@@ -31,7 +31,7 @@
* Copyright(c) 2012 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
* Copyright(c) 2016 - 2017 Intel Deutschland GmbH
- * Copyright(c) 2018 - 2019 Intel Corporation
+ * Copyright(c) 2018 - 2020 Intel Corporation
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -566,6 +566,7 @@ void iwl_mvm_rx_rx_mpdu(struct iwl_mvm *mvm, struct napi_struct *napi,
struct iwl_mvm_stat_data {
struct iwl_mvm *mvm;
+ __le32 flags;
__le32 mac_id;
u8 beacon_filter_average_energy;
void *general;
@@ -606,6 +607,13 @@ static void iwl_mvm_stat_iterator(void *_data, u8 *mac,
-general->beacon_average_energy[vif_id];
}
+ /* make sure that beacon statistics don't go backwards with TCM
+ * request to clear statistics
+ */
+ if (le32_to_cpu(data->flags) & IWL_STATISTICS_REPLY_FLG_CLEAR)
+ mvmvif->beacon_stats.accu_num_beacons +=
+ mvmvif->beacon_stats.num_beacons;
+
if (mvmvif->id != id)
return;
@@ -763,6 +771,7 @@ void iwl_mvm_handle_rx_statistics(struct iwl_mvm *mvm,
flags = stats->flag;
}
+ data.flags = flags;
iwl_mvm_rx_stats_check_trigger(mvm, pkt);
--
2.25.1
From: Johannes Berg <[email protected]>
Due to some hardware issues, queue 32 isn't usable on devices that have
32 queues (7000, 8000, 9000 families), which is correctly reflected in
the configuration and TX queue initialization.
However, the firmware API and queue allocation code assumes that there
are 32 queues, and if something actually attempts to use #31 this leads
to a NULL-pointer dereference since it's not allocated.
Fix this by limiting to 31 in the IWL_MVM_DQA_MAX_DATA_QUEUE, and also
add some code to catch this earlier in the future, if the configuration
changes perhaps.
Cc: [email protected] # v4.9+
Signed-off-by: Johannes Berg <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 +++++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
index 73196cbc7fbe..75d958bab0e3 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
@@ -8,7 +8,7 @@
* Copyright(c) 2007 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
* Copyright(c) 2016 - 2017 Intel Deutschland GmbH
- * Copyright(c) 2019 Intel Corporation
+ * Copyright(c) 2019 - 2020 Intel Corporation
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of version 2 of the GNU General Public License as
@@ -31,7 +31,7 @@
* Copyright(c) 2005 - 2014 Intel Corporation. All rights reserved.
* Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
* Copyright(c) 2016 - 2017 Intel Deutschland GmbH
- * Copyright(c) 2019 Intel Corporation
+ * Copyright(c) 2019 - 2020 Intel Corporation
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -99,7 +99,7 @@ enum iwl_mvm_dqa_txq {
IWL_MVM_DQA_MAX_MGMT_QUEUE = 8,
IWL_MVM_DQA_AP_PROBE_RESP_QUEUE = 9,
IWL_MVM_DQA_MIN_DATA_QUEUE = 10,
- IWL_MVM_DQA_MAX_DATA_QUEUE = 31,
+ IWL_MVM_DQA_MAX_DATA_QUEUE = 30,
};
enum iwl_mvm_tx_fifo {
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index 64ef3f3ba23b..251d6fbb1da5 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -722,6 +722,11 @@ static int iwl_mvm_find_free_queue(struct iwl_mvm *mvm, u8 sta_id,
lockdep_assert_held(&mvm->mutex);
+ if (WARN(maxq >= mvm->trans->trans_cfg->base_params->num_of_queues,
+ "max queue %d >= num_of_queues (%d)", maxq,
+ mvm->trans->trans_cfg->base_params->num_of_queues))
+ maxq = mvm->trans->trans_cfg->base_params->num_of_queues - 1;
+
/* This should not be hit with new TX path */
if (WARN_ON(iwl_mvm_has_new_tx_api(mvm)))
return -ENOSPC;
--
2.25.1
From: Ilan Peer <[email protected]>
As this was not supposed to be enabled to begin with.
Cc: [email protected] # v4.19+
Signed-off-by: Ilan Peer <[email protected]>
Signed-off-by: Luca Coelho <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c b/drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c
index 9e9810d2b262..ccf0bc16465d 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-nvm-parse.c
@@ -532,8 +532,7 @@ static struct ieee80211_sband_iftype_data iwl_he_capa[] = {
IEEE80211_HE_MAC_CAP1_TF_MAC_PAD_DUR_16US |
IEEE80211_HE_MAC_CAP1_MULTI_TID_AGG_RX_QOS_8,
.mac_cap_info[2] =
- IEEE80211_HE_MAC_CAP2_32BIT_BA_BITMAP |
- IEEE80211_HE_MAC_CAP2_ACK_EN,
+ IEEE80211_HE_MAC_CAP2_32BIT_BA_BITMAP,
.mac_cap_info[3] =
IEEE80211_HE_MAC_CAP3_OMI_CONTROL |
IEEE80211_HE_MAC_CAP3_MAX_AMPDU_LEN_EXP_VHT_2,
@@ -617,8 +616,7 @@ static struct ieee80211_sband_iftype_data iwl_he_capa[] = {
IEEE80211_HE_MAC_CAP1_TF_MAC_PAD_DUR_16US |
IEEE80211_HE_MAC_CAP1_MULTI_TID_AGG_RX_QOS_8,
.mac_cap_info[2] =
- IEEE80211_HE_MAC_CAP2_BSR |
- IEEE80211_HE_MAC_CAP2_ACK_EN,
+ IEEE80211_HE_MAC_CAP2_BSR,
.mac_cap_info[3] =
IEEE80211_HE_MAC_CAP3_OMI_CONTROL |
IEEE80211_HE_MAC_CAP3_MAX_AMPDU_LEN_EXP_VHT_2,
--
2.25.1
I was looking into this as part of
https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
fix in flight. My concern was that queue_owner being used outside of
the RCU might be an issue as now you have no guaranty that the
eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
going to be valid. The only way to work around this is instead of
storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
*. If you open the bug you will see the latest version of my work as
the attached patch. I am not an RCU expert so I am curious to hear
your thoughts.
On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[email protected]> wrote:
>
> From: Johannes Berg <[email protected]>
>
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
>
> Cc: [email protected] # v5.4+
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> index 56ae72debb96..9ca433fdf634 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> @@ -1184,17 +1184,15 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
> for_each_set_bit(i, &changetid_queues, IWL_MAX_HW_QUEUES)
> iwl_mvm_change_queue_tid(mvm, i);
>
> + rcu_read_unlock();
> +
> if (free_queue >= 0 && alloc_for_sta != IWL_MVM_INVALID_STA) {
> ret = iwl_mvm_free_inactive_queue(mvm, free_queue, queue_owner,
> alloc_for_sta);
> - if (ret) {
> - rcu_read_unlock();
> + if (ret)
> return ret;
> - }
> }
>
> - rcu_read_unlock();
> -
> return free_queue;
> }
>
> --
> 2.25.1
>
On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <[email protected]> wrote:
>
> From: Johannes Berg <[email protected]>
>
> Due to some hardware issues, queue 32 isn't usable on devices that have
> 32 queues (7000, 8000, 9000 families), which is correctly reflected in
> the configuration and TX queue initialization.
This will not fix the issue on the 1000, 2000, 5000 and 6000 devices.
You need further protection on these as their are only 20
(IWLAGN_NUM_QUEUES) queues. I sent out a patch on March 19th with a
fix.
Mark
>
> However, the firmware API and queue allocation code assumes that there
> are 32 queues, and if something actually attempts to use #31 this leads
> to a NULL-pointer dereference since it's not allocated.
>
> Fix this by limiting to 31 in the IWL_MVM_DQA_MAX_DATA_QUEUE, and also
> add some code to catch this earlier in the future, if the configuration
> changes perhaps.
>
> Cc: [email protected] # v4.9+
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
> ---
> drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
> drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 +++++
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> index 73196cbc7fbe..75d958bab0e3 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> @@ -8,7 +8,7 @@
> * Copyright(c) 2007 - 2014 Intel Corporation. All rights reserved.
> * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> - * Copyright(c) 2019 Intel Corporation
> + * Copyright(c) 2019 - 2020 Intel Corporation
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of version 2 of the GNU General Public License as
> @@ -31,7 +31,7 @@
> * Copyright(c) 2005 - 2014 Intel Corporation. All rights reserved.
> * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> - * Copyright(c) 2019 Intel Corporation
> + * Copyright(c) 2019 - 2020 Intel Corporation
> * All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> @@ -99,7 +99,7 @@ enum iwl_mvm_dqa_txq {
> IWL_MVM_DQA_MAX_MGMT_QUEUE = 8,
> IWL_MVM_DQA_AP_PROBE_RESP_QUEUE = 9,
> IWL_MVM_DQA_MIN_DATA_QUEUE = 10,
> - IWL_MVM_DQA_MAX_DATA_QUEUE = 31,
> + IWL_MVM_DQA_MAX_DATA_QUEUE = 30,
> };
>
> enum iwl_mvm_tx_fifo {
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> index 64ef3f3ba23b..251d6fbb1da5 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> @@ -722,6 +722,11 @@ static int iwl_mvm_find_free_queue(struct iwl_mvm *mvm, u8 sta_id,
>
> lockdep_assert_held(&mvm->mutex);
>
> + if (WARN(maxq >= mvm->trans->trans_cfg->base_params->num_of_queues,
> + "max queue %d >= num_of_queues (%d)", maxq,
> + mvm->trans->trans_cfg->base_params->num_of_queues))
> + maxq = mvm->trans->trans_cfg->base_params->num_of_queues - 1;
> +
> /* This should not be hit with new TX path */
> if (WARN_ON(iwl_mvm_has_new_tx_api(mvm)))
> return -ENOSPC;
> --
> 2.25.1
>
On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[email protected]> wrote:
>
> From: Johannes Berg <[email protected]>
I sent Johannes part of this fix weeks ago and heard nothing back. I
am far from a glory hound but something is wrong with this list if
fixes are sat on for weeks and then the fix shows up with any
acknowledgment lost. At minimum a note saying that a fix existed and
would be merged shortly would have been nice.
Mark
>
> The function iwl_mvm_remove_inactive_tids() returns bool, so we
> should just check "if (ret)", not "if (ret >= 0)" (which would
> do nothing useful here). We obviously therefore cannot use the
> return value of the function for the free_queue, we need to use
> the queue (i) we're currently dealing with instead.
>
> Cc: [email protected] # v5.4+
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> index 251d6fbb1da5..56ae72debb96 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> @@ -1169,9 +1169,9 @@ static int iwl_mvm_inactivity_check(struct iwl_mvm *mvm, u8 alloc_for_sta)
> inactive_tid_bitmap,
> &unshare_queues,
> &changetid_queues);
> - if (ret >= 0 && free_queue < 0) {
> + if (ret && free_queue < 0) {
> queue_owner = sta;
> - free_queue = ret;
> + free_queue = i;
> }
> /* only unlock sta lock - we still need the queue info lock */
> spin_unlock_bh(&mvmsta->lock);
> --
> 2.25.1
>
On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <[email protected]> wrote:
>
> On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <[email protected]> wrote:
> >
> > From: Johannes Berg <[email protected]>
> >
> > Due to some hardware issues, queue 32 isn't usable on devices that have
> > 32 queues (7000, 8000, 9000 families),
Is this statement really correct? All these devices have 31 queues
according to (.num_of_queues = 31). Without a HW specification I can't
be 100% sure but you should have this information within Intel. From
the details of my patch and my investigation, this should be nack'd
along with an explanation as to why my fix is not valid.
Mark
> > which is correctly reflected in
> > the configuration and TX queue initialization.
>
> This will not fix the issue on the 1000, 2000, 5000 and 6000 devices.
> You need further protection on these as their are only 20
> (IWLAGN_NUM_QUEUES) queues. I sent out a patch on March 19th with a
> fix.
>
> Mark
>
> >
> > However, the firmware API and queue allocation code assumes that there
> > are 32 queues, and if something actually attempts to use #31 this leads
> > to a NULL-pointer dereference since it's not allocated.
> >
> > Fix this by limiting to 31 in the IWL_MVM_DQA_MAX_DATA_QUEUE, and also
> > add some code to catch this earlier in the future, if the configuration
> > changes perhaps.
> >
> > Cc: [email protected] # v4.9+
> > Signed-off-by: Johannes Berg <[email protected]>
> > Signed-off-by: Luca Coelho <[email protected]>
> > ---
> > drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
> > drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 +++++
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > index 73196cbc7fbe..75d958bab0e3 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > @@ -8,7 +8,7 @@
> > * Copyright(c) 2007 - 2014 Intel Corporation. All rights reserved.
> > * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > - * Copyright(c) 2019 Intel Corporation
> > + * Copyright(c) 2019 - 2020 Intel Corporation
> > *
> > * This program is free software; you can redistribute it and/or modify
> > * it under the terms of version 2 of the GNU General Public License as
> > @@ -31,7 +31,7 @@
> > * Copyright(c) 2005 - 2014 Intel Corporation. All rights reserved.
> > * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > - * Copyright(c) 2019 Intel Corporation
> > + * Copyright(c) 2019 - 2020 Intel Corporation
> > * All rights reserved.
> > *
> > * Redistribution and use in source and binary forms, with or without
> > @@ -99,7 +99,7 @@ enum iwl_mvm_dqa_txq {
> > IWL_MVM_DQA_MAX_MGMT_QUEUE = 8,
> > IWL_MVM_DQA_AP_PROBE_RESP_QUEUE = 9,
> > IWL_MVM_DQA_MIN_DATA_QUEUE = 10,
> > - IWL_MVM_DQA_MAX_DATA_QUEUE = 31,
> > + IWL_MVM_DQA_MAX_DATA_QUEUE = 30,
> > };
> >
> > enum iwl_mvm_tx_fifo {
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > index 64ef3f3ba23b..251d6fbb1da5 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > @@ -722,6 +722,11 @@ static int iwl_mvm_find_free_queue(struct iwl_mvm *mvm, u8 sta_id,
> >
> > lockdep_assert_held(&mvm->mutex);
> >
> > + if (WARN(maxq >= mvm->trans->trans_cfg->base_params->num_of_queues,
> > + "max queue %d >= num_of_queues (%d)", maxq,
> > + mvm->trans->trans_cfg->base_params->num_of_queues))
> > + maxq = mvm->trans->trans_cfg->base_params->num_of_queues - 1;
> > +
> > /* This should not be hit with new TX path */
> > if (WARN_ON(iwl_mvm_has_new_tx_api(mvm)))
> > return -ENOSPC;
> > --
> > 2.25.1
> >
On Fri, 2020-04-03 at 10:46 -0400, Mark Asselstine wrote:
> On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[email protected]> wrote:
> > From: Johannes Berg <[email protected]>
>
> I sent Johannes part of this fix weeks ago and heard nothing back. I
> am far from a glory hound but something is wrong with this list if
> fixes are sat on for weeks and then the fix shows up with any
> acknowledgment lost. At minimum a note saying that a fix existed and
> would be merged shortly would have been nice.
Uh, sorry. I really didn't have your fix on my radar when developing
this, and cannot even remember it now.
I guess I could've saved myself some work there ...
Sorry!
johannes
On Fri, 2020-04-03 at 20:58 +0200, Johannes Berg wrote:
> On Fri, 2020-04-03 at 10:46 -0400, Mark Asselstine wrote:
> > On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[email protected]> wrote:
> > > From: Johannes Berg <[email protected]>
> >
> > I sent Johannes part of this fix weeks ago and heard nothing back. I
> > am far from a glory hound but something is wrong with this list if
> > fixes are sat on for weeks and then the fix shows up with any
> > acknowledgment lost. At minimum a note saying that a fix existed and
> > would be merged shortly would have been nice.
>
> Uh, sorry. I really didn't have your fix on my radar when developing
> this, and cannot even remember it now.
>
> I guess I could've saved myself some work there ...
This is my fault, sorry. I've been sitting on patches sent to the list
for some time now. I have a big backlog of patches in our internal
tree to send out and have been prioritizing those before processing
patches coming from the community.
I'm finally catching up now with fixes for v5.7 (and stable) and I
promise to do better from now on.
My sincere apologies.
--
Cheers,
Luca.
On Fri, Apr 3, 2020 at 3:08 PM Luca Coelho <[email protected]> wrote:
>
> On Fri, 2020-04-03 at 20:58 +0200, Johannes Berg wrote:
> > On Fri, 2020-04-03 at 10:46 -0400, Mark Asselstine wrote:
> > > On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <[email protected]> wrote:
> > > > From: Johannes Berg <[email protected]>
> > >
> > > I sent Johannes part of this fix weeks ago and heard nothing back. I
> > > am far from a glory hound but something is wrong with this list if
> > > fixes are sat on for weeks and then the fix shows up with any
> > > acknowledgment lost. At minimum a note saying that a fix existed and
> > > would be merged shortly would have been nice.
> >
> > Uh, sorry. I really didn't have your fix on my radar when developing
> > this, and cannot even remember it now.
> >
> > I guess I could've saved myself some work there ...
>
> This is my fault, sorry. I've been sitting on patches sent to the list
> for some time now. I have a big backlog of patches in our internal
> tree to send out and have been prioritizing those before processing
> patches coming from the community.
>
> I'm finally catching up now with fixes for v5.7 (and stable) and I
> promise to do better from now on.
>
> My sincere apologies.
np. More than anything its the duplication of work that sucks. In the
end we all want the same goal, to improve the functionality of the
drivers so let's keep pushing forward with that. Stay safe and have a
good weekend.
Mark
>
> --
> Cheers,
> Luca.
>
On Fri, Apr 3, 2020 at 1:10 PM Mark Asselstine <[email protected]> wrote:
>
> On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <[email protected]> wrote:
> >
> > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <[email protected]> wrote:
> > >
> > > From: Johannes Berg <[email protected]>
> > >
> > > Due to some hardware issues, queue 32 isn't usable on devices that have
> > > 32 queues (7000, 8000, 9000 families),
>
> Is this statement really correct? All these devices have 31 queues
> according to (.num_of_queues = 31). Without a HW specification I can't
> be 100% sure but you should have this information within Intel. From
> the details of my patch and my investigation, this should be nack'd
> along with an explanation as to why my fix is not valid.
>
> Mark
>
> > > which is correctly reflected in
> > > the configuration and TX queue initialization.
> >
> > This will not fix the issue on the 1000, 2000, 5000 and 6000 devices.
Just correcting myself here. These use dvm so are OK, but I think we
still have a problem with the 7000, 8000 and 9000 series with the
change as is.
Mark
> > You need further protection on these as their are only 20
> > (IWLAGN_NUM_QUEUES) queues. I sent out a patch on March 19th with a
> > fix.
> >
> > Mark
> >
> > >
> > > However, the firmware API and queue allocation code assumes that there
> > > are 32 queues, and if something actually attempts to use #31 this leads
> > > to a NULL-pointer dereference since it's not allocated.
> > >
> > > Fix this by limiting to 31 in the IWL_MVM_DQA_MAX_DATA_QUEUE, and also
> > > add some code to catch this earlier in the future, if the configuration
> > > changes perhaps.
> > >
> > > Cc: [email protected] # v4.9+
> > > Signed-off-by: Johannes Berg <[email protected]>
> > > Signed-off-by: Luca Coelho <[email protected]>
> > > ---
> > > drivers/net/wireless/intel/iwlwifi/fw/api/txq.h | 6 +++---
> > > drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 5 +++++
> > > 2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > index 73196cbc7fbe..75d958bab0e3 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/txq.h
> > > @@ -8,7 +8,7 @@
> > > * Copyright(c) 2007 - 2014 Intel Corporation. All rights reserved.
> > > * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > > * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > > - * Copyright(c) 2019 Intel Corporation
> > > + * Copyright(c) 2019 - 2020 Intel Corporation
> > > *
> > > * This program is free software; you can redistribute it and/or modify
> > > * it under the terms of version 2 of the GNU General Public License as
> > > @@ -31,7 +31,7 @@
> > > * Copyright(c) 2005 - 2014 Intel Corporation. All rights reserved.
> > > * Copyright(c) 2013 - 2015 Intel Mobile Communications GmbH
> > > * Copyright(c) 2016 - 2017 Intel Deutschland GmbH
> > > - * Copyright(c) 2019 Intel Corporation
> > > + * Copyright(c) 2019 - 2020 Intel Corporation
> > > * All rights reserved.
> > > *
> > > * Redistribution and use in source and binary forms, with or without
> > > @@ -99,7 +99,7 @@ enum iwl_mvm_dqa_txq {
> > > IWL_MVM_DQA_MAX_MGMT_QUEUE = 8,
> > > IWL_MVM_DQA_AP_PROBE_RESP_QUEUE = 9,
> > > IWL_MVM_DQA_MIN_DATA_QUEUE = 10,
> > > - IWL_MVM_DQA_MAX_DATA_QUEUE = 31,
> > > + IWL_MVM_DQA_MAX_DATA_QUEUE = 30,
> > > };
> > >
> > > enum iwl_mvm_tx_fifo {
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > index 64ef3f3ba23b..251d6fbb1da5 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> > > @@ -722,6 +722,11 @@ static int iwl_mvm_find_free_queue(struct iwl_mvm *mvm, u8 sta_id,
> > >
> > > lockdep_assert_held(&mvm->mutex);
> > >
> > > + if (WARN(maxq >= mvm->trans->trans_cfg->base_params->num_of_queues,
> > > + "max queue %d >= num_of_queues (%d)", maxq,
> > > + mvm->trans->trans_cfg->base_params->num_of_queues))
> > > + maxq = mvm->trans->trans_cfg->base_params->num_of_queues - 1;
> > > +
> > > /* This should not be hit with new TX path */
> > > if (WARN_ON(iwl_mvm_has_new_tx_api(mvm)))
> > > return -ENOSPC;
> > > --
> > > 2.25.1
> > >
On Fri, 2020-04-03 at 13:10 -0400, Mark Asselstine wrote:
> On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <[email protected]> wrote:
> > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <[email protected]> wrote:
> > > From: Johannes Berg <[email protected]>
> > >
> > > Due to some hardware issues, queue 32 isn't usable on devices that have
> > > 32 queues (7000, 8000, 9000 families),
>
> Is this statement really correct?
No, it should've said "queue 31" since they're numbered 0-based ...
> All these devices have 31 queues
> according to (.num_of_queues = 31).
Well, they were supposed to have 32, but there's some issue with the
last one. I don't really even remember what's up with it, but we just
never use it.
> Without a HW specification I can't
> be 100% sure but you should have this information within Intel. From
> the details of my patch and my investigation, this should be nack'd
> along with an explanation as to why my fix is not valid.
I don't see any real difference to your fix? Your fix marks them as used
before, whereas mine just avoids looking at them.
johannes
On Tue, 2020-04-14 at 13:29 +0200, Johannes Berg wrote:
> On Fri, 2020-04-03 at 13:10 -0400, Mark Asselstine wrote:
> > On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <[email protected]> wrote:
> > > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <[email protected]> wrote:
> > > > From: Johannes Berg <[email protected]>
> > > >
> > > > Due to some hardware issues, queue 32 isn't usable on devices that have
> > > > 32 queues (7000, 8000, 9000 families),
> >
> > Is this statement really correct?
>
> No, it should've said "queue 31" since they're numbered 0-based ...
I will fix this in the commit message and send v2 of the entire series
today.
--
Cheers,
Luca.
On Fri, 2020-04-03 at 11:29 +0300, Luca Coelho wrote:
> From: Mordechay Goodstein <[email protected]>
>
> There is an issue in the HW DMA engine in the 9000 family of devices
> when more than 6 RX queues are used. The issue is that the FW may
> hang when IWL_MVM_RXQ_NSSN_SYNC notifications are sent.
>
> Fix this by limiting the number of RX queues to 6 in the 9000 family
> of devices.
>
> Cc: [email protected]
> Signed-off-by: Mordechay Goodstein <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
> ---
As Emmanuel pointed out, we have disabled NSSN in the driver for now,
so this doesn't have to go to -fixes and stable. I'll drop it in v2 of
this patchset.
--
Cheers,
Luca.
On Fri, 2020-04-03 at 10:28 -0400, Mark Asselstine wrote:
> I was looking into this as part of
> https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
> fix in flight. My concern was that queue_owner being used outside of
> the RCU might be an issue as now you have no guaranty that the
> eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
> going to be valid. The only way to work around this is instead of
> storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
> adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
> whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
> *. If you open the bug you will see the latest version of my work as
> the attached patch. I am not an RCU expert so I am curious to hear
> your thoughts.
I asked Johannes to check your comment. For now, I'm going to drop it
from v2 of this patchset, so we can go ahead with the other patches.
--
Cheers,
Luca.
Sorry, missed your comment.
On Fri, 2020-04-03 at 10:28 -0400, Mark Asselstine wrote:
> I was looking into this as part of
> https://bugzilla.kernel.org/show_bug.cgi?id=206971 and had a similar
> fix in flight. My concern was that queue_owner being used outside of
> the RCU might be an issue
Yes, that does *look* questionable, and I missed it completely.
However, that's only because the code makes weak assumptions when it's
under much stronger guarantees. There's no reason for it to use RCU here
for the station lookup, since it's holding the write-side lock (which is
the mvm->mutex).
IOW, we could just change
sta = rcu_dereference(mvm->fw_id_to_mac_id[sta_id]);
to
sta = rcu_dereference_protected(mvm->fw_id_to_mac_id[sta_id], ...);
and then it's clear that there's no issue.
> as now you have no guaranty that the
> eventual use of sta->txq[tid] in iwl_mvm_free_inactive_queue() is
> going to be valid. The only way to work around this is instead of
> storing queue_owner, store mvmtxq = iwl_mvm_txq_from_tid(sta, i), then
> adjust iwl_mvm_free_inactive_queue(), iwl_mvm_disable_txq() and
> whatnot to take struct iwl_mvm_txq * instead of struct ieee80211_sta
> *. If you open the bug you will see the latest version of my work as
> the attached patch. I am not an RCU expert so I am curious to hear
> your thoughts.
You could do that too, but it seems overly complex to me.
johannes
Luca Coelho <[email protected]> wrote:
> From: Johannes Berg <[email protected]>
>
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
>
> Cc: [email protected] # v5.4+
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
I'll queue this for v5.8.
--
https://patchwork.kernel.org/patch/11472099/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Luca Coelho <[email protected]> wrote:
> From: Johannes Berg <[email protected]>
>
> iwl_mvm_free_inactive_queue() will sleep in synchronize_net() under
> some circumstances, so don't call it under RCU. There doesn't appear
> to be a need for RCU protection around this particular call.
>
> Cc: [email protected] # v5.4+
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Luca Coelho <[email protected]>
Patch applied to wireless-drivers.git, thanks.
fbb1461ad1d6 iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU
--
https://patchwork.kernel.org/patch/11472099/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches