[dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe

Ferruh Yigit ferruh.yigit at intel.com
Wed May 6 11:48:35 CEST 2020


On 5/6/2020 6:17 AM, Sardar, Shamsher singh wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> Thanks for knowledge sharing.
> Yes etlt - 0x09 is nothing but indicate " ■ 4’b1001: The packet is type packet with Single CVLAN tag."
> And you are right it should be as below and will do changes on same:
> 
> if (vlan) {
>         mbuf->ol_flags |= PKT_RX_VLAN;
>         mbuf->vlan_tci = xxx
>         if (vlan_stripped) {
>                 mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
>         }
> }
> 
> But rest of the items will do as incremental development.
> 1. Like in axgbe_dev_configure() need to check for default setting when system comes up.

OK

> 2. There are lot of changes need to be add for QinQ support.

I just would like to be sure you are not confusing QinQ support with
'DEV_RX_OFFLOAD_VLAN_EXTEND', because 'DEV_RX_OFFLOAD_VLAN_EXTEND' is not very
clearly defined.
In the 'axgbe_vlan_extend_enable()', it mentions from 'qinq', so it is confusing.
If you are clear on what 'DEV_RX_OFFLOAD_VLAN_EXTEND' is, that is OK. If not I
sugggest dropping the 'ETH_VLAN_EXTEND_MASK' part.

> Will add all as incremental development work.
> Currently working to make other vendor's SFP to work and in parallel above changes will add for upstream.
> Hope this should be fine.

OK

> 
> Can you please put some more light on below, what type issues may occur?
> " And I can see you may hit the problem becuase of VLAN offload but the issue seems generic, not directly related to VLAN support, and this can be separate fix I think."

I was refering to the changes in 'axgbe_dev_tx_queue_setup()',
a) Why those changes are done at all?
b) Why they are done in this patch? Should it be a seperate fix?

> 
> Thanks & regards
> S Shamsher Singh
> (+91)7259016141
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at intel.com>
> Sent: Friday, May 1, 2020 5:04 PM
> To: Sardar, Shamsher singh <Shamshersingh.Sardar at amd.com>; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe
> 
> [CAUTION: External Email]
> 
> On 4/30/2020 7:59 AM, ssardar at amd.com wrote:
>> From: Sardar Shamsher Singh <Shamshersingh.Sardar at 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 at amd.com>
> 
> <...>
> 
>> +static int
>> +axgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask) {
>> +     struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>> +     struct axgbe_port *pdata = dev->data->dev_private;
>> +
>> +     /* Indicate that VLAN Tx CTAGs come from context descriptors */
>> +     AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, CSVL, 0);
>> +     AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, VLTI, 1);
>> +
>> +     if (mask & ETH_VLAN_STRIP_MASK) {
>> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
>> +                     PMD_DRV_LOG(DEBUG, "Strip ON for device = %s\n",
>> +                                 pdata->eth_dev->device->name);
>> +                     pdata->hw_if.enable_rx_vlan_stripping(pdata);
>> +             } else {
>> +                     PMD_DRV_LOG(DEBUG, "Strip OFF for device = %s\n",
>> +                                 pdata->eth_dev->device->name);
>> +                     pdata->hw_if.disable_rx_vlan_stripping(pdata);
>> +             }
>> +     }
>> +     if (mask & ETH_VLAN_FILTER_MASK) {
>> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) {
>> +                     PMD_DRV_LOG(DEBUG, "Filter ON for device = %s\n",
>> +                                 pdata->eth_dev->device->name);
>> +                     pdata->hw_if.enable_rx_vlan_filtering(pdata);
>> +             } else {
>> +                     PMD_DRV_LOG(DEBUG, "Filter OFF for device = %s\n",
>> +                                 pdata->eth_dev->device->name);
>> +                     pdata->hw_if.disable_rx_vlan_filtering(pdata);
>> +             }
>> +     }
>> +     if (mask & ETH_VLAN_EXTEND_MASK) {
>> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND) {
>> +                     PMD_DRV_LOG(DEBUG, "enabling vlan extended mode\n");
>> +                     axgbe_vlan_extend_enable(pdata);
>> +                     /* Set global registers with default ethertype*/
>> +                     axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_OUTER,
>> +                                         RTE_ETHER_TYPE_VLAN);
>> +                     axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER,
>> +                                         RTE_ETHER_TYPE_VLAN);
>> +             } else {
>> +                     PMD_DRV_LOG(DEBUG, "disabling vlan extended mode\n");
>> +                     axgbe_vlan_extend_disable(pdata);
>> +             }
>> +     }
> 
> 
> Is the intention here to enable disable QinQ stip, because enable/disable fucntions talks about qinq, if so 'ETH_QINQ_STRIP_MASK' should be used.
> 
> <...>
>> @@ -275,6 +275,23 @@ axgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>>               /* Get the RSS hash */
>>               if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, RSV))
>>                       mbuf->hash.rss =
>> rte_le_to_cpu_32(desc->write.desc1);
>> +             etlt = AXGMAC_GET_BITS_LE(desc->write.desc3,
>> +                             RX_NORMAL_DESC3, ETLT);
>> +             if (!err || !etlt) {
>> +                     if (etlt == 0x09 &&
>> +                     (rxq->pdata->eth_dev->data->dev_conf.rxmode.offloads &
>> +                             DEV_RX_OFFLOAD_VLAN_STRIP)) {
>> +                             mbuf->ol_flags |=
>> +                                     PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED;
>> +                             mbuf->vlan_tci =
>> +                                     AXGMAC_GET_BITS_LE(desc->write.desc0,
>> +                                                     RX_NORMAL_DESC0,
>> + OVT);
> 
> I don't know HW capabilities, but if 'etlt == 0x09' means packet has VLAN tag, you can use it independent from strip, like below, up to you.
> 
> if (vlan) {
>         mbuf->ol_flags |= PKT_RX_VLAN;
>         mbuf->vlan_tci = xxx
>         if (vlan_stripped) {
>                 mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
>         }
> }
> 
> <...>
> 
>> @@ -487,6 +520,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       struct axgbe_tx_queue *txq;
>>       unsigned int tsize;
>>       const struct rte_memzone *tz;
>> +     uint64_t offloads;
>>
>>       tx_desc = nb_desc;
>>       pdata = dev->data->dev_private;
>> @@ -506,7 +540,8 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>>       if (!txq)
>>               return -ENOMEM;
>>       txq->pdata = pdata;
>> -
>> +     offloads = tx_conf->offloads |
>> +             txq->pdata->eth_dev->data->dev_conf.txmode.offloads;
> 
> As far as I can see PMD doesn't support queue specific offloads, so 'tx_conf->offloads' should be always 0.
> 
> And since there is no queue specific offload and this 'offloads' information is only used to set burst function, which is again only port bases (this will keep overwrite same info per each queue), why not do this check in the 'axgbe_dev_configure()' instead of per queue?
> 
> And I can see you may hit the problem becuase of VLAN offload but the issue seems generic, not directly related to VLAN support, and this can be separate fix I think.
> 



More information about the dev mailing list