> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com] > Sent: Friday, 15 September 2023 17.55 > > On 9/8/2023 5:07 PM, Mykola Kostenok wrote: > > From: Christian Koue Muf <c...@napatech.com> > > > > The NTNIC PMD does not rely on a kernel space Napatech driver, > > thus all defines related to the register layout is part of the PMD > > code, which will be added in later commits. > > > > Signed-off-by: Christian Koue Muf <c...@napatech.com> > > Reviewed-by: Mykola Kostenok <mko-...@napatech.com> > > > > Hi Mykola, Christiam, > > This PMD scares me, overall it is a big drop: > "249 files changed, 87128 insertions(+)" > > I think it is not possible to review all in one release cycle, and it is > not even possible to say if all code used or not. > > I can see code is already developed, and it is difficult to restructure > developed code, but restructure it into small pieces really helps for > reviews. > > > Driver supports good list of features, can it be possible to distribute > upstream effort into multiple release. > Starting from basic functionality and add features gradually. > Target for this release can be providing datapath, and add more if we > have time in the release, what do you think? > > > Also there are large amount of base code (HAL / FPGA code), instead of > adding them as a bulk, relevant ones with a feature can be added with > the feature patch, this eliminates dead code in the base code layer, > also helps user/review to understand the link between driver code and > base code.
Jumping in here with an opinion about welcoming new NIC vendors to the community: Generally, if a NIC vendor supplies a PMD for their NIC, I expect the vendor to take responsibility for the quality of the PMD, including providing a maintainer and support backporting of fixes to the PMD in LTS releases. This should align with the vendor's business case for upstreaming their driver. If the vendor provides one big patch series, which may be difficult to understand/review, the fallout mainly hits the vendor's customers (and thus the vendor's support organization), not the community as a whole. We, the community, should not make it too difficult for vendors trying to upstream their drivers. I certainly consider it unreasonable to ask a vendor to postpone the release of some existing features by effectively an entire year (considering that only LTS releases are relevant for most of us) because we want the vendor to refactor the patch series to match our preferences within an unrealistic timeframe. > > > As far as I understand last patch opens a socket interface and an > external application can sent control commands via this interface. > I am not sure about this side control channel, what is missing in the > DPDK API? Can we try to address them in the DPDK layer instead of a > driver specific solution? That would be great. AFAIK, other vendors also has a bunch of out-of-band communication, e.g. magical EAL parameters to the MLX drivers. So let's not be too hard on the newcomers. ;-) > > > Thanks, > ferruh Thank you, Ferruh, for taking good care of the community by providing constructive feedback like this to new NIC vendors! Please note that my feedback is entirely process related. I didn’t review the driver, so I have no technical comments to the patch series. -Morten
