Stable Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
@ 2019-11-20 13:26 Emmanuel Grumbach
  2019-11-21 18:32 ` Kalle Valo
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Emmanuel Grumbach @ 2019-11-20 13:26 UTC (permalink / raw)
  To: linux-wireless; +Cc: luciano.coelho, Emmanuel Grumbach, stable

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>
---
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 75a7af5ad7b2..8925fe5976cb 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -521,7 +521,11 @@ static void iwl_mvm_sync_nssn(struct iwl_mvm *mvm, u8 baid, u16 nssn)
 		.nssn_sync.nssn = nssn,
 	};
 
-	iwl_mvm_sync_rx_queues_internal(mvm, (void *)&notif, sizeof(notif));
+	/*
+	 * This allow to synchronize the queues, but it has been reported
+	 * to cause FH issues. Don't send the notification for now.
+	 * iwl_mvm_sync_rx_queues_internal(mvm, (void *)&notif, sizeof(notif));
+	 */
 }
 
 #define RX_REORDER_BUF_TIMEOUT_MQ (HZ / 10)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-11-20 13:26 [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues Emmanuel Grumbach
@ 2019-11-21 18:32 ` Kalle Valo
  2019-11-21 18:45 ` [PATCH v2] " Emmanuel Grumbach
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2019-11-21 18:32 UTC (permalink / raw)
  To: Emmanuel Grumbach
  Cc: linux-wireless, luciano.coelho, Emmanuel Grumbach, stable

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

> 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>

New warning:

drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c: In function 'iwl_mvm_sync_nssn':
drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c:517:32: warning: unused variable 'notif' [-Wunused-variable]
  struct iwl_mvm_rss_sync_notif notif = {
                                ^~~~~

Patch set to Changes Requested.

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

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-11-20 13:26 [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues Emmanuel Grumbach
  2019-11-21 18:32 ` Kalle Valo
@ 2019-11-21 18:45 ` " Emmanuel Grumbach
  2019-12-02 14:43   ` Kalle Valo
  2019-12-03  7:58 ` [PATCH v3] " Emmanuel Grumbach
  2019-12-03  8:08 ` [PATCH v4] " Emmanuel Grumbach
  3 siblings, 1 reply; 8+ messages in thread
From: Emmanuel Grumbach @ 2019-11-21 18:45 UTC (permalink / raw)
  To: linux-wireless; +Cc: Emmanuel Grumbach, stable

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: avoid the unused variable warning
---
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 75a7af5ad7b2..392bfa4b496c 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -514,14 +514,20 @@ static bool iwl_mvm_is_sn_less(u16 sn1, u16 sn2, u16 buffer_size)
 
 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));
+	/*
+	 * This allow to synchronize the queues, but it has been reported
+	 * to cause FH issues. Don't send the notification for now.
+	 *
+	 * 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));
+	 */
 }
 
 #define RX_REORDER_BUF_TIMEOUT_MQ (HZ / 10)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-11-21 18:45 ` [PATCH v2] " Emmanuel Grumbach
@ 2019-12-02 14:43   ` Kalle Valo
  0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2019-12-02 14:43 UTC (permalink / raw)
  To: Emmanuel Grumbach; +Cc: linux-wireless, stable

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: avoid the unused variable warning
> ---
>  drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 22 ++++++++++++-------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> index 75a7af5ad7b2..392bfa4b496c 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> @@ -514,14 +514,20 @@ static bool iwl_mvm_is_sn_less(u16 sn1, u16 sn2, u16 buffer_size)
>  
>  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));
> +	/*
> +	 * This allow to synchronize the queues, but it has been reported
> +	 * to cause FH issues. Don't send the notification for now.
> +	 *
> +	 * 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));
> +	 */

Please don't comment out code, instead remove entirely. You can find the
old code from the git history anyway.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-11-20 13:26 [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues Emmanuel Grumbach
  2019-11-21 18:32 ` Kalle Valo
  2019-11-21 18:45 ` [PATCH v2] " Emmanuel Grumbach
@ 2019-12-03  7:58 ` " Emmanuel Grumbach
  2019-12-03  8:08 ` [PATCH v4] " Emmanuel Grumbach
  3 siblings, 0 replies; 8+ messages in thread
From: Emmanuel Grumbach @ 2019-12-03  7:58 UTC (permalink / raw)
  To: linux-wireless; +Cc: Emmanuel Grumbach, stable

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

Use the regular constant infra we have to make it easy to
re-enable this mechanism when the firmware will be able to cope
with the DMA stall.

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
---
 .../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
 
 #endif /* __MVM_CONSTANTS_H */
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 75a7af5ad7b2..27d15fa6fe04 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -514,14 +514,17 @@ static bool iwl_mvm_is_sn_less(u16 sn1, u16 sn2, u16 buffer_size)
 
 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));
+	}
 }
 
 #define RX_REORDER_BUF_TIMEOUT_MQ (HZ / 10)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-11-20 13:26 [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues Emmanuel Grumbach
                   ` (2 preceding siblings ...)
  2019-12-03  7:58 ` [PATCH v3] " Emmanuel Grumbach
@ 2019-12-03  8:08 ` " Emmanuel Grumbach
  2019-12-03  9:03   ` Kalle Valo
  3 siblings, 1 reply; 8+ messages in thread
From: Emmanuel Grumbach @ 2019-12-03  8:08 UTC (permalink / raw)
  To: linux-wireless; +Cc: Emmanuel Grumbach, stable

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
 
 #endif /* __MVM_CONSTANTS_H */
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 75a7af5ad7b2..0bfb7295d6ae 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -514,14 +514,17 @@ static bool iwl_mvm_is_sn_less(u16 sn1, u16 sn2, u16 buffer_size)
 
 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));
+	}
 }
 
 #define RX_REORDER_BUF_TIMEOUT_MQ (HZ / 10)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-12-03  8:08 ` [PATCH v4] " Emmanuel Grumbach
@ 2019-12-03  9:03   ` Kalle Valo
  2019-12-03  9:22     ` Luca Coelho
  0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2019-12-03  9:03 UTC (permalink / raw)
  To: Emmanuel Grumbach; +Cc: linux-wireless, stable

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
  2019-12-03  9:03   ` Kalle Valo
@ 2019-12-03  9:22     ` Luca Coelho
  0 siblings, 0 replies; 8+ messages in thread
From: Luca Coelho @ 2019-12-03  9:22 UTC (permalink / raw)
  To: Kalle Valo, Emmanuel Grumbach; +Cc: linux-wireless, stable

On Tue, 2019-12-03 at 09:03 +0000, Kalle Valo wrote:
> 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.

Hi Kalle,

We already have a system like this where we have some constants to
enable/disable things or hardcode certain parameters in our driver. 
The main reason for having this is that we have a debugging/maturation
system internally where we can enable and disable such options
dynamically (actually with a specific .ini file we create).

Some advantages of doing this are:

1. The code that goes upstream is not so different from our internal
development trees;

2. By reducing the delta from the internal tree to upstream, we reduce
the merge conflicts, rebase and merge damages etc.;

3. It's easy to enable and disable features that are not completely
mature in the FW without having to diverge much (again) from the
internal tree, because we need to support it internally while it's
still under development in the driver;

4. It's easy to tell the community how to enable or disable a feature
when we need help debugging;

5. All of this is to make it easier for us to have an upstream driver
that is as close as possible to our internal development trees. :)


I hope this makes sense.  If not, one of the alternatives would be to
convert this macro into a Kconfig option, but I think that just
clutters things and the code would still be dead until we tell people
that it's stable enough to use it...

What do you think?

--
Cheers,
Luca.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 13:26 [PATCH] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues Emmanuel Grumbach
2019-11-21 18:32 ` Kalle Valo
2019-11-21 18:45 ` [PATCH v2] " Emmanuel Grumbach
2019-12-02 14:43   ` Kalle Valo
2019-12-03  7:58 ` [PATCH v3] " Emmanuel Grumbach
2019-12-03  8:08 ` [PATCH v4] " Emmanuel Grumbach
2019-12-03  9:03   ` Kalle Valo
2019-12-03  9:22     ` Luca Coelho

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org
	public-inbox-index stable

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git