Re: [PATCH v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues

From: Kalle Valo <kvalo@codeaurora.org>
To: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: linux-wireless@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
Date: Tue, 3 Dec 2019 09:03:48 +0000	[thread overview]
Message-ID: <0101016ecb005b15-cd83cdda-61c0-46bd-86e5-ae4449c195ef-000000@us-west-2.amazonses.com> (raw)
In-Reply-To: <20191203080849.12013-1-emmanuel.grumbach@intel.com> (Emmanuel Grumbach's message of "Tue, 3 Dec 2019 10:08:49 +0200")

Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> The purpose of this was to keep all the queues updated with
> the Rx sequence numbers because unlikely yet possible
> situations where queues can't understand if a specific
> packet needs to be dropped or not.
>
> Unfortunately, it was reported that this caused issues in
> our DMA engine. We don't fully understand how this is related,
> but this is being currently debugged. For now, just don't send
> this notification to the Rx queues. This de-facto reverts my
> commit 3c514bf831ac12356b695ff054bef641b9e99593:
>
> iwlwifi: mvm: add a loose synchronization of the NSSN across Rx queues
>
> This issue was reported here:
> https://bugzilla.kernel.org/show_bug.cgi?id=204873
> https://bugzilla.kernel.org/show_bug.cgi?id=205001
> and others maybe.
>
> Fixes: 3c514bf831ac ("iwlwifi: mvm: add a loose synchronization of the NSSN across Rx queues")
> CC: <stable@vger.kernel.org> # 5.3+
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---
> v2: fix an unused variable warning
> v3: don't comment out the code
> v4: fix checkpatch issues
> ---
>  .../wireless/intel/iwlwifi/mvm/constants.h    |  1 +
>  drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 19 +++++++++++--------
>  2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/constants.h b/drivers/net/wireless/intel/iwlwifi/mvm/constants.h
> index 60aff2ecec12..58df25e2fb32 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/constants.h
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/constants.h
> @@ -154,5 +154,6 @@
>  #define IWL_MVM_D3_DEBUG			false
>  #define IWL_MVM_USE_TWT				false
>  #define IWL_MVM_AMPDU_CONSEC_DROPS_DELBA	10
> +#define IWL_MVM_USE_NSSN_SYNC			0
>  

[...]

>  static void iwl_mvm_sync_nssn(struct iwl_mvm *mvm, u8 baid, u16 nssn)
>  {
> -	struct iwl_mvm_rss_sync_notif notif = {
> -		.metadata.type = IWL_MVM_RXQ_NSSN_SYNC,
> -		.metadata.sync = 0,
> -		.nssn_sync.baid = baid,
> -		.nssn_sync.nssn = nssn,
> -	};
> -
> -	iwl_mvm_sync_rx_queues_internal(mvm, (void *)&notif, sizeof(notif));
> +	if (IWL_MVM_USE_NSSN_SYNC) {
> +		struct iwl_mvm_rss_sync_notif notif = {
> +			.metadata.type = IWL_MVM_RXQ_NSSN_SYNC,
> +			.metadata.sync = 0,
> +			.nssn_sync.baid = baid,
> +			.nssn_sync.nssn = nssn,
> +		};
> +
> +		iwl_mvm_sync_rx_queues_internal(mvm, (void *)&notif,
> +						sizeof(notif));
> +	}

This is dead code, which is frowned upon and we most likely get cleanup
patches removing it in no time. Please just remove the code, and maybe
even the function. You can easily add it back with git-revert when you
want to fix it. Let's not leave dead code lying around.

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