On 7/22/2019 1:04 PM, Iremonger, Bernard wrote: > Hi Ferruh, > >> -----Original Message----- >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ferruh Yigit >> Sent: Friday, July 19, 2019 5:53 PM >> To: viveksha...@marvell.com; dev@dpdk.org >> Cc: intoviveksha...@gmail.com >> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: support QinQ strip offload >> >> On 7/17/2019 8:45 AM, viveksha...@marvell.com wrote: >>> From: Vivek Sharma <viveksha...@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?
Since there is a command that replaces the removed functionality I think it is OK. Also I am not sure what level of backward compatibility we should provide for testpmd commands. > > >> 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. yes they are different, I think nobody said they are same. What is the concern here, can you please detail more? > > >> 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 <viveksha...@marvell.com> >> > > Regards, > > Bernard >
