On Fri, Mar 23, 2018 at 4:11 PM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
> On 3/23/2018 3:34 AM, Somnath Kotur wrote: > > Hi Ferruh, > > > > > > On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yi...@intel.com > > <mailto:ferruh.yi...@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 ..." :) > Yes i was aware of the irony, while i was asking the question, was just thinking /introspecting aloud :) Thank you for the detailed explanation. All things considered, i think it's better for me to right now go with the 'fix' since it helps in so many ways that i hadn't considered ... So will send out the respin soon with the 'fix' intact > > 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.ko...@broadcom.com > > <mailto:somnath.ko...@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 > >
