Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/23/2018 3:34 AM, Somnath Kotur wrote:
> Hi Ferruh,
> 
> 
> On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 2/22/2018 2:58 AM, Somnath Kotur wrote:
> 
>     Please start with lowercase after "net/bnxt: fix"
> 
>     > When user reissues same flow director cmd with a different queue
>     > update the existing filter to redirect flow to the new desired
>     > queue as destination just like the other filters like 5 tuple and
>     > generic flow.
> 
>     Can you please add a fixes line?
> 
> Actually, I'm not sure if this qualifies as a 'fix for a regression', it
> prescribes a new behavior for this scenario (of same flow-director cmd ,
> different queue)  we never handled this before at all , Do you still think it
> needs it ?

Patch title starts with "fix bug with ..." :)

Previously it was returning error when filter exist, now it is updating the
existing filter with new destination. If this is fix or not depends on expected
behavior [1].

Practical reasons of having fixes tag:
1- Stable trees checks for Fixes tag and gets patch to stable trees, if you want
you patch backported the tag is required.

2- To help other developers that wants to work on your code, these tags helps to
trace easier and understand code easier.

3- Helps reviewers understand the scope and help on prioritization.


If this is not fix please drop "fix" from patch title and be aware that this
won't be included into stable trees. If this is a fix please add the fixes line.


[1]
technically a code requires update:
- To add a new feature (justify a new expectation)
- To refactor (improve readability, improve re-usability etc.. but same
functionality)
- To fix the functionality to make it behave as expected.

It is hard to claim a fix without clear expectations / requirements. And I think
code updates triggered because of expectation changes fits into first category.

> 
> 
>     Also ./devtools/check-git-log.sh complains about long title, what about
>     something like (just a sample):
>     "net/bnxt: fix flow director with same cmd different queue"
> 
>     >
>     > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com
>     <mailto:somnath.kotur@broadcom.com>>
> 
>     <...>
> 
>     > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
>     >                       goto free_filter;
>     >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
>     >
>     > -             match = bnxt_match_fdir(bp, filter);
>     > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
>     > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
>     > +             else
>     > +                     vnic =
>     > +                     STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
> 
>     Is this done because of column limit? If so I would prefer a few extra chars
>     instead of this assignment.
> 
> Yes, please thank you , it was going over by 1 character :) 
> 
> 
>     btw, not related to this patch, but in this switch there are a few "/*
>     FALLTHROUGH */" comments but they may not be required (or wrong), can you please
>     check.
> 
> Sure , will do. 
> 
>     <...>
> 
> 
> Thanks
> Som