| Message ID | 20191203080849.12013-1-emmanuel.grumbach@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d829229e35f302fd49c052b5c5906c90ecf9911d |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v4] iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues | expand |
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 *)¬if, 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 *)¬if, > + 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.
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 *)¬if, 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 *)¬if, > > + 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.
Luca Coelho <luca@coelho.fi> writes: > 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 *)¬if, 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 *)¬if, >> > + 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... Yeah, Kconfig option is even worse. > What do you think? As long as I have been around here a clear rule has been that no dead code should be left in kernel code. I understand your points but I'm not sure if they justify leaving dead code around.
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> Patch applied to wireless-drivers.git, thanks. d829229e35f3 iwlwifi: mvm: don't send the IWL_MVM_RXQ_NSSN_SYNC notif to Rx queues
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 *)¬if, 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 *)¬if, + sizeof(notif)); + } } #define RX_REORDER_BUF_TIMEOUT_MQ (HZ / 10)
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(-)