Re: [PATCH v2] net/i40e: support enabling/disabling source pruning

[Thomas Monjalon <thomas@monjalon.net>]

31/01/2023 04:28, Zhang, Ke1X:
> From: David Marchand <david.marchand@redhat.com>
> > On Mon, Jan 30, 2023 at 9:23 AM Ke Zhang <ke1x.zhang@intel.com> wrote:
> > >
> > > VRRP advertisement packets are dropped on i40e PF devices because
> > when
> > > a MAC address is added to a device, packets originating from that MAC
> > > address are dropped.
> > >
> > > This patch adds a PMD specific API to enable/disable source pruning to
> > > fix above issue.
> > >
> > > Bugzilla ID: 648
> > >
> > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > > ---
> > >  app/test-pmd/cmdline.c          | 84
> > +++++++++++++++++++++++++++++++++
> > >  drivers/net/i40e/i40e_ethdev.c  | 43 +++++++++++++++++
> > > drivers/net/i40e/i40e_ethdev.h  |  1 +
> > > drivers/net/i40e/rte_pmd_i40e.c | 20 ++++++++
> > > drivers/net/i40e/rte_pmd_i40e.h | 16 +++++++
> > >  5 files changed, 164 insertions(+)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > cb8c174020..76a574affd 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -776,6 +776,9 @@ static void cmd_help_long_parsed(void
> > > *parsed_result,
> > >
> > >                         "port cleanup (port_id) txq (queue_id) (free_cnt)\n"
> > >                         "    Cleanup txq mbufs for a specific Tx queue\n\n"
> > > +
> > > +                       "port config (port_id|all) src_prune (on|off)\n"
> > > +                       "    Set source prune on port_id, or all.\n\n"
> > >                 );
> > >         }
> > >
> > 
> > - This seems i40e specific, please move to drivers/net/i40e/i40e_testpmd.c.
> > 
> > - Besides, I would prefer that something in the command name clearly states
> > this is driver (here, i40e) specific.
> > Like "port config XX i40e_src_prune" or maybe the other way around, start
> > with a "driver i40e" prefix.
> > 
> > Maybe others have an opinion.
> > 
> > 
> > --
> > David Marchand
> 
> Thanks for your comments, this command could be used for other Intel NIC like ice,
> And It is only finished for i40e in this story: https://jira.devtools.intel.com/browse/DPDK-29564

Only Intel has access to this URL.

On the principle, you should not update testpmd commands in a patch for i40e.
We add testpmd commands when adding new features in ethdev.

Here specifically, you are adding rte_pmd_i40e_set_src_prune()
with a vague explanation: "Enable/Disable source prune on all the PF."
Without more information, I cannot judge whether the feature is generic or not.
So for now, this is a nack.