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

[Kalle Valo <kvalo@codeaurora.org>]

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 *)&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...

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