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

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

On 5/1/2020 12:33 PM, Ferruh Yigit wrote:
> 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)) {

Also checking these offload flags only in the '.vlan_offload_set' dev_ops is not
enough, user may be set these flags but not called the '.vlan_offload_set' at
all, that is why configure() fucntion should do similar checks and configuration.

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