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

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

On 4/30/2020 7:59 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>

<...>

> +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.