Re: [PATCH v5 01/25] net: move intel drivers to intel subdirectory

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

On Thu, Jan 23, 2025 at 03:16:40PM +0100, David Marchand wrote:
> Hello Bruce, Thomas,
> 
> On Mon, Jan 20, 2025 at 1:00 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > Consolidate all Intel HW NIC drivers into a driver/net/intel  This
> > matches the layout used for drivers in the kernel, and potentially
> > enabling easier sharing among drivers.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> - This deserves a RN entry has it impacts how users select compiled drivers.
> 
> 
> - Trying to select net/intel/* triggers a meson error:
> 
> $ meson configure build -Denable_drivers=net/intel/*
> $ ninja -C build
> ...
> Message: drivers/net/intel/i40e: Defining dependency "net_i40e"
> 
> ../drivers/net/intel/iavf/meson.build:33:46: ERROR: Unknown variable
> "static_rte_common_iavf".
> 

Thanks for catching that. I suspect in my testing with intel/* I also added
common/iavf out of habit! Will fix in v6.

> A full log can be found at
> /home/dmarchan/git/pub/dpdk.org/dedup/build/meson-logs/meson-log.txt
> FAILED: build.ninja
> /usr/bin/meson --internal regenerate /home/dmarchan/git/pub/dpdk.org/dedup .
> 
> 
> - I see some remaining references to the old path. One is to be fixed:
> doc/guides/nics/ice.rst:  These ICE_DBG_XXX are defined in
> ``drivers/net/ice/base/ice_type.h``.
> 

Ack, will fix.

> 
> - Thomas, please have a look at this part.
> 
> On the check-git-log.sh update, we will have many warnings with current update.
> 
> Wrong headline prefix:
>     net/intel/common: add pkt reassembly fn for intel drivers
>     net/intel/common: provide common Tx entry structures
>     net/intel/common: add Tx mbuf ring replenish fn
>     net/intel: align Tx queue struct field names
>     net/intel: add prefix for driver-specific structs
>     net/intel/common: merge ice and i40e Tx queue struct
>     net/iavf: use common Tx queue structure
>     net/ixgbe: use common Tx queue structure
>     net/intel/common: pack Tx queue structure
>     net/intel/common: add post-Tx buffer free function
>     net/intel/common: add Tx buffer free fn for AVX-512
>     net/iavf: use common Tx free fn for AVX-512
>     net/ice: move Tx queue mbuf cleanup fn to common
>     net/iavf: use common Tx queue mbuf cleanup fn
>     net/ice: use vector SW ring for all vector paths
>     net/intel/common: remove unneeded code
>     net/intel/common: create common mbuf initializer fn
>     net/intel/common: extract common Rx vector criteria
> 
> Invalid patch(es) found - checked 25 patches
> 
> I tried to tweak this a bit, with the following heuristic:
> * if touching only net/intel/common, accept net/intel/common:
> * if touching multiple drivers under net/intel, then accept net/intel:
> as prefix,
> * if touching some net/intel/$drv (and optionnally net/intel/common),
> accept net/$drv,
> 
> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index b2da013f6c..41c290f0ca 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -79,11 +79,18 @@ bad=$(for commit in $commits ; do
>         [ -z "$(echo "$files" | grep -v '^\(drivers\|doc\|config\)/')" ] ||
>                 continue
>         drv=$(echo "$files" | grep '^drivers/' | cut -d "/" -f 2,3 | sort -u)
> -       # for drivers/net/intel/* use 2nd and 4th fields not 2nd and 3rd
>         if [ "$drv" = "net/intel" ] ; then
> -               drv=$(echo "$files" | grep '^drivers/' | cut -d "/" -f
> 2,4 | sort -u)
> +               drvgrp="net/intel"
> +               drv=$(echo "$files" | grep '^drivers/' | grep -v
> '^drivers/net/intel/common' |
> +                       cut -d "/" -f 2,4 | sort -u)
> +               if [ $(echo "$drv" | wc -l) -eq 0 ] ; then
> +                       drv='net/intel/common:'
> +               elif [ $(echo "$drv" | wc -l) -gt 1 ] ; then
> +                       drv='net/intel:'
> +               fi
> +       else
> +               drvgrp=$(echo "$drv" | cut -d "/" -f 1 | uniq)
>         fi
> -       drvgrp=$(echo "$drv" | cut -d "/" -f 1 | uniq)
>         if [ $(echo "$drvgrp" | wc -l) -gt 1 ] ; then
>                 echo "$headline" | grep -v '^drivers:'
>         elif [ $(echo "$drv" | wc -l) -gt 1 ] ; then
> 
> Which then complains on patches in this series that touch many drivers
> (but have net/intel/common: as prefix where I would suggest net/intel:
> instead).
> 

I tend to disagree with this suggestion. I think that a prefix can be valid
so long as the prefix matches at least one component in the patch.  For example,
for the first patch in the set, I think net/intel/common is a better prefix
than just "net/intel". I don't massively object to your suggestion, I just
prefer patches identify the most relevant component, if possible, rather
than generalities.

> Wrong headline prefix:
>     net/intel/common: add pkt reassembly fn for intel drivers
>     net/intel/common: provide common Tx entry structures
>     net/intel/common: add Tx mbuf ring replenish fn
>     net/intel/common: merge ice and i40e Tx queue struct
>     net/intel/common: pack Tx queue structure
>     net/intel/common: add post-Tx buffer free function
>     net/intel/common: add Tx buffer free fn for AVX-512
>     net/iavf: use common Tx free fn for AVX-512
>     net/iavf: use common Tx queue mbuf cleanup fn
>     net/intel/common: remove unneeded code
>     net/intel/common: create common mbuf initializer fn
>     net/intel/common: extract common Rx vector criteria
> 
> 
> 
> -- 
> David Marchand
>