Re: [PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Valo, Kalle]

Kalle Valo  writes:

>> Since we've been working on this for so long and already use the term
>> DQA broadly, we thought it wouldn't be necessary to explain more when
>> we are finally enabling it by default.  But of course I can change that
>> if you prefer.
>
> I guessed I could find more information from the history and I know this
> is obvious to your team, but it's not obvious to everyone. The commit
> log should always answer the questions "Why?" and this isn't answering
> that. For example, I need this information when sending pull requests to
> Dave and I'm sure lots of other people find it useful as well,
> especially when enabling a new feature.
>
> So I'm not asking for a long essay, something like this would be
> adequate:
>
> "Run DQA flows by default, as long as the FW supports it. It's currently
> supported on 1234, 3456 and 7654, maybe more in the future. DQA improves
> latency when X is used or throughput when Y is disabled. On the downside
> it sometimes slows down throughput when using Z but that's still
> accetable as it's so rarely used."

Oh, I forgot that the acronym should be also spelled out in the commit
log.

-- 
Kalle Valo

Re: [PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Kalle Valo]

"Coelho, Luciano"  writes:

> Hi Kalle,
>
> On Wed, 2016-10-26 at 09:32 +0300, Kalle Valo wrote:
>> Luca Coelho  writes:
>> 
>> > From: Liad Kaufman 
>> > 
>> > Run DQA flows by default, as long as the FW supports it.
>> > 
>> > Signed-off-by: Liad Kaufman 
>> > Signed-off-by: Luca Coelho 
>> 
>> What's this DQA mode? A short summary would have been nice why it's
>> useful.
>
> DQA is Dynamic Queue Allocation.  We have been working on this for
> quite a while now (since v4.7) and have many patches mentioning that
> are already merged.  The first patch is this:
>
> commit 24afba7690e4 ("iwlwifi: mvm: support bss dynamic alloc/dealloc
> of queues")

Yeah, but that's commited over six month ago.

> Its commit message has some explanation and we also have a DOC section
> in our driver that explains it in further details:
>
> https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/sta.h#n82

And that's too detailed. I was hoping more like an executive summary in
few sentences.

> Since we've been working on this for so long and already use the term
> DQA broadly, we thought it wouldn't be necessary to explain more when
> we are finally enabling it by default.  But of course I can change that
> if you prefer.

I guessed I could find more information from the history and I know this
is obvious to your team, but it's not obvious to everyone. The commit
log should always answer the questions "Why?" and this isn't answering
that. For example, I need this information when sending pull requests to
Dave and I'm sure lots of other people find it useful as well,
especially when enabling a new feature.

So I'm not asking for a long essay, something like this would be
adequate:

"Run DQA flows by default, as long as the FW supports it. It's currently
supported on 1234, 3456 and 7654, maybe more in the future. DQA improves
latency when X is used or throughput when Y is disabled. On the downside
it sometimes slows down throughput when using Z but that's still
accetable as it's so rarely used."

But no need to change anything for this commit, but just keep in mind
for the future.

-- 
Kalle Valo

Re: [PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Coelho, Luciano]

Hi Kalle,

On Wed, 2016-10-26 at 09:32 +0300, Kalle Valo wrote:
> Luca Coelho  writes:
> 
> > From: Liad Kaufman 
> > 
> > Run DQA flows by default, as long as the FW supports it.
> > 
> > Signed-off-by: Liad Kaufman 
> > Signed-off-by: Luca Coelho 
> 
> What's this DQA mode? A short summary would have been nice why it's
> useful.

DQA is Dynamic Queue Allocation.  We have been working on this for
quite a while now (since v4.7) and have many patches mentioning that
are already merged.  The first patch is this:

commit 24afba7690e4 ("iwlwifi: mvm: support bss dynamic alloc/dealloc
of queues")

Its commit message has some explanation and we also have a DOC section
in our driver that explains it in further details:

https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/sta.h#n82

Since we've been working on this for so long and already use the term
DQA broadly, we thought it wouldn't be necessary to explain more when
we are finally enabling it by default.  But of course I can change that
if you prefer.

--
Cheers,
Luca.

Re: [PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Kalle Valo]

Luca Coelho  writes:

> From: Liad Kaufman 
>
> Run DQA flows by default, as long as the FW supports it.
>
> Signed-off-by: Liad Kaufman 
> Signed-off-by: Luca Coelho 

What's this DQA mode? A short summary would have been nice why it's
useful.

-- 
Kalle Valo

[PATCH 09/10] iwlwifi: mvm: operate in dqa mode

[Luca Coelho]

From: Liad Kaufman 

Run DQA flows by default, as long as the FW supports it.

Signed-off-by: Liad Kaufman 
Signed-off-by: Luca Coelho 
---
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h 
b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index 726ba48..cde8c6c 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -,9 +,8 @@ static inline bool iwl_mvm_is_d0i3_supported(struct 
iwl_mvm *mvm)
 
 static inline bool iwl_mvm_is_dqa_supported(struct iwl_mvm *mvm)
 {
-   /* Make sure DQA isn't allowed in driver until feature is complete */
-   return false && fw_has_capa(>fw->ucode_capa,
-   IWL_UCODE_TLV_CAPA_DQA_SUPPORT);
+   return fw_has_capa(>fw->ucode_capa,
+  IWL_UCODE_TLV_CAPA_DQA_SUPPORT);
 }
 
 static inline bool iwl_mvm_enter_d0i3_on_suspend(struct iwl_mvm *mvm)
-- 
2.9.3