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.yi...@intel.com>
> Sent: Friday, May 1, 2020 5:04 PM
> To: Sardar, Shamsher singh <shamshersingh.sar...@amd.com>; dev@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, ssar...@amd.com wrote:
>> From: Sardar Shamsher Singh <shamshersingh.sar...@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.sar...@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.
>
