Re: [dpdk-dev] [PATCH] app/testpmd: support QinQ strip offload

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Ferruh,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, July 19, 2019 5:53 PM
> To: viveksharma@marvell.com; dev@dpdk.org
> Cc: intoviveksharma@gmail.com
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support QinQ strip offload
> 
> On 7/17/2019 8:45 AM, viveksharma@marvell.com wrote:
> > From: Vivek Sharma <viveksharma@marvell.com>
> >
> > Support QinQ strip RX offload configuration through testpmd command
> > line and boot time arguments.
> 
> For the testpmd command part, unfortunately there are two set of
> commands for same purpose, the new ones are (lets both port and queue
> level):
> "port config <port_id> rx_offload ..."
> "port (port_id) rxq (queue_id) rx_offload ..."
> "port config (port_id) tx_offload ..."
> "port (port_id) txq (queue_id) tx_offload ..."
> 
> These are better implementation comparing the old one:
> "port config all ..."
> 
> Would you mind sending a patch to remove "port config all ..." variant of
> setting offloads?
> And you can make your changes to the new commands above.

Is it ok to remove "port config all ..." commands as they may be in use by the community?

 
> For the application argument, ``--enable-hw-vlan-extend``, instead of adding
> a parameter of each offload argument, (and event it is not clear if it is only for
> Rx or Tx), have a "--rx-offloads" argument and feed the list via this, like:
> "--rx-ofloads=disable-crc-strip,enable-rx-timestamp"
> 
> 
> 
> And lastly for the  "vlan set ..." update, I think "qinq" was already defined but
> it was calling 'vlan_extend_set()', now you are changing it and making it call
> 'rx_vlan_qinq_strip_set()', I think this is OK, but can you please update the
> 'cmd_help_long_parsed()' accordingly?

'vlan_extend_set()'  and 'rx_vlan_qinq_strip_set()'  are different commands.
vlan_extend_set()  is for adding the second vlan  and rx_vlan_qinq_strip_set() is for removing the second vlan.


> And in original 'cmd_help_long_parsed()' for 'vlan set ...", it doesn't need to
> have separate lines for "strip, filter & qinq", can you please merge them, and
> later the 'extend' one?
> Than change needs to be documented on "testpmd_funcs.rst"
> 
> 
> And as a last thing, can you please send this as multiple patches:
> 1) Command line change for setting qinq offload
> 2) Application argument change
> 3) "vlan set " related changes
> 
> >
> > Signed-off-by: Vivek Sharma <viveksharma@marvell.com>
> 

Regards,

Bernard