From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f51.google.com (mail-it0-f51.google.com [209.85.214.51]) by dpdk.org (Postfix) with ESMTP id 7A0745B32 for ; Fri, 23 Mar 2018 12:41:29 +0100 (CET) Received: by mail-it0-f51.google.com with SMTP id d13-v6so2244525itf.0 for ; Fri, 23 Mar 2018 04:41:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=UhY/+yRLcG1cidmSqcEHb4y90du2VySARyZhwyw/p10=; b=USZqx2MrQEeAwFtEoxhEeJ+QztlmzZqsS/Cvv/GaZ6UT7MsP2GQPCSfNId6Mc1BVE3 oaN6/ErFu0doEauoyM5I2MYONCDAsCoMWCvO3y8xJB3EVH/wQ852qeeBAdTDkmQj5ZSR NL/9f+5Q3CYXy5or9cel7zxVEoqu1oaiFuwzo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UhY/+yRLcG1cidmSqcEHb4y90du2VySARyZhwyw/p10=; b=Xsr6+mjeLcLGPC8mF5uhtpCdWn0HEnuCGeMDKEt2SwdglIoWjdYN6HB7CODfr7RUCT zVCpOdYChpYngODVHnpgrxPtkdx7pOuJ+ZxWOJmXRzw6s0pNQU0+Sv11uoYeY1pNcSzE Qrcy269MHHBDAtvWqPP4bzGRNWovnwAg88Vwp/CLMMi/4yfE7h3oBINL7LcjXNeLZyGg Tn247hB+r5ika0nR3ZbuxL7kB4sbHX8wNvZG25Vr3Iz7Ei0G/orwXD9v9+txGeCe79ON by12jpieP+PuiMNmNbATvgynevIlE0avkkJi8mf9YFD7msrZZyiBSq+OZlVAZBEaV9ku kQ4A== X-Gm-Message-State: AElRT7EsL1DhAnjm1/LJgBY9HlmF8Qw2CYIj+Oh5xbXlO1Gl12IFH7yd c2J1lYzXuWEYgzQkW9UCnfRBY9XSO1cczLSW/mNYQg== X-Google-Smtp-Source: AG47ELvr5I3BW+/sEKXJP3L2oNFBcBVff9M37K+tCD7YQF0Vb+m5OGK5tchTl6bRzXDpnoG3CX6lihNPh9DM7Bm3wQs= X-Received: by 2002:a24:9302:: with SMTP id y2-v6mr12443086itd.24.1521805288736; Fri, 23 Mar 2018 04:41:28 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.168.163 with HTTP; Fri, 23 Mar 2018 04:41:08 -0700 (PDT) In-Reply-To: <091eb993-5f3e-f56e-61f3-1b53ab409b5d@intel.com> References: <20180222025822.18421-1-somnath.kotur@broadcom.com> <8712ecbf-57d5-121f-2171-2a573a49871f@intel.com> <091eb993-5f3e-f56e-61f3-1b53ab409b5d@intel.com> From: Somnath Kotur Date: Fri, 23 Mar 2018 17:11:08 +0530 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 11:41:29 -0000 On Fri, Mar 23, 2018 at 4:11 PM, Ferruh Yigit wrote: > 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 ..." :) > 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 > > > > > > <...> > > > > > @@ -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 > >