Luca Coelho <luca@xxxxxxxxx> writes: > On Tue, 2019-12-03 at 09:03 +0000, Kalle Valo wrote: >> Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> # 5.3+ >> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx> >> > --- >> > 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. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
