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 2A15369F9 for ; Wed, 8 Jun 2016 12:21:28 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 08 Jun 2016 03:21:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,438,1459839600"; d="scan'208";a="971341110" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.95]) by orsmga001.jf.intel.com with SMTP; 08 Jun 2016 03:21:25 -0700 Received: by (sSMTP sendmail emulation); Wed, 08 Jun 2016 11:21:24 +0025 Date: Wed, 8 Jun 2016 11:21:23 +0100 From: Bruce Richardson To: Stephen Hurd Cc: dev@dpdk.org, ajit.khaparde@broadcom.com Message-ID: <20160608102123.GB11724@bricha3-MOBL3> References: <1465250923-78695-1-git-send-email-stephen.hurd@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1465250923-78695-1-git-send-email-stephen.hurd@broadcom.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v4 01/39] bnxt: new driver for Broadcom NetXtreme-C devices 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: Wed, 08 Jun 2016 10:21:29 -0000 On Mon, Jun 06, 2016 at 03:08:05PM -0700, Stephen Hurd wrote: > From: Ajit Khaparde > > This patch adds the initial skeleton for bnxt driver along with the > nic guide to tie into the build system. > At this point, the driver simply fails init. > > v4: > Fix a warning that the document isn't included in any toctree > Also remove a PCI ID added erroneously. > > Signed-off-by: Ajit Khaparde > Reviewed-by: David Christensen > Signed-off-by: Stephen Hurd > --- Hi Stephen, Ajit, in the absense of a cover letter, I'll post my overall comments on this set here. Thanks for the updated v4, I'm not seeing any checkpatch issues with the patches that have applied and compiled up cleanly. However, * the build is broken by patch 30, and none of the later patches 31-38 seem to fix it for me. Is there a header file include missing in that patch or something? [I'm using gcc 5.3.1 on Fedora 23] * patch 39 fails to apply for me with rejects on other files in the driver, which is very strange. [drivers/net/bnxt/bnxt_hwrm.c, drivers/net/bnxt/bnxt_ring.c and drivers/net/bnxt/bnxt_ring.h] Apart from this, the other concern I still have is with the explanation accompaning some of the patches, especially for those to with rings. There are many patches throughout the set which seem to be doing the same thing, adding allocate and free functions for rings. For example: Patch 28 is titled "add ring alloc, free and group init". For a start it's unclear from the title, whether the alloc and free refers to individual rings or to the groups. If it's referring to the rings themselves, then how is this different functionality from: Patch 7: add ring structs and free() func Patch 10/11: add TX/RX queue create/destroy operations Patch 15: code to alloc/free ring Patch 24: add HWRM ring alloc/free functions Or if it's to do with allocating and freeing the groups, it would seem to be the same functionality as patch 25: "add ring group alloc/free functions". In some cases, the commit message does add some detail, e.g. patches 7 and 10 point out what they don't cover, but the rest is still very unclear, as to what each of the 5/6 patches for ring create/free are really doing and how they work together. I'm not sure exactly how best to do this without understanding the details of these patches, but one way might be to list out the different part of the ring allocation/free in each patch and then explain what part of that process this patch is doing and how it fits in the sequence. Otherwise, maybe some of the patches may need to be merged if they are very closely related. Can you please look to improve the commit messages when you do rework to fix the compilation and patch application errors. Thanks, /Bruce