From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D27925F2A for ; Fri, 23 Mar 2018 11:41:02 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Mar 2018 03:41:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,349,1517904000"; d="scan'208";a="41523212" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.63]) ([10.237.221.63]) by orsmga001.jf.intel.com with ESMTP; 23 Mar 2018 03:41:00 -0700 To: Somnath Kotur Cc: dev@dpdk.org References: <20180222025822.18421-1-somnath.kotur@broadcom.com> <8712ecbf-57d5-121f-2171-2a573a49871f@intel.com> From: Ferruh Yigit Message-ID: <091eb993-5f3e-f56e-61f3-1b53ab409b5d@intel.com> Date: Fri, 23 Mar 2018 10:41:00 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Mar 2018 10:41:04 -0000 On 3/23/2018 3:34 AM, Somnath Kotur wrote: > Hi Ferruh, > > > On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit > 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 > > > <...> > > > @@ -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