[PATCH v5.7 0/8] iwlwifi: fixes intended for v5.7 2020-04-03

[PATCH v5.7 0/8] iwlwifi: fixes intended for v5.7 2020-04-03

[Luca Coelho]

From: Luca Coelho <luciano.coelho@intel.com>

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

[PATCH v5.7 1/8] iwlwifi: pcie: actually release queue memory in TVQM

[Luca Coelho]

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

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 <johannes.berg@intel.com>
Fixes: 6b35ff91572f ("iwlwifi: pcie: introduce a000 TX queues management")
Cc: stable@vger.kernel.org # v4.14+
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

[PATCH v5.7 2/8] iwlwifi: mvm: beacon statistics shouldn't go backwards

[Luca Coelho]

From: Mordechay Goodstein <mordechay.goodstein@intel.com>

We reset statistics also in case that we didn't reassoc so in
this cases keep last beacon counter.

Cc: stable@vger.kernel.org # v4.19+
Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

[PATCH v5.7 3/8] iwlwifi: pcie: indicate correct RB size to device

[Luca Coelho]

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

In the context info, we need to indicate the correct RB size
to the device so that it will not think we have 4k when we
only use 2k. This seems to not have caused any issues right
now, likely because the hardware no longer supports putting
multiple entries into a single RB, and practically all of
the entries should be smaller than 2k.

Nevertheless, it's a bug, and we must advertise the right
size to the device.

Note that right now we can only tell it 2k vs. 4k, so for
the cases where we have more, still use 4k. This needs to
be fixed by the firmware first.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Fixes: cfdc20efebdc ("iwlwifi: pcie: use partial pages if applicable")
Cc: stable@vger.kernel.org # v5.6
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 .../intel/iwlwifi/pcie/ctxt-info-gen3.c        | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info-gen3.c b/drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info-gen3.c
index 01f248ba8fec..9d5b1e51b50d 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info-gen3.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/ctxt-info-gen3.c
@@ -129,6 +129,18 @@ int iwl_pcie_ctxt_info_gen3_init(struct iwl_trans *trans,
 	int cmdq_size = max_t(u32, IWL_CMD_QUEUE_SIZE,
 			      trans->cfg->min_txq_size);
 
+	switch (trans_pcie->rx_buf_size) {
+	case IWL_AMSDU_DEF:
+		return -EINVAL;
+	case IWL_AMSDU_2K:
+		break;
+	case IWL_AMSDU_4K:
+	case IWL_AMSDU_8K:
+	case IWL_AMSDU_12K:
+		control_flags |= IWL_PRPH_SCRATCH_RB_SIZE_4K;
+		break;
+	}
+
 	/* Allocate prph scratch */
 	prph_scratch = dma_alloc_coherent(trans->dev, sizeof(*prph_scratch),
 					  &trans_pcie->prph_scratch_dma_addr,
@@ -143,10 +155,8 @@ int iwl_pcie_ctxt_info_gen3_init(struct iwl_trans *trans,
 		cpu_to_le16((u16)iwl_read32(trans, CSR_HW_REV));
 	prph_sc_ctrl->version.size = cpu_to_le16(sizeof(*prph_scratch) / 4);
 
-	control_flags = IWL_PRPH_SCRATCH_RB_SIZE_4K |
-			IWL_PRPH_SCRATCH_MTR_MODE |
-			(IWL_PRPH_MTR_FORMAT_256B &
-			 IWL_PRPH_SCRATCH_MTR_FORMAT);
+	control_flags |= IWL_PRPH_SCRATCH_MTR_MODE;
+	control_flags |= IWL_PRPH_MTR_FORMAT_256B & IWL_PRPH_SCRATCH_MTR_FORMAT;
 
 	/* initialize RX default queue */
 	prph_sc_ctrl->rbd_cfg.free_rbd_addr =
-- 
2.25.1

[PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Luca Coelho]

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

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: stable@vger.kernel.org # v4.9+
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Mark Asselstine]

On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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: stable@vger.kernel.org # v4.9+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  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
>

Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Mark Asselstine]

On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <asselsm@gmail.com> wrote:
>
> On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
> >
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > 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: stable@vger.kernel.org # v4.9+
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> >  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
> >

Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Mark Asselstine]

On Fri, Apr 3, 2020 at 1:10 PM Mark Asselstine <asselsm@gmail.com> wrote:
>
> On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <asselsm@gmail.com> wrote:
> >
> > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
> > >
> > > From: Johannes Berg <johannes.berg@intel.com>
> > >
> > > 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: stable@vger.kernel.org # v4.9+
> > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > > ---
> > >  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
> > >

Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Johannes Berg]

On Fri, 2020-04-03 at 13:10 -0400, Mark Asselstine wrote:
> On Fri, Apr 3, 2020 at 10:38 AM Mark Asselstine <asselsm@gmail.com> wrote:
> > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > 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

Re: [PATCH v5.7 4/8] iwlwifi: mvm: limit maximum queue appropriately

[Luca Coelho]

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 <asselsm@gmail.com> wrote:
> > > On Fri, Apr 3, 2020 at 4:32 AM Luca Coelho <luca@coelho.fi> wrote:
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > 
> > > > 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.

[PATCH v5.7 5/8] iwlwifi: mvm: Do not declare support for ACK Enabled Aggregation

[Luca Coelho]

From: Ilan Peer <ilan.peer@intel.com>

As this was not supposed to be enabled to begin with.

Cc: stable@vger.kernel.org # v4.19+
Signed-off-by: Ilan Peer <ilan.peer@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

[PATCH v5.7 6/8] iwlwifi: msix: limit max RX queues for 9000 family

[Luca Coelho]

From: Mordechay Goodstein <mordechay.goodstein@intel.com>

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: stable@vger.kernel.org
Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

Re: [PATCH v5.7 6/8] iwlwifi: msix: limit max RX queues for 9000 family

[Luca Coelho]

On Fri, 2020-04-03 at 11:29 +0300, Luca Coelho wrote:
> From: Mordechay Goodstein <mordechay.goodstein@intel.com>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---

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.

[PATCH v5.7 7/8] iwlwifi: mvm: fix inactive TID removal return value usage

[Luca Coelho]

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

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: stable@vger.kernel.org # v5.4+
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

Re: [PATCH v5.7 7/8] iwlwifi: mvm: fix inactive TID removal return value usage

[Mark Asselstine]

On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <luca@coelho.fi> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>

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: stable@vger.kernel.org # v5.4+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  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
>

Re: [PATCH v5.7 7/8] iwlwifi: mvm: fix inactive TID removal return value usage

[Johannes Berg]

On Fri, 2020-04-03 at 10:46 -0400, Mark Asselstine wrote:
> On Fri, Apr 3, 2020 at 4:31 AM Luca Coelho <luca@coelho.fi> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> 
> 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

Re: [PATCH v5.7 7/8] iwlwifi: mvm: fix inactive TID removal return value usage

[Luca Coelho]

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 <luca@coelho.fi> wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > 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.

Re: [PATCH v5.7 7/8] iwlwifi: mvm: fix inactive TID removal return value usage

[Mark Asselstine]

On Fri, Apr 3, 2020 at 3:08 PM Luca Coelho <luca@coelho.fi> 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 <luca@coelho.fi> wrote:
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > >
> > > 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.
>

[PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Luca Coelho]

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

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: stable@vger.kernel.org # v5.4+
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 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

Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Mark Asselstine]

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 <luca@coelho.fi> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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: stable@vger.kernel.org # v5.4+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  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
>

Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Luca Coelho]

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.

Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Johannes Berg]

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

Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Kalle Valo]

Luca Coelho <luca@coelho.fi> wrote:

> From: Johannes Berg <johannes.berg@intel.com>
> 
> 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: stable@vger.kernel.org # v5.4+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

I'll queue this for v5.8.

-- 
https://patchwork.kernel.org/patch/11472099/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v5.7 8/8] iwlwifi: mvm: don't call iwl_mvm_free_inactive_queue() under RCU

[Kalle Valo]

Luca Coelho <luca@coelho.fi> wrote:

> From: Johannes Berg <johannes.berg@intel.com>
> 
> 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: stable@vger.kernel.org # v5.4+
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

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