Re: [dpdk-dev] [PATCH v2] net/axgbe: vlan support enabling in axgbe

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/5/2020 6:00 PM, Sardar, Shamsher singh wrote:
> +dev@dpdk.org
> 
> -----Original Message-----
> From: Sardar, Shamsher singh 
> Sent: Friday, April 3, 2020 10:12 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Kumar, Ravi1 <Ravi1.Kumar@amd.com>
> Subject: RE: [dpdk-dev] [PATCH v2] net/axgbe: vlan support enabling in axgbe
> 
> Hi Yigit,
> 
> 1. The Rx vlan filter set calls vlan_offload_set as shown below:
> rx_vlan_filter_set() <-- app/test-pmd/config.c  ...
>   rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) <--lib/librte_ethdev/rte_ethdev.c  {  ...
>  ret = (*dev->dev_ops->vlan_offload_set) <-- this calls the respective driver's func  ...
>  }
>  2. The tx vlan check for flags and based on same it add/remove as shown below the code flow:
> tx_vlan_set(portid_t port_id, uint16_t vlan_id)  <-- app/test-pmd/config.c  ...
>   rte_eth_dev_info_get(port_id, &dev_info);  <-- collect flags from driver  check capability DEV_TX_OFFLOAD_VLAN_INSERT  ...
>  ports[port_id].tx_vlan_id = vlan_id; <--insert vlan to respective port id

Hi Shamsher singh,

VLAN filter looks good, but in the VLAN insert/strip proper 'mbuf.ol_flags' and
'mbuf.vlan_tci' should be set.

In the Rx path, driver should set 'PKT_RX_VLAN' and/or 'PKT_RX_VLAN_STRIPPED'
flags (please check documentation) and 'mbuf.vlan_tci', otherwise how
application can know about the vlan tag and use it.
Like if the 'PKT_RX_VLAN' is set you will see additional log in the testpmd
packet view, "VLAN tci=0x%x", showing the received VLAN tag.

Similar in Tx, driver should check 'PKT_TX_VLAN' flag to insert 'mbuf.vlan_tci'
VLAN tag for the packet it has this flag.

Regards,
ferruh

> 
> Note: Also attached logs to check call for filter flow and strip on/off. 
> In strip log by default the strip is off and hence header shown when sent vlan pkt from pktgen running in other device and in strip on the header is getting removed.
> Let me know if you need more logs.
> 
> Note: In pkt-gen log, the log captured for filtering with different vlan settings to check data flow post addition/removal of vlan in both side (pkt-gen and DUT, device under test).
> 
> Thanks & regards
> S Shamsher Singh
> Mob:7259016141
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, April 3, 2020 6:26 PM
> To: Sardar, Shamsher singh <Shamshersingh.Sardar@amd.com>; dev@dpdk.org
> Cc: Kumar, Ravi1 <Ravi1.Kumar@amd.com>
> Subject: Re: [dpdk-dev] [PATCH v2] net/axgbe: vlan support enabling in axgbe
> 
> [CAUTION: External Email]
> 
> On 4/3/2020 7:20 AM, ssardar@amd.com wrote:
>> From: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>
>>
>> adding below APIs for axgbe
>> - axgbe_enable_rx_vlan_stripping: to enable vlan header stipping
>> - axgbe_disable_rx_vlan_stripping: to disable vlan header stipping
>> - axgbe_enable_rx_vlan_filtering: to enable vlan filter mode
>> - axgbe_disable_rx_vlan_filtering: to disable vlan filter mode
>> - axgbe_update_vlan_hash_table: crc calculation and hash table update 
>> based on vlan values post filter enable
>> - axgbe_vlan_filter_set: setting of active vlan out of max 4K values 
>> before doing hash update of same
>> - axgbe_vlan_tpid_set: setting of default tpid values
>> - axgbe_vlan_offload_set: a top layer function to call stip/filter etc 
>> based on mask values
>>
>> Signed-off-by: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>
>> ---
>>  doc/guides/nics/features/axgbe.ini |   1 +
>>  drivers/net/axgbe/axgbe_common.h   |  29 +++++
>>  drivers/net/axgbe/axgbe_dev.c      | 171 +++++++++++++++++++++++++++--
>>  drivers/net/axgbe/axgbe_ethdev.c   | 169 +++++++++++++++++++++++++++-
>>  drivers/net/axgbe/axgbe_ethdev.h   |  16 +++
>>  drivers/net/axgbe/axgbe_rxtx.c     |   6 +
>>  6 files changed, 381 insertions(+), 11 deletions(-)
>>
>> diff --git a/doc/guides/nics/features/axgbe.ini
>> b/doc/guides/nics/features/axgbe.ini
>> index 0becaa097..b7b4dd992 100644
>> --- a/doc/guides/nics/features/axgbe.ini
>> +++ b/doc/guides/nics/features/axgbe.ini
>> @@ -11,6 +11,7 @@ Scattered Rx         = Y
>>  Promiscuous mode     = Y
>>  Allmulticast mode    = Y
>>  RSS hash             = Y
>> +VLAN                 = Y
> 
> There is no "VLAN" feautre defined [1], but "VLAN filter" [2] & "VLAN offload"
> [3], and in this patch I guess you mean both.
> 
> But for "VLAN offload", I don't see the driver sets "PKT_RX_VLAN" or "PKT_RX_VLAN_STRIPPED" mbuf flags at all. And it doesn't use 'mbuf.vlan_tci' at all, in both Rx & Tx. Are you sure VLAN offload supported by driver?
> 
> And for Tx, driver still uses 'PKT_TX_VLAN_PKT' which is deprecated, can you please replace it with 'PKT_TX_VLAN'?
> 
> 
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flxr.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Fdoc%2Fguides%2Fnics%2Ffeatures%2Fdefault.ini&amp;data=02%7C01%7CShamshersingh.Sardar%40amd.com%7C1c359b4b479d4ac7090608d7d7ce5e33%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637215153650573793&amp;sdata=Ujp55KpjRyP%2FFFa9Y8v1RJQdCtS2qGjRj7sdE3y7uAk%3D&amp;reserved=0
> 
> [2]
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flxr.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Fdoc%2Fguides%2Fnics%2Ffeatures.rst%23L363&amp;data=02%7C01%7CShamshersingh.Sardar%40amd.com%7C1c359b4b479d4ac7090608d7d7ce5e33%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637215153650583782&amp;sdata=QUZF1xle9plBK1HeGEf4FMJaXrcMAr0%2BFpvyv7BB7ic%3D&amp;reserved=0
> 
> [3]
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flxr.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Fdoc%2Fguides%2Fnics%2Ffeatures.rst%23L474&amp;data=02%7C01%7CShamshersingh.Sardar%40amd.com%7C1c359b4b479d4ac7090608d7d7ce5e33%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637215153650583782&amp;sdata=cU%2FdVldaEzozUdoOdi3jkjIhx9K95NGPuZjdxM4YFl8%3D&amp;reserved=0
>