Re: [dpdk-dev] [PATCH v4 01/39] bnxt: new driver for Broadcom NetXtreme-C devices

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jun 08, 2016 at 11:21:23AM +0100, Bruce Richardson wrote:
> On Mon, Jun 06, 2016 at 03:08:05PM -0700, Stephen Hurd wrote:
> > From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > 
> > 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 <ajit.khaparde@broadcom.com>
> > Reviewed-by: David Christensen <david.christensen@broadcom.com>
> > Signed-off-by: Stephen Hurd <stephen.hurd@broadcom.com>
> > ---
> 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]

Sorry, my bad here, the build is not broken, the patch 30 now applies fine
and compiles ok. I had somehow missed applying an earlier patch (patch 28)
after reviewing it, and that causes the failures.

Please ignore the above comments.

> 
> 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.

Since the build break was my mistake, a new rev of the patches may not be
absolutely necessary. If it's more convenient and is not too complicated, you
can perhaps just post updated comments for the above-mentioned patches to the
list and I can fix up the commit messages on patch apply. However, the patches
and commit messages are quite confusing to read as they are right now.

Thanks,
/Bruce
> 
> Thanks,
> /Bruce