Re: [dpdk-dev] [PATCH] net/e1000: i350 VLAN tags in loopback packets are big endian

["Roger B. Melton" <rmelton@cisco.com>]

Hi Wenzhou,

On 10/10/17 9:32 PM, Lu, Wenzhuo wrote:
> Hi Roger,
>
>> -----Original Message-----
>> From: Roger B Melton [mailto:rmelton@cisco.com]
>> Sent: Saturday, October 7, 2017 7:34 AM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Cc: dev@dpdk.org; Roger B Melton <rmelton@cisco.com>
>> Subject: [PATCH] net/e1000: i350 VLAN tags in loopback packets are big
>> endian
>>
>> When copying VLAN tags from the RX descriptor to the vlan_tci field in the
>> mbuf header,  igb_rxtx.c:eth_igb_recv_pkts() and
>> eth_igb_recv_scattered_pkts() both assume that the VLAN tag is always little
>> endian.  While i350 VLAN non-loopback packets are stored little endian,
>> VLAN tags for i350 VLAN loopback packets are big endian.
>>
>> For i350 VLAN loopback packets, swap the tag when copying from the RX
>> descriptor to the mbuf header.  This will ensure that the mbuf vlan_tci is
>> always little endian.
>>
>> Signed-off-by: Roger B Melton <rmelton@cisco.com>
>> ---
>>   drivers/net/e1000/igb_rxtx.c | 56
>> ++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c index
>> 1c80a2a..8432d8b 100644
>> --- a/drivers/net/e1000/igb_rxtx.c
>> +++ b/drivers/net/e1000/igb_rxtx.c
>
>> @@ -2278,6 +2300,18 @@ eth_igb_rx_init(struct rte_eth_dev *dev)
>>
>>   		rxq = dev->data->rx_queues[i];
>>
>> +		rxq->flags = 0;
>> +		/*
>> +		 * The i210, i211, i350, i354 and i350VF LB vlan packets
>> +		 * have vlan tags byte swapped.
>> +		 */
>> +		if (hw->mac.type >= e1000_i350) {
> Many thanks for the patch.
> I think we need to check i350 and i354 here as LB bit is only meaningful on these NICs.

Agreed, I'll change this to:

    if ((hw->mac.type == e1000_i350) || (hw->mac.type == e1000_i354)) {...



>
>> +			rxq->flags |= IGB_RXQ_FLAG_LB_BSWAP_VLAN;
>> +			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap
>> required");
>> +		} else {
>> +			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap not
>> required");
>> +		}
>> +
>>   		/* Allocate buffers for descriptor rings and set up queue */
>>   		ret = igb_alloc_rx_queue_mbufs(rxq);
>>   		if (ret)
>> @@ -2557,6 +2591,18 @@ eth_igbvf_rx_init(struct rte_eth_dev *dev)
>>
>>   		rxq = dev->data->rx_queues[i];
>>
>> +		rxq->flags = 0;
>> +		/*
>> +		 * The i210, i211, i350, i354 and i350VF LB vlan packets
>> +		 * have vlan tags byte swapped.
>> +		 */
>> +		if (hw->mac.type >= e1000_i350) {
> We need to check e1000_vfadapt_i350 here to exclude e1000_vfadapt.

Agreed, I'll change this to:

    if (hw->mac.type == e1000_vfadapt_i350) {...


I'll make these changes and submit an updated patch.

>
>> +			rxq->flags |= IGB_RXQ_FLAG_LB_BSWAP_VLAN;
>> +			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap
>> required");
>> +		} else {
>> +			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap not
>> required");
>> +		}
>> +
>>   		/* Allocate buffers for descriptor rings and set up queue */
>>   		ret = igb_alloc_rx_queue_mbufs(rxq);
>>   		if (ret)
>> --
>> 2.10.3.dirty
> .
>