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