From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 76CFE2934 for ; Tue, 23 Feb 2016 16:13:09 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 23 Feb 2016 07:13:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,489,1449561600"; d="scan'208";a="919368506" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.208.64]) by orsmga002.jf.intel.com with SMTP; 23 Feb 2016 07:13:05 -0800 Received: by (sSMTP sendmail emulation); Tue, 23 Feb 2016 15:13:04 +0025 Date: Tue, 23 Feb 2016 15:13:04 +0000 From: Bruce Richardson To: dev@dpdk.org Message-ID: <20160223151304.GA17644@bricha3-MOBL3> References: <1454063522-1948-1-git-send-email-adrien.mazarguil@6wind.com> <1454063522-1948-5-git-send-email-adrien.mazarguil@6wind.com> <20160217171343.GA11736@bricha3-MOBL3> <20160218161016.GQ27079@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160218161016.GQ27079@6wind.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 4/5] mlx5: add support for flow director X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Feb 2016 15:13:09 -0000 On Thu, Feb 18, 2016 at 05:10:16PM +0100, Adrien Mazarguil wrote: > Hi Bruce, > > On Wed, Feb 17, 2016 at 05:13:44PM +0000, Bruce Richardson wrote: > > On Fri, Jan 29, 2016 at 11:32:01AM +0100, Adrien Mazarguil wrote: > > > From: Yaacov Hazan > > > > > > Add support for flow director filters (RTE_FDIR_MODE_PERFECT and > > > RTE_FDIR_MODE_PERFECT_MAC_VLAN modes). > > > > > > This feature requires MLNX_OFED 3.2. > > > > > > Signed-off-by: Yaacov Hazan > > > Signed-off-by: Adrien Mazarguil > > > --- > > Hi Adrien, Yaacov, > > > > this patch raises a lot of warnings (17) with checkpatch. Can you perhaps look > > to see if this number can be reduced. > > We actually did it before submitting that patch, there is indeed a bunch of > remaining warnings that have been left on purpose. Not sure if we have the > same configuration for checkpatch, but they should fall into the following > categories: > > - "WARNING: return of an errno should typically be negative" - All return > values are documented in the code. Since this PMD uses syscalls in its > control path, it uses positive errno values internally for > consistency. Public callback functions however return negative error > values. > > - "WARNING: line over 80 characters" - Well, although I'm a big fan of the > 80 characters limit, breaking those would have made the code harder to > read. > > - "WARNING: Missing a blank line after declarations" - It's actually a > declaration through a macro, there is no missing blank line. > > - "WARNING: networking block comments don't use an empty /* line" - Not sure > if we really care? I don't particularly mind. > > - "CHECK: Comparison to NULL could be written "!" - I do not mind either, > writing the full check seems clearer to me. > > - "CHECK: Unnecessary parentheses around fdir_info->mask" - Looks like a > valid, although minor error. > > Please tell me which of these still need to be fixed. > > -- Hi Adrien, sorry for the delayed reply, I was out for a couple of days. As none of the above are errors, I'm not going to mandate that they be fixed before I merge in the patch, so long as you as maintainer are happy with them. My request mainly came about because of the sheer number of warnings that were being flagged. To keep the codebase clean requires constant discipline, so I don't like seeing patches where 17 warnings are flagged. I was hoping since a new rev of the set was likely anyway that some steps could be taken to reduce that number. Thomas, any thoughts here, since I'm still "learning the ropes" as committer. Do you have any rules-of-thumb or guidelines as regards checkpatch warnings? The contributor guide only seems to cover running checkpatch, not anything about what to do with the output. Regards, /Bruce