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

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

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]

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